From bafbd2be7cfdfa8349dd66a3d59a132858237d03 Mon Sep 17 00:00:00 2001 From: Brian Miller Date: Sat, 22 Aug 2026 21:28:43 +0000 Subject: [PATCH] test: assert the entity spec table holds together This repo had no tests at all, so CI's `go test ./...` ran over nothing. The entity table is the right place to start: it is the CLI's entire surface -- registerCRUD turns each spec into a command tree and pull/apply enumerate the same list -- and it is hand-maintained on purpose, since it is not derived from the platform's schema.json. Every failure mode it has is silent. Nothing in the type system objects to a typo'd field type, and both addFieldFlags and collectFields switch on Type without a default, so the field registers no flag and quietly cannot be set. A verb spelled "list" makes hasVerb answer false for every verb the spec meant to allow, and the command goes missing. A duplicated alias is not an error in cobra; one command shadows the other and an entity stops being reachable. A LookupKey naming a field the collection lacks turns every natural-key lookup into an opaque 400. So the tests assert: types are dispatchable, verbs are known and unique, LookupKey names a declared field, select/multiselect carry Values and nothing else does, flags are unique within a spec and do not collide with the flags ls, get, delete and create register themselves or with root.go's persistent ones, required fields only exist where create does, and no command name or alias is claimed twice across the whole table. Plus unit coverage of the helpers that back them -- aliases, flagName, hasVerb, composeOrgFilter, splitFields -- and a short explicit list of the collections that are not org-scoped, which is the one place the CLI's model of platform tenancy is written down. Each check was verified by planting the defect it describes and confirming the failure, then reverting: a passing test that cannot fail is not evidence. composeOrgFilter's parenthesisation is pinned for the same reason it exists -- an OR inside a user's --filter must not escape the org scope. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/entity_test.go | 320 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 320 insertions(+) create mode 100644 cmd/entity_test.go diff --git a/cmd/entity_test.go b/cmd/entity_test.go new file mode 100644 index 0000000..2780581 --- /dev/null +++ b/cmd/entity_test.go @@ -0,0 +1,320 @@ +package cmd + +import ( + "fmt" + "strings" + "testing" +) + +// The entity table in entity.go is the CLI's whole surface: registerCRUD turns +// each spec into a command tree, and pull/apply enumerate the same list. It is +// also hand-maintained -- deliberately, since it is not derived from the +// platform's schema.json -- so the failure modes below are all silent. Nothing +// in the Go type system objects to a typo'd field type, a duplicated alias, or a +// verb spelled "list", and none of them produce an error at startup: they +// produce a CLI that is quietly missing a flag, a command, or an entity. +// +// These tests are that objection. + +// knownFieldTypes is the set addFieldFlags and collectFields actually dispatch +// on. Both switch without a default, so a type outside this set registers no +// flag at all and silently drops the field from create and update. +var knownFieldTypes = map[FieldType]bool{ + FString: true, + FInt: true, + FBool: true, + FJSON: true, + FID: true, + FIDs: true, + FSelect: true, + FMSelect: true, +} + +// knownVerbs is what hasVerb is asked about and what registerCRUD builds. A +// spec listing a verb outside this set does not fail -- hasVerb simply returns +// false for the real verb it meant, so the command goes missing. +var knownVerbs = map[string]bool{ + "ls": true, "get": true, "create": true, "update": true, "delete": true, "edit": true, +} + +// reservedFlags maps a verb to the flags its command registers itself. A field +// whose flag collides with one of these on the same command is a duplicate +// registration in cobra, not a merge. +var reservedFlags = map[string][]string{ + "ls": {"filter", "sort", "fields"}, + "get": {"fields"}, + "delete": {"yes", "y"}, + // create only registers this when the spec has a password field, which is + // checked per-spec below rather than here. +} + +// persistentFlags are registered on the root command in root.go and are +// therefore in scope for every subcommand. +var persistentFlags = []string{"context", "output", "o", "debug"} + +func TestEntitySpecsAreWellFormed(t *testing.T) { + if len(entitySpecs) == 0 { + t.Fatal("entitySpecs is empty: the CLI would expose no entity commands at all") + } + + for _, spec := range entitySpecs { + t.Run(spec.Name, func(t *testing.T) { + if spec.Name == "" || spec.Plural == "" || spec.Collection == "" { + t.Fatalf("Name/Plural/Collection must all be set, got %q/%q/%q", + spec.Name, spec.Plural, spec.Collection) + } + + // Every list view needs columns; without them `ls` prints rows of + // nothing but ids. + if len(spec.KeyColumns) == 0 { + t.Error("KeyColumns is empty, so `ls` has no columns to show") + } + + for _, v := range spec.Verbs { + if !knownVerbs[v] { + t.Errorf("unknown verb %q: hasVerb will answer false for every verb this spec meant to allow", v) + } + } + if dup := firstDuplicate(spec.Verbs); dup != "" { + t.Errorf("verb %q is listed twice", dup) + } + + // LookupKey is used to build a PocketBase filter in + // resolveRecordID. Naming a field the collection does not have + // turns every natural-key lookup into an opaque 400. + if spec.LookupKey != "" && spec.field(spec.LookupKey) == nil { + t.Errorf("LookupKey %q is not a declared field, so natural-key lookup filters on a column that may not exist", spec.LookupKey) + } + + seenField := map[string]bool{} + seenFlag := map[string]bool{} + for _, f := range spec.Fields { + if f.Name == "" { + t.Error("a field has no Name, so it has no JSON key to write") + continue + } + if seenField[f.Name] { + t.Errorf("field %q is declared twice", f.Name) + } + seenField[f.Name] = true + + if !knownFieldTypes[f.Type] { + t.Errorf("field %q has type %q, which addFieldFlags does not dispatch on: no flag would be registered and the field would be silently unsettable", f.Name, f.Type) + } + + // Values is meaningful only for the two enum types, and + // required for them: collectFields validates against it and + // an empty list rejects every value the user could pass. + switch f.Type { + case FSelect, FMSelect: + if len(f.Values) == 0 { + t.Errorf("field %q is a %s with no Values, so collectFields rejects every value passed to it", f.Name, f.Type) + } + default: + if len(f.Values) > 0 { + t.Errorf("field %q is type %s but declares Values, which nothing reads", f.Name, f.Type) + } + } + + flag := f.flagName() + if seenFlag[flag] { + t.Errorf("field %q maps to flag --%s, which another field already claims", f.Name, flag) + } + seenFlag[flag] = true + + for _, p := range persistentFlags { + if flag == p { + t.Errorf("field %q maps to --%s, which root.go registers as a persistent flag", f.Name, flag) + } + } + + // A required field on an entity that cannot be created is + // dead config: nothing ever enforces it. + if f.Required && !spec.hasVerb("create") { + t.Errorf("field %q is Required, but this spec has no create verb", f.Name) + } + } + + // Per-verb collisions, only for the verbs this spec actually has. + for verb, reserved := range reservedFlags { + if !spec.hasVerb(verb) { + continue + } + for _, r := range reserved { + if seenFlag[r] { + t.Errorf("a field maps to --%s, which the %s command registers itself", r, verb) + } + } + } + // buildCreateCmd adds --random-password only when a password field + // exists, so the collision is only possible on those specs. + if spec.hasVerb("create") && spec.field("password") != nil && seenFlag["random-password"] { + t.Error("a field maps to --random-password, which buildCreateCmd registers for password-bearing entities") + } + }) + } +} + +// Command names and aliases all land in one cobra namespace. A collision is not +// an error there -- one command shadows the other, and the shadowed entity +// simply stops being reachable. +func TestEntityCommandNamesAndAliasesAreUnique(t *testing.T) { + owner := map[string]string{} + + claim := func(token, entity, kind string) { + if prev, ok := owner[token]; ok { + t.Errorf("%s %q is claimed by both %q and %q: one shadows the other in cobra", kind, token, prev, entity) + return + } + owner[token] = entity + } + + for _, spec := range entitySpecs { + claim(spec.Name, spec.Name, "command name") + for _, a := range spec.aliases() { + claim(a, spec.Name, "alias") + } + } +} + +// Documented in aliases(): "nats-user" is reachable as nats-users, nats_users +// and nats_user. This pins that behaviour, including that the canonical name is +// never repeated as an alias (cobra would list it twice in help output). +func TestAliasesCoverTheFormsUsersType(t *testing.T) { + spec := EntitySpec{Name: "nats-user", Plural: "nats-users", Collection: "nats_users"} + + got := spec.aliases() + for _, want := range []string{"nats-users", "nats_users", "nats_user"} { + if !containsString(got, want) { + t.Errorf("alias %q missing from %v", want, got) + } + } + if containsString(got, spec.Name) { + t.Errorf("aliases include the canonical name %q: %v", spec.Name, got) + } + if dup := firstDuplicate(got); dup != "" { + t.Errorf("alias %q is repeated in %v", dup, got) + } +} + +func TestFlagNameDerivation(t *testing.T) { + if got := (Field{Name: "floorplan_position"}).flagName(); got != "floorplan-position" { + t.Errorf("underscores should become hyphens, got %q", got) + } + // An explicit Flag wins, which is the escape hatch for a field whose JSON + // key makes an awkward flag. + if got := (Field{Name: "role_id", Flag: "role"}).flagName(); got != "role" { + t.Errorf("explicit Flag should win, got %q", got) + } +} + +func TestHasVerbTreatsEmptyAsEverything(t *testing.T) { + all := EntitySpec{} + for verb := range knownVerbs { + if !all.hasVerb(verb) { + t.Errorf("an empty Verbs list should allow %q", verb) + } + } + + limited := EntitySpec{Verbs: []string{"ls", "get"}} + if !limited.hasVerb("ls") || limited.hasVerb("delete") { + t.Error("a non-empty Verbs list should allow exactly what it lists") + } +} + +func TestComposeOrgFilter(t *testing.T) { + orgScoped := EntitySpec{OrgScoped: true} + global := EntitySpec{} + + cases := []struct { + name string + spec EntitySpec + org string + extra string + want string + }{ + {"org scope only", orgScoped, "abc", "", `organization="abc"`}, + // The extra filter is parenthesised, so an OR inside it cannot escape + // the org scope and widen the query across tenants. + {"org scope and extra", orgScoped, "abc", `a=1 || b=2`, `organization="abc" && (a=1 || b=2)`}, + {"extra only, global entity", global, "abc", "name='x'", `(name='x')`}, + {"nothing", global, "", "", ""}, + {"org scoped but no org resolved", orgScoped, "", "", ""}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := composeOrgFilter(c.spec, c.org, c.extra); got != c.want { + t.Errorf("got %q, want %q", got, c.want) + } + }) + } +} + +func TestSplitFields(t *testing.T) { + cases := map[string][]string{ + "code,name": {"code", "name"}, + " code , name ": {"code", "name"}, + "code,,name": {"code", "name"}, + "": nil, + ",": nil, + } + for in, want := range cases { + got := splitFields(in) + if fmt.Sprint(got) != fmt.Sprint(want) { + t.Errorf("splitFields(%q) = %v, want %v", in, got, want) + } + } +} + +// firstDuplicate returns the first repeated element, or "" if there are none. +func firstDuplicate(items []string) string { + seen := map[string]bool{} + for _, s := range items { + if seen[s] { + return s + } + seen[s] = true + } + return "" +} + +// Guard against a spec that claims to be org-scoped for a collection the +// platform does not scope by organization, and vice versa. Kept as a short +// explicit list rather than derived: it is the one place the CLI's model of the +// platform's tenancy is written down, and it should be reviewed by a human when +// it changes. +func TestOrgScopingMatchesThePlatformsModel(t *testing.T) { + notOrgScoped := map[string]bool{ + // An organization is not inside an organization, and a membership + // binds a user to one rather than living in one -- the CLI filters + // memberships by the user instead. + "organizations": true, + "memberships": true, + } + + for _, spec := range entitySpecs { + want := !notOrgScoped[spec.Collection] + if spec.OrgScoped != want { + t.Errorf("%s (collection %q): OrgScoped is %v, want %v", + spec.Name, spec.Collection, spec.OrgScoped, want) + } + } +} + +func TestEveryEntityIsReachableByAtLeastOneVerb(t *testing.T) { + for _, spec := range entitySpecs { + var have []string + for verb := range knownVerbs { + if spec.hasVerb(verb) { + have = append(have, verb) + } + } + if len(have) == 0 { + t.Errorf("%s has no verbs, so registerCRUD builds a command tree nobody can use", spec.Name) + } + if !spec.hasVerb("ls") && !spec.hasVerb("get") { + t.Errorf("%s supports neither ls nor get: %v", spec.Name, strings.Join(have, ",")) + } + } +}