Repository navigation
test(memory): fail the build on a duplicated, skipped or misnamed migration step - #8355
Conversation
…ration step Three PRs claimed overlapping `migrate_vN` numbers on one day, and only one of the two failure modes that produces announces itself. A duplicate number is silent, and silent only where it matters. `run_step!` fires on `current_version < N`, so a second step claiming an already-taken `N` never executes on an installation that has passed `N`. A fresh database starts at `user_version == 0` and runs every step, so it is correct — and a fresh database is the only kind CI ever creates. The feature then returns 500 on upgraded installations and works perfectly on new ones. The duplicate `fn` name is a compile error, which is why this looks handled; resolving the collision by renumbering only the audit `INSERT`, or by bumping `SCHEMA_VERSION` without adding the `run_step!` line, produces the silent version with nothing red anywhere. A gap is caught, but by `test_every_migration_records_audit_row`, which reports "some migrate_vN is recording its audit row under a version other than its own" — true of nothing on such a branch, so the reader looks in the wrong place. The guard reads the `run_step!` lines out of the module's own source rather than out of a migrated database, because the duplicate is invisible at runtime on the path CI takes. It asserts the ladder is strictly increasing, contiguous, ends exactly at `SCHEMA_VERSION`, and that each step calls the function named for its own version — the last clause because renumbering that moves the `run_step!` line and leaves the function behind is how a duplicate survives a rename. Verified it discriminates rather than merely passing. Four mutations, one per clause, each failing with its own message: - two `run_step!(60, migrate_v60)` lines -> "the ladder must strictly increase" - v59 renumbered to v61 -> "nothing claims v59 between ..." - v58 and v59 swapping callees -> "calls a function not named for its own version" - `SCHEMA_VERSION` bumped to 61 alone -> "the ladder ends at ... but SCHEMA_VERSION is 61" `docs/development/database-migrations.md` writes the convention down: the four edits that move together, why whoever merges second renumbers all four rather than just the `run_step!` line, and why a post-migration assertion belongs against `SCHEMA_VERSION` instead of a literal — three such assertions went stale in one renumber, in a file that already used the constant form two hundred lines earlier. `cargo nextest run -p librefang-memory`: 470 passed, 0 failed. `cargo clippy -p librefang-memory --all-targets -- -D warnings`: 0. `cargo fmt --all --check`: 0.
The fragment was written before the PR existed and guessed its number. Fragments sort by that number and the generated-line suppression matches on the trailing group, so a wrong one both sorts astray and lets a duplicate generated line through.
|
This one earns its place, and I can show that it would already be catching something. #7991 and #7974 both claim v61 right now, with different bodies: Which is exactly the shape the changelog describes: whichever merges second stops firing on any installation already past 61, so its column never appears on an upgrade while CI — which only ever builds a fresh database and runs every step — stays green. New installs work, upgraded ones 500. #7974's own comment says the migration "will be renumbered before" merge. Nothing enforces that today. This test is what would. Verified against the current tree rather than assumed: parsing Two notes on the parser, neither blocking. A step whose invocation does not fit the one-line shape is dropped silently rather than failing. The The doc comment is the most valuable part of the change — the distinction between "invisible at runtime on the only path CI ever takes" and "fails on upgrade" is the thing that makes the next person renumber instead of jumping to a free number. Suggest merging this ahead of #7991 and #7974 so the collision surfaces in CI rather than in whichever one merges second. |
Closes #8140.
The two failure modes, and why only one announces itself
A duplicate number is silent, and silent only where it matters.
run_step!fires oncurrent_version < N, so a second step claiming an already-takenNnever executes on an installation that has passedN.A fresh database starts at
user_version == 0and runs every step, so it is correct — and a fresh database is the only kind CI ever creates.The feature then returns 500 on upgraded installations and works perfectly on new ones.
The duplicate
fnname is a compile error, which is why this looks handled. It is not: resolving the collision by renumbering only the auditINSERT, or by bumpingSCHEMA_VERSIONwithout adding therun_step!line, produces the silent version with nothing red anywhere.A gap is caught, but the message points elsewhere.
test_every_migration_records_audit_rowreports "some migrate_vN is recording its audit row under a version other than its own", which is true of nothing on such a branch.Changes
crates/librefang-memory/src/migration.rs:the_migration_ladder_is_contiguous_and_named_for_its_versions. Parses therun_step!lines out of the module's own source — not out of a migrated database, because the duplicate is invisible at runtime on the path CI takes — and asserts the ladder is strictly increasing, contiguous, ends exactly atSCHEMA_VERSION, and that each step calls the function named for its own version.docs/development/database-migrations.md: the convention. The four edits that move together, why whoever merges second renumbers all four rather than just therun_step!line, and what the guard cannot check.changelog.d/added/8354-migration-ladder-guard.md.The fourth clause exists because renumbering that moves the
run_step!line and leaves the function behind is how a duplicate survives a rename.Verification
Each clause was mutated against working code and had to go red on its own:
run_step!(60, migrate_v60)linesthe ladder must strictly increasenothing claims v59 between run_step!(58, …) and run_step!(61, …)calls a function not named for its own versionSCHEMA_VERSIONbumped to 61 alonethe ladder ends at run_step!(60, …) but SCHEMA_VERSION is 61The third needed the swap rather than a single rename, because leaving
migrate_v58uncalled isdead_codeand fails to compile before the assertion can run.cargo nextest run -p librefang-memory: 470 passed, 0 failed.cargo clippy -p librefang-memory --all-targets -- -D warnings: 0.cargo fmt --all --check: 0.Not included
The guard cannot check that
migrate_v{N}'s body does what its name and audit description claim. Two sibling tests already cover that half:test_every_migration_records_audit_rowandno_migration_relies_on_the_audit_backfill.The two instances named in #8140 are already resolved on their branches — measured and reported in #8140 (comment). This PR is the convention and the guard, not a repair.