From 129ad80f14ee66abfc989d34e0900590e97ff290 Mon Sep 17 00:00:00 2001 From: mithral Date: Mon, 12 Oct 2026 00:06:01 -0400 Subject: [PATCH] Guard the storage seam against erosion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 built the seam; nothing stopped it being undone one convenient import at a time. Two rules, both proven able to fail. TestDatabaseSQLImportersAreDeclared freezes the blast radius of database/sql. Twenty packages import it today and that set is now explicit. A new package that needs its own SQL must be added deliberately, with a reason. The rule also fails in the other direction — if a listed package stops importing database/sql, the list must shrink, so it cannot rot into a meaningless allowlist. TestThreadedConsumersStayOffSQL is the concrete regression guard: internal/ playback was moved onto catalog.Store, and this fails if it reaches for *sql.DB again. Both checks read non-test files only, so a package that needs database/sql purely to build a test database is not forced onto the list. Proven by breaking each on purpose: - dropping internal/media from the list -> "imports database/sql but is not in sqlImporters" - adding internal/natsort (no SQL) -> "is listed ... but no longer imports database/sql" - re-adding database/sql to playback -> "undoes the seam" All restored afterwards and the files verified unchanged. Seven arch rules now, each able to fail. gofmt clean, build 0, vet 0, 25 packages pass. --- internal/arch/arch_test.go | 119 +++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/internal/arch/arch_test.go b/internal/arch/arch_test.go index 845858c..8496b60 100644 --- a/internal/arch/arch_test.go +++ b/internal/arch/arch_test.go @@ -239,3 +239,122 @@ func hasGoFiles(dir string) bool { } return false } + +// allImportsOf returns every import of a package's non-test files, including +// standard library ones. importsOf only reports internal packages, which is +// right for the layering rules and wrong for asking "does this package know +// about SQL". +func allImportsOf(t *testing.T, dir string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read %s: %v", dir, err) + } + seen := map[string]bool{} + for _, e := range entries { + if e.IsDir() || !strings.HasSuffix(e.Name(), ".go") || strings.HasSuffix(e.Name(), "_test.go") { + continue + } + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, filepath.Join(dir, e.Name()), nil, parser.ImportsOnly) + if err != nil { + t.Fatalf("parse %s: %v", e.Name(), err) + } + for _, imp := range f.Imports { + seen[strings.Trim(imp.Path.Value, `"`)] = true + } + } + out := make([]string, 0, len(seen)) + for p := range seen { + out = append(out, p) + } + sort.Strings(out) + return out +} + +// sqlImporters are the packages permitted to import database/sql in their +// non-test files. +// +// This list is the guard against the storage seam eroding by drift. The seam +// exists so the engine is swappable; it only holds if the set of packages that +// know about *sql.DB does not quietly grow. Adding a package here is a +// deliberate act with a reason, not a side effect of a feature. +// +// A package that only needs database/sql to build a TEST database does not +// belong here — the check ignores _test.go files on purpose. +var sqlImporters = map[string]bool{ + "internal/apitoken": true, + "internal/auth": true, + "internal/catalog": true, + "internal/cli": true, + "internal/core": true, + "internal/home": true, + "internal/httpapi": true, + "internal/jellyfin": true, + "internal/jobs": true, + "internal/library": true, + "internal/media": true, + "internal/metadata": true, + "internal/modules/comics": true, + "internal/modules/ebooks": true, + "internal/modules/games": true, + "internal/modules/photos": true, + "internal/progress": true, + "internal/scanner": true, + "internal/search": true, + "internal/subsonic": true, +} + +// threadedConsumers must NOT import database/sql. These are packages that have +// been moved onto the catalog.Store interface; if one of them reaches for +// *sql.DB again, the seam is being undone and this test says so. +var threadedConsumers = []string{ + "internal/playback", +} + +// TestDatabaseSQLImportersAreDeclared freezes the blast radius of database/sql. +// A new package that needs it must be added to the list deliberately, so the +// seam cannot erode one convenient import at a time. +func TestDatabaseSQLImportersAreDeclared(t *testing.T) { + root := repoRoot(t) + for pkg := range layers { + dir := filepath.Join(root, filepath.FromSlash(pkg)) + if _, err := os.Stat(dir); err != nil { + continue + } + usesSQL := false + for _, imp := range allImportsOf(t, dir) { + if imp == "database/sql" { + usesSQL = true + break + } + } + switch { + case usesSQL && !sqlImporters[pkg]: + t.Errorf("%s imports database/sql but is not in sqlImporters.\n"+ + "If it genuinely needs its own SQL, add it to the list with a reason.\n"+ + "If it is reading items, take a catalog.Store instead.", pkg) + case !usesSQL && sqlImporters[pkg]: + t.Errorf("%s is listed in sqlImporters but no longer imports database/sql.\n"+ + "Remove it from the list so the list keeps meaning something.", pkg) + } + } +} + +// TestThreadedConsumersStayOffSQL is the concrete regression guard: a package +// already moved onto the seam must not drift back. +func TestThreadedConsumersStayOffSQL(t *testing.T) { + root := repoRoot(t) + for _, pkg := range threadedConsumers { + dir := filepath.Join(root, filepath.FromSlash(pkg)) + if _, err := os.Stat(dir); err != nil { + continue + } + for _, imp := range allImportsOf(t, dir) { + if imp == "database/sql" { + t.Errorf("%s is on the catalog.Store seam but imports database/sql again.\n"+ + "That undoes the seam: take a catalog.Store, do not reach for *sql.DB.", pkg) + } + } + } +}