diff --git a/internal/recordmirror/readers_test.go b/internal/recordmirror/readers_test.go index c57a7b4c..26f2e806 100644 --- a/internal/recordmirror/readers_test.go +++ b/internal/recordmirror/readers_test.go @@ -5,8 +5,10 @@ import ( "go/parser" "go/token" "io/fs" + "os" "path/filepath" "slices" + "strconv" "strings" "testing" @@ -24,17 +26,87 @@ var mayNameAMirror = []string{ "internal/catalogue/state.go", } +// The packages whose names stand for a mirror, by import path, and the names in each. +const ( + brokerPath = "github.com/novox/mesh-controller/internal/broker" + recordmirrorPath = "github.com/novox/mesh-controller/internal/recordmirror" +) + +var mirrorNames = map[string][]string{ + brokerPath: {"AssignmentsBucket", "DeclaredConfigBucket", "MirrorAssignments", "MirrorDeclaredConfig", + "ControllerMirrors"}, + recordmirrorPath: {"Assignments", "DeclaredConfig", "Mirrors"}, +} + // **Nothing in the controller reads a mirror to decide anything** (ADR 0289 §19 as ADR 0356 §2 changes it): no -// file of the controller's packages outside its tests names a mirror's bucket, by its Go name or its bus name, -// but the audit and a mirror's own writer. A reader elsewhere fails here, naming the file and what it named. +// file of the controller's packages outside its tests names a mirror's bucket but the audit and a mirror's own +// writer. A reader elsewhere fails here, naming the file and what it named. +// +// **What it catches**: a name that stands for a mirror, qualified by the package it comes from under any +// local name the file imports it as (`broker.AssignmentsBucket`, `b.AssignmentsBucket`); the same names bare +// in a file of package broker or package recordmirror; and a string literal holding a mirror's bucket name. +// **What it does not catch**: a bucket name built at run time, such as `BucketName(ControllerSeat, +// "assignments")` or literals concatenated into one, and a mirror reached through a value passed in from an +// allowed file. Those are deliberate evasions rather than ordinary code; a reviewer judges them. func TestOnlyTheAuditAndAMirrorsWriterReadAMirror(t *testing.T) { - names := []string{"AssignmentsBucket", "DeclaredConfigBucket", "MirrorAssignments", "MirrorDeclaredConfig", - "ControllerMirrors", "Assignments", "DeclaredConfig", "Mirrors"} var buckets []string for _, m := range broker.ControllerMirrors() { buckets = append(buckets, broker.BucketName(broker.ControllerSeat, m)) } root := filepath.Join("..", "..") + found := mirrorReaders(t, root, buckets) + if len(found) > 0 { + t.Fatalf("a package of the controller reads a record mirror, which only the audit and the mirror's "+ + "writer may (ADR 0289 §19, ADR 0356 §2):\n %s", strings.Join(found, "\n ")) + } +} + +// **The reader test sees what it says it sees**: in a planted tree, each planted reader is named, and an allowed +// file and a selector on a value are not. +func TestTheReaderTestFindsPlantedReaders(t *testing.T) { + root := t.TempDir() + plant := func(path, src string) { + full := filepath.Join(root, path) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(src), 0o644); err != nil { + t.Fatal(err) + } + } + plant("cmd/mesh-controller/qualified.go", "package main\n\nimport \""+brokerPath+"\"\n\nvar a = broker.AssignmentsBucket\n") + plant("internal/x/aliased.go", "package x\n\nimport b \""+brokerPath+"\"\n\nvar a = b.DeclaredConfigBucket\n") + plant("internal/broker/zz_reader.go", "package broker\n\nfunc zzR() string { return AssignmentsBucket }\n") + plant("internal/inventory/literal.go", "package inventory\n\nvar a = \"mesh-controller_declared-config\"\n") + plant("internal/y/writer.go", "package y\n\nimport \""+recordmirrorPath+"\"\n\nvar a = recordmirror.Assignments\n") + // Allowed: a mirror's definition and its writer. + plant("internal/broker/controller_buckets.go", "package broker\n\nvar AssignmentsBucket = \"x\"\n") + plant("internal/broker/other.go", "package broker\n\nfunc other() string { return \"nothing\" }\n") + // A selector on a value (v.AssignmentsBucket) is no use of the mirror, but declaring a field of a mirror's + // name in package broker is named, on the safe side; and a bare use inside a call under a selector is found. + plant("internal/broker/field.go", "package broker\n\ntype s struct{ AssignmentsBucket string }\n\nfunc f(v s) string { return v.AssignmentsBucket }\n") + plant("internal/broker/nested.go", "package broker\n\nfunc g() int { return len(h(DeclaredConfigBucket).x) }\n") + found := mirrorReaders(t, root, []string{"mesh-controller_assignments", "mesh-controller_declared-config"}) + want := []string{ + "cmd/mesh-controller/qualified.go: broker.AssignmentsBucket", + "internal/broker/field.go: AssignmentsBucket", + "internal/broker/nested.go: DeclaredConfigBucket", + "internal/broker/zz_reader.go: AssignmentsBucket", + "internal/inventory/literal.go: \"mesh-controller_declared-config\"", + "internal/x/aliased.go: b.DeclaredConfigBucket", + "internal/y/writer.go: recordmirror.Assignments", + } + slices.Sort(found) + if !slices.Equal(found, want) { + t.Errorf("the planted readers were found as\n %s\nwant\n %s", strings.Join(found, "\n "), + strings.Join(want, "\n ")) + } +} + +// mirrorReaders is every place under root's cmd and internal, outside tests and the allowed files, that names a +// mirror. +func mirrorReaders(t *testing.T, root string, buckets []string) []string { + t.Helper() var found []string for _, dir := range []string{"cmd", "internal"} { err := filepath.WalkDir(filepath.Join(root, dir), func(path string, e fs.DirEntry, err error) error { @@ -53,13 +125,43 @@ func TestOnlyTheAuditAndAMirrorsWriterReadAMirror(t *testing.T) { if err != nil { return err } + // Each import's local name, mapped to the package it stands for. + imported := map[string]string{} + for _, imp := range f.Imports { + p, _ := strconv.Unquote(imp.Path.Value) + if _, ok := mirrorNames[p]; !ok { + continue + } + local := filepath.Base(p) + if imp.Name != nil { + local = imp.Name.Name + } + imported[local] = p + } + // A file of the package that defines the names may use them bare. + var bare []string + switch f.Name.Name { + case "broker": + bare = mirrorNames[brokerPath] + case "recordmirror": + bare = mirrorNames[recordmirrorPath] + } ast.Inspect(f, func(n ast.Node) bool { switch n := n.(type) { case *ast.SelectorExpr: pkg, ok := n.X.(*ast.Ident) - if ok && (pkg.Name == "broker" || pkg.Name == "recordmirror") && slices.Contains(names, n.Sel.Name) { + if !ok { + return true + } + if p, imp := imported[pkg.Name]; imp && slices.Contains(mirrorNames[p], n.Sel.Name) { found = append(found, rel+": "+pkg.Name+"."+n.Sel.Name) } + // Neither a selector's own name nor its package is a bare use. + return false + case *ast.Ident: + if slices.Contains(bare, n.Name) { + found = append(found, rel+": "+n.Name) + } case *ast.BasicLit: for _, b := range buckets { if n.Kind == token.STRING && strings.Contains(n.Value, b) { @@ -75,8 +177,5 @@ func TestOnlyTheAuditAndAMirrorsWriterReadAMirror(t *testing.T) { t.Fatal(err) } } - if len(found) > 0 { - t.Fatalf("a package of the controller reads a record mirror, which only the audit and the mirror's "+ - "writer may (ADR 0289 §19, ADR 0356 §2):\n %s", strings.Join(found, "\n ")) - } + return found }