diff --git a/pkg/appview/db/drift.go b/pkg/appview/db/drift.go new file mode 100644 index 0000000..ecbeb08 --- /dev/null +++ b/pkg/appview/db/drift.go @@ -0,0 +1,246 @@ +package db + +import ( + "database/sql" + "fmt" + "regexp" + "slices" + "sort" + "strings" +) + +// schemaSnapshot is a structural description of a database: the column set of +// every user table, plus the explicitly-created indexes. +// +// Columns are held as sorted descriptor strings rather than as ordered lists, +// because ordinal position legitimately differs between a fresh install and an +// upgraded one. Migrations append with ALTER TABLE ADD COLUMN; schema.sql places +// the same column mid-table. Comparing as a set is what makes the two +// comparable at all. +type schemaSnapshot struct { + Tables map[string][]string + Indexes []string +} + +var driftWhitespaceRun = regexp.MustCompile(`\s+`) + +// introspectSchema reads the structure of db. +// +// SQLite internal tables (sqlite_sequence and friends, created implicitly by +// AUTOINCREMENT) are skipped: they are an artifact of the engine rather than +// something either schema.sql or a migration declares. +func introspectSchema(db DBTX) (schemaSnapshot, error) { + snap := schemaSnapshot{Tables: make(map[string][]string)} + + tables, err := listUserTables(db) + if err != nil { + return snap, err + } + for _, table := range tables { + cols, err := describeColumns(db, table) + if err != nil { + return snap, err + } + snap.Tables[table] = cols + } + + snap.Indexes, err = describeIndexes(db) + if err != nil { + return snap, err + } + return snap, nil +} + +func listUserTables(db DBTX) ([]string, error) { + rows, err := db.Query(` + SELECT name FROM sqlite_master + WHERE type = 'table' AND name NOT LIKE 'sqlite_%' + ORDER BY name + `) + if err != nil { + return nil, fmt.Errorf("list tables: %w", err) + } + defer rows.Close() + + var names []string + for rows.Next() { + var n string + if err := rows.Scan(&n); err != nil { + return nil, fmt.Errorf("scan table name: %w", err) + } + names = append(names, n) + } + return names, rows.Err() +} + +// describeColumns returns one sorted descriptor per column: name, declared +// type, not-null, default and primary-key position. Ordinal position is +// deliberately excluded; see schemaSnapshot. +func describeColumns(db DBTX, table string) ([]string, error) { + rows, err := db.Query(fmt.Sprintf("PRAGMA table_info(%q)", table)) + if err != nil { + return nil, fmt.Errorf("table_info(%s): %w", table, err) + } + defer rows.Close() + + var cols []string + for rows.Next() { + var ( + cid int + name string + colType string + notNull int + dfltValue sql.NullString + pk int + ) + if err := rows.Scan(&cid, &name, &colType, ¬Null, &dfltValue, &pk); err != nil { + return nil, fmt.Errorf("scan table_info(%s): %w", table, err) + } + dflt := "" + if dfltValue.Valid { + dflt = dfltValue.String + } + cols = append(cols, fmt.Sprintf("%s type=%s notnull=%d default=%s pk=%d", + name, strings.ToUpper(colType), notNull, dflt, pk)) + } + if err := rows.Err(); err != nil { + return nil, err + } + sort.Strings(cols) + return cols, nil +} + +// describeIndexes returns normalized DDL for the explicitly-created indexes. +// Indexes SQLite creates implicitly for UNIQUE and PRIMARY KEY constraints have +// a NULL sql column and are skipped, since the column set already implies them. +func describeIndexes(db DBTX) ([]string, error) { + rows, err := db.Query(` + SELECT sql FROM sqlite_master + WHERE type = 'index' AND sql IS NOT NULL + `) + if err != nil { + return nil, fmt.Errorf("list indexes: %w", err) + } + defer rows.Close() + + var out []string + for rows.Next() { + var ddl string + if err := rows.Scan(&ddl); err != nil { + return nil, fmt.Errorf("scan index sql: %w", err) + } + norm := driftWhitespaceRun.ReplaceAllString(strings.TrimSpace(ddl), " ") + norm = strings.ReplaceAll(norm, "IF NOT EXISTS ", "") + out = append(out, norm) + } + if err := rows.Err(); err != nil { + return nil, err + } + sort.Strings(out) + return out, nil +} + +// referenceSnapshot builds the structure schema.sql describes, by applying it to +// a throwaway in-memory database and introspecting that. +// +// Introspecting a real database beats parsing the DDL: SQLite's own resolution +// of types, defaults and implicit indexes is the thing we want to compare +// against, and reimplementing it in a parser would drift from the engine. +func referenceSnapshot() (schemaSnapshot, error) { + connector, err := openLibsqlLocalConnector(":memory:") + if err != nil { + return schemaSnapshot{}, fmt.Errorf("open reference connector: %w", err) + } + ref := sql.OpenDB(connector) + // One connection, so every statement lands in the same in-memory database. + ref.SetMaxOpenConns(1) + defer ref.Close() + + for i, stmt := range splitSQLStatements(schemaSQL) { + if _, err := ref.Exec(stmt); err != nil { + return schemaSnapshot{}, fmt.Errorf("apply schema.sql statement %d: %w", i+1, err) + } + } + return introspectSchema(ref) +} + +// SchemaDrift compares a live database against what schema.sql describes and +// returns one human-readable line per difference. An empty result means they +// agree. +// +// This exists because migrations can be recorded without being executed. That is +// not hypothetical: migration 0009 had to clean up after 0004, which production +// recorded but never applied, leaving eleven columns behind that fresh installs +// did not have. Nothing reported that at the time; it surfaced later as +// confusing behavior. A check at boot turns that class of problem into a log +// line at deploy time. +// +// Callers should warn on the result and continue. Drift is a signal to write a +// corrective migration, not a reason to refuse to serve traffic: a database that +// is merely ahead of or behind schema.sql is usually still perfectly able to run +// the application, and failing the boot would turn a cosmetic diff into an +// outage. +func SchemaDrift(live DBTX) ([]string, error) { + want, err := referenceSnapshot() + if err != nil { + return nil, err + } + got, err := introspectSchema(live) + if err != nil { + return nil, fmt.Errorf("introspect live database: %w", err) + } + + var findings []string + + wantTables := sortedKeys(want.Tables) + gotTables := sortedKeys(got.Tables) + + for _, table := range wantTables { + if !slices.Contains(gotTables, table) { + findings = append(findings, fmt.Sprintf( + "table %q is in schema.sql but missing from the database (a migration to create it is probably missing)", table)) + continue + } + for _, col := range want.Tables[table] { + if !slices.Contains(got.Tables[table], col) { + findings = append(findings, fmt.Sprintf( + "table %q: schema.sql expects column [%s] but the database does not have it", table, col)) + } + } + for _, col := range got.Tables[table] { + if !slices.Contains(want.Tables[table], col) { + findings = append(findings, fmt.Sprintf( + "table %q: database has column [%s] which schema.sql does not declare", table, col)) + } + } + } + + for _, table := range gotTables { + if !slices.Contains(wantTables, table) { + findings = append(findings, fmt.Sprintf( + "table %q exists in the database but is not declared in schema.sql", table)) + } + } + + for _, idx := range want.Indexes { + if !slices.Contains(got.Indexes, idx) { + findings = append(findings, fmt.Sprintf("index missing from the database: %s", idx)) + } + } + for _, idx := range got.Indexes { + if !slices.Contains(want.Indexes, idx) { + findings = append(findings, fmt.Sprintf("index in the database but not in schema.sql: %s", idx)) + } + } + + return findings, nil +} + +func sortedKeys(m map[string][]string) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} diff --git a/pkg/appview/db/migrations/README.md b/pkg/appview/db/migrations/README.md index b356812..38dcca5 100644 --- a/pkg/appview/db/migrations/README.md +++ b/pkg/appview/db/migrations/README.md @@ -6,13 +6,43 @@ This directory contains database migrations for the ATCR AppView database. **`schema.sql`** (in parent directory) contains the **complete base schema** for fresh database installations. It includes all tables, indexes, and constraints. -**Migrations** (this directory) handle **changes to existing databases**. They are only for: +**Migrations** (this directory) handle **changes to existing databases**: +- `CREATE TABLE` statements (see below — new tables need a migration too) - `ALTER TABLE` statements (add/modify/drop columns) - `UPDATE` statements (data transformations) - `DELETE` statements (data cleanup) - Creating/modifying indexes on existing tables -**NEW TABLES go in `schema.sql`, NOT in migrations.** +### New tables go in BOTH places + +`InitDB` skips `schema.sql` entirely once `schema_migrations` has any rows (see +`hasAppliedMigrations` in `schema.go`). **A table added only to `schema.sql` will +never be created on an existing database** — it appears on fresh installs, works +in every test, and is silently absent in production. + +So a new table needs two changes: +1. `schema.sql`, for fresh installs. +2. A migration with `CREATE TABLE IF NOT EXISTS`, for existing databases. + +`TestSchemaMatchesMigrations` enforces this. It builds the schema both ways and +fails if they disagree, so forgetting either half is caught before it ships. + +### Two more rules the test cannot enforce for you + +**Migrations must not return rows.** go-libsql rejects a row-returning statement +passed to `Exec` with `Execute returned rows`. A migration that opens with a bare +`SELECT` fails on every database that has not already recorded it. (Migration +0001 does exactly this; it survives only because every real database recorded it +years ago.) + +**Rebuild migrations must name their columns.** Column order in `schema.sql` is +illustrative, not authoritative: migrations append with `ADD COLUMN` while +`schema.sql` places the same column mid-table, so a fresh database and an +upgraded one legitimately differ in column order on `manifests`, `users`, +`devices` and `repo_pages`. `INSERT INTO new_table SELECT * FROM old_table` will +therefore silently write the wrong values into the wrong columns. Always write +`INSERT INTO new_table (a, b, c) SELECT a, b, c FROM old_table`, as migrations +0009 and 0011 do. ## Migration Format @@ -101,7 +131,9 @@ query: | - **Never modify existing migrations** - Once applied, they're immutable - **Test migrations** before committing - Ensure they work on existing databases -- **Version numbers must be unique** - The migration system will fail if duplicates exist +- **Version numbers must be unique** - The migration system silently skips a duplicate, so the second file never runs (see `TestMigrationVersionsAreUnique`) - **Migrations run automatically** on `InitDB()` - Schema first, then migrations -- **CRITICAL: Update `schema.sql` for structural changes** - When you ALTER a table or add columns, update both the migration AND `schema.sql` so fresh installations have the same structure -- **New tables go in `schema.sql` only** - Don't create migration files for new tables +- **CRITICAL: Update `schema.sql` for every structural change** - Columns, tables and indexes all need both the migration AND the `schema.sql` entry, or fresh and existing databases diverge. `TestSchemaMatchesMigrations` fails the build if they do +- **Migrations must not return rows** - a bare `SELECT` fails under go-libsql with `Execute returned rows` +- **Rebuild migrations must name columns explicitly** - never `INSERT INTO new SELECT * FROM old`; column order differs between fresh and upgraded databases +- **Drift is reported at boot** - `InitDB` logs a warning for any difference between an existing database and `schema.sql`. It never fails the boot; treat the warning as a request for a corrective migration diff --git a/pkg/appview/db/schema.go b/pkg/appview/db/schema.go index 51edf7e..417de94 100644 --- a/pkg/appview/db/schema.go +++ b/pkg/appview/db/schema.go @@ -125,9 +125,40 @@ func InitDB(path string, cfg LibsqlConfig) (*sql.DB, error) { return nil, err } + // Report any structural drift from schema.sql. Only meaningful for existing + // databases: a fresh one was just built from schema.sql, so it agrees by + // construction. Warn and continue — see SchemaDrift for why this never + // fails the boot. + if isExisting { + reportSchemaDrift(db) + } + return db, nil } +// reportSchemaDrift logs any difference between the live database and +// schema.sql. Migrations can be recorded without being executed (0004 was, on +// production, which is the whole reason migration 0009 exists), and until now +// nothing surfaced that. Failure to run the check is itself only a warning: an +// introspection problem must not stop the server from starting. +func reportSchemaDrift(db *sql.DB) { + findings, err := SchemaDrift(db) + if err != nil { + slog.Warn("Could not check database schema for drift", "error", err) + return + } + if len(findings) == 0 { + slog.Debug("Database schema matches schema.sql") + return + } + slog.Warn("Database schema differs from schema.sql", + "differences", len(findings), + "hint", "a migration may have been recorded without being executed; a corrective migration is probably needed") + for _, f := range findings { + slog.Warn("Schema drift", "detail", f) + } +} + // hasAppliedMigrations checks if this is an existing database with migrations applied func hasAppliedMigrations(db *sql.DB) (bool, error) { // Check if schema_migrations table exists diff --git a/pkg/appview/db/schema_drift_test.go b/pkg/appview/db/schema_drift_test.go index 3c7a0c8..8beffdb 100644 --- a/pkg/appview/db/schema_drift_test.go +++ b/pkg/appview/db/schema_drift_test.go @@ -3,10 +3,10 @@ package db import ( "database/sql" "fmt" + "log/slog" "os" - "regexp" + "path/filepath" "slices" - "sort" "strings" "testing" ) @@ -40,8 +40,17 @@ func TestSchemaMatchesMigrations(t *testing.T) { migrated := migratedSchemaDB(t) defer migrated.Close() - freshTables := tableNames(t, fresh) - migratedTables := tableNames(t, migrated) + freshSnap, err := introspectSchema(fresh) + if err != nil { + t.Fatalf("introspect fresh database: %v", err) + } + migratedSnap, err := introspectSchema(migrated) + if err != nil { + t.Fatalf("introspect migrated database: %v", err) + } + + freshTables := sortedKeys(freshSnap.Tables) + migratedTables := sortedKeys(migratedSnap.Tables) if diff := diffStringSets(freshTables, migratedTables); diff != "" { t.Errorf("table sets differ between schema.sql and the migrations:\n%s\n\n"+ @@ -51,23 +60,118 @@ func TestSchemaMatchesMigrations(t *testing.T) { } for _, table := range freshTables { - if !contains(migratedTables, table) { + if !slices.Contains(migratedTables, table) { continue // already reported above } - freshCols := columnSet(t, fresh, table) - migratedCols := columnSet(t, migrated, table) - if diff := diffStringSets(freshCols, migratedCols); diff != "" { + if diff := diffStringSets(freshSnap.Tables[table], migratedSnap.Tables[table]); diff != "" { t.Errorf("table %q differs between schema.sql and the migrations:\n%s", table, diff) } } - freshIdx := indexSet(t, fresh) - migratedIdx := indexSet(t, migrated) - if diff := diffStringSets(freshIdx, migratedIdx); diff != "" { + if diff := diffStringSets(freshSnap.Indexes, migratedSnap.Indexes); diff != "" { t.Errorf("index sets differ between schema.sql and the migrations:\n%s", diff) } } +// TestSchemaDriftCleanOnFreshDatabase pins the other end of the same invariant: +// SchemaDrift, which runs at boot against real databases, must report nothing +// for a database that was just built from schema.sql. If this ever fails, the +// startup check has become noisy and operators will learn to ignore it, which +// costs more than not having it at all. +func TestSchemaDriftCleanOnFreshDatabase(t *testing.T) { + db := freshSchemaDB(t) + defer db.Close() + + findings, err := SchemaDrift(db) + if err != nil { + t.Fatalf("SchemaDrift: %v", err) + } + if len(findings) != 0 { + t.Errorf("expected no drift against a database built from schema.sql, got %d:\n %s", + len(findings), strings.Join(findings, "\n ")) + } +} + +// TestSchemaDriftDetectsMissingColumn proves SchemaDrift actually compares +// rather than always returning clean. A database missing a column schema.sql +// declares is exactly the shape of the 0004 incident. +func TestSchemaDriftDetectsMissingColumn(t *testing.T) { + db := freshSchemaDB(t) + defer db.Close() + + // Rebuild users without registry_domain, imitating a migration that was + // recorded but never executed. + stmts := []string{ + `CREATE TABLE users_drifted ( + did TEXT PRIMARY KEY, + handle TEXT NOT NULL, + pds_endpoint TEXT NOT NULL, + avatar TEXT, + default_hold_did TEXT, + oci_client TEXT DEFAULT '', + last_seen TIMESTAMP NOT NULL, + UNIQUE(handle) + )`, + `DROP TABLE users`, + `ALTER TABLE users_drifted RENAME TO users`, + } + for _, s := range stmts { + if _, err := db.Exec(s); err != nil { + t.Fatalf("set up drifted schema: %v", err) + } + } + + findings, err := SchemaDrift(db) + if err != nil { + t.Fatalf("SchemaDrift: %v", err) + } + if !anyContains(findings, "registry_domain") { + t.Errorf("expected a finding naming the missing registry_domain column, got: %v", findings) + } +} + +// TestInitDBWarnsOnDriftAndStillBoots covers the integration rather than the +// comparison: an existing database whose schema has drifted must still open. +// +// The check is warn-only on purpose. A database that is merely ahead of or +// behind schema.sql is almost always still able to serve traffic, so refusing to +// boot would convert a diff that wants a corrective migration into an outage, +// and it would do so during a deploy, which is the worst possible moment. +func TestInitDBWarnsOnDriftAndStillBoots(t *testing.T) { + path := filepath.Join(t.TempDir(), "app.db") + + first, err := InitDB(path, LibsqlConfig{}) + if err != nil { + t.Fatalf("first InitDB: %v", err) + } + // Imitate a migration that was recorded but never executed. + if _, err := first.Exec(`ALTER TABLE users DROP COLUMN registry_domain`); err != nil { + t.Fatalf("drop column: %v", err) + } + first.Close() + + var logged strings.Builder + restore := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logged, &slog.HandlerOptions{Level: slog.LevelWarn}))) + defer slog.SetDefault(restore) + + // The second open takes the existing-database path, which is the only one + // that runs the drift check. + second, err := InitDB(path, LibsqlConfig{}) + if err != nil { + t.Fatalf("second InitDB must succeed despite drift, got: %v", err) + } + defer second.Close() + + out := logged.String() + if !strings.Contains(out, "Schema drift") { + t.Errorf("expected a schema drift warning at startup, got log output:\n%s", out) + } + if !strings.Contains(out, "registry_domain") { + t.Errorf("expected the warning to name the missing column, got log output:\n%s", out) + } +} + // freshSchemaDB returns a database built the way a fresh install builds one: // schema.sql applied directly, migrations recorded but not executed. func freshSchemaDB(t *testing.T) *sql.DB { @@ -110,8 +214,8 @@ func migratedSchemaDB(t *testing.T) *sql.DB { } // freshDB=false so the DDL actually executes rather than merely being - // recorded. base_schema.sql creates schema_migrations empty, so every - // migration is pending. + // recorded. base_schema.sql records versions 1-8 as already applied, so + // everything from 0009 onward is pending. if err := runMigrations(db, false); err != nil { db.Close() t.Fatalf("runMigrations against the base snapshot: %v", err) @@ -119,132 +223,28 @@ func migratedSchemaDB(t *testing.T) *sql.DB { return db } -// tableNames returns the user tables in db, sorted. SQLite internal tables -// (sqlite_sequence and friends, created implicitly by AUTOINCREMENT) are -// excluded since they are an artifact of the engine, not of our schema. -func tableNames(t *testing.T, db *sql.DB) []string { - t.Helper() - rows, err := db.Query(` - SELECT name FROM sqlite_master - WHERE type = 'table' AND name NOT LIKE 'sqlite_%' - ORDER BY name - `) - if err != nil { - t.Fatalf("list tables: %v", err) - } - defer rows.Close() - - var names []string - for rows.Next() { - var n string - if err := rows.Scan(&n); err != nil { - t.Fatalf("scan table name: %v", err) - } - names = append(names, n) - } - if err := rows.Err(); err != nil { - t.Fatalf("iterate tables: %v", err) - } - return names -} - -// columnSet returns one descriptor per column: name, type, not-null, default and -// primary-key position. Ordinal position (cid) is deliberately excluded. -func columnSet(t *testing.T, db *sql.DB, table string) []string { - t.Helper() - rows, err := db.Query(fmt.Sprintf("PRAGMA table_info(%q)", table)) - if err != nil { - t.Fatalf("table_info(%s): %v", table, err) - } - defer rows.Close() - - var cols []string - for rows.Next() { - var ( - cid int - name string - colType string - notNull int - dfltValue sql.NullString - pk int - ) - if err := rows.Scan(&cid, &name, &colType, ¬Null, &dfltValue, &pk); err != nil { - t.Fatalf("scan table_info(%s): %v", table, err) - } - dflt := "" - if dfltValue.Valid { - dflt = dfltValue.String - } - cols = append(cols, fmt.Sprintf("%s type=%s notnull=%d default=%s pk=%d", - name, strings.ToUpper(colType), notNull, dflt, pk)) - } - if err := rows.Err(); err != nil { - t.Fatalf("iterate table_info(%s): %v", table, err) - } - sort.Strings(cols) - return cols -} - -var whitespaceRun = regexp.MustCompile(`\s+`) - -// indexSet returns the explicitly-created indexes as normalized DDL. Indexes -// SQLite creates implicitly for UNIQUE and PRIMARY KEY constraints have a NULL -// sql column and are skipped: they are already implied by the column set. -func indexSet(t *testing.T, db *sql.DB) []string { - t.Helper() - rows, err := db.Query(` - SELECT sql FROM sqlite_master - WHERE type = 'index' AND sql IS NOT NULL - `) - if err != nil { - t.Fatalf("list indexes: %v", err) - } - defer rows.Close() - - var out []string - for rows.Next() { - var ddl string - if err := rows.Scan(&ddl); err != nil { - t.Fatalf("scan index sql: %v", err) - } - norm := whitespaceRun.ReplaceAllString(strings.TrimSpace(ddl), " ") - norm = strings.ReplaceAll(norm, "IF NOT EXISTS ", "") - out = append(out, norm) - } - if err := rows.Err(); err != nil { - t.Fatalf("iterate indexes: %v", err) - } - sort.Strings(out) - return out -} - // diffStringSets returns a human-readable diff, or "" when the sets match. // "schema.sql" is the a side, "migrations" the b side. func diffStringSets(a, b []string) string { - var onlyA, onlyB []string + var sb strings.Builder for _, s := range a { - if !contains(b, s) { - onlyA = append(onlyA, s) + if !slices.Contains(b, s) { + fmt.Fprintf(&sb, " only in schema.sql: %s\n", s) } } for _, s := range b { - if !contains(a, s) { - onlyB = append(onlyB, s) + if !slices.Contains(a, s) { + fmt.Fprintf(&sb, " only in migrations: %s\n", s) } } - if len(onlyA) == 0 && len(onlyB) == 0 { - return "" - } - var sb strings.Builder - for _, s := range onlyA { - fmt.Fprintf(&sb, " only in schema.sql: %s\n", s) - } - for _, s := range onlyB { - fmt.Fprintf(&sb, " only in migrations: %s\n", s) - } return strings.TrimRight(sb.String(), "\n") } -func contains(haystack []string, needle string) bool { - return slices.Contains(haystack, needle) +func anyContains(haystack []string, needle string) bool { + for _, s := range haystack { + if strings.Contains(s, needle) { + return true + } + } + return false }