test(schema): let the oracle declare a second migration lineage, for identifiers that cannot be renumbered - #905
Open
vishk23 wants to merge 1 commit into
Open
Conversation
…identifiers that cannot be renumbered 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
testGrdbMigrationIdentifiersAreUniqueAndSequential(and its Kotlin twin) asserts that every GRDBmigration identifier's
vNequals its registration index. That is the right default, and this PRkeeps it as the default. What it adds is a way to say "these identifiers are numbered separately,
here is why" — for the one case where the rule asks for something that cannot safely be done.
Renumbering is not always available
The test's remedy is "Renumber before merging." For two unmerged PRs that both claim a
vN— the#369/#475case the doc comment cites — that is exactly right, and this PR does not change it.It stops being available once an identifier has reached a database. GRDB keys migrations by
identifier string: rename
v26-footov32-fooand a device that already ran it no longer findsthat name in
grdb_migrations, so the migrator reads it as un-applied and re-runs its body against aschema that already has the table. The migration then throws, and the store fails to open. So 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 express that short of deleting the check.
Concretely: a fork carrying four iOS-only migrations numbered from
v26before upstream's ownv26..v29existed ends up with both lineages claiming the same four numbers, no way to renumber, andfour red tests on every merge from upstream.
The mechanism
An optional
grdbMigrationLineagesin the oracle declares a set of identifiers numberedindependently of the baseline:
Whatever is not declared is the baseline lineage, and the baseline is still held to exactly
v1..vNwith no gaps and no repeats.Nothing is weakened by default
bit-for-bit what it is on
maintoday — this is a no-op for upstream until someone declares one.genuinely duplicated identifier still fails, because GRDB would silently skip the second body.
#369vs#475is caught exactly as before.stops being true fails rather than rotting — the same rule
divergenceReasonsalready lives by.vNmust strictly increase.dodge the sequence check fails anyway, because removing it from the baseline shifts everything
after it.
The escape hatch therefore costs a written-down reason and cannot be taken by accident, which is the
same bargain
divergenceReasonsstrikes for schema drift.Verification
On this branch:
swift testinPackages/WhoopStore: 330/330,SchemaOracleTests7/7../gradlew testFullDebugUnitTest:SchemaOracleTest4/4; suite 3189 tests withone failure,
DeepCaptureMigrationTest.repositoryInsertV18Aux_insertsThenPrunes. That one isred on
mainindependently of this change — it is what fix(android): repositoryInsertV18Aux_insertsThenPrunes has been red on main since #888 #897 fixes — and nothing here touches it.Then, to confirm the new key cannot be used to wave a real problem through, each of these was
injected on this branch and fails on both halves:
vNdoes not strictly increaseThe undeclared-collision case cannot be staged here, because
main's registered migrations are aclean
v1..vN; it was exercised on a branch that really does register two lineages claiming the samefour numbers, where removing the declaration puts the test straight back to failing.
The
_readmein both oracle copies documents the new key; the copies stay byte-identical.