fix(db): open a real v0.6.0 database in the test suite, and stop copying broken migrations - #597
Merged
Merged
Conversation
…ing broken migrations Closes #580 #582. NOTHING IN THIS REPO OPENED A DATABASE BUILT FROM A SHIPPED SCHEMA. schema.sql is CREATE TABLE IF NOT EXISTS, so a column declared only there reaches fresh installs and never an upgrade -- an asymmetry that has caused data loss here before. Every test was blind to it: dbtest's template is a fresh install, each migration test hand-builds only the table it cares about, and the "0.6.x-shaped" fixture in schema_version_test.go is a hand-written five-column users table rather than the schema v0.6.0 actually shipped. So four migrations were correct because four changes each REMEMBERED one. That is rung zero, and it held four times. The device is the previous release's real schema.sql, checked in as a fixture, opened through the ordinary Open path and compared object-for-object -- columns included, because a table present in both but short a column is the case that matters -- against a fresh install. Verified by removing MigrateHookAllowPrivateTarget from Open: it fails naming `column hooks.allow_private_target`. The fixture is a historical artefact. When this test fails the answer is a Migrate* on Open's path, never an edit to the fixture, and the test says so. TWO MIGRATIONS RETURNED BARE while nine closed the handle and wrapped. They were the two most recently added, because the way you write a migration here is to copy the one above it -- and these two had been copied from each other. A leaked handle on a path about to exit is small; an inconsistency the next copy inherits is not. The AST guard that now holds that shape found a third case the issue had not reported, and it turned out to be legitimate: MigrateSchemaVersion closes but does not wrap, because its own error already reads `stamp schema version 1: ...` and a "migrate: " prefix would add nothing. It is recorded as an exemption with that reason rather than being "fixed" into consistency it does not need. Three comments had drifted off the calls they describe, including one that attributed MigrateSources's ordering constraint -- "Last, because it reads settings and writes to destinations" -- to a hooks column migration inserted between the comment and its call. Claude-Session: https://claude.ai/code/session_01A8N3W5ct9SZtHK9sCDD9cL
|
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.
Closes #580 #582.
Nothing in this repo opened a database built from a shipped schema
schema.sqlisCREATE TABLE IF NOT EXISTS, so a column declared only there reaches fresh installs and never an upgrade — the asymmetry that has caused data loss here before.Every test was blind to it:
dbtest's template is a fresh install, each migration test hand-builds only the table it cares about, and the "0.6.x-shaped" fixture inschema_version_test.gois a hand-written five-columnuserstable, not the schema v0.6.0 actually shipped.So four migrations were correct because four separate changes each remembered one. That's rung zero, and it happened to hold four times.
The device: the previous release's real
schema.sql, checked in as a fixture, opened through the ordinaryOpenpath and compared object-for-object — columns included, because a table present in both but short a column is the case that matters — against a fresh install.Verified by removing
MigrateHookAllowPrivateTargetfromOpen: it fails namingcolumn hooks.allow_private_target.The fixture is a historical artefact. When this test fails the answer is a
Migrate*onOpen's path, never an edit to the fixture — and the test says so, because that's the tempting wrong fix.Two migrations returned bare
Nine closed the handle and wrapped; two didn't. They were the two most recently added — because the way you write a migration here is to copy the one above it, and these two had been copied from each other.
The AST guard that now holds that shape found a third case the issue hadn't reported, and it turned out to be legitimate:
MigrateSchemaVersioncloses but doesn't wrap, because its own error already readsstamp schema version 1: …and amigrate:prefix would add nothing. It's recorded as an exemption with that reason rather than "fixed" into consistency it doesn't need.Warning rung, not Control — Go can't express "this call must be followed by those two statements". Control would need the migrations behind a runner that owns the handle; worth doing at the tenth, not the eleventh line of a fix.
Three comments had drifted off their calls
Including one attributing
MigrateSources's ordering constraint — "Last, because it reads settings and writes to destinations" — to a hooks migration inserted between the comment and its call.Both guards mutation-verified.
gofmt,go vet,internal/dbgreen.https://claude.ai/code/session_01A8N3W5ct9SZtHK9sCDD9cL