fix(db): open a real v0.6.0 database in the test suite, and stop copying broken migrations

internal/db/db.go

@@ -257,10 +257,13 @@ func Open(path string, opts ...Option) (*DB, error) {
 		sqldb.Close()
 		return nil, fmt.Errorf("migrate: %w", err)
 	}
-	// And once more for the rendition aspect-conversion columns.
+	// platform_accounts.scope_ver, so a token minted before a scope was added is
+	// re-consented rather than silently used with the narrower grant it has.
 	if err := d.MigratePlatformAccountScopeVer(); err != nil {
-		return nil, err
+		sqldb.Close()
+		return nil, fmt.Errorf("migrate: %w", err)
 	}
+	// And once more for the rendition aspect-conversion columns.
 	if err := d.MigrateRenditionAspect(); err != nil {
 		sqldb.Close()
 		return nil, fmt.Errorf("migrate: %w", err)
@@ -284,14 +287,17 @@ func Open(path string, opts ...Option) (*DB, error) {
 		sqldb.Close()
 		return nil, fmt.Errorf("migrate: %w", err)
 	}
+	// hooks.allow_private_target, so a hook aimed at a private address is
+	// refused at save time unless the operator opted in on that hook.
+	if err := d.MigrateHookAllowPrivateTarget(); err != nil {
+		sqldb.Close()
+		return nil, fmt.Errorf("migrate: %w", err)
+	}
 	// Last, because it reads settings and writes to destinations, renditions
 	// and recordings: every column those tables are going to have must already
 	// be there. It also creates the first source from the existing ingest
 	// configuration, which is what keeps an upgraded install reachable by the
 	// encoder that was already pointed at it.
-	if err := d.MigrateHookAllowPrivateTarget(); err != nil {
-		return nil, err
-	}
 	if err := d.MigrateSources(); err != nil {
 		sqldb.Close()
 		return nil, fmt.Errorf("migrate: %w", err)

internal/db/migration_call_shape_test.go

@@ -0,0 +1,131 @@
+package db
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"sort"
+	"strings"
+	"testing"
+)
+
+// Every Migrate* call in Open closes the handle, and wraps its error unless it
+// is recorded here as already carrying its own context.
+//
+// Nine of them did and two did not, and the two that did not were the two most
+// recently added -- because the way you write a migration here is to copy the
+// one above it, and both had been copied from each other rather than from the
+// nine. A leaked handle on a path that is about to exit is small; an
+// inconsistency that the next copy inherits is not.
+//
+// An AST check rather than a lint rule because there is no golangci-lint config
+// in this repo, and rather than a comment because a comment is what the two
+// wrong ones already had above them.
+//
+// Warning rung, not Control: Go cannot express "this call must be followed by
+// those two statements". Control would need the migrations behind a runner that
+// owns the handle -- worth doing when there is a tenth, not for the eleventh
+// line of a fix.
+func TestEveryMigrationInOpenClosesAndWraps(t *testing.T) {
+	fset := token.NewFileSet()
+	f, err := parser.ParseFile(fset, "db.go", nil, 0)
+	if err != nil {
+		t.Fatalf("parse db.go: %v", err)
+	}
+
+	var open *ast.FuncDecl
+	ast.Inspect(f, func(n ast.Node) bool {
+		fn, ok := n.(*ast.FuncDecl)
+		if ok && fn.Name.Name == "Open" && fn.Recv == nil {
+			open = fn
+		}
+		return true
+	})
+	if open == nil {
+		t.Fatal("no top-level Open in db.go; this test guards its migration block and has lost it")
+	}
+
+	var bare []string
+	ast.Inspect(open.Body, func(n ast.Node) bool {
+		ifs, ok := n.(*ast.IfStmt)
+		if !ok || ifs.Init == nil {
+			return true
+		}
+		name := migrateCallName(ifs.Init)
+		if name == "" {
+			return true
+		}
+		closes, wraps := false, false
+		ast.Inspect(ifs.Body, func(m ast.Node) bool {
+			if sel, ok := m.(*ast.SelectorExpr); ok && sel.Sel.Name == "Close" {
+				closes = true
+			}
+			if lit, ok := m.(*ast.BasicLit); ok && strings.Contains(lit.Value, "migrate: %w") {
+				wraps = true
+			}
+			return true
+		})
+		// CLOSING is the hard requirement: it is the actual resource leak, and
+		// it is what the two copied-from-each-other migrations were missing.
+		if !closes {
+			bare = append(bare, name+" (does not close the handle)")
+		}
+		// Wrapping is about the message an operator reads, so a migration whose
+		// own error already names the operation is allowed to skip it -- but it
+		// has to say so here, where the next reader will see it.
+		if !wraps && !wrapExempt[name] {
+			bare = append(bare, name+" (does not wrap with \"migrate: %w\")")
+		}
+		return true
+	})
+
+	// A scan that finds nothing agrees with any expectation at all.
+	if got := countMigrateCalls(open.Body); got < 5 {
+		t.Fatalf("found only %d Migrate* calls in Open; the scan has stopped "+
+			"matching and would pass however the block was written", got)
+	}
+
+	sort.Strings(bare)
+	if len(bare) > 0 {
+		t.Errorf("these migrations neither close the handle nor wrap their error:\n  %s\n\n"+
+			"Every other one does `sqldb.Close()` and `fmt.Errorf(\"migrate: %%w\", err)`. "+
+			"The next migration will be written by copying one of these, which is how "+
+			"the last two came to be wrong.", strings.Join(bare, "\n  "))
+	}
+}
+
+// wrapExempt records the migrations whose own error is already specific enough
+// that "migrate: " would only add a prefix. An entry here is a claim someone
+// checked; it is not a way to quiet the test.
+var wrapExempt = map[string]bool{
+	// Returns `stamp schema version %d: %w`, which already names what failed
+	// and at which version. See MigrateSchemaVersion.
+	"MigrateSchemaVersion": true,
+}
+
+func migrateCallName(init ast.Stmt) string {
+	as, ok := init.(*ast.AssignStmt)
+	if !ok || len(as.Rhs) != 1 {
+		return ""
+	}
+	call, ok := as.Rhs[0].(*ast.CallExpr)
+	if !ok {
+		return ""
+	}
+	sel, ok := call.Fun.(*ast.SelectorExpr)
+	if !ok || !strings.HasPrefix(sel.Sel.Name, "Migrate") {
+		return ""
+	}
+	return sel.Sel.Name
+}
+
+func countMigrateCalls(body *ast.BlockStmt) int {
+	n := 0
+	ast.Inspect(body, func(node ast.Node) bool {
+		if ifs, ok := node.(*ast.IfStmt); ok && ifs.Init != nil && migrateCallName(ifs.Init) != "" {
+			n++
+		}
+		return true
+	})
+	return n
+}

internal/db/previous_release_schema_test.go

@@ -0,0 +1,159 @@
+package db
+
+import (
+	"database/sql"
+	"os"
+	"path/filepath"
+	"sort"
+	"strings"
+	"testing"
+)
+
+// Opening a REAL previous-release database must converge on a fresh install.
+//
+// schema.sql is CREATE TABLE IF NOT EXISTS, so a column declared only there
+// reaches fresh installs and NEVER an upgrade. That asymmetry has caused data
+// loss in this repo before, and nothing in the test suite could see it:
+// dbtest's template is a fresh install, each migration test hand-builds only
+// the one 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 every upgrade was correct because four separate changes each remembered a
+// Migrate*, which is rung zero. This is the device: the previous release's real
+// schema.sql, checked in, opened through the ordinary Open path, and compared
+// object-for-object against a fresh install.
+//
+// WHEN THIS FAILS, READ IT AS "a schema.sql change has no migration". The fix
+// is a Migrate* on Open's path -- not an edit to the fixture, which is a
+// historical artefact and must never be updated to make this pass.
+//
+// AT THE NEXT RELEASE: re-point the fixture at the new previous release with
+//
+//	git show v0.7.0:internal/db/schema.sql > internal/db/testdata/schema-v0.7.0.sql
+//
+// and update prevRelease below. Leaving it on v0.6.0 keeps testing an
+// upgrade nobody performs any more.
+const prevRelease = "v0.6.0"
+
+func TestOpeningAPreviousReleaseDatabaseConvergesOnAFreshInstall(t *testing.T) {
+	fixture := filepath.Join("testdata", "schema-"+prevRelease+".sql")
+	raw, err := os.ReadFile(fixture)
+	if err != nil {
+		t.Fatalf("read %s: %v. This fixture IS the test -- without it nothing "+
+			"here opens a database built from a shipped schema.", fixture, err)
+	}
+
+	// The old install, built exactly as that release built it.
+	oldPath := filepath.Join(t.TempDir(), "old.db")
+	raw0, err := sql.Open("sqlite", oldPath)
+	if err != nil {
+		t.Fatalf("open %s: %v", prevRelease, err)
+	}
+	if _, err := raw0.Exec(string(raw)); err != nil {
+		raw0.Close()
+		t.Fatalf("apply %s schema: %v", prevRelease, err)
+	}
+	if err := raw0.Close(); err != nil {
+		t.Fatalf("close %s: %v", prevRelease, err)
+	}
+
+	// Upgrade it the only way an operator can: run the current binary at it.
+	upgraded, err := Open(oldPath)
+	if err != nil {
+		t.Fatalf("Open refused a %s database: %v. An operator upgrading from "+
+			"%s cannot start the server at all.", prevRelease, prevRelease, err)
+	}
+	defer upgraded.Close()
+
+	fresh, err := Open(filepath.Join(t.TempDir(), "fresh.db"))
+	if err != nil {
+		t.Fatalf("Open on a fresh install: %v", err)
+	}
+	defer fresh.Close()
+
+	up, fr := objectSet(t, upgraded), objectSet(t, fresh)
+
+	var missing, extra []string
+	for k := range fr {
+		if _, ok := up[k]; !ok {
+			missing = append(missing, k)
+		}
+	}
+	for k := range up {
+		if _, ok := fr[k]; !ok {
+			extra = append(extra, k)
+		}
+	}
+	sort.Strings(missing)
+	sort.Strings(extra)
+
+	if len(missing) > 0 {
+		t.Errorf("an upgraded %s database is MISSING what a fresh install has:\n  %s\n\n"+
+			"Each of these was added to schema.sql without a Migrate* on Open's path, so "+
+			"fresh installs have it and every existing one does not. Add the migration; "+
+			"do NOT edit the fixture.", prevRelease, strings.Join(missing, "\n  "))
+	}
+	if len(extra) > 0 {
+		t.Errorf("an upgraded %s database has objects a fresh install does not:\n  %s\n\n"+
+			"A migration created something schema.sql no longer declares, so the two "+
+			"populations have permanently diverged.", prevRelease, strings.Join(extra, "\n  "))
+	}
+}
+
+// objectSet is every table, index, trigger and view, plus each table's columns.
+// Names alone would miss the case that matters most: a table that exists in
+// both but is short a column on the upgraded side.
+func objectSet(t *testing.T, d *DB) map[string]bool {
+	t.Helper()
+	out := map[string]bool{}
+
+	rows, err := d.sql.Query(`SELECT type, name FROM sqlite_master
+		WHERE name NOT LIKE 'sqlite_%' ORDER BY type, name`)
+	if err != nil {
+		t.Fatalf("read sqlite_master: %v", err)
+	}
+	var tables []string
+	for rows.Next() {
+		var kind, name string
+		if err := rows.Scan(&kind, &name); err != nil {
+			rows.Close()
+			t.Fatalf("scan sqlite_master: %v", err)
+		}
+		out[kind+" "+name] = true
+		if kind == "table" {
+			tables = append(tables, name)
+		}
+	}
+	rows.Close()
+	if err := rows.Err(); err != nil {
+		t.Fatalf("sqlite_master: %v", err)
+	}
+
+	for _, tb := range tables {
+		// A literal, not a bind parameter: PRAGMA refuses them. The name comes
+		// from sqlite_master, not from a caller, so there is nothing to inject.
+		cols, err := d.sql.Query(`PRAGMA table_info(` + quoteIdent(tb) + `)`)
+		if err != nil {
+			t.Fatalf("table_info(%s): %v", tb, err)
+		}
+		for cols.Next() {
+			var cid int
+			var name, ctype string
+			var notnull, pk int
+			var dflt any
+			if err := cols.Scan(&cid, &name, &ctype, &notnull, &dflt, &pk); err != nil {
+				cols.Close()
+				t.Fatalf("scan table_info(%s): %v", tb, err)
+			}
+			out["column "+tb+"."+name] = true
+		}
+		cols.Close()
+		if err := cols.Err(); err != nil {
+			t.Fatalf("table_info(%s): %v", tb, err)
+		}
+	}
+	return out
+}
+
+func quoteIdent(s string) string { return `"` + strings.ReplaceAll(s, `"`, `""`) + `"` }