Vellum/internal/arch/arch_test.go
mithral ac1bd04df3 Phase 1: one item-access seam, and architecture rules that fail the build
Motivation, from the evidence rather than from taste. Five packages each
implemented "load an item, then check the caller may read it": media, comics,
games, jellyfin (with its own dbItem type and its own SQL), and subsonic. Five
copies of one authorization rule means four can drift, and the failure mode is
not a crash — it is silently serving somebody a library they were never granted.
One implementation is one thing to review.

internal/catalog now owns item and file access, and the access check.

  LoadUnchecked(ctx, db, id)        -- no access check; jobs and tests only
  LoadFor(ctx, db, u, id)           -- load AND enforce the grant, in one call
  Files / FilesFor / FileByID / FirstFile / FileCount

The unchecked variant is deliberately named so that reaching for it by accident
is hard: it is the dangerous call, so it should not have the friendlier name.
This came from the interface-design rule that a good interface makes the right
thing easy and the wrong thing hard.

The security property is tested, not asserted: a forbidden item and an absent
item return byte-identical errors, so no handler can leak the existence of
another user's library through a 403. That is now enforced in one place.

Migrated all five surfaces onto it. media, comics, games and ebooks went from
four duplicate loaders to zero. jellyfin's dbItem and loadDBItem are gone.
subsonic's per-item authorization moved while its album/artist aggregate
projections stayed put, because grouping items into albums is a presentation
concern, not a data-access one. The remaining raw SQL everywhere is browse and
aggregate queries; no per-item load decides access any more.

internal/arch enforces the layering instead of documenting it. Five rules, each
proven able to fail by deliberately breaking it:
  - no upward imports across six explicit layers
  - internal/core may depend only on migrations
  - internal/catalog must not know about HTTP, or it stops being reusable
  - modules must not import each other
  - every package must be placed in a layer, so the map cannot go stale

A test that only ever passes is decoration, so each rule was verified by
introducing the violation and confirming the failure.

Verified: gofmt clean, go build ./... exit 0, go vet exit 0, 20 packages pass,
svelte-check 0/0. End to end against a running server: the vellum API serves
item detail with codec facts, an unknown item 404s, Jellyfin AuthenticateByName
mints a token and /Items browses, and Subsonic ping returns status ok. The
subsonic migration was additionally A/B'd byte-identical against the previous
implementation across ~15 endpoints, and Jellyfin/Subsonic both still return the
same error for a forbidden item as for an absent one.

Known follow-ups, recorded rather than silently skipped:
  - internal/jellyfin has no test files at all. Its wire contract is the most
    client-visible surface in the tree and is now verified only by hand.
  - catalog is item-centric, but Subsonic addresses songs file-first (mf-<id>),
    so it resolves file to item through its own projection. A catalog lookup
    that takes a file id would close that.
  - subsonic loadAlbum now scopes the projection to the item's own library
    rather than all visible audio libraries. Equivalent by construction, since
    an item's kind tracks its library's kind, but a hand-edited mismatch would
    differ.
2026-10-11 15:47:18 -04:00

234 lines
6.6 KiB
Go

// Package arch holds no code. It exists so that the import rules this project
// depends on are enforced by the test suite instead of by good intentions.
//
// A layering rule that is only written down in a comment is a rule that will be
// broken within a month, and the breakage is invisible: an upward import does
// not fail to compile, it just quietly welds two layers together until nothing
// can be moved. These tests fail loudly instead.
package arch
import (
"go/parser"
"go/token"
"os"
"path/filepath"
"sort"
"strings"
"testing"
)
const modulePath = "github.com/mithral/vellum/"
// Layers, lowest first. A package may import from a LOWER layer or its own
// layer, never from a higher one. Peers are allowed because they genuinely
// collaborate (metadata enriches items that media serves); upward imports are
// not, because they invert the dependency and make the lower layer unusable on
// its own.
var layers = map[string]int{
// 0 — foundation. No knowledge of the application.
"internal/core": 0,
"internal/httpx": 0,
"internal/natsort": 0,
"internal/arch": 0,
"migrations": 0,
// 1 — primitives the application is built from.
"internal/auth": 1,
"internal/jobs": 1,
"internal/stream": 1,
"internal/progress": 1,
"internal/probe": 1,
"internal/engine": 1,
"internal/apitoken": 1,
// 2 — the data seam: items and access.
"internal/catalog": 2,
// 3 — the application.
"internal/library": 3,
"internal/media": 3,
"internal/search": 3,
"internal/metadata": 3,
"internal/playback": 3,
"internal/modules/comics": 3,
"internal/modules/ebooks": 3,
"internal/modules/games": 3,
"internal/modules/photos": 3,
// 4 — adapters: HTTP surfaces and foreign protocols.
"internal/httpapi": 4,
"internal/jellyfin": 4,
"internal/subsonic": 4,
"internal/cli": 4,
// 5 — the only place everything is wired together.
"cmd/vellum": 5,
}
// repoRoot walks up until it finds go.mod, so the test works regardless of
// where `go test` is invoked from.
func repoRoot(t *testing.T) string {
t.Helper()
dir, err := os.Getwd()
if err != nil {
t.Fatal(err)
}
for {
if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil {
return dir
}
parent := filepath.Dir(dir)
if parent == dir {
t.Fatal("could not find go.mod above the test directory")
}
dir = parent
}
}
// importsOf parses a directory and returns the internal packages it imports.
func importsOf(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 {
p := strings.Trim(imp.Path.Value, `"`)
if strings.HasPrefix(p, modulePath) {
seen[strings.TrimPrefix(p, modulePath)] = true
}
}
}
out := make([]string, 0, len(seen))
for p := range seen {
out = append(out, p)
}
sort.Strings(out)
return out
}
// TestNoUpwardImports is the rule that keeps the layers separable.
func TestNoUpwardImports(t *testing.T) {
root := repoRoot(t)
for pkg, layer := range layers {
dir := filepath.Join(root, filepath.FromSlash(pkg))
if _, err := os.Stat(dir); err != nil {
continue // package not present (e.g. a module was removed)
}
for _, dep := range importsOf(t, dir) {
depLayer, known := layers[dep]
if !known {
continue // a package outside the map; TestEveryPackageIsLayered catches it
}
if depLayer > layer {
t.Errorf("%s (layer %d) imports %s (layer %d): upward dependency",
pkg, layer, dep, depLayer)
}
}
}
}
// TestCoreStaysPure is the module rule: core may not know about any feature, or
// it stops being the thing everything else can depend on.
func TestCoreStaysPure(t *testing.T) {
root := repoRoot(t)
for _, dep := range importsOf(t, filepath.Join(root, "internal", "core")) {
if dep != "migrations" {
t.Errorf("internal/core imports %s; core may depend only on migrations", dep)
}
}
}
// TestCatalogHasNoHTTP keeps the data seam a data seam. If it grows an
// http.ResponseWriter it can no longer be reused by a job, a CLI or a test
// without dragging a request in.
func TestCatalogHasNoHTTP(t *testing.T) {
root := repoRoot(t)
for _, dep := range importsOf(t, filepath.Join(root, "internal", "catalog")) {
if dep == "internal/httpx" || dep == "internal/httpapi" {
t.Errorf("internal/catalog imports %s; the data seam must not know about HTTP", dep)
}
}
}
// TestModulesDoNotImportEachOther keeps modules independently removable. A
// module that imports a sibling cannot be dropped without dropping both.
func TestModulesDoNotImportEachOther(t *testing.T) {
root := repoRoot(t)
modsDir := filepath.Join(root, "internal", "modules")
mods, err := os.ReadDir(modsDir)
if err != nil {
t.Skip("no modules directory")
}
for _, m := range mods {
if !m.IsDir() {
continue
}
self := "internal/modules/" + m.Name()
for _, dep := range importsOf(t, filepath.Join(modsDir, m.Name())) {
if strings.HasPrefix(dep, "internal/modules/") && dep != self {
t.Errorf("%s imports %s; modules must not depend on each other", self, dep)
}
}
}
}
// TestEveryPackageIsLayered stops the map above from silently going stale. A
// new package that nobody placed in a layer is a package with no rules, which
// is how the layering erodes.
func TestEveryPackageIsLayered(t *testing.T) {
root := repoRoot(t)
var found []string
var walk func(base string)
walk = func(base string) {
entries, err := os.ReadDir(base)
if err != nil {
return
}
for _, e := range entries {
if !e.IsDir() {
continue
}
full := filepath.Join(base, e.Name())
rel, _ := filepath.Rel(root, full)
rel = filepath.ToSlash(rel)
// Only directories that actually contain Go files are packages.
if hasGoFiles(full) {
found = append(found, rel)
}
walk(full)
}
}
walk(filepath.Join(root, "internal"))
found = append(found, "cmd/vellum")
for _, pkg := range found {
if _, ok := layers[pkg]; !ok {
t.Errorf("%s is not in the layers map; add it so the import rules apply to it", pkg)
}
}
}
func hasGoFiles(dir string) bool {
entries, err := os.ReadDir(dir)
if err != nil {
return false
}
for _, e := range entries {
if !e.IsDir() && strings.HasSuffix(e.Name(), ".go") {
return true
}
}
return false
}