From 94a0be49be7a897255543306de065a988c1c45eb Mon Sep 17 00:00:00 2001 From: vishk23 <119831996+vishk23@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:51:31 -0400 Subject: [PATCH] test(schema): let the oracle declare a second migration lineage, for identifiers that cannot be renumbered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit testGrdbMigrationIdentifiersAreUniqueAndSequential asserts every migration identifier's vN equals its registration index, and tells the author to "Renumber before merging" otherwise. For two unmerged PRs both claiming a vN — the #369 / #475 case the doc comment cites — that is the right remedy, and this does not change it. It stops being available once an identifier has reached a database. GRDB keys migrations by identifier STRING: rename v26-foo to v32-foo and a device that already ran it no longer finds that name in grdb_migrations, so the migrator reads it as un-applied and re-runs its body against a schema that already has the table. For a fork, a long-lived branch, or any identifier already shipped in a release, the identifier is frozen, and the suite has no way to say so short of deleting the check. So the oracle gains an optional grdbMigrationLineages: a named set of identifiers numbered independently of the baseline, carrying the reason they cannot be renumbered. Whatever is NOT declared is the baseline lineage and is still held to exactly v1..vN with no gaps and no repeats. Nothing is weakened by default. No lineage is declared here, so upstream behaviour is bit-for-bit unchanged. Full-identifier uniqueness stays absolute, checked before any subtraction, because a genuine duplicate makes GRDB silently skip the second body. An undeclared collision still fails. A declared lineage must be true — every member registered — so an entry that stops being true fails rather than rotting, the rule divergenceReasons already lives by. A lineage's own vN must strictly increase. And a gap cannot be laundered through a lineage: declaring a baseline migration as part of one to dodge the check fails anyway, because removing it from the baseline shifts everything after it. Verified on this branch: Swift 330/330 (SchemaOracleTests 7/7); Android SchemaOracleTest 4/4. The suite's one failure, DeepCaptureMigrationTest.repositoryInsertV18Aux_insertsThenPrunes, is red on main independently of this change and is what #897 fixes. Each of the four abuse mutations above was injected and fails on both halves. --- .../Resources/schema_oracle.json | 2 +- .../WhoopStoreTests/SchemaOracleTests.swift | 64 ++++++++++++++++--- .../java/com/noop/data/SchemaOracleTest.kt | 52 +++++++++++++-- .../app/src/test/resources/schema_oracle.json | 2 +- 4 files changed, 103 insertions(+), 17 deletions(-) diff --git a/Packages/WhoopStore/Tests/WhoopStoreTests/Resources/schema_oracle.json b/Packages/WhoopStore/Tests/WhoopStoreTests/Resources/schema_oracle.json index 72081fadf5..0de99c3e75 100644 --- a/Packages/WhoopStore/Tests/WhoopStoreTests/Resources/schema_oracle.json +++ b/Packages/WhoopStore/Tests/WhoopStoreTests/Resources/schema_oracle.json @@ -1,5 +1,5 @@ { - "_readme": "SHARED Room<->GRDB SCHEMA ORACLE (#775). Two byte-identical copies: Packages/WhoopStore/Tests/WhoopStoreTests/Resources/schema_oracle.json and android/app/src/test/resources/schema_oracle.json. SchemaOracleTests.swift compares GRDB's PRAGMA table_info/index_list against it; SchemaOracleTest.kt compares Room's exported schema JSON against it. `columns` is the iOS/GRDB shape in GRDB column order (macOS is the reference implementation); every way Android differs is spelled out in an `android` / `iosAbsent` / `androidColumnOrder` override naming a key in `divergenceReasons`. Adding a column, reordering one, or changing a type/nullability on one platform only fails both suites until it is either fixed or written down here.", + "_readme": "SHARED Room<->GRDB SCHEMA ORACLE (#775). Two byte-identical copies: Packages/WhoopStore/Tests/WhoopStoreTests/Resources/schema_oracle.json and android/app/src/test/resources/schema_oracle.json. SchemaOracleTests.swift compares GRDB's PRAGMA table_info/index_list against it; SchemaOracleTest.kt compares Room's exported schema JSON against it. `columns` is the iOS/GRDB shape in GRDB column order (macOS is the reference implementation); every way Android differs is spelled out in an `android` / `iosAbsent` / `androidColumnOrder` override naming a key in `divergenceReasons`. Adding a column, reordering one, or changing a type/nullability on one platform only fails both suites until it is either fixed or written down here. The optional `grdbMigrationLineages` is the same idea for migration IDENTIFIERS: a fork or long-lived branch that numbered its own migrations before upstream reached those numbers declares them there instead of renumbering \u2014 GRDB keys migrations by identifier string, so renaming one wedges a database that already ran it. Whatever is not declared is the baseline lineage and is still checked as exactly v1..vN.", "roomVersion": 25, "grdbMigrations": [ "v1", diff --git a/Packages/WhoopStore/Tests/WhoopStoreTests/SchemaOracleTests.swift b/Packages/WhoopStore/Tests/WhoopStoreTests/SchemaOracleTests.swift index 5cba234060..30853408ae 100644 --- a/Packages/WhoopStore/Tests/WhoopStoreTests/SchemaOracleTests.swift +++ b/Packages/WhoopStore/Tests/WhoopStoreTests/SchemaOracleTests.swift @@ -32,6 +32,15 @@ final class SchemaOracleTests: XCTestCase { let grdbMigrations: [String] let divergenceReasons: [String: String] let tables: [String: OracleTable] + /// Absent upstream, where there is only the baseline lineage. See `MigrationLineage`. + let grdbMigrationLineages: [String: MigrationLineage]? + var lineages: [String: MigrationLineage] { grdbMigrationLineages ?? [:] } + } + /// A set of migration identifiers numbered independently of the baseline lineage — a fork that + /// reached `vN` before upstream did. Declaring one is what lets the two coexist without a renumber. + struct MigrationLineage: Decodable { + let reason: String + let migrations: [String] } struct OracleTable: Decodable { let platform: String // "both" | "ios_only" | "android_only" @@ -201,23 +210,58 @@ final class SchemaOracleTests: XCTestCase { /// entirely, because the first already recorded that name in `grdb_migrations`. /// /// So: identifiers unique, and their `vN` prefixes exactly 1...N with no gaps and no repeats. + /// + /// The one sanctioned exception is a DECLARED lineage. A fork that numbered its own migrations + /// before upstream reached those numbers cannot renumber its way out — GRDB keys by identifier + /// string, so renaming one makes it read as un-applied on a device that already ran it and the + /// migrator re-runs its body against a schema that already has the table. Such a fork lists its + /// identifiers under `grdbMigrationLineages`; everything NOT listed is the baseline lineage and is + /// still held to exactly v1...vN. So the escape hatch costs a written-down reason and cannot be + /// taken by accident — an undeclared collision still fails, which is what catches #369 vs #475. func testGrdbMigrationIdentifiersAreUniqueAndSequential() throws { + let oracle = try loadOracle() let ids = WhoopStore.makeMigrator().migrations XCTAssertEqual(Set(ids).count, ids.count, "duplicate GRDB migration identifier — GRDB would silently SKIP the second body") - var numbers: [Int] = [] - for id in ids { - let digits = id.dropFirst().prefix { $0.isNumber } - guard id.hasPrefix("v"), let n = Int(digits) else { - return XCTFail("migration identifier '\(id)' is not of the form v[-slug]") + func version(of id: String) -> Int? { + guard id.hasPrefix("v"), let n = Int(id.dropFirst().prefix(while: \.isNumber)) else { return nil } + return n + } + for id in ids where version(of: id) == nil { + return XCTFail("migration identifier '\(id)' is not of the form v[-slug]") + } + + // A declared lineage must be true (every member registered) and self-consistent (its own vN + // strictly increasing), so the ledger can only shrink deliberately — same rule the divergence + // reasons live by. + var claimedBy: [String: String] = [:] + for (name, lineage) in oracle.lineages.sorted(by: { $0.key < $1.key }) { + XCTAssertFalse(lineage.migrations.isEmpty, "lineage '\(name)' declares no migrations — delete it") + for id in lineage.migrations { + if let other = claimedBy.updateValue(name, forKey: id) { + return XCTFail("'\(id)' is claimed by both lineage '\(other)' and '\(name)'") + } + guard ids.contains(id) else { + return XCTFail("lineage '\(name)' declares '\(id)', which is not registered — a lineage " + + "entry that stopped being true must be deleted, not left to rot") + } + } + let numbers = lineage.migrations.compactMap(version(of:)) + for (a, b) in zip(numbers, numbers.dropFirst()) where b <= a { + return XCTFail("lineage '\(name)' goes v\(a) then v\(b) — a lineage numbers its own " + + "migrations, so its vN must strictly increase") } - numbers.append(n) } - for (offset, n) in numbers.enumerated() where n != offset + 1 { - return XCTFail("GRDB migration '\(ids[offset])' claims v\(n) but is #\(offset + 1) in " - + "registration order — two migrations claiming the same vN, or a gap, makes the " - + "GRDB-name <-> Room-version mapping ambiguous. Renumber before merging.") + + let baseline = ids.filter { claimedBy[$0] == nil } + for (offset, id) in baseline.enumerated() where version(of: id) != offset + 1 { + return XCTFail("GRDB migration '\(id)' claims v\(version(of: id)!) but is #\(offset + 1) in the " + + "BASELINE lineage (registration order minus every lineage schema_oracle.json " + + "declares) — two migrations claiming the same vN, or a gap, makes the " + + "GRDB-name <-> Room-version mapping ambiguous. Renumber before merging, or, if " + + "the identifier already shipped and renaming it would wedge a live database, " + + "declare it in grdbMigrationLineages with a reason.") } } diff --git a/android/app/src/test/java/com/noop/data/SchemaOracleTest.kt b/android/app/src/test/java/com/noop/data/SchemaOracleTest.kt index 14aec7c2f4..a482a41d4b 100644 --- a/android/app/src/test/java/com/noop/data/SchemaOracleTest.kt +++ b/android/app/src/test/java/com/noop/data/SchemaOracleTest.kt @@ -238,25 +238,67 @@ class SchemaOracleTest { * migrations by NAME and applies them in registration order; Room keys by integer version, so the * only thing that can be compared mechanically is the RESULTING schema — which is what the tests * above do. Pinning the two identifier spaces is what forces them to be re-checked together. + * + * The one sanctioned exception is a DECLARED lineage. A fork that numbered its own migrations before + * upstream reached those numbers cannot renumber its way out — GRDB keys by identifier string, so + * renaming one makes it read as un-applied on a device that already ran it. Such a fork lists its + * identifiers under `grdbMigrationLineages`; everything NOT listed is the baseline lineage and is + * still held to exactly v1..vN. An UNDECLARED collision still fails, which is the #369-vs-#475 case. + * Mirrors `testGrdbMigrationIdentifiersAreUniqueAndSequential` in the Swift half. */ @Test fun pinnedMigrationIdentifiersAreCoherent() { val oracle = loadOracle() val grdb = oracle.getJSONArray("grdbMigrations").strings() assertEquals("duplicate GRDB migration identifier in schema_oracle.json", grdb.size, grdb.toSet().size) - grdb.forEachIndexed { i, id -> - val n = id.removePrefix("v").takeWhile { it.isDigit() }.toIntOrNull() + + // A declared lineage must be true (every member pinned) and self-consistent (its own vN strictly + // increasing), so the ledger can only shrink deliberately — same rule the divergence reasons live by. + val lineages = oracle.optJSONObject("grdbMigrationLineages") ?: JSONObject() + val claimedBy = HashMap() + for (name in lineages.keys().asSequence().sorted()) { + val migrations = lineages.getJSONObject(name).getJSONArray("migrations").strings() + assertTrue("lineage '$name' declares no migrations — delete it", migrations.isNotEmpty()) + val numbers = migrations.map { id -> + val other = claimedBy.put(id, name) + assertTrue("'$id' is claimed by both lineage '$other' and '$name'", other == null) + assertTrue( + "lineage '$name' declares '$id', which is not in grdbMigrations — a lineage entry that " + + "stopped being true must be deleted, not left to rot", + grdb.contains(id), + ) + val n = migrationVersion(id) + assertNotNull("lineage '$name' member '$id' is not of the form v[-slug]", n) + n!! + } + numbers.zipWithNext { a, b -> + assertTrue( + "lineage '$name' goes v$a then v$b — a lineage numbers its own migrations, so its vN " + + "must strictly increase", + b > a, + ) + } + } + + grdb.filterNot { claimedBy.containsKey(it) }.forEachIndexed { i, id -> assertEquals( - "GRDB migration '$id' claims v$n but is #${i + 1} in registration order — two migrations " + - "claiming the same vN, or a gap, makes the GRDB-name <-> Room-version mapping ambiguous.", + "GRDB migration '$id' claims v${migrationVersion(id)} but is #${i + 1} in the BASELINE " + + "lineage (grdbMigrations minus every lineage grdbMigrationLineages declares) — two " + + "migrations claiming the same vN, or a gap, makes the GRDB-name <-> Room-version " + + "mapping ambiguous. Renumber before merging, or, if the identifier already shipped and " + + "renaming it would wedge a live database, declare it in grdbMigrationLineages.", i + 1, - n, + migrationVersion(id), ) } // loadRoomSchema asserts the exported version equals this; call it so the check is not vacuous. loadRoomSchema(oracle.getInt("roomVersion")) } + /** The `N` of a `v[-slug]` migration identifier, or null if it is not of that form. */ + private fun migrationVersion(id: String): Int? = + id.removePrefix("v").takeWhile { it.isDigit() }.toIntOrNull() + /** * The Android and Swift copies of the oracle MUST be byte-identical, so neither platform can edit its * fixture without the other. Skips gracefully if the Swift tree isn't present. diff --git a/android/app/src/test/resources/schema_oracle.json b/android/app/src/test/resources/schema_oracle.json index 72081fadf5..0de99c3e75 100644 --- a/android/app/src/test/resources/schema_oracle.json +++ b/android/app/src/test/resources/schema_oracle.json @@ -1,5 +1,5 @@ { - "_readme": "SHARED Room<->GRDB SCHEMA ORACLE (#775). Two byte-identical copies: Packages/WhoopStore/Tests/WhoopStoreTests/Resources/schema_oracle.json and android/app/src/test/resources/schema_oracle.json. SchemaOracleTests.swift compares GRDB's PRAGMA table_info/index_list against it; SchemaOracleTest.kt compares Room's exported schema JSON against it. `columns` is the iOS/GRDB shape in GRDB column order (macOS is the reference implementation); every way Android differs is spelled out in an `android` / `iosAbsent` / `androidColumnOrder` override naming a key in `divergenceReasons`. Adding a column, reordering one, or changing a type/nullability on one platform only fails both suites until it is either fixed or written down here.", + "_readme": "SHARED Room<->GRDB SCHEMA ORACLE (#775). Two byte-identical copies: Packages/WhoopStore/Tests/WhoopStoreTests/Resources/schema_oracle.json and android/app/src/test/resources/schema_oracle.json. SchemaOracleTests.swift compares GRDB's PRAGMA table_info/index_list against it; SchemaOracleTest.kt compares Room's exported schema JSON against it. `columns` is the iOS/GRDB shape in GRDB column order (macOS is the reference implementation); every way Android differs is spelled out in an `android` / `iosAbsent` / `androidColumnOrder` override naming a key in `divergenceReasons`. Adding a column, reordering one, or changing a type/nullability on one platform only fails both suites until it is either fixed or written down here. The optional `grdbMigrationLineages` is the same idea for migration IDENTIFIERS: a fork or long-lived branch that numbered its own migrations before upstream reached those numbers declares them there instead of renumbering \u2014 GRDB keys migrations by identifier string, so renaming one wedges a database that already ran it. Whatever is not declared is the baseline lineage and is still checked as exactly v1..vN.", "roomVersion": 25, "grdbMigrations": [ "v1",