diff --git a/pkg/appview/db/drift.go b/pkg/appview/db/drift.go index ecbeb08..b945caf 100644 --- a/pkg/appview/db/drift.go +++ b/pkg/appview/db/drift.go @@ -22,7 +22,42 @@ type schemaSnapshot struct { Indexes []string } -var driftWhitespaceRun = regexp.MustCompile(`\s+`) +var ( + driftWhitespaceRun = regexp.MustCompile(`\s+`) + // Space that sits immediately before "(", ")" or "," carries no meaning in + // index DDL, and space immediately after "(" or "," carries none either. + // Space *after* ")" is deliberately left alone, since removing it would run + // a trailing clause into the column list. + driftSpaceBeforePunct = regexp.MustCompile(` ([(),])`) + driftSpaceAfterPunct = regexp.MustCompile(`([(,]) `) +) + +// normalizeIndexDDL reduces an index definition to a form that can be compared +// by string equality regardless of who emitted the text. +// +// This matters because the two sides of the comparison do not come from the same +// engine. referenceSnapshot applies schema.sql to a local libsql database, which +// stores the CREATE statement verbatim; a production database is an embedded +// replica whose writes go to the remote first, and the remote re-emits the DDL +// through its own parser. The two spellings differ only in spacing: +// +// schema.sql: CREATE INDEX idx_webhooks_user ON webhooks(user_did) +// replica: CREATE INDEX idx_webhooks_user ON webhooks (user_did) +// +// Collapsing whitespace *runs* cannot fix that, because the difference is a +// space that exists on one side and not the other. Left unhandled, every index +// was reported twice on every boot (missing, and unexpected), burying any real +// drift under the noise. +// +// Only the spacing is touched; identifiers, ordering (DESC) and uniqueness all +// still have to match exactly. +func normalizeIndexDDL(ddl string) string { + norm := driftWhitespaceRun.ReplaceAllString(strings.TrimSpace(ddl), " ") + norm = strings.ReplaceAll(norm, "IF NOT EXISTS ", "") + norm = driftSpaceBeforePunct.ReplaceAllString(norm, "$1") + norm = driftSpaceAfterPunct.ReplaceAllString(norm, "$1") + return norm +} // introspectSchema reads the structure of db. // @@ -129,9 +164,7 @@ func describeIndexes(db DBTX) ([]string, error) { 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) + out = append(out, normalizeIndexDDL(ddl)) } if err := rows.Err(); err != nil { return nil, err diff --git a/pkg/appview/db/schema_drift_test.go b/pkg/appview/db/schema_drift_test.go index 8beffdb..c027e51 100644 --- a/pkg/appview/db/schema_drift_test.go +++ b/pkg/appview/db/schema_drift_test.go @@ -130,6 +130,150 @@ func TestSchemaDriftDetectsMissingColumn(t *testing.T) { } } +// TestSchemaDriftIgnoresIndexSpacing covers the case that made the boot-time +// check useless in production. +// +// referenceSnapshot applies schema.sql to a LOCAL libsql database, which stores +// the CREATE statement byte for byte. Production runs an embedded replica: the +// write goes to the remote first and the remote re-emits the DDL through its own +// parser, which puts a space before the column list. Every index then failed the +// equality check from both directions at once, so all 38 were reported twice and +// the warning read "differences=76" on every single boot. Real drift would have +// been one line in seventy-seven. +// +// Nothing caught it because every test runs local-only, where both sides are +// produced by the same writer and therefore agree by accident. This test injects +// the remote's spelling by hand so the discrimination is actually exercised. +func TestSchemaDriftIgnoresIndexSpacing(t *testing.T) { + db := freshSchemaDB(t) + defer db.Close() + + // Each pair is (index to drop, spaced respelling of the identical index). + // The spellings on the right are what a remote-parsed database stores. + respell := []struct{ drop, create string }{ + {"idx_webhooks_user", `CREATE INDEX idx_webhooks_user ON webhooks (user_did)`}, + {"idx_manifests_did_repo", `CREATE INDEX idx_manifests_did_repo ON manifests (did, repository)`}, + {"idx_manifests_created_at", `CREATE INDEX idx_manifests_created_at ON manifests ( created_at DESC )`}, + {"idx_instance_leases_expires", `CREATE INDEX idx_instance_leases_expires ON instance_leases (expires_at)`}, + } + for _, r := range respell { + if _, err := db.Exec("DROP INDEX " + r.drop); err != nil { + t.Fatalf("drop %s: %v", r.drop, err) + } + if _, err := db.Exec(r.create); err != nil { + t.Fatalf("recreate %s: %v", r.drop, err) + } + } + + findings, err := SchemaDrift(db) + if err != nil { + t.Fatalf("SchemaDrift: %v", err) + } + if len(findings) != 0 { + t.Errorf("index spacing is not a schema difference, but SchemaDrift reported %d:\n %s", + len(findings), strings.Join(findings, "\n ")) + } +} + +// TestSchemaDriftDetectsChangedIndex is the other half of the pair above. +// Normalizing spacing must not blunt the check: an index that genuinely differs +// in its columns, its table or its uniqueness still has to be reported, in the +// spaced spelling as much as the unspaced one. +func TestSchemaDriftDetectsChangedIndex(t *testing.T) { + cases := []struct { + name string + drop string + create string + wantHit string + }{ + { + name: "different column", + drop: "idx_webhooks_user", + create: `CREATE INDEX idx_webhooks_user ON webhooks (url)`, + wantHit: "idx_webhooks_user", + }, + { + name: "extra column", + drop: "idx_manifests_did_repo", + create: `CREATE INDEX idx_manifests_did_repo ON manifests (did, repository, digest)`, + wantHit: "idx_manifests_did_repo", + }, + { + name: "uniqueness changed", + drop: "idx_stars_starrer", + create: `CREATE UNIQUE INDEX idx_stars_starrer ON stars (starrer_did)`, + wantHit: "idx_stars_starrer", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + db := freshSchemaDB(t) + defer db.Close() + + if _, err := db.Exec("DROP INDEX " + tc.drop); err != nil { + t.Fatalf("drop %s: %v", tc.drop, err) + } + if _, err := db.Exec(tc.create); err != nil { + t.Fatalf("create replacement index: %v", err) + } + + findings, err := SchemaDrift(db) + if err != nil { + t.Fatalf("SchemaDrift: %v", err) + } + if !anyContains(findings, tc.wantHit) { + t.Errorf("expected a finding naming %s, got: %v", tc.wantHit, findings) + } + }) + } +} + +// TestNormalizeIndexDDL pins the normalization itself, without a database in the +// way: spacing around punctuation is noise, everything else is signal. +func TestNormalizeIndexDDL(t *testing.T) { + same := [][]string{ + { + `CREATE INDEX idx_webhooks_user ON webhooks(user_did)`, + `CREATE INDEX idx_webhooks_user ON webhooks (user_did)`, + `CREATE INDEX IF NOT EXISTS idx_webhooks_user ON webhooks ( user_did )`, + "CREATE INDEX idx_webhooks_user\n ON webhooks\n (user_did)", + }, + { + `CREATE INDEX i ON t(a, b)`, + `CREATE INDEX i ON t(a,b)`, + `CREATE INDEX i ON t ( a , b )`, + }, + { + `CREATE INDEX i ON t(created_at DESC)`, + `CREATE INDEX i ON t (created_at DESC)`, + }, + } + for _, group := range same { + want := normalizeIndexDDL(group[0]) + for _, variant := range group[1:] { + if got := normalizeIndexDDL(variant); got != want { + t.Errorf("spelling difference survived normalization:\n %q -> %q\n %q -> %q", + group[0], want, variant, got) + } + } + } + + different := [][2]string{ + {`CREATE INDEX i ON t(a)`, `CREATE INDEX i ON t(b)`}, + {`CREATE INDEX i ON t(a)`, `CREATE INDEX i ON u(a)`}, + {`CREATE INDEX i ON t(a)`, `CREATE UNIQUE INDEX i ON t(a)`}, + {`CREATE INDEX i ON t(a, b)`, `CREATE INDEX i ON t(b, a)`}, + {`CREATE INDEX i ON t(a)`, `CREATE INDEX i ON t(a DESC)`}, + } + for _, pair := range different { + if normalizeIndexDDL(pair[0]) == normalizeIndexDDL(pair[1]) { + t.Errorf("normalization collapsed two genuinely different indexes: %q and %q", + pair[0], pair[1]) + } + } +} + // TestInitDBWarnsOnDriftAndStillBoots covers the integration rather than the // comparison: an existing database whose schema has drifted must still open. //