diff --git a/internal/store/items_test.go b/internal/store/items_test.go index 5173d7b4..698fe6ec 100644 --- a/internal/store/items_test.go +++ b/internal/store/items_test.go @@ -1,7 +1,9 @@ package store import ( + "context" "fmt" + "log/slog" "testing" "github.com/xarmian/pad/internal/models" @@ -1356,6 +1358,164 @@ func TestListDocuments_HyphenatedQuery(t *testing.T) { } } +// TestStartupInvariants_AllFTSTriggersExist asserts every trigger in the +// canonical expectedFTSTriggers list is present after migrations run on a +// fresh DB. This is a forward-looking guard — any future migration that +// inadvertently breaks one of these will fail this test, before it can +// silently drift on production DBs the way BUG-822 did. +func TestStartupInvariants_AllFTSTriggersExist(t *testing.T) { + s := testStore(t) + + if s.dialect.Driver() != DriverSQLite { + t.Skip("FTS triggers are SQLite-specific; Postgres uses a different model") + } + + for _, want := range expectedFTSTriggers { + var name string + err := s.db.QueryRow( + `SELECT name FROM sqlite_master WHERE type='trigger' AND tbl_name=? AND name=?`, + want.table, want.name, + ).Scan(&name) + if err != nil { + t.Errorf("expected trigger %q on table %q to exist after migrations, got error: %v", + want.name, want.table, err) + } + } +} + +// TestExpectedFTSTriggers_MatchesActual catches drift in the *opposite* +// direction from TestStartupInvariants_AllFTSTriggersExist: if a future +// migration adds a new trigger on items / comments / documents and the +// author forgets to add it to expectedFTSTriggers, the invariant check +// won't know to monitor it. This test compares the actual set of triggers +// on those tables against expectedFTSTriggers and fails if anything is +// missing from the list. +// +// If a new non-FTS trigger is legitimately added to one of these tables, +// either add it to expectedFTSTriggers (if it serves an FTS-like role) or +// extend the exclusion below. +func TestExpectedFTSTriggers_MatchesActual(t *testing.T) { + s := testStore(t) + + if s.dialect.Driver() != DriverSQLite { + t.Skip("FTS triggers are SQLite-specific") + } + + expected := map[string]bool{} + for _, e := range expectedFTSTriggers { + expected[e.table+"/"+e.name] = true + } + + rows, err := s.db.Query(` + SELECT name, tbl_name FROM sqlite_master + WHERE type='trigger' AND tbl_name IN ('items', 'comments', 'documents') + `) + if err != nil { + t.Fatalf("query triggers: %v", err) + } + defer rows.Close() + + for rows.Next() { + var name, table string + if err := rows.Scan(&name, &table); err != nil { + t.Fatalf("scan: %v", err) + } + key := table + "/" + name + if !expected[key] { + t.Errorf("found trigger %q on table %q that's not in expectedFTSTriggers — "+ + "update the list in store.go (or exclude in this test if it's not an FTS-style trigger)", + name, table) + } + } +} + +// TestStartupInvariants_LogsOnMissingTrigger verifies the alarm actually +// fires: drop a trigger, run validateFTSInvariants, capture the slog +// output, assert a warning was emitted naming the missing trigger. +// +// Without this test, the validator could regress (e.g. a typo in the +// SELECT, a dialect check that always returns early) and the BUG-822 +// class of drift would go undetected again. +func TestStartupInvariants_LogsOnMissingTrigger(t *testing.T) { + s := testStore(t) + + if s.dialect.Driver() != DriverSQLite { + t.Skip("FTS trigger invariant check runs only on SQLite") + } + + // Drop a known trigger to simulate the BUG-822 broken state. + target := "documents_ai" + if _, err := s.db.Exec("DROP TRIGGER " + target); err != nil { + t.Fatalf("DROP TRIGGER %s: %v", target, err) + } + + // Capture slog output via a custom handler that records records into + // a slice we can inspect after. + var captured []slog.Record + prev := slog.Default() + t.Cleanup(func() { slog.SetDefault(prev) }) + slog.SetDefault(slog.New(&recordCapturingHandler{records: &captured})) + + s.validateFTSInvariants() + + // Look for a warning record that mentions the trigger name we dropped. + found := false + for _, r := range captured { + if r.Level != slog.LevelWarn { + continue + } + mentioned := false + r.Attrs(func(a slog.Attr) bool { + if a.Key == "trigger" && a.Value.String() == target { + mentioned = true + return false + } + return true + }) + if mentioned { + found = true + break + } + } + if !found { + t.Errorf("expected a slog.Warn record naming trigger=%q after dropping it, got %d records", target, len(captured)) + for _, r := range captured { + t.Logf(" %s: %s", r.Level, r.Message) + } + } +} + +// recordCapturingHandler is a minimal slog.Handler used by +// TestStartupInvariants_LogsOnMissingTrigger to capture records without +// emitting them to stderr. Not safe for concurrent use; tests are +// single-threaded. +type recordCapturingHandler struct { + records *[]slog.Record + attrs []slog.Attr +} + +func (h *recordCapturingHandler) Enabled(_ context.Context, _ slog.Level) bool { + return true +} +func (h *recordCapturingHandler) Handle(_ context.Context, r slog.Record) error { + // slog.Record has internal shared state; clone before retaining so we + // don't depend on the caller refraining from mutating it after Handle. + r = r.Clone() + for _, a := range h.attrs { + r.AddAttrs(a) + } + *h.records = append(*h.records, r) + return nil +} +func (h *recordCapturingHandler) WithAttrs(attrs []slog.Attr) slog.Handler { + clone := *h + clone.attrs = append(append([]slog.Attr{}, h.attrs...), attrs...) + return &clone +} +func (h *recordCapturingHandler) WithGroup(_ string) slog.Handler { + return h +} + // TestMigration046_DocumentsFTSTriggersExist verifies that after all // migrations run on a fresh DB, the three documents_* triggers are present. // This regression-protects BUG-822 — production DBs ended up missing these diff --git a/internal/store/store.go b/internal/store/store.go index 66932bcc..bc03b345 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -5,6 +5,7 @@ import ( "embed" "fmt" "io/fs" + "log/slog" "sort" "strings" "time" @@ -229,9 +230,84 @@ func (s *Store) migrate() error { } } + // Defensive: log a warning for any FTS trigger that should exist but + // doesn't. Catches the BUG-822 class of regression — a table-rebuild + // migration that recorded as applied but silently failed to recreate + // its triggers, leaving search broken until someone notices. + s.validateFTSInvariants() + return nil } +// expectedFTSTriggers is the canonical list of FTS5 triggers that ship with +// the SQLite migrations. Each entry is (trigger name, table the trigger is +// attached to). The migrations file at the right of each row defines them: +// +// - items_fts_* — internal/store/migrations/001_initial.sql +// - comments_fts_* — internal/store/migrations/007_comments.sql +// - documents_* — internal/store/migrations/001_initial.sql, +// restored by 046 after BUG-822 drift. +// +// If a future migration intentionally renames or removes any of these, +// update this list in the same commit. +var expectedFTSTriggers = []struct { + name string + table string +}{ + {"items_fts_insert", "items"}, + {"items_fts_update", "items"}, + {"items_fts_delete", "items"}, + {"comments_fts_insert", "comments"}, + {"comments_fts_update", "comments"}, + {"comments_fts_delete", "comments"}, + {"documents_ai", "documents"}, + {"documents_au", "documents"}, + {"documents_ad", "documents"}, +} + +// validateFTSInvariants checks that every trigger in expectedFTSTriggers +// exists. Missing triggers are logged as structured warnings; this function +// is intentionally non-fatal and non-repairing. +// +// Non-fatal: a missing trigger doesn't prevent server startup — the +// operator may have legitimately removed one and just not updated this +// list yet, and we'd rather warn loudly than refuse to boot. +// +// Non-repairing: auto-creating triggers here would mask future legitimate +// removals and obscure the source of truth (the migrations directory). +// The recovery path is a targeted migration like 046_restore_documents_fts_triggers.sql. +// +// SQLite-only: Postgres uses a different trigger model and has its own +// search_vector update functions in pgmigrations. +func (s *Store) validateFTSInvariants() { + if s.dialect.Driver() != DriverSQLite { + return + } + for _, t := range expectedFTSTriggers { + var name string + err := s.db.QueryRow( + `SELECT name FROM sqlite_master WHERE type='trigger' AND tbl_name=? AND name=?`, + t.table, t.name, + ).Scan(&name) + if err == sql.ErrNoRows { + slog.Warn( + "FTS trigger missing — search may silently miss new rows; consider a restoration migration like 046_restore_documents_fts_triggers.sql", + "trigger", t.name, + "table", t.table, + ) + continue + } + if err != nil { + slog.Warn( + "FTS trigger check errored", + "trigger", t.name, + "table", t.table, + "err", err, + ) + } + } +} + // migratePostgres applies PostgreSQL migrations. // PostgreSQL supports multi-statement execution natively, so we don't need execMulti. func (s *Store) migratePostgres() error {