mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-23 02:53:31 +00:00
feat: warn at startup when shipped FTS triggers are missing (TASK-824) (#265)
* feat(store): warn at startup when shipped FTS triggers are missing (TASK-824) Defensive follow-up to BUG-822, where the documents_* triggers had silently drifted off some production DBs and search was broken until a user noticed. The migration runner had no notion that the triggers should exist — the only invariant was "this migration ran without erroring," which is too weak when SQLite's table-rebuild path can leave auxiliary objects in a different state than the migration intended. Add a hardcoded list of expected FTS5 triggers (one row per trigger, naming the table it's attached to) and a one-shot validateFTSInvariants step at the end of Store.migrate(). Each missing trigger emits a structured slog.Warn that points the operator at the recovery migration (046). Choices: - SQLite-only. Postgres uses tsvector update functions in pgmigrations with a different invariant model. - Logging-only, no auto-repair. Auto-creating triggers here would mask legitimate future removals and obscure the source of truth (the migrations directory). The recovery path is a targeted migration like 046_restore_documents_fts_triggers.sql. - Non-fatal. A missing trigger doesn't block startup; the operator may have intentionally removed one and just not updated the list yet, and we'd rather warn loudly than refuse to boot. Tests: - TestStartupInvariants_AllFTSTriggersExist — fresh DB has all 9 expected triggers (forward-looking guard against future migrations that break one). - TestStartupInvariants_LogsOnMissingTrigger — drop a trigger, run validator, capture slog records, assert a warning naming the missing trigger was emitted. Manual verification on the production DB: - Clean DB (after migration 046): no warnings on startup. - After manually `DROP TRIGGER documents_ai`: server logs `level=WARN msg="FTS trigger missing — ..." trigger=documents_ai table=documents` immediately on startup. * test(store): address Codex review on TASK-824 — bidirectional drift + Record.Clone Two LOW findings from Codex's first pass: 1. recordCapturingHandler.Handle stored slog.Record values without cloning. Records have internal shared state; the documented pattern for retaining them is r.Clone() first. Test passed today only because nothing mutated the record after Handle, but the helper was relying on slog internals. 2. TestStartupInvariants_AllFTSTriggersExist only proved every entry in expectedFTSTriggers exists. It didn't catch the inverse: a future migration adding a new FTS-style trigger on items/comments/documents without also adding it to expectedFTSTriggers, leaving the new trigger off the invariant check forever. Add TestExpectedFTSTriggers_MatchesActual which queries sqlite_master for every trigger on items/comments/documents and asserts each is in the expected list. A new trigger that isn't tracked fails this test with a clear "update the list in store.go" message. If a future trigger on these tables is legitimately not FTS-related, the test failure points the developer at this guard and they can either add it to expectedFTSTriggers or extend the exclusion.
This commit is contained in:
xarmian
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user