diff --git a/ir/auth.go b/ir/auth.go index 15bf923..7e48bb0 100644 --- a/ir/auth.go +++ b/ir/auth.go @@ -39,6 +39,24 @@ const ( AuthKindCustom AuthKind = "custom" ) +// Valid reports whether k is one of the mechanisms declared above. AuthKind is a +// bare string enum, so nothing rejects an empty, misspelled or stale value on +// the wire, and a scheme naming no mechanism is indistinguishable from one +// naming oauth2 to every structural check that reads only its key and its ID. +// irverify calls this so such a scheme is reported as the compiler bug it is. +func (k AuthKind) Valid() bool { + switch k { + case AuthKindAPIKey, AuthKindHTTPBasic, AuthKindHTTPBearer, AuthKindOAuth2, + AuthKindOpenIDConnect, AuthKindMutualTLS, AuthKindUserPassword, AuthKindX509, + AuthKindSymmetricEncryption, AuthKindAsymmetricEncryption, + AuthKindSASLPlain, AuthKindSASLSCRAMSHA256, AuthKindSASLSCRAMSHA512, + AuthKindSASLGSSAPI, AuthKindCustom: + return true + default: + return false + } +} + // AuthScheme is a named authentication scheme in Document.Auth (ir-design §9). type AuthScheme struct { // ID is the scheme's stable synthetic identity. diff --git a/ir/auth_test.go b/ir/auth_test.go index 494e6eb..3433003 100644 --- a/ir/auth_test.go +++ b/ir/auth_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/dexpace/morphic/ir" ) @@ -70,6 +71,36 @@ func TestAuthKind_Constants(t *testing.T) { }, "unspecified") } +// TestAuthKind_TiesToConstBlock closes the gap a bare string enum leaves: +// nothing rejects an empty or misspelled mechanism on deserialization, and no +// structural check can tell one from oauth2 by reading a scheme's key and its +// ID, so irverify has Valid and nothing else to test against. Valid therefore +// has to stay tied to the const block, and it is tied here by parsing the ir +// sources rather than by a list. +// +// Adding a mechanism without teaching Valid about it fails here rather than +// surfacing later as a spurious ir/unknown-auth-kind on a document that names +// the mechanism correctly. +func TestAuthKind_TiesToConstBlock(t *testing.T) { + t.Parallel() + declared := declaredConstsOfType(t, "AuthKind") + require.NotEmpty(t, declared, "the ir sources must declare AuthKind constants") + for _, c := range declared { + assert.True(t, ir.AuthKind(c.value).Valid(), "declared mechanism %q must be Valid", c.value) + } +} + +// TestAuthKind_UnknownIsInvalid pins the other direction: Valid must reject a +// mechanism no const declares. The empty string is the case that motivated the +// check — a scheme interned naming no mechanism at all — and the other two are +// the near misses a compiler writes by hand instead of using the constant. +func TestAuthKind_UnknownIsInvalid(t *testing.T) { + t.Parallel() + assert.False(t, ir.AuthKind("").Valid()) + assert.False(t, ir.AuthKind("api_key").Valid()) + assert.False(t, ir.AuthKind("OAuth2").Valid()) +} + // TestOAuthFlow_ZeroValueShape pins OAuthFlow's omitempty contract: every // field is optional, since only the flow kinds present in Scheme.Flows carry // meaning for a given AuthScheme. diff --git a/ir/helpers_test.go b/ir/helpers_test.go index ce74d15..7f8a6f4 100644 --- a/ir/helpers_test.go +++ b/ir/helpers_test.go @@ -7,9 +7,14 @@ package ir_test import ( "encoding/json" + "go/ast" + "go/parser" + "go/token" "os" "path/filepath" "runtime" + "slices" + "strconv" "strings" "testing" @@ -45,6 +50,93 @@ func irSourceFiles(t *testing.T) []string { return out } +// parseIRSources parses every file irSourceFiles lists, under one FileSet. +func parseIRSources(t *testing.T) []*ast.File { + t.Helper() + paths := irSourceFiles(t) + fset := token.NewFileSet() + out := make([]*ast.File, 0, len(paths)) + for _, path := range paths { + f, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + require.NoError(t, err, "parsing %s", path) + out = append(out, f) + } + require.NotEmpty(t, out, "the ir package must have production sources") + return out +} + +// namedConst is one string constant as the ir sources declare it: the Go +// identifier and the wire value it spells. +type namedConst struct { + name string + value string +} + +// declaredConstsOfType returns every constant of the named type the ir package's +// production sources declare, sorted by identifier. +// +// Deriving the set from the source is the point: any list of an enum's members +// written by hand is one commit away from disagreeing with the enum, and +// disagreeing silently. A bare string enum has no other guard — nothing rejects +// an undeclared value on the wire — so the tests that tie one to its Valid +// method all start here. +func declaredConstsOfType(t *testing.T, typeName string) []namedConst { + t.Helper() + var out []namedConst + for _, f := range parseIRSources(t) { + for _, decl := range f.Decls { + gd, isGen := decl.(*ast.GenDecl) + if !isGen || gd.Tok != token.CONST { + continue + } + out = append(out, constsOfType(t, gd, typeName)...) + } + } + slices.SortFunc(out, func(a, b namedConst) int { return strings.Compare(a.name, b.name) }) + return out +} + +// constsOfType returns the constants of the named type in one const group. A +// spec declaring neither type nor value repeats the previous spec, so the +// group's last explicit type carries forward; a spec with its own value declares +// its own type. +func constsOfType(t *testing.T, gd *ast.GenDecl, typeName string) []namedConst { + t.Helper() + var out []namedConst + isWanted := false + for _, spec := range gd.Specs { + vs, isValue := spec.(*ast.ValueSpec) + require.True(t, isValue, "const spec is not a ValueSpec: %#v", spec) + switch { + case vs.Type != nil: + id, isIdent := vs.Type.(*ast.Ident) + isWanted = isIdent && id.Name == typeName + case len(vs.Values) > 0: + isWanted = false + } + if !isWanted { + continue + } + for i, name := range vs.Names { + require.Less(t, i, len(vs.Values), + "%s constant %s must declare its own value", typeName, name.Name) + out = append(out, namedConst{name: name.Name, value: stringLit(t, name.Name, vs.Values[i])}) + } + } + return out +} + +// stringLit returns the string a constant's value expression spells out. +func stringLit(t *testing.T, constName string, expr ast.Expr) string { + t.Helper() + lit, isLit := expr.(*ast.BasicLit) + require.True(t, isLit, "constant %s must be declared as a string literal", constName) + require.Equal(t, token.STRING, lit.Kind, "constant %s must be declared as a string literal", constName) + unquoted, err := strconv.Unquote(lit.Value) + require.NoError(t, err, "unquoting the value of %s", constName) + return unquoted +} + // assertRoundTrip marshals want, unmarshals into a fresh T, and asserts the // result equals want via cmp.Diff (never reflect.DeepEqual, per CLAUDE.md). // This is the Class B (populated round-trip) workhorse shared by every diff --git a/ir/irverify/bigval.go b/ir/irverify/bigval.go new file mode 100644 index 0000000..bc3a9cf --- /dev/null +++ b/ir/irverify/bigval.go @@ -0,0 +1,87 @@ +package irverify + +import ( + "reflect" + "strconv" + + "github.com/dexpace/morphic/ir" +) + +var ( + bigValType = reflect.TypeFor[ir.BigVal]() + bigValPtrType = reflect.TypeFor[*ir.BigVal]() +) + +// checkBigVals asserts every numeric literal the document carries is what +// ir.BigVal promises: a decimal literal that reads back as a JSON number, in the +// canonical form ir.NewBigVal produces. +// +// This is the same hazard checkRawPayloads covers for the other verbatim-text +// carrier, and it has the same shape: the text is copied wherever the value +// goes, and the grammar it must satisfy is enforced only at construction. +// ir.BigVal is a defined string type, so ir.BigVal(raw) compiles and skips +// ir.NewBigVal entirely, and — unlike the sealed TypeDef sum — it carries no +// UnmarshalJSON, so a document decoded from JSON never meets the constructor at +// all (GitHub #282). Round-tripping is not the safety net here: the value is +// carried faithfully precisely because it is a string. +// +// The two codes are separate because they name different repairs. A value the +// constructor rejects is not a number at all; a value it accepts but rewrites is +// a number spelled a way JSON does not admit — a leading "+", a redundant +// leading zero, a bare leading dot — which a consumer splicing the text into +// generated source or into JSON emits as invalid output. +// +// Reaching the literals through the walk rather than a list of carriers is what +// makes this complete: Constraints.Min, Max and MultipleOf and Value.Num are the +// fields today, and a numeric field added to the IR is covered the moment it +// exists. +func checkBigVals(doc *ir.Document, _ declarations) ([]Violation, bool) { + var vs []Violation + truncated := ir.WalkValues(doc, ir.DocumentPath, func(v reflect.Value, path string) bool { + switch v.Type() { + case bigValPtrType: + // A bound carried by pointer is present because something set it, so + // an empty literal there is a defect rather than an absence. Reading + // it here rather than descending is what tells the two apart. + if !v.IsNil() { + vs = appendBigVal(vs, v.Elem().String(), path) + } + return false + case bigValType: + // Value.Num is not a pointer and is the zero string on every Value + // that is not a number — most of the values a document holds — so an + // empty one here says the field is unused, not that it is broken. + if literal := v.String(); literal != "" { + vs = appendBigVal(vs, literal, path) + } + return false + default: + return true + } + }) + return vs, truncated +} + +// appendBigVal reports the two ways one literal can break ir.BigVal's contract. +// The literal is quoted because the values worth reporting are the ones that do +// not look like numbers, the empty string among them. +func appendBigVal(vs []Violation, literal, path string) []Violation { + canonical, err := ir.NewBigVal(literal) + if err != nil { + return append(vs, Violation{ + Code: "ir/bigval-not-numeric", + Message: "numeric value " + strconv.Quote(literal) + + " is not a decimal literal, so it does not read back as a JSON number", + Path: path, + }) + } + if string(canonical) == literal { + return vs + } + return append(vs, Violation{ + Code: "ir/bigval-not-canonical", + Message: "numeric value " + strconv.Quote(literal) + " is not the JSON form " + + strconv.Quote(string(canonical)) + " ir.NewBigVal produces for it", + Path: path, + }) +} diff --git a/ir/irverify/bigval_test.go b/ir/irverify/bigval_test.go new file mode 100644 index 0000000..095e6fa --- /dev/null +++ b/ir/irverify/bigval_test.go @@ -0,0 +1,165 @@ +package irverify_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irverify" +) + +// bigVal returns a pointer to the literal as written, which is how the three +// bound fields carry one and the only way to write one past ir.NewBigVal. +func bigVal(literal string) *ir.BigVal { + v := ir.BigVal(literal) + return &v +} + +// numericValue is a number-kinded ir.Value carrying the literal as written. +func numericValue(literal string) ir.Value { + return ir.Value{Kind: ir.ValueNumber, Num: ir.BigVal(literal)} +} + +// bigValCarriers builds one document per field in the IR that carries a +// ir.BigVal, each holding literal at that field and nothing else out of place. +// The issue's own warning is that a check can be written and still not reach a +// carrier, so each is planted and asserted separately rather than all at once. +func bigValCarriers(literal string) map[string]*ir.Document { + prov := ir.Provenance{Source: ir.NoSource} + scalar := func(c *ir.Constraints) *ir.Document { + return &ir.Document{Types: ir.TypeRegistry{"t/x/S": &ir.Scalar{ + TypeCommon: ir.TypeCommon{ID: "t/x/S", Name: named("s"), Provenance: prov}, + Constraints: c, + }}} + } + valued := func(v ir.Value) *ir.Document { + return &ir.Document{Types: ir.TypeRegistry{"t/x/L": &ir.Literal{ + TypeCommon: ir.TypeCommon{ID: "t/x/L", Name: named("l"), Provenance: prov}, + Value: v, + }}} + } + property := func(p ir.Property) *ir.Document { + return &ir.Document{Types: ir.TypeRegistry{"t/x/M": &ir.Model{ + TypeCommon: ir.TypeCommon{ID: "t/x/M", Name: named("m"), Provenance: prov}, + Properties: []ir.Property{p}, + }}} + } + prop := ir.Property{ID: "p/x/M/f", Name: named("f"), Provenance: prov} + prop.Type = ir.TypeRef{Target: "t/x/M"} + + withDefault, withBound := prop, prop + def := numericValue(literal) + withDefault.Default = &def + withBound.Constraints = &ir.Constraints{Max: bigVal(literal)} + + return map[string]*ir.Document{ + "doc.Types[t/x/S].Constraints.Min": scalar(&ir.Constraints{Min: bigVal(literal)}), + "doc.Types[t/x/S].Constraints.Max": scalar(&ir.Constraints{Max: bigVal(literal)}), + "doc.Types[t/x/S].Constraints.MultipleOf": scalar(&ir.Constraints{MultipleOf: bigVal(literal)}), + "doc.Types[t/x/L].Value.Num": valued(numericValue(literal)), + "doc.Types[t/x/M].Properties[0].Default.Num": property(withDefault), + "doc.Types[t/x/M].Properties[0].Constraints.Max": property(withBound), + } +} + +// assertBigValCode plants literal at every ir.BigVal carrier in turn and asserts +// each document yields exactly the one violation, at that carrier's path. +func assertBigValCode(t *testing.T, literal, code string) { + t.Helper() + for path, doc := range bigValCarriers(literal) { + t.Run(path, func(t *testing.T) { + t.Parallel() + got := irverify.Verify(doc) + require.Len(t, got, 1, "the document is sound apart from the literal") + assert.Equal(t, code, got[0].Code) + assert.Equal(t, path, got[0].Path) + assert.Contains(t, got[0].Message, literal) + }) + } +} + +// TestVerify_NonNumericBigValIsAViolation drives the half ir.NewBigVal rejects +// outright. ir.BigVal is a defined string type with no UnmarshalJSON, so every +// one of these reaches a document either by conversion or by a JSON decode that +// meets no constructor. +func TestVerify_NonNumericBigValIsAViolation(t *testing.T) { + t.Parallel() + for _, literal := range []string{"abc", "1.2.3", "0x1f", "NaN", "1e", "1 ; DROP TABLE"} { + t.Run(literal, func(t *testing.T) { + t.Parallel() + assertBigValCode(t, literal, "ir/bigval-not-numeric") + }) + } +} + +// TestVerify_NonCanonicalBigValIsAViolation drives the half ir.NewBigVal +// accepts and rewrites. Each of these is a number, and none of them is one JSON +// admits, so a consumer splicing the text into JSON or into generated source +// emits something that will not parse. +func TestVerify_NonCanonicalBigValIsAViolation(t *testing.T) { + t.Parallel() + for _, literal := range []string{"+5", "007", ".5", "5.", "00.5"} { + t.Run(literal, func(t *testing.T) { + t.Parallel() + assertBigValCode(t, literal, "ir/bigval-not-canonical") + }) + } +} + +// TestVerify_CanonicalBigValIsClean is the silent half: a check that cannot stay +// quiet reports every document that carries a number. The values span what +// BigVal exists to carry — a magnitude and a precision no float64 holds, an +// exponent, and a signed zero — so the rule cannot be passing them by narrowing +// what a number is. +func TestVerify_CanonicalBigValIsClean(t *testing.T) { + t.Parallel() + for _, literal := range []string{ + "1", "-0", "0.5", "1e10", "1E-30", "9007199254740993", + "123456789012345678901234567890.123456789", + } { + t.Run(literal, func(t *testing.T) { + t.Parallel() + for path, doc := range bigValCarriers(literal) { + assert.Empty(t, irverify.Verify(doc), "%s", path) + } + }) + } +} + +// TestVerify_UnusedNumIsNotABound pins the one distinction the walk has to draw. +// Value.Num is not a pointer and is the zero string on every Value that is not a +// number — most of the values a document holds — so an empty one there says the +// field is unused. A bound is carried by pointer and is present because +// something set it, so an empty one there is a bound that constrains nothing. +func TestVerify_UnusedNumIsNotABound(t *testing.T) { + t.Parallel() + prov := ir.Provenance{Source: ir.NoSource} + unused := &ir.Document{Types: ir.TypeRegistry{"t/x/L": &ir.Literal{ + TypeCommon: ir.TypeCommon{ID: "t/x/L", Name: named("l"), Provenance: prov}, + Value: ir.Value{Kind: ir.ValueString, Str: "not a number"}, + }}} + assert.Empty(t, irverify.Verify(unused), "a non-numeric Value carries no literal to check") + + empty := &ir.Document{Types: ir.TypeRegistry{"t/x/S": &ir.Scalar{ + TypeCommon: ir.TypeCommon{ID: "t/x/S", Name: named("s"), Provenance: prov}, + Constraints: &ir.Constraints{Min: bigVal("")}, + }}} + got := irverify.Verify(empty) + require.Len(t, got, 1) + assert.Equal(t, "ir/bigval-not-numeric", got[0].Code) + assert.Equal(t, "doc.Types[t/x/S].Constraints.Min", got[0].Path) +} + +// TestVerify_AbsentBoundIsClean holds the other side of that distinction: a nil +// bound is the ordinary case — most constraints set none — and reading one as a +// literal would report every document in the corpus. +func TestVerify_AbsentBoundIsClean(t *testing.T) { + t.Parallel() + doc := &ir.Document{Types: ir.TypeRegistry{"t/x/S": &ir.Scalar{ + TypeCommon: ir.TypeCommon{ID: "t/x/S", Name: named("s"), Provenance: ir.Provenance{Source: ir.NoSource}}, + Constraints: &ir.Constraints{Pattern: "^[a-z]+$"}, + }}} + assert.Empty(t, irverify.Verify(doc)) +} diff --git a/ir/irverify/declared.go b/ir/irverify/declared.go new file mode 100644 index 0000000..e85d191 --- /dev/null +++ b/ir/irverify/declared.go @@ -0,0 +1,101 @@ +package irverify + +import ( + "reflect" + + "github.com/dexpace/morphic/ir" +) + +// idFieldName is the field a node declares its own identity through. It is +// spelled here as well as in ir because this check reads the one declaration +// ir.DeclaredIDs drops — the empty one — and so cannot ask ir for the answer. +const idFieldName = "ID" + +// checkDeclaredIDs asserts every node that declares an identity of its own +// carries a non-empty one. +// +// An ID is derived from the source pointer of its defining occurrence, so an +// empty one means a compiler minted nothing where it was supposed to mint an +// identity — our bug, on the same reasoning as ir/duplicate--id. What it +// costs downstream is worse than the missing ID: every reference to the node +// resolves against a registry that does not contain it, so the defect reads as a +// dangling reference at the *referring* site rather than as the missing +// declaration it is. +// +// Nothing else reaches it. checkRegistryKeys reports an empty ID from the +// registry *key*, and only Types, Channels, Messages and Auth are maps; an +// Operation is declared inside the Service→OperationGroup tree, a Service sits +// in a slice, and a Property is a position inside its model, so none of the +// three has a key for that rule to read. checkDuplicateIDs never sees one +// either, because ir.DeclaredIDs drops an empty ID before the declaration list +// is built — correctly, since nothing can reference one and treating several +// nodes carrying one as duplicates of each other would name the wrong defect +// (GitHub #289). The claim that a node meant to have an identity carries one is +// separate, and this is where it is made. +// +// The classes checkRegistryKeys already covers are skipped, so one defect stays +// one report rather than becoming two under the same code. Which those are is +// read off Document's own shape through ir.DocumentRegistries rather than listed +// here, so a registry added to Document moves its class across on its own. +// +// The code is the one the map-keyed classes already use — ir/empty--id — +// so one defect reads under one code whichever rule reports it. +func checkDeclaredIDs(doc *ir.Document, _ declarations) ([]Violation, bool) { + keyed := ir.DocumentRegistries(doc) + var vs []Violation + truncated := ir.WalkValues(doc, ir.DocumentPath, func(v reflect.Value, path string) bool { + if v.Kind() != reflect.Struct { + return true + } + class, id, declares := declaredID(v) + if !declares || id != "" { + return true + } + if _, hasRegistry := keyed[class]; hasRegistry { + return true // checkRegistryKeys reads this class from its registry key + } + noun := ir.RefNoun(class) + vs = append(vs, Violation{ + Code: "ir/empty-" + noun + "-id", + Message: noun + " declares no identity of its own, so nothing can reference it", + Path: path, + }) + return true + }) + return vs, truncated +} + +// declaredID returns the identity v declares for itself: the class of ID, its +// value, and whether v declares one at all. +// +// It repeats the predicate ir.declaredID applies — a field named ID that v's own +// type declares, of a named string type — because that function answers only for +// the non-empty ones. Repeating it exactly is the point: the two have to agree on +// what declares an identity, or this rule and ir.DeclaredIDs disagree about which +// nodes exist. +// +// A promoted field is not its own declaration. Every type node embeds TypeCommon +// and so promotes its TypeID, which ir.DeclaredIDs would record a second time — +// here such a node is skipped as registry-keyed before that matters, so the +// clause is carried for fidelity with ir rather than for an effect of its own. +// +// Both narrowing clauses are pinned by TestDeclaredID_ClassifiesEachShape rather +// than by the walk comparison beside it: no Document separates them, since every +// promoted ID is registry-keyed and the IR declares no plain-string ID, so +// dropping either leaves every fixture-driven test here green. +// TestCheckDeclaredIDs_ReachesEveryIDDeclaringNode holds the walk in step with +// ir.DeclaredIDs; this holds the predicate. +func declaredID(v reflect.Value) (class reflect.Type, id string, declares bool) { + f, isDeclared := v.Type().FieldByName(idFieldName) + if !isDeclared || len(f.Index) != 1 || !namedString(f.Type) { + return nil, "", false + } + return f.Type, v.Field(f.Index[0]).String(), true +} + +// namedString reports whether t is a named string type, the shape every ID class +// takes. A plain string is not one: it names something rather than identifying +// it. +func namedString(t reflect.Type) bool { + return t.Kind() == reflect.String && t.PkgPath() != "" +} diff --git a/ir/irverify/declared_test.go b/ir/irverify/declared_test.go new file mode 100644 index 0000000..c2e3da6 --- /dev/null +++ b/ir/irverify/declared_test.go @@ -0,0 +1,223 @@ +package irverify + +import ( + "reflect" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" +) + +// declaredIDViolations runs the empty-identity check and drops the truncation +// flag, which the cases below assert nothing about; +// TestWalkChecks_EachReportsTruncation holds that half. +func declaredIDViolations(doc *ir.Document) []Violation { + vs, _ := checkDeclaredIDs(doc, declarations{}) + return vs +} + +// idBearingDoc holds one node of every type in the IR that declares an ID of its +// own. present says whether the classes Document keys no map by carry theirs; +// the map-keyed ones always do, because an empty key is checkRegistryKeys' +// business and a fixture that left one empty would be testing that rule instead. +func idBearingDoc(present bool) *ir.Document { + model := &ir.Model{ + TypeCommon: ir.TypeCommon{ID: "t/x/M"}, + Properties: []ir.Property{{ + ID: pick(present, ir.PropID("p/x/M/f")), + Type: ir.TypeRef{Target: "t/x/M"}, + }}, + } + return &ir.Document{ + Types: ir.TypeRegistry{model.ID: model}, + Channels: map[ir.ChannelID]ir.Channel{"c/x/C": {ID: "c/x/C"}}, + Messages: map[ir.MessageID]ir.Message{"m/x/M": {ID: "m/x/M"}}, + Auth: map[ir.AuthID]ir.AuthScheme{"auth/x/A": {ID: "auth/x/A"}}, + Services: []ir.Service{{ + ID: pick(present, ir.ServiceID("s/x/S")), + Groups: []ir.OperationGroup{{ + Operations: []ir.Operation{{ID: pick(present, ir.OpID("op/x/S/op"))}}, + }}, + }}, + } +} + +// pick returns want when present and the zero identity otherwise, so one fixture +// spells both halves of the comparison below. +func pick[T ~string](present bool, want T) T { + if present { + return want + } + return "" +} + +// TestCheckDeclaredIDs_ReachesEveryIDDeclaringNode is the guard on this check's +// copy of ir.declaredID's predicate. ir.DeclaredIDs answers only for the +// non-empty identities, so the check reads the value graph itself, and a copy +// that drifted would go silent on whatever class it stopped recognizing rather +// than fail. +// +// The two are compared on the same fixture read two ways: every declaration +// ir.DeclaredIDs finds for a class with no registry must be reported when that +// same node carries nothing, at the same path. Nothing here is hand-listed, so a +// new ID-bearing node type joins both sides at once. +func TestCheckDeclaredIDs_ReachesEveryIDDeclaringNode(t *testing.T) { + t.Parallel() + filled := idBearingDoc(true) + decls, truncated := ir.DeclaredIDs(filled) + require.False(t, truncated) + require.NotEmpty(t, decls) + + keyed := ir.DocumentRegistries(filled) + var want []string + for _, d := range decls { + if _, hasRegistry := keyed[d.Class]; hasRegistry { + continue + } + want = append(want, "ir/empty-"+ir.RefNoun(d.Class)+"-id "+d.Path) + } + require.NotEmpty(t, want, "the fixture must declare a class Document keys no map by") + + reported := declaredIDViolations(idBearingDoc(false)) + got := make([]string, 0, len(reported)) + for _, v := range reported { + got = append(got, v.Code+" "+v.Path) + } + assert.Empty(t, cmp.Diff(want, got), + "this check and ir.DeclaredIDs must find the same declarations (-ir +irverify)") +} + +// TestCheckDeclaredIDs_PopulatedIDsAreClean is the silent half. The fixture +// differs from the one above only in whether the identities are there, so +// nothing but the identity can be what the check reads. +func TestCheckDeclaredIDs_PopulatedIDsAreClean(t *testing.T) { + t.Parallel() + assert.Empty(t, declaredIDViolations(idBearingDoc(true))) +} + +// TestCheckDeclaredIDs_EmptyOperationAndServiceIDs is the reported shape: +// neither class has a registry key for checkRegistryKeys to read, and +// ir.DeclaredIDs drops an empty ID before checkDuplicateIDs could see it, so +// before this check nothing said anything about either. +func TestCheckDeclaredIDs_EmptyOperationAndServiceIDs(t *testing.T) { + t.Parallel() + doc := &ir.Document{Services: []ir.Service{{ + Groups: []ir.OperationGroup{{Operations: []ir.Operation{{}, {}}}}, + }}} + + got := declaredIDViolations(doc) + require.Len(t, got, 3, "one service and both operations declare nothing") + assert.Equal(t, "ir/empty-service-id", got[0].Code) + assert.Equal(t, "doc.Services[0]", got[0].Path) + assert.Equal(t, "ir/empty-op-id", got[1].Code) + assert.Equal(t, "doc.Services[0].Groups[0].Operations[0]", got[1].Path) + assert.Equal(t, "doc.Services[0].Groups[0].Operations[1]", got[2].Path) +} + +// TestCheckDeclaredIDs_EmptyPropertyID covers the third class with no registry +// key. A property is a position inside its model rather than a document-level +// entry, but its ID is minted from a source pointer like any other and every +// PropID reference in the document — a PropPath segment, an encoding key, a +// discriminator's tag — resolves against it. +func TestCheckDeclaredIDs_EmptyPropertyID(t *testing.T) { + t.Parallel() + doc := &ir.Document{Types: ir.TypeRegistry{"t/x/M": &ir.Model{ + TypeCommon: ir.TypeCommon{ID: "t/x/M"}, + Properties: []ir.Property{{Type: ir.TypeRef{Target: "t/x/M"}}}, + }}} + + got := declaredIDViolations(doc) + require.Len(t, got, 1) + assert.Equal(t, "ir/empty-prop-id", got[0].Code) + assert.Equal(t, "doc.Types[t/x/M].Properties[0]", got[0].Path) +} + +// TestCheckDeclaredIDs_MapKeyedClassesAreLeftToTheRegistryRule pins the +// exclusion. checkRegistryKeys already reports an empty identity in these four +// classes from the registry key, so reporting it again here would give one +// defect two reports under one code. +func TestCheckDeclaredIDs_MapKeyedClassesAreLeftToTheRegistryRule(t *testing.T) { + t.Parallel() + doc := &ir.Document{ + Types: ir.TypeRegistry{"": &ir.Any{}}, + Channels: map[ir.ChannelID]ir.Channel{"": {}}, + Messages: map[ir.MessageID]ir.Message{"": {}}, + Auth: map[ir.AuthID]ir.AuthScheme{"": {}}, + } + assert.Empty(t, declaredIDViolations(doc)) + + codes := map[string]bool{} + for _, v := range Verify(doc) { + codes[v.Code] = true + } + for _, noun := range []string{"type", "channel", "message", "auth"} { + assert.True(t, codes["ir/empty-"+noun+"-id"], + "checkRegistryKeys must still be the one reporting an empty %s identity", noun) + } +} + +// TestVerify_ReportsEmptyDeclaredIDs pins that Verify runs the check, not just +// the test: a node with no identity has to reach a caller that only ever calls +// Verify. +func TestVerify_ReportsEmptyDeclaredIDs(t *testing.T) { + t.Parallel() + doc := &ir.Document{Services: []ir.Service{{ + Name: ir.Naming{Source: "svc", Canonical: "svc"}, + Groups: []ir.OperationGroup{{Name: ir.Naming{Source: "g", Canonical: "g"}}}, + }}} + + found := Verify(doc) + codes := make([]string, 0, len(found)) + for _, v := range found { + codes = append(codes, v.Code) + } + assert.Contains(t, codes, "ir/empty-service-id") +} + +// The four shapes declaredID has to tell apart. They are declared here rather +// than found in the IR because the IR holds only the first: every promoted ID is +// a TypeID, which checkDeclaredIDs skips as registry-keyed, and no node declares +// a plain-string ID. A fixture-driven comparison therefore cannot separate the +// predicate's clauses — see TestDeclaredID_ClassifiesEachShape. +type ( + idDeclarer struct{ ID ir.OpID } + idPromoter struct{ idDeclarer } + plainID struct{ ID string } + noID struct{ Name string } +) + +// TestDeclaredID_ClassifiesEachShape pins each clause of the predicate this +// package copies from ir. Dropping either narrowing clause leaves every +// Document-driven test in this package green, because no Document separates +// them, so the claim that the copy stays in step with ir's own is made here +// rather than by the walk comparison beside it. +func TestDeclaredID_ClassifiesEachShape(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + value any + declares bool + why string + }{ + {"a named string ID the type declares", idDeclarer{ID: "op/a"}, true, + "the shape every identified node takes"}, + {"an ID reached only by promotion", idPromoter{}, false, + "the embedded field is its declaration, not this one"}, + {"a plain string named ID", plainID{ID: "x"}, false, + "a plain string names something rather than identifying it"}, + {"no ID field at all", noID{Name: "x"}, false, "nothing to declare"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, id, declares := declaredID(reflect.ValueOf(tc.value)) + + assert.Equal(t, tc.declares, declares, tc.why) + if tc.declares { + assert.Equal(t, "op/a", id, "the value read is the field's own") + } + }) + } +} diff --git a/ir/irverify/duplicates.go b/ir/irverify/duplicates.go index ef22096..0e40f6d 100644 --- a/ir/irverify/duplicates.go +++ b/ir/irverify/duplicates.go @@ -2,13 +2,19 @@ package irverify import ( "reflect" + "strings" "github.com/dexpace/morphic/ir" ) -// propIDType is the reflect.Type of the one ID class checkDuplicateIDs does not -// hold to being declared once; see there for why. -var propIDType = reflect.TypeFor[ir.PropID]() +var ( + // propIDType is the reflect.Type of the one ID class checkDuplicateIDs holds + // to a weaker claim than "declared once"; see there for why. + propIDType = reflect.TypeFor[ir.PropID]() + // propertyType is the node that class identifies, which is what has to be + // fingerprinted to make that weaker claim. + propertyType = reflect.TypeFor[ir.Property]() +) // identity is one declared ID together with the class it belongs to, which is // the pair that has to be unique: an OpID and a TypeID spelling the same string @@ -18,6 +24,14 @@ type identity struct { id string } +// declaredAt is where an identity was first declared, and the fingerprint of the +// node that declared it, so a later declaration of the same identity can be +// compared against it rather than only counted. +type declaredAt struct { + path string + fingerprint string +} + // checkDuplicateIDs asserts no two nodes declare the same identity (invariant // #3). It reads the declarations rather than walking for them, and passes on // whether the walk that produced them was cut short; Verify folds that into the @@ -34,43 +48,79 @@ type identity struct { // sharing one means a compiler minted the same pointer twice — our bug, not // something a spec author wrote or can fix. // -// ir.PropID is outside the claim, because a repeated PropID is usually not a -// second declaration. A response declared once in components and referenced by -// three operations materializes into all three — responses are embedded by value, -// not interned — so the header property it declares appears at three paths under -// the one ID its defining occurrence derives -// (testdata/conformance/openapi/component-reuse.yaml). That the ID stays the -// declaration's rather than the use site's is what #107 fixed, so the repeat is -// invariant 3 holding, not breaking: the copies are one property and a lookup for -// that ID is unambiguous. +// ir.PropID is held to the same claim by way of a fingerprint, because a +// repeated PropID is often not a second declaration. A response declared once in +// components and referenced by three operations materializes into all three — +// responses are embedded by value, not interned — so the header property it +// declares appears at three paths under the one ID its defining occurrence +// derives (testdata/conformance/openapi/component-reuse.yaml). That the ID stays +// the declaration's rather than the use site's is what #107 fixed, so the repeat +// is invariant 3 holding, not breaking: the copies are one property and a lookup +// for that ID is unambiguous. Two *genuinely different* properties on one PropID +// are the defect, and skipping the class outright hid them with the copies +// (GitHub #280). // -// The skip is wider than that reason, and deliberately provisional rather than -// settled: two genuinely *different* properties minted at one PropID go -// unreported with them, which is the same defect this check exists for. Telling -// the two apart needs a fingerprint of the node rather than its ID alone — -// GitHub #280 carries it, and this comment is the debt until it lands. +// The fingerprint is the property's wire identity and its type: source name, +// wire name, and the ID its TypeRef targets. Copies of one declaration agree on +// all three because they are copies; two properties agreeing on all three are +// indistinguishable to a consumer that looks one up by ID, which is the only +// thing a duplicate ID costs. Nothing wider is read, because a fingerprint that +// separates two copies reports every document that reuses a component. // // The first declaration in walk order stands and every later one is reported, so // n nodes on one ID yield n-1 violations rather than n. Walk order is // deterministic (invariant 7), so which one stands does not vary between runs. -func checkDuplicateIDs(_ *ir.Document, decls declarations) ([]Violation, bool) { - first := make(map[identity]string, len(decls.ids)) +func checkDuplicateIDs(doc *ir.Document, decls declarations) ([]Violation, bool) { + fingerprints, truncated := propertyFingerprints(doc) + first := make(map[identity]declaredAt, len(decls.ids)) var vs []Violation for _, d := range decls.ids { - if d.Class == propIDType { - continue - } key := identity{class: d.Class, id: d.ID} at, taken := first[key] if !taken { - first[key] = d.Path + first[key] = declaredAt{path: d.Path, fingerprint: fingerprints[d.Path]} continue } + if d.Class == propIDType && at.fingerprint == fingerprints[d.Path] { + continue // one property materialized at several paths, not two properties + } vs = append(vs, Violation{ Code: "ir/duplicate-" + ir.RefNoun(d.Class) + "-id", - Message: "id " + d.ID + " is declared here and at " + at, + Message: "id " + d.ID + " is declared here and at " + at.path, Path: d.Path, }) } - return vs, decls.truncated + return vs, decls.truncated || truncated +} + +// propertyFingerprints returns every property the document holds, fingerprinted, +// keyed by the path that declares it — the same path ir.DeclaredIDs reports for +// the same node, since both walks spell a node's location the one way. +func propertyFingerprints(doc *ir.Document) (map[string]string, bool) { + fingerprints := map[string]string{} + truncated := ir.WalkValues(doc, ir.DocumentPath, func(v reflect.Value, path string) bool { + if v.Kind() != reflect.Struct || v.Type() != propertyType { + return true + } + fingerprints[path] = fingerprintOf(v) + return true + }) + return fingerprints, truncated +} + +// fingerprintOf renders what separates one property from another (see +// checkDuplicateIDs). It reads fields off the walked value rather than +// converting it back to an ir.Property, because a value the walk reached through +// an unexported field cannot be converted (see ir.WalkValues); checkNaming's +// namingChannels reads its three channels the same way. +// +// The parts are joined on NUL, which no source name, wire name or ID contains, +// so no two properties can agree on the rendering while disagreeing on the +// parts. +func fingerprintOf(prop reflect.Value) string { + return strings.Join([]string{ + prop.FieldByName("Name").FieldByName("Source").String(), + prop.FieldByName("WireName").String(), + prop.FieldByName("Type").FieldByName("Target").String(), + }, "\x00") } diff --git a/ir/irverify/duplicates_test.go b/ir/irverify/duplicates_test.go index da4f8fa..1844321 100644 --- a/ir/irverify/duplicates_test.go +++ b/ir/irverify/duplicates_test.go @@ -97,6 +97,74 @@ func TestCheckDuplicateIDs_RepeatedPropIDIsClean(t *testing.T) { assert.Empty(t, duplicateViolations(doc)) } +// modelWithProp wraps one property in a model of its own, so two of them collide +// on the PropID and on nothing else. +func modelWithProp(id ir.TypeID, prop ir.Property) *ir.Model { + return &ir.Model{TypeCommon: ir.TypeCommon{ID: id}, Properties: []ir.Property{prop}} +} + +// TestCheckDuplicateIDs_TwoDifferentPropertiesOnOnePropID is what the class-wide +// skip hid alongside the copies above: two properties that are not copies of one +// declaration, minted at one ID. Every PropID lookup downstream then resolves to +// whichever the reader reaches first, with nothing in the document saying which +// that is. +// +// One subtest per fingerprint component, because a component the fingerprint +// stopped reading would leave that pair silently indistinguishable — and this +// check would go on passing its other cases. +func TestCheckDuplicateIDs_TwoDifferentPropertiesOnOnePropID(t *testing.T) { + const dup ir.PropID = "p/x/dup" + base := ir.Property{ID: dup, Name: ir.Naming{Source: "alpha"}, WireName: "alpha", + Type: ir.TypeRef{Target: "t/x/A"}} + + differs := map[string]func(p ir.Property) ir.Property{ + "source name": func(p ir.Property) ir.Property { p.Name = ir.Naming{Source: "beta"}; return p }, + "wire name": func(p ir.Property) ir.Property { p.WireName = "beta"; return p }, + "type": func(p ir.Property) ir.Property { p.Type = ir.TypeRef{Target: "t/x/B"}; return p }, + } + for field, differ := range differs { + t.Run(field, func(t *testing.T) { + doc := &ir.Document{Types: ir.TypeRegistry{ + "t/x/A": modelWithProp("t/x/A", base), + "t/x/B": modelWithProp("t/x/B", differ(base)), + }} + + got := duplicateViolations(doc) + require.Len(t, got, 1, "the first declaration stands and only the second is reported") + assert.Equal(t, "ir/duplicate-prop-id", got[0].Code) + assert.Equal(t, "doc.Types[t/x/B].Properties[0]", got[0].Path) + assert.Contains(t, got[0].Message, string(dup)) + assert.Contains(t, got[0].Message, "doc.Types[t/x/A].Properties[0]") + }) + } +} + +// TestCheckDuplicateIDs_CopiesDifferingOutsideTheFingerprintAreClean holds the +// fingerprint to being no wider than it has to be, which is a claim the corpus +// cannot make: today the three copies in component-reuse.yaml are equal in every +// field, so a fingerprint over the whole property would pass it. Nothing +// guarantees they stay that way — a position-carried field such as provenance or +// a required flag is exactly what a later lowering would differ on — and the +// day one does, a wider fingerprint reports every document that reuses a +// component, which is the failure the class-wide skip was avoiding. Only what a +// duplicate ID actually costs is read: which property a lookup for that ID +// reaches. +func TestCheckDuplicateIDs_CopiesDifferingOutsideTheFingerprintAreClean(t *testing.T) { + base := ir.Property{ID: "p/x/dup", Name: ir.Naming{Source: "alpha"}, WireName: "alpha", + Type: ir.TypeRef{Target: "t/x/A"}} + elsewhere := base + elsewhere.Provenance = ir.Provenance{Source: ir.NoSource, Pointer: "/paths/~1b/get"} + elsewhere.Required = true + elsewhere.Docs = ir.Docs{Summary: "copied into a second position"} + + doc := &ir.Document{Types: ir.TypeRegistry{ + "t/x/A": modelWithProp("t/x/A", base), + "t/x/B": modelWithProp("t/x/B", elsewhere), + }} + + assert.Empty(t, duplicateViolations(doc)) +} + // TestVerify_ReportsDuplicateIDs pins that Verify runs the check, not just the // test: an ambiguous identity has to reach a caller that only ever calls Verify. func TestVerify_ReportsDuplicateIDs(t *testing.T) { @@ -130,7 +198,7 @@ var identityClasses = map[string]string{ "AuthID": "identity: Document.Auth keys it; resolved and held as TypeID is", "OpID": "identity, no map: ir.Registries.WithDeclarations resolves references against the operations the document declares, checkDuplicateIDs holds them unique", "ServiceID": "identity, no map: resolved and held as OpID is, against the services the document declares", - "PropID": "identity, model-scoped: pass.Validate resolves references (checkPropIDRefs, checkEncodingKeys); not yet held unique, because a component's property is copied into every position referencing it — provisional, GitHub #280, see checkDuplicateIDs", + "PropID": "identity, model-scoped: pass.Validate resolves references (checkPropIDRefs, checkEncodingKeys); checkDuplicateIDs holds no two *different* properties to one ID, the copies a component makes of one property being exempt by fingerprint", "BigVal": "arbitrary-precision decimal, not an identity", "PrimKind": "primitive leaf kind; ir.PrimTypeID derives an ID from it, but the kind is not one", diff --git a/ir/irverify/ids.go b/ir/irverify/ids.go index 86f4da5..35bfe7f 100644 --- a/ir/irverify/ids.go +++ b/ir/irverify/ids.go @@ -51,9 +51,9 @@ func checkIDs(doc *ir.Document) []Violation { // produced by a compiler outside this tree, or rewritten by a pass is held by // this and nothing else — the reasoning that put the naming grammar in ir. // -// Deliberately not checked: whether the kind is one ir declares. This asks the -// ID to agree with the kind, and an invented kind agrees with itself — the node -// is consistent and wrong. Holding PrimKind to its constants is GitHub #240. +// Not checked here: whether the kind is one ir declares. This asks the ID to +// agree with the kind, and an invented kind agrees with itself — the node is +// consistent and wrong — so it is a separate claim, made by checkPrimKinds. func checkPrimIDs(doc *ir.Document) []Violation { var vs []Violation for id, td := range doc.Types { diff --git a/ir/irverify/ids_test.go b/ir/irverify/ids_test.go index 7c5931b..a5c12ef 100644 --- a/ir/irverify/ids_test.go +++ b/ir/irverify/ids_test.go @@ -131,6 +131,10 @@ func TestVerify_PrimitiveAwayFromItsSharedIDIsAViolation(t *testing.T) { // which is not an ID at all, so the message must not offer it as the place the // node belongs — a reader sent there fixes the wrong end, and the check would be // telling them to write an ID checkIDs reports as malformed. +// +// The kindless primitive breaks two claims at once and each is stated: the ID is +// not the one its kind derives, and the kind is not one ir declares. They name +// different repairs, so neither subsumes the other. func TestVerify_KindlessPrimitiveIsReportedOnItsOwnTerms(t *testing.T) { t.Parallel() const id ir.TypeID = "t/openapi/components/schemas/Name" @@ -139,11 +143,12 @@ func TestVerify_KindlessPrimitiveIsReportedOnItsOwnTerms(t *testing.T) { }} got := irverify.Verify(doc) - require.Len(t, got, 1) + require.Len(t, got, 2) assert.Equal(t, "ir/prim-id-not-derived", got[0].Code) assert.Contains(t, got[0].Message, "carries no kind") assert.NotContains(t, got[0].Message, string(ir.PrimTypeID("")), "t/prim/ is not an ID; naming it as the destination sends the reader to the wrong end") + assert.Equal(t, "ir/unknown-prim-kind", got[1].Code) } // TestVerify_NonPrimitiveInThePrimSpaceIsAViolation covers the other direction. diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index ec779b6..d2dccbc 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -31,6 +31,8 @@ func Verify(doc *ir.Document) []Violation { vs := checkRegistryKeys(doc) vs = append(vs, checkIDs(doc)...) vs = append(vs, checkPrimIDs(doc)...) + vs = append(vs, checkPrimKinds(doc)...) + vs = append(vs, checkAuthKinds(doc)...) vs = append(vs, checkDiagnostics(doc)...) vs = append(vs, runWalkChecks(doc)...) @@ -82,10 +84,12 @@ func walkChecks() []func(*ir.Document, declarations) ([]Violation, bool) { return []func(*ir.Document, declarations) ([]Violation, bool){ checkReferentialIntegrity, checkDuplicateIDs, + checkDeclaredIDs, checkNaming, checkRawPayloads, checkProvenance, checkIndices, + checkBigVals, } } diff --git a/ir/irverify/kinds.go b/ir/irverify/kinds.go new file mode 100644 index 0000000..8df20b8 --- /dev/null +++ b/ir/irverify/kinds.go @@ -0,0 +1,77 @@ +package irverify + +import ( + "strconv" + + "github.com/dexpace/morphic/ir" +) + +// checkPrimKinds asserts every primitive names a kind ir declares. +// +// checkPrimIDs cannot reach this and says so: it asks the ID to agree with the +// kind, and an invented kind agrees with itself — the node is consistent and +// wrong (GitHub #240). PrimKind is the vocabulary an emitter switches on to pick +// a target type, so a kind outside the declared set is the one case an emitter +// can neither lower nor report usefully, by which point nothing is left to say +// where the kind came from. +// +// Nothing rejects it earlier either. PrimKind is a plain string type with no +// UnmarshalJSON, so a document decoded from JSON or produced by a compiler +// outside this tree carries an invented kind in unchallenged. +// +// Document.Types is the only place a Primitive lives, so iterating it is the +// whole population. A primitive whose kind is empty is also reported by +// checkPrimIDs wherever it is interned somewhere other than the ID that derives +// from it: the two are different repairs — the ID is wrong there, the kind is +// wrong here — so each is stated on its own terms. +func checkPrimKinds(doc *ir.Document) []Violation { + var vs []Violation + for id, td := range doc.Types { + if ir.IsNilTypeDef(td) { + continue // checkRegistryKeys reports the nil entry itself + } + prim, isPrim := td.(*ir.Primitive) + if !isPrim || prim.Prim.Valid() { + continue + } + vs = append(vs, Violation{ + Code: "ir/unknown-prim-kind", + Message: "primitive carries undeclared kind " + strconv.Quote(string(prim.Prim)), + Path: "types[" + string(id) + "]", + }) + } + return vs +} + +// checkAuthKinds asserts every interned auth scheme names a mechanism ir +// declares. +// +// Document.Auth is reached otherwise only by checkRegistryKeys and checkIDs, +// which hold an entry to its key and its ID shape; nothing looks at what the +// scheme says, so a scheme whose mechanism is empty or misspelled verifies +// exactly like one that names oauth2 (GitHub #295). Kind is the only field of an +// AuthScheme backed by a declared constant set — In and the OAuth flow URLs are +// documented shapes rather than Go constants, and holding those here would be +// re-validating the source rather than checking a structure this repository +// minted. +// +// This is the class-level guard for a defect one compiler already refuses at the +// source (GitHub #294, #296). The compiler-side refusal could not be caught here +// or through internal/harness — harness.Check returns at the first error +// diagnostic, before Verify runs — so this exists for the reachable case: a +// compiler that mints the shape from an otherwise clean spec, including the +// compilers not written yet. +func checkAuthKinds(doc *ir.Document) []Violation { + var vs []Violation + for id, scheme := range doc.Auth { + if scheme.Kind.Valid() { + continue + } + vs = append(vs, Violation{ + Code: "ir/unknown-auth-kind", + Message: "auth scheme names undeclared mechanism " + strconv.Quote(string(scheme.Kind)), + Path: "auth[" + string(id) + "]", + }) + } + return vs +} diff --git a/ir/irverify/kinds_test.go b/ir/irverify/kinds_test.go new file mode 100644 index 0000000..cefd581 --- /dev/null +++ b/ir/irverify/kinds_test.go @@ -0,0 +1,111 @@ +package irverify_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irverify" +) + +// primDoc interns one primitive of kind at the ID that kind derives, so the +// document is sound in every way except the kind itself — checkPrimIDs has +// nothing to say about it and only the new claim can fire. +func primDoc(kind ir.PrimKind) *ir.Document { + id := ir.PrimTypeID(kind) + return &ir.Document{Types: ir.TypeRegistry{id: &ir.Primitive{ + TypeCommon: ir.TypeCommon{ID: id, Provenance: ir.Provenance{Source: ir.NoSource}}, + Prim: kind, + }}} +} + +// TestVerify_UndeclaredPrimKindIsAViolation is the case checkPrimIDs is blind +// to: the ID is derived from the kind, so it agrees with it — consistently, and +// wrongly. An emitter switching on PrimKind has no arm for any of these. +func TestVerify_UndeclaredPrimKindIsAViolation(t *testing.T) { + t.Parallel() + tests := []struct { + name string + kind ir.PrimKind + }{ + {name: "an invented kind", kind: "flt"}, + {name: "a re-cased declared kind", kind: "Float64"}, + {name: "a near miss on a declared kind", kind: "datetimeOffset"}, + {name: "not an identifier at all", kind: "¡no such kind¡"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := irverify.Verify(primDoc(tc.kind)) + require.Len(t, got, 1, "the ID derives from the kind, so nothing else can be wrong") + assert.Equal(t, "ir/unknown-prim-kind", got[0].Code) + assert.Equal(t, "types["+string(ir.PrimTypeID(tc.kind))+"]", got[0].Path) + assert.Contains(t, got[0].Message, string(tc.kind)) + }) + } +} + +// TestVerify_DeclaredPrimKindIsClean is the other half of the proof: a check +// that cannot stay silent is no better than one that cannot fire. The documents +// differ from the ones above only in the kind, so nothing but the kind can be +// what the check reads. +func TestVerify_DeclaredPrimKindIsClean(t *testing.T) { + t.Parallel() + for _, kind := range []ir.PrimKind{ir.PrimString, ir.PrimFloat64, ir.PrimDatetimeOffset, ir.PrimAny} { + t.Run(string(kind), func(t *testing.T) { + t.Parallel() + assert.Empty(t, irverify.Verify(primDoc(kind))) + }) + } +} + +// authDoc interns one auth scheme naming kind, keyed by and carrying a +// well-formed ID and a full naming, so nothing but the mechanism is open to +// question. +func authDoc(kind ir.AuthKind) *ir.Document { + const id ir.AuthID = "auth/openapi/components/securitySchemes/token" + return &ir.Document{Auth: map[ir.AuthID]ir.AuthScheme{id: { + ID: id, Name: named("token"), Kind: kind, + Provenance: ir.Provenance{Source: ir.NoSource}, + }}} +} + +// TestVerify_UndeclaredAuthKindIsAViolation covers the class the compiler-side +// refusal closed one instance of. The empty kind is the shape that reached the +// IR: a scheme interned naming no mechanism at all, indistinguishable from a +// sound one to every check that reads only a key and an ID. +func TestVerify_UndeclaredAuthKindIsAViolation(t *testing.T) { + t.Parallel() + tests := []struct { + name string + kind ir.AuthKind + }{ + {name: "no mechanism at all", kind: ""}, + {name: "a misspelled mechanism", kind: "api_key"}, + {name: "an invented mechanism", kind: "mtls-but-not-really"}, + {name: "a re-cased mechanism", kind: "OAuth2"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := irverify.Verify(authDoc(tc.kind)) + require.Len(t, got, 1) + assert.Equal(t, "ir/unknown-auth-kind", got[0].Code) + assert.Equal(t, "auth[auth/openapi/components/securitySchemes/token]", got[0].Path) + }) + } +} + +// TestVerify_DeclaredAuthKindIsClean holds the silent half: the same document +// with a mechanism ir declares must yield nothing. +func TestVerify_DeclaredAuthKindIsClean(t *testing.T) { + t.Parallel() + for _, kind := range []ir.AuthKind{ir.AuthKindOAuth2, ir.AuthKindAPIKey, ir.AuthKindX509, ir.AuthKindCustom} { + t.Run(string(kind), func(t *testing.T) { + t.Parallel() + assert.Empty(t, irverify.Verify(authDoc(kind))) + }) + } +} diff --git a/ir/registries.go b/ir/registries.go index 9abd000..5423864 100644 --- a/ir/registries.go +++ b/ir/registries.go @@ -210,10 +210,10 @@ type IDDeclaration struct { // the wrong defect. // // Whether the empty ID is itself reported is a separate claim, and one this -// derivation does not make. A class Document keys a map by is covered — an empty -// or disagreeing key is what irverify.checkRegistryKeys reads — but an Operation -// and a Service have no key for it to read, so an empty ID on either goes -// unreported (GitHub #289). +// derivation does not make. A class Document keys a map by is covered by the key +// — an empty or disagreeing one is what irverify.checkRegistryKeys reads — and a +// class with no key, an Operation, a Service or a Property, is covered by +// irverify.checkDeclaredIDs walking for what this drops. func DeclaredIDs(doc *Document) ([]IDDeclaration, bool) { var decls []IDDeclaration truncated := WalkValues(doc, DocumentPath, func(v reflect.Value, path string) bool { diff --git a/ir/typedef_completeness_test.go b/ir/typedef_completeness_test.go index 9bfd0fd..26a36e7 100644 --- a/ir/typedef_completeness_test.go +++ b/ir/typedef_completeness_test.go @@ -3,12 +3,8 @@ package ir_test import ( "encoding/json" "go/ast" - "go/parser" - "go/token" "reflect" "slices" - "strconv" - "strings" "testing" "github.com/google/go-cmp/cmp" @@ -200,62 +196,15 @@ func concreteName(t *testing.T, td ir.TypeDef) string { // disagreeing silently: that is what every test in this file exists to catch. func declaredTypeKinds(t *testing.T) []typeKindConst { t.Helper() - var out []typeKindConst - for _, f := range parseIRSources(t) { - for _, decl := range f.Decls { - gd, isGen := decl.(*ast.GenDecl) - if !isGen || gd.Tok != token.CONST { - continue - } - out = append(out, typeKindConstsIn(t, gd)...) - } + declared := declaredConstsOfType(t, "TypeKind") + require.NotEmpty(t, declared, "the ir sources must declare TypeKind constants") + out := make([]typeKindConst, 0, len(declared)) + for _, c := range declared { + out = append(out, typeKindConst{name: c.name, kind: ir.TypeKind(c.value)}) } - require.NotEmpty(t, out, "the ir sources must declare TypeKind constants") - slices.SortFunc(out, func(a, b typeKindConst) int { return strings.Compare(a.name, b.name) }) return out } -// typeKindConstsIn returns the TypeKind constants of one const group. A spec -// declaring neither type nor value repeats the previous spec, so the group's last -// explicit type carries forward; a spec with its own value declares its own type. -func typeKindConstsIn(t *testing.T, gd *ast.GenDecl) []typeKindConst { - t.Helper() - var out []typeKindConst - isKind := false - for _, spec := range gd.Specs { - vs, isValue := spec.(*ast.ValueSpec) - require.True(t, isValue, "const spec is not a ValueSpec: %#v", spec) - switch { - case vs.Type != nil: - id, isIdent := vs.Type.(*ast.Ident) - isKind = isIdent && id.Name == "TypeKind" - case len(vs.Values) > 0: - isKind = false - } - if !isKind { - continue - } - for i, name := range vs.Names { - require.Less(t, i, len(vs.Values), - "TypeKind constant %s must declare its own value", name.Name) - value := stringLit(t, name.Name, vs.Values[i]) - out = append(out, typeKindConst{name: name.Name, kind: ir.TypeKind(value)}) - } - } - return out -} - -// stringLit returns the string a constant's value expression spells out. -func stringLit(t *testing.T, constName string, expr ast.Expr) string { - t.Helper() - lit, isLit := expr.(*ast.BasicLit) - require.True(t, isLit, "constant %s must be declared as a string literal", constName) - require.Equal(t, token.STRING, lit.Kind, "constant %s must be declared as a string literal", constName) - unquoted, err := strconv.Unquote(lit.Value) - require.NoError(t, err, "unquoting the value of %s", constName) - return unquoted -} - // sealedTypeDefImpls returns, sorted, the name of every ir type carrying the // unexported typeDef() marker. func sealedTypeDefImpls(t *testing.T) []string { @@ -287,18 +236,3 @@ func receiverTypeName(t *testing.T, fd *ast.FuncDecl) string { require.True(t, isIdent, "receiver of %s must be a named type, got %#v", fd.Name.Name, expr) return id.Name } - -// parseIRSources parses every file irSourceFiles lists, under one FileSet. -func parseIRSources(t *testing.T) []*ast.File { - t.Helper() - paths := irSourceFiles(t) - fset := token.NewFileSet() - out := make([]*ast.File, 0, len(paths)) - for _, path := range paths { - f, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) - require.NoError(t, err, "parsing %s", path) - out = append(out, f) - } - require.NotEmpty(t, out, "the ir package must have production sources") - return out -} diff --git a/ir/types.go b/ir/types.go index 314ec23..6c9418e 100644 --- a/ir/types.go +++ b/ir/types.go @@ -61,6 +61,27 @@ const ( PrimAny PrimKind = "any" ) +// Valid reports whether k is one of the kinds declared above. PrimKind is a bare +// string enum, so nothing rejects an invented or stale value on the wire, and +// PrimTypeID derives a consistent ID from any string it is handed — an invented +// kind agrees with its own ID and reads as sound. irverify calls this so a kind +// no emitter can switch on is reported as the compiler bug it is, rather than +// reaching a target that has no type to lower it to. +func (k PrimKind) Valid() bool { + switch k { + case PrimBool, PrimString, PrimBytes, + PrimInt8, PrimInt16, PrimInt32, PrimInt64, + PrimUint8, PrimUint16, PrimUint32, PrimUint64, + PrimInteger, PrimFloat32, PrimFloat64, PrimFloat, PrimNumber, + PrimDecimal, PrimDecimal128, + PrimDate, PrimTime, PrimDatetime, PrimDatetimeOffset, PrimDuration, + PrimURL, PrimUUID, PrimAny: + return true + default: + return false + } +} + // AdditionalMode describes the openness of a model's property set beyond its // declared properties and AdditionalProps (ir-design §4.3). type AdditionalMode string diff --git a/ir/types_test.go b/ir/types_test.go index a5d3aab..83ab237 100644 --- a/ir/types_test.go +++ b/ir/types_test.go @@ -382,6 +382,50 @@ func TestPrimKind_Constants(t *testing.T) { assertConstantSpellings(t, primKindSpellings, "unspecified") } +// TestPrimKind_TiesToConstBlock closes the gap a bare string enum leaves: +// nothing rejects an invented kind on deserialization, and ir.PrimTypeID derives +// a consistent ID from whatever string it is handed, so irverify has Valid and +// nothing else to test against. Valid therefore has to stay tied to the const +// block, and it is tied here by parsing the ir sources rather than by a list — +// the shape typedef_completeness_test.go uses for the TypeKind sum. +// +// Two directions are enforced: primKindSpellings and the declared constants name +// the same set, and every declared kind is Valid. Adding a constant without +// teaching Valid about it fails here rather than surfacing later as a spurious +// ir/unknown-prim-kind on a perfectly good document. +// +// The converse of the second direction cannot be enforced from here: a case +// added to Valid for a kind no constant declares leaves this green, because a +// switch body is not enumerable at run time. +func TestPrimKind_TiesToConstBlock(t *testing.T) { + t.Parallel() + declared := declaredConstsOfType(t, "PrimKind") + require.NotEmpty(t, declared, "the ir sources must declare PrimKind constants") + + values := make([]string, 0, len(declared)) + for _, c := range declared { + values = append(values, c.value) + assert.True(t, ir.PrimKind(c.value).Valid(), "declared kind %q must be Valid", c.value) + } + + spelled := make([]string, 0, len(primKindSpellings)) + for _, s := range primKindSpellings { + spelled = append(spelled, s) + } + require.ElementsMatch(t, spelled, values) +} + +// TestPrimKind_UnknownIsInvalid pins the other direction: Valid must reject a +// kind no const declares, including the zero value a compiler leaves behind when +// it forgets the field, and a re-cased spelling of a real one. +func TestPrimKind_UnknownIsInvalid(t *testing.T) { + t.Parallel() + assert.False(t, ir.PrimKind("").Valid()) + assert.False(t, ir.PrimKind("flt").Valid()) + assert.False(t, ir.PrimKind("Float64").Valid()) + assert.False(t, ir.PrimKind("datetimeOffset").Valid()) +} + // TestAdditionalMode_Constants pins the on-disk spelling of every // AdditionalMode value, including the empty-string "unspecified" state. func TestAdditionalMode_Constants(t *testing.T) {