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
 }