diff --git a/compilers/openapi/internal/annotation/annotation.go b/compilers/openapi/internal/annotation/annotation.go index f222497..8ba4b1e 100644 --- a/compilers/openapi/internal/annotation/annotation.go +++ b/compilers/openapi/internal/annotation/annotation.go @@ -503,6 +503,12 @@ func DeclaredSchema(js *oas3.JSONSchema[oas3.Referenceable]) *oas3.JSONSchema[oa // RawChildNode returns the raw YAML value node of a mapping child keyed by the // on-wire name, unwrapping a document node first; nil when absent. It reads exact // literals the high-level model does not preserve (links, servers, content maps). +// +// The last pair spelling the key wins, which is the pair the parser reads: +// marshaller skips every occurrence of a repeated key but the last. Returning +// the first instead described a mapping by a value nothing else in the compiler +// uses — reachable once a key can be spelled two ways, since an explicit pair +// and an aliased one are one key to the parser and two nodes here. func RawChildNode(root *yaml.Node, key string) *yaml.Node { if root == nil { return nil @@ -513,12 +519,28 @@ func RawChildNode(root *yaml.Node, key string) *yaml.Node { if root.Kind != yaml.MappingNode { return nil } + var found *yaml.Node for i := 0; i+1 < len(root.Content); i += 2 { - if root.Content[i].Value == key { - return root.Content[i+1] + if keyName(root.Content[i]) == key { + found = root.Content[i+1] } } - return nil + return found +} + +// keyName is the on-wire name a mapping key node spells, following an alias to +// the scalar it stands for. +// +// yaml.v3 leaves an alias node's own Value as the anchor name, so a key written +// as an alias matches nothing when read raw — while the parser reads that key +// under the name it resolves to, which is the name every caller here looks up. +// Comparing the two spellings is what let a key the model reported as +// undeclared reach no Unmodeled entry at all (GitHub #297). +func keyName(n *yaml.Node) string { + if n.Kind == yaml.AliasNode && n.Alias != nil { + return n.Alias.Value + } + return n.Value } // The readers below consume a Site the caller supplies rather than resolving diff --git a/compilers/openapi/internal/annotation/readers_internal_test.go b/compilers/openapi/internal/annotation/readers_internal_test.go index e0ee66f..093de97 100644 --- a/compilers/openapi/internal/annotation/readers_internal_test.go +++ b/compilers/openapi/internal/annotation/readers_internal_test.go @@ -515,6 +515,51 @@ func TestRawChildNode_ReadsOnlyAMappingChild(t *testing.T) { assert.Nil(t, RawChildNode(&yaml.Node{Kind: yaml.DocumentNode}, "a"), "nor an empty document") } +// TestRawChildNode_FindsAKeyWrittenAsAnAlias pins the one spelling where the raw +// tree and the parsed model disagree about a key's name. yaml.v3 leaves an alias +// node's own Value as the anchor, so matching it raw looks for "k" while the +// parser has already read the pair under "aliasedKey" — and every caller here +// asks by the name the parser used. A key the census reported as undeclared then +// reached no Unmodeled entry and, before this, no diagnostic either. +func TestRawChildNode_FindsAKeyWrittenAsAnAlias(t *testing.T) { + t.Parallel() + var doc yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("anchor: &k aliasedKey\n*k : found\n"), &doc)) + + found := RawChildNode(&doc, "aliasedKey") + + require.NotNil(t, found, "the key is looked up by the name it resolves to") + assert.Equal(t, "found", found.Value) + assert.Nil(t, RawChildNode(&doc, "k"), "and not by the anchor it is written as") +} + +// TestRawChildNode_RepeatedKeyReadsTheLastPair holds this reader to the pair the +// parser reads: marshaller skips every occurrence of a repeated key but the +// last, so returning the first would describe the mapping by a value nothing +// else in the compiler uses. +// +// Spelled with an alias, because that is how the case is reachable — yaml.v3 +// refuses a key written twice the same way, while an explicit pair and an +// aliased one are two nodes here and one key to the parser. +func TestRawChildNode_RepeatedKeyReadsTheLastPair(t *testing.T) { + t.Parallel() + for _, tc := range []struct{ name, body, want string }{ + {"aliased pair last", "anchor: &k dup\ndup: first\n*k : last\n", "last"}, + {"aliased pair first", "anchor: &k dup\n*k : first\ndup: last\n", "last"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var doc yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(tc.body), &doc)) + + found := RawChildNode(&doc, "dup") + + require.NotNil(t, found) + assert.Equal(t, tc.want, found.Value, "the last pair spelling the key is the effective one") + }) + } +} + // TestRawPropertyNode_NilSchemaReadsNothing pins the nil guard on the schema // side of the same reader, which every caller relies on to ask about a position // that may have no body written at it. diff --git a/compilers/openapi/internal/annotation/unknown.go b/compilers/openapi/internal/annotation/unknown.go new file mode 100644 index 0000000..bcb0326 --- /dev/null +++ b/compilers/openapi/internal/annotation/unknown.go @@ -0,0 +1,301 @@ +package annotation + +import ( + "reflect" + "slices" + + oas3 "github.com/speakeasy-api/openapi/jsonschema/oas3" + yaml "gopkg.in/yaml.v3" + + "github.com/dexpace/morphic/compilers/openapi/internal/diag" + "github.com/dexpace/morphic/compilers/openapi/internal/ids" + "github.com/dexpace/morphic/ir" +) + +// unreachableKeyDiag reports a key the census named whose value the raw mapping +// does not present, so nothing of it reached the IR. +// +// It is a key the document does write. The parser reads a mapping through its +// `<<` merge keys, so a key merged in from an anchored mapping is reported here +// while the mapping this reads holds no pair for it; resolving that needs the +// merge-expanded view (internal/nodeview), which every raw-node reader in this +// package lacks and which is a mechanism of its own to thread through — see +// GitHub #395. Announced rather than passed over in the meantime, since a key +// that reaches the IR in no form at all is the loss this census exists to end. +// +// Warning rather than the error UnpreservableDiag gives a value that cannot be +// rendered: a document merging keys is legal input and still lowers, and an +// error would both refuse it under the default --fail-on and stop harness.Check +// before the invariant checks run. +func unreachableKeyDiag(entry, at string, srcIndex int) ir.Diagnostic { + return diag.Newf(ir.SeverityWarning, diag.UnknownKeyUnreachable, + ir.Provenance{Source: srcIndex, Pointer: at}, + "%s is written at a key the source mapping does not present directly, most likely "+ + "merged in through a `<<`; it is represented in the IR in no form at all", entry) +} + +// occupiedEntryDiag reports a key whose Unmodeled entry is already held by a +// construct written somewhere else, so this one reached the IR in no form. +// +// The carriers that hold more than one object's entries are where this happens: +// an ir.Parameter's map carries the parameter's own keys and everything its +// schema had no home for, both unscoped, so a parameter writing a key its schema +// also writes as a keyword spells one entry between them. Which of the two +// survives is decided by lowering order rather than by the document — see +// GitHub #396, which is the namespace this census cannot settle on its own, +// since the entries it would collide with are three other mechanisms' and moving +// either side moves keys they already publish. +func occupiedEntryDiag(entry, at, held string, srcIndex int) ir.Diagnostic { + return diag.Newf(ir.SeverityWarning, diag.UnknownKeyEntryTaken, + ir.Provenance{Source: srcIndex, Pointer: at}, + "%s is already held by the construct at %q, so this key is represented in the IR in "+ + "no form at all", entry, held) +} + +// MaxUnknownKeys bounds how many keys one object contributes to the IR. +// +// The key set is the document's to choose the size of, and every collection in +// this compiler is bounded, so this one is too. It sits far above what a +// document writes by accident, so an object reaching it is generated or hostile +// rather than merely sloppy, and what it discards is announced under +// diag.UnknownKeyBudget rather than dropped in silence. +// +// It bounds the keys this census answers for, not the keys the object wrote: one +// another reader already kept is filtered out before the bound applies, since +// spending a slot on an entry that is in the document either way would drop a key +// that is not. A key that is counted but proves unreachable still spends its slot +// — reachability costs the same lookup as keeping it, so a bound that excluded +// those would have to do the work twice to decide what it bounds. +// +// It bounds the diagnostics too, at one per key plus the budget's own, which is +// what keeps an object whose every key is unreachable from reporting without end. +const MaxUnknownKeys = 64 + +// DecidedKeywords are the JSON Schema keywords the library's schema model names +// no field for and this compiler has already decided about, so the census must +// not claim them as unread. Each decision is recorded where it was made, and the +// schema walk's 2020-12 vocabulary test fails if one starts being carried: +// +// - $comment — 2020-12 §8.3 forbids presenting it to end users, so no SDK +// emitter may see it. Dropped on purpose. +// - $dynamicAnchor — read by the anchor index as a reference target, which is +// what lets a $dynamicRef expand; declaring one says nothing about the shape. +// - $dynamicRef — carried by the dynamic-reference lowering, which either +// expands it into the position's type or keeps it under a reason of its own. +// An entry beside an expanded one would tell a consumer the compiler ignored +// a reference it had in fact resolved. +// +// The other 2020-12 keywords with no field of their own — $vocabulary and +// dependentRequired — need no entry here. Their readers write to the same map, +// so the census finds them already recorded and leaves them alone. +var DecidedKeywords = []string{"$comment", "$dynamicAnchor", "$dynamicRef"} + +// UnknownKeywordsIn records on p the keywords s writes that no field of the JSON +// Schema model names, and announces each. +// +// OpenAPI 3.1 schemas are JSON Schema 2020-12, where an unrecognized keyword is +// legal input: the specification requires an implementation to ignore what it +// does not recognize and allows such a keyword to carry meaning for other +// tooling. So this reports a decision rather than a fault, and is graded +// accordingly — see diag.UnknownSchemaKeyword. +// +// It keeps only what no other reader kept, which is why it runs after all of +// them: `$vocabulary` and `dependentRequired` have no field in the model either +// and are read straight off the raw node by readers with more to say about them, +// so the census finds those already recorded and leaves them alone. A keyword no +// reader leaves a trace of needs naming in DecidedKeywords instead. +func UnknownKeywordsIn(p *ir.Unmodeled, s *oas3.Schema, pointer string, srcIndex int) []ir.Diagnostic { + return census(p, s, srcIndex, pointer, "", keyClass{ + code: diag.UnknownSchemaKeyword, + severity: ir.SeverityInfo, + skip: DecidedKeywords, + message: "keyword %q has no field in the schema model this compiler lowers and no IR " + + "position of its own; kept verbatim under Unmodeled", + }) +} + +// UnknownKeysIn records on p the keys an OpenAPI object writes that the +// specification neither defines nor admits as an extension, for an object +// lowering to a node with an Unmodeled map of its own. owner is the object's own +// source pointer. +// +// Unlike its schema neighbour this reports a fault: OpenAPI gives each of its +// objects a closed key set and requires every extension to be prefixed x-, so a +// key that is neither is nothing the document is permitted to write — in +// practice a misspelling of the field beside it. It is kept all the same, +// because invariant 2 does not bend for invalid input, and a misspelt key is the +// one a reader most needs to find. +func UnknownKeysIn(p *ir.Unmodeled, model any, srcIndex int, owner string) []ir.Diagnostic { + return UnknownKeysUnder(p, model, srcIndex, owner, "") +} + +// UnknownKeysUnder is UnknownKeysIn with every entry keyed beneath scope, for +// the objects with no Unmodeled map of their own, whose keys ride on the nearest +// node that has one — an info object's on the document, a tag's on the document. +// +// scope says which object wrote them: the source path from the carrier down to +// the object. Several objects reach one map, where "openapi:status" from two of +// them would be a single key and the entry that survived would depend on which +// lowering ran last. +func UnknownKeysUnder(p *ir.Unmodeled, model any, srcIndex int, owner, scope string) []ir.Diagnostic { + return census(p, model, srcIndex, owner, scope, keyClass{ + code: diag.UnknownObjectKey, + severity: ir.SeverityWarning, + message: "key %q is not defined by the OpenAPI object it is written on and is not an " + + "x- extension; kept verbatim under Unmodeled", + }) +} + +// keyClass is how a key the model does not name is graded: which diagnostic +// announces it, and at what severity. +// +// The reason is not part of it. Both classes carry ReasonOutOfScope, because +// that is a property of the construct rather than of the document: no IR node is +// coming for a key the format does not define, nor for one a schema dialect +// defines and this compiler does not model, so an emitter policy layer is the +// only consumer either has. Which of the two a key is says something about the +// source, and the diagnostic channel is where this compiler says that. +type keyClass struct { + code string + severity ir.Severity + skip []string // keywords already decided about; see DecidedKeywords + message string // one %q, filled with the key +} + +// census records on p every key model's source object wrote that its own model +// names no field for, each under its own key beneath scope. +// +// A key p already holds for this very construct is left alone and not announced: +// the census is the complement of everything the compiler read, not only of what +// the model names, and a reader with a reason of its own for a keyword has +// already said it better. Those are filtered before the bound applies — see +// MaxUnknownKeys. An entry held for a construct written elsewhere is a collision +// rather than a keyword already handled, and keep reports it. +func census(p *ir.Unmodeled, model any, srcIndex int, owner, scope string, cl keyClass) []ir.Diagnostic { + keys, root := undeclaredKeys(model) + fresh := unrecorded(p, keys, owner, scope, cl.skip) + if len(fresh) == 0 { + return nil + } + var diags []ir.Diagnostic + if len(fresh) > MaxUnknownKeys { + diags = append(diags, budgetDiag(len(fresh), owner, srcIndex)) + fresh = fresh[:MaxUnknownKeys] + } + for _, key := range fresh { + diags = append(diags, keep(p, root, key, srcIndex, owner, scope, cl)...) + } + return diags +} + +// keep writes one key's value under its entry and announces it, or says why it +// could not. +func keep(p *ir.Unmodeled, root *yaml.Node, key string, srcIndex int, owner, scope string, cl keyClass) []ir.Diagnostic { + entry, at := "openapi:"+scoped(scope, key), owner+ids.Ptr(key) + if taken, occupied := (*p)[entry]; occupied { + return []ir.Diagnostic{occupiedEntryDiag(entry, at, taken.Provenance.Pointer, srcIndex)} + } + node := RawChildNode(root, key) + if node == nil { + return []ir.Diagnostic{unreachableKeyDiag(entry, at, srcIndex)} + } + kept, diags := PreserveNodeInto(p, entry, node, ir.ReasonOutOfScope, at, srcIndex) + if !kept { + return diags + } + return append(diags, diag.Newf(cl.severity, cl.code, + ir.Provenance{Source: srcIndex, Pointer: at}, cl.message, key)) +} + +// unrecorded returns the keys this census has to answer for: the ones cl has not +// decided about, less the ones a reader already recorded for the very construct +// this census would record. +// +// Sameness is the entry's provenance, not the entry's presence. A reader with +// more to say about a keyword writes it at the pointer the census would use — +// `$vocabulary` and `dependentRequired` on a schema's own map — and there the +// census has nothing to add. An entry pointing somewhere else is a different +// construct that happens to spell the same key, which is a collision rather than +// a keyword already handled, and keep reports it. +func unrecorded(p *ir.Unmodeled, keys []string, owner, scope string, skip []string) []string { + out := make([]string, 0, len(keys)) + for _, key := range keys { + if slices.Contains(skip, key) { + continue + } + if e, recorded := (*p)["openapi:"+scoped(scope, key)]; recorded && + e.Provenance.Pointer == owner+ids.Ptr(key) { + continue + } + out = append(out, key) + } + return out +} + +// scoped spells one entry's key on the carrier holding it. +// +// The key is escaped as one segment while scope is a path of literals already +// spelled that way, which is what stops a key holding a "/" from reading as a +// scope of its own: a root key spelled "info/contact/slack" would otherwise be +// the very entry the contact object's own "slack" keys under, and the second +// site to reach the carrier would find the first already there and drop its key +// without a word. ids.Scope records that rule for the scopes a document chooses +// the segments of; a key the document chose every character of needs it too. +func scoped(scope, key string) string { + if scope == "" { + return ids.Scope(key) + } + return scope + "/" + ids.Scope(key) +} + +// budgetDiag reports the keys past MaxUnknownKeys, which reach the IR in no form. +func budgetDiag(total int, owner string, srcIndex int) ir.Diagnostic { + return diag.Newf(ir.SeverityWarning, diag.UnknownKeyBudget, + ir.Provenance{Source: srcIndex, Pointer: owner}, + "object writes %d keys its model names no field for and no other reader kept, past the "+ + "%d this compiler keeps; the rest are represented in the IR in no form at all", + total, MaxUnknownKeys) +} + +// parsedObject is the part of a parsed model the census reads: its core, which +// holds the census the unmarshaller took, and the mapping node the keys were +// written on. +// +// Declared here rather than taken from the library, so this package depends on +// the shape it uses rather than on the marshaller package, and so a test can +// drive the branches below with a model of its own. +type parsedObject interface { + GetCoreAny() any + GetRootNode() *yaml.Node +} + +// unknownReporter is a core model's own record of the keys it did not name. +type unknownReporter interface{ GetUnknownProperties() []string } + +// undeclaredKeys returns, sorted, the keys model's source object wrote that its +// model names no field for, and the mapping node they were written on. +// +// Sorted, and on a copy: the library fills that list from a parallel walk of the +// mapping under a mutex, so its order is neither source order nor stable, and +// the slice it hands back is the model's own. An unsorted read would order this +// compiler's diagnostics by something the source does not decide, which +// invariant 7 forbids. +// +// A model reporting no census yields nothing rather than panicking. The receiver +// may be a typed nil — an absent object is what the getters return for one the +// document omitted — and a promoted method on one of those dereferences it. +func undeclaredKeys(model any) ([]string, *yaml.Node) { + v := reflect.ValueOf(model) + if v.Kind() == reflect.Pointer && v.IsNil() { + return nil, nil + } + obj, ok := model.(parsedObject) + if !ok { + return nil, nil + } + core, ok := obj.GetCoreAny().(unknownReporter) + if !ok { + return nil, nil + } + return slices.Sorted(slices.Values(core.GetUnknownProperties())), obj.GetRootNode() +} diff --git a/compilers/openapi/internal/annotation/unknown_internal_test.go b/compilers/openapi/internal/annotation/unknown_internal_test.go new file mode 100644 index 0000000..e92a1e6 --- /dev/null +++ b/compilers/openapi/internal/annotation/unknown_internal_test.go @@ -0,0 +1,292 @@ +package annotation + +import ( + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + yaml "gopkg.in/yaml.v3" + + "github.com/dexpace/morphic/ir" +) + +// TestUnknownKeywordsIn_KeepsWhatTheModelDoesNotName is the census at its own +// subject: a keyword with no field in the schema model reaches the IR under its +// own key, located at itself, and is announced at info. +func TestUnknownKeywordsIn_KeepsWhatTheModelDoesNotName(t *testing.T) { + t.Parallel() + s := schemaFromYAML(t, "type: string\nx-ray: kept\nnotAKeyword: 7\n") + + var got ir.Unmodeled + diags := UnknownKeywordsIn(&got, s, "/components/schemas/A", 3) + + require.Len(t, got, 1, "the x-* is the extension reader's, not the census's; got %v", got) + entry := got["openapi:notAKeyword"] + assert.Equal(t, ir.ReasonOutOfScope, entry.Reason) + assert.Equal(t, ir.RawValue("7"), entry.Value) + assert.Equal(t, ir.Provenance{Source: 3, Pointer: "/components/schemas/A/notAKeyword"}, entry.Provenance) + + require.Len(t, diags, 1) + assert.Equal(t, ir.SeverityInfo, diags[0].Severity) + assert.Equal(t, "openapi/unknown-schema-keyword", diags[0].Code) + assert.Contains(t, diags[0].Message, `"notAKeyword"`) +} + +// TestUnknownKeywordsIn_DecidedKeywordsAreLeftAlone holds the census to the +// decisions already recorded elsewhere. Each of these has no field in the schema +// model, so the census would otherwise claim all three and overrule a +// deliberate drop — or, for an expanded $dynamicRef, say the compiler ignored a +// reference it resolved. +func TestUnknownKeywordsIn_DecidedKeywordsAreLeftAlone(t *testing.T) { + t.Parallel() + assert.Equal(t, []string{"$comment", "$dynamicAnchor", "$dynamicRef"}, DecidedKeywords, + "a keyword joining or leaving the exclusion must be decided here too") + + var body strings.Builder + body.WriteString("type: string\n") + for _, keyword := range DecidedKeywords { + body.WriteString(keyword + ": v\n") + } + s := schemaFromYAML(t, body.String()) + + var got ir.Unmodeled + diags := UnknownKeywordsIn(&got, s, "/components/schemas/A", 0) + + assert.Empty(t, got, "each is decided about elsewhere, so the census keeps none of them") + assert.Empty(t, diags) +} + +// TestUnknownKeywordsIn_AlreadyRecordedKeyIsLeftAlone is why the census runs +// last. dependentRequired has no field in the model either and is kept by the +// validation-only reader under a reason that says what it is; a census that +// overwrote it would replace that with a weaker one and announce the keyword +// twice. +func TestUnknownKeywordsIn_AlreadyRecordedKeyIsLeftAlone(t *testing.T) { + t.Parallel() + s := schemaFromYAML(t, "type: object\ndependentRequired: {a: [b]}\n") + already := ir.UnmodeledEntry{ + Reason: ir.ReasonValidationOnly, + Value: ir.RawValue(`{"a":["b"]}`), + Provenance: ir.Provenance{Pointer: "/A/dependentRequired"}, + } + got := ir.Unmodeled{"openapi:dependentRequired": already} + + diags := UnknownKeywordsIn(&got, s, "/A", 0) + + assert.Equal(t, ir.Unmodeled{"openapi:dependentRequired": already}, got) + assert.Empty(t, diags, "a keyword another reader already announced is not announced twice") +} + +// TestUnknownKeywordsIn_UnpreservableValueIsReportedNotKept holds the census to +// GitHub #144's rule: a value that cannot be rendered as JSON keeps nothing, and +// says so, rather than announcing a preservation that did not happen. +func TestUnknownKeywordsIn_UnpreservableValueIsReportedNotKept(t *testing.T) { + t.Parallel() + s := schemaFromYAML(t, "type: string\nnotAKeyword: .nan\n") + + var got ir.Unmodeled + diags := UnknownKeywordsIn(&got, s, "/A", 0) + + assert.Empty(t, got) + require.Len(t, diags, 1) + assert.Equal(t, ir.SeverityError, diags[0].Severity) + assert.Equal(t, "openapi/unpreservable-construct", diags[0].Code) +} + +// TestUnknownKeysUnder_KeysBeneathTheScopeAndSorted pins the OpenAPI-object +// class: entries key under the path that says which object wrote them, the +// grading is a warning because the format admits no such key, and both the +// entries and the findings come out in key order. +// +// Sorted matters on its own. The library builds its census from a parallel walk +// of the mapping, so the order it hands back is neither source order nor stable, +// and diagnostics ordered by it would make the document non-deterministic +// (invariant 7). +func TestUnknownKeysUnder_KeysBeneathTheScopeAndSorted(t *testing.T) { + t.Parallel() + reported := []string{"zeta", "alpha"} + obj := fakeObject{core: &fakeCore{keys: reported}, root: parsedMapping(t, "zeta: 1\nalpha: 2\n")} + + var got ir.Unmodeled + diags := UnknownKeysUnder(&got, obj, 1, "/info/contact", "info/contact") + + assert.Equal(t, []string{"zeta", "alpha"}, reported, "the model's own slice is not reordered") + require.Len(t, got, 2) + assert.Equal(t, ir.RawValue("2"), got["openapi:info/contact/alpha"].Value) + assert.Equal(t, ir.Provenance{Source: 1, Pointer: "/info/contact/zeta"}, + got["openapi:info/contact/zeta"].Provenance) + + require.Len(t, diags, 2) + assert.Equal(t, ir.SeverityWarning, diags[0].Severity) + assert.Equal(t, "openapi/unknown-object-key", diags[0].Code) + assert.Contains(t, diags[0].Message, `"alpha"`, "the findings follow the sorted keys") + assert.Contains(t, diags[1].Message, `"zeta"`) +} + +// TestUnknownKeysIn_BudgetBoundsWhatOneObjectContributes exercises the bound. An +// object writing more undeclared keys than MaxUnknownKeys keeps exactly that +// many and reports the remainder, which is what stops the discarded tail from +// being the silent loss this census exists to end. +func TestUnknownKeysIn_BudgetBoundsWhatOneObjectContributes(t *testing.T) { + t.Parallel() + const over = MaxUnknownKeys + 3 + keys := make([]string, 0, over) + var body strings.Builder + for i := range over { + // Zero-padded, so sorting the census is sorting the source order too and the + // key that survives the truncation is a predictable one. + key := "k" + strconv.Itoa(1000+i) + keys = append(keys, key) + body.WriteString(key + ": " + strconv.Itoa(i) + "\n") + } + obj := fakeObject{core: &fakeCore{keys: keys}, root: parsedMapping(t, body.String())} + + var got ir.Unmodeled + diags := UnknownKeysIn(&got, obj, 0, "/x") + + assert.Len(t, got, MaxUnknownKeys) + assert.NotContains(t, got, "openapi:k"+strconv.Itoa(1000+over-1), "the tail past the bound is dropped") + require.NotEmpty(t, diags) + assert.Equal(t, "openapi/unknown-key-budget", diags[0].Code) + assert.Equal(t, ir.SeverityWarning, diags[0].Severity) + assert.Equal(t, ir.Provenance{Pointer: "/x"}, diags[0].Provenance) +} + +// TestUnknownKeysIn_KeyWithNoSourceNodeIsReported covers the one outcome +// PreserveNodeInto's contract does not: a key whose value node is not in the +// mapping. +// +// Everywhere else an absent node means the construct was never written, which is +// why it records nothing and says nothing. Here the parser has already reported +// the key as present, so an absent node means this reader could not reach one +// the document does write — a merged-in key, in practice — and passing over it +// would be the silent loss this census exists to end (GitHub #395). +func TestUnknownKeysIn_KeyWithNoSourceNodeIsReported(t *testing.T) { + t.Parallel() + obj := fakeObject{core: &fakeCore{keys: []string{"absent"}}, root: parsedMapping(t, "present: 1\n")} + + var got ir.Unmodeled + diags := UnknownKeysIn(&got, obj, 2, "/x") + + assert.Empty(t, got, "there was no node to read") + require.Len(t, diags, 1) + assert.Equal(t, ir.SeverityWarning, diags[0].Severity) + assert.Equal(t, "openapi/unknown-key-unreachable", diags[0].Code) + assert.Equal(t, ir.Provenance{Source: 2, Pointer: "/x/absent"}, diags[0].Provenance) +} + +// TestUnknownKeysUnder_KeyHoldingASeparatorIsItsOwnEntry pins the escaping that +// keeps a key from spelling another object's scope. Both keys below reach the +// same carrier, and unescaped both key under "openapi:info/contact/slack": the +// second site found the first already recorded, took the branch meant for a +// keyword another reader kept, and dropped a key with no diagnostic at all. +func TestUnknownKeysUnder_KeyHoldingASeparatorIsItsOwnEntry(t *testing.T) { + t.Parallel() + root := fakeObject{ + core: &fakeCore{keys: []string{"info/contact/slack"}}, + root: parsedMapping(t, "info/contact/slack: fromRoot\n"), + } + contact := fakeObject{ + core: &fakeCore{keys: []string{"slack"}}, + root: parsedMapping(t, "slack: fromContact\n"), + } + + var got ir.Unmodeled + diags := UnknownKeysUnder(&got, root, 0, "", "") + diags = append(diags, UnknownKeysUnder(&got, contact, 0, "/info/contact", "info/contact")...) + + assert.Equal(t, ir.RawValue(`"fromRoot"`), got["openapi:info~1contact~1slack"].Value, + "the root's key is one segment, escaped") + assert.Equal(t, ir.RawValue(`"fromContact"`), got["openapi:info/contact/slack"].Value, + "the scoped entry is the contact object's, and the root cannot spell it") + assert.Len(t, diags, 2, "two keys survive, so two are announced") +} + +// TestUnknownKeysIn_EntryHeldByAnotherConstructIsReported separates the two +// reasons an entry can already exist, which the census used to treat alike. +// +// A reader with more to say about a keyword writes it at the pointer the census +// would use, and there the census has nothing to add and says nothing — +// $vocabulary and dependentRequired on a schema's own map. An entry pointing +// somewhere else belongs to a different construct that happens to spell the same +// key, so the key this census holds is lost; it is announced instead of skipped +// (GitHub #396). +func TestUnknownKeysIn_EntryHeldByAnotherConstructIsReported(t *testing.T) { + t.Parallel() + obj := fakeObject{core: &fakeCore{keys: []string{"same", "other"}}, root: parsedMapping(t, "same: 1\nother: 2\n")} + got := ir.Unmodeled{ + "openapi:same": {Reason: ir.ReasonValidationOnly, Value: ir.RawValue("9"), + Provenance: ir.Provenance{Pointer: "/x/same"}}, + "openapi:other": {Reason: ir.ReasonNoIRHome, Value: ir.RawValue("9"), + Provenance: ir.Provenance{Pointer: "/x/schema/other"}}, + } + + diags := UnknownKeysIn(&got, obj, 0, "/x") + + assert.Equal(t, ir.RawValue("9"), got["openapi:same"].Value, "neither entry is overwritten") + assert.Equal(t, ir.RawValue("9"), got["openapi:other"].Value) + require.Len(t, diags, 1, "the key recorded for this very construct is not announced again") + assert.Equal(t, "openapi/unknown-key-entry-taken", diags[0].Code) + assert.Equal(t, ir.SeverityWarning, diags[0].Severity) + assert.Equal(t, "/x/other", diags[0].Provenance.Pointer) + assert.Contains(t, diags[0].Message, "/x/schema/other", "the holder is named, so the clash is findable") +} + +// TestUnknownKeysIn_ModelWithNoCensusRecordsNothing covers the shapes the reader +// must survive rather than panic on. The absent object is the one that occurs: +// the getters hand back a typed nil for an object the document omitted, and a +// promoted method on one of those dereferences it. +func TestUnknownKeysIn_ModelWithNoCensusRecordsNothing(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + model any + }{ + {"an object the document omitted", (*fakeObjectPtr)(nil)}, + {"an untyped nil", nil}, + {"a value that is no parsed model", 42}, + {"a model whose core keeps no census", fakeObject{core: "not a core"}}, + {"a model with an empty census", fakeObject{core: &fakeCore{}}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var got ir.Unmodeled + + diags := UnknownKeysIn(&got, tc.model, 0, "/x") + + assert.Nil(t, got) + assert.Empty(t, diags) + }) + } +} + +// fakeObject is a parsed model standing in for the library's, so the census can +// be driven at shapes no real document produces — an unsorted census, one past +// the bound, and a core that keeps none. +type fakeObject struct { + core any + root *yaml.Node +} + +func (f fakeObject) GetCoreAny() any { return f.core } +func (f fakeObject) GetRootNode() *yaml.Node { return f.root } + +// fakeObjectPtr is fakeObject's pointer-receiver twin, for the typed-nil case: +// a promoted method on a nil pointer is what the guard exists for, and a +// value-receiver method on a nil pointer would not reach it. +type fakeObjectPtr struct{ fakeObject } + +// fakeCore is a core model's census, reported exactly as given. +type fakeCore struct{ keys []string } + +func (c *fakeCore) GetUnknownProperties() []string { return c.keys } + +// parsedMapping parses body into the mapping node the census reads values from. +func parsedMapping(t *testing.T, body string) *yaml.Node { + t.Helper() + var doc yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(body), &doc)) + return &doc +} diff --git a/compilers/openapi/internal/auth/auth.go b/compilers/openapi/internal/auth/auth.go index c9e36aa..764f2e0 100644 --- a/compilers/openapi/internal/auth/auth.go +++ b/compilers/openapi/internal/auth/auth.go @@ -133,6 +133,11 @@ func lowerSecurityScheme(c lowering.Ctx, name string, ss *soa.SecurityScheme, } diags = preserveUnreadFields(c, &scheme, ss, decl) diags = append(diags, applySchemeExtensions(c, &scheme, ss, decl)...) + // Distinct from preserveUnreadFields above it: that keeps the fields OpenAPI + // defines for a securityScheme which this entry's own mechanism gives no + // meaning to, while this keeps the keys OpenAPI defines for no securityScheme + // at all. + diags = append(diags, annotation.UnknownKeysIn(&scheme.Unmodeled, ss, c.SrcIndex, decl)...) // After the extensions, whose entries are what a promotion reads. return scheme, true, append(diags, c.PromoteDeprecation(scheme.Unmodeled, scheme.Deprecation, &scheme.Provenance)...) diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index bb1fe52..80d7a0e 100644 --- a/compilers/openapi/internal/diag/diag.go +++ b/compilers/openapi/internal/diag/diag.go @@ -245,6 +245,60 @@ const ( // DegradedConstruct's constructs survive in a weaker shape, and these survive // in none (GitHub #144). UnpreservableConstruct = "openapi/unpreservable-construct" + // UnknownSchemaKeyword reports a JSON Schema keyword no field of the schema + // model names, kept verbatim under Unmodeled. + // + // Info, because the document did nothing wrong: JSON Schema requires an + // implementation to ignore a keyword it does not recognize, and says such a + // keyword may carry meaning for other tooling, so an unrecognized keyword is + // legal input rather than a defect. What is recorded is this compiler's own + // decision — that it read no meaning from the keyword and kept the text — which + // is the same thing ValidationOnlyKeyword records beside it. + UnknownSchemaKeyword = "openapi/unknown-schema-keyword" + // UnknownObjectKey reports a key on an OpenAPI object that the specification + // neither defines nor admits as an extension, kept verbatim under Unmodeled. + // + // Warning rather than info, because unlike its schema neighbour this one is a + // defect: OpenAPI gives its objects a closed key set and requires every + // extension to be prefixed x-, so a key that is neither is a document error — + // in practice a misspelling of the field beside it, which is precisely the + // class of mistake that survives when the compiler swallows the key in silence. + // + // Warning rather than error for the reason ReservedHeaderName is one: the + // document still lowers, everything the key was written beside is unaffected, + // and harness.Check stops at the first error diagnostic, which would hide every + // later finding in the same spec and make any fixture carrying a stray key + // unable to reach the invariant checks. + UnknownObjectKey = "openapi/unknown-object-key" + // UnknownKeyBudget reports an object declaring more keys the model does not + // name than the compiler keeps, so the ones past the bound reached the IR in no + // form at all. + // + // Every collection here is bounded, and this one is over a key set the document + // chooses the size of. The bound is far above what any document writes by + // accident, so tripping it is either a generated file or a hostile one; the + // diagnostic is what keeps the discarded remainder from being a silent loss. + UnknownKeyBudget = "openapi/unknown-key-budget" + // UnknownKeyUnreachable reports a key the parsed model reported as undeclared + // whose value the raw mapping does not present, so nothing of it reached the + // IR. + // + // Distinct from UnpreservableConstruct beside it, which is a value that was + // found and could not be rendered. This one was never reached: the parser + // reads a mapping through its `<<` merge keys and the raw readers here do not, + // so a merged-in key is named by the census and has no pair to read. Warning + // rather than that one's error because such a document is legal and still + // lowers. + UnknownKeyUnreachable = "openapi/unknown-key-unreachable" + // UnknownKeyEntryTaken reports a key whose Unmodeled entry is already held by + // a construct declared somewhere else, so the key reached the IR in no form. + // + // The carriers holding more than one object's entries are where two constructs + // can spell one entry: a parameter's own keys and the keywords its schema had + // no home for share one unscoped namespace on ir.Parameter. Warning rather + // than error because the document is otherwise lowered whole, and the entry + // that did survive is in it. + UnknownKeyEntryTaken = "openapi/unknown-key-entry-taken" ) // Newf builds an ir.Diagnostic with a formatted message. It is the single diff --git a/compilers/openapi/internal/diag/diag_test.go b/compilers/openapi/internal/diag/diag_test.go index 7b61549..76fec0c 100644 --- a/compilers/openapi/internal/diag/diag_test.go +++ b/compilers/openapi/internal/diag/diag_test.go @@ -141,6 +141,8 @@ func codes() []string { diag.UnattachableRequired, diag.InternalInvariant, diag.DuplicateOperationID, diag.IncompleteSecurityScheme, diag.ReservedHeaderName, diag.UnpreservableConstruct, + diag.UnknownSchemaKeyword, diag.UnknownObjectKey, diag.UnknownKeyBudget, + diag.UnknownKeyUnreachable, diag.UnknownKeyEntryTaken, } } diff --git a/compilers/openapi/internal/operation/content.go b/compilers/openapi/internal/operation/content.go index e75ce66..e959227 100644 --- a/compilers/openapi/internal/operation/content.go +++ b/compilers/openapi/internal/operation/content.go @@ -89,7 +89,8 @@ func lowerContent(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorIndex if len(ext) > 0 { content.Unmodeled = annotation.MergeUnmodeled(content.Unmodeled, ext) } - return content, diags + return content, append(diags, + annotation.UnknownKeysIn(&content.Unmodeled, media, c.SrcIndex, mediaPtr)...) } // fillSequential lowers 3.2 sequential-media fields: itemSchema becomes the @@ -555,6 +556,7 @@ func applyHeaderAnnotations(c lowering.Ctx, p *ir.Property, h *soa.Header, hdecl hExt, extDiags := schema.ExtensionsOf(c, h.GetExtensions(), hdecl) diags = append(diags, extDiags...) p.Unmodeled = annotation.MergeUnmodeled(p.Unmodeled, hExt) + diags = append(diags, annotation.UnknownKeysIn(&p.Unmodeled, h, c.SrcIndex, hdecl)...) return append(diags, c.PromoteDeprecation(p.Unmodeled, p.Deprecation, &p.Provenance)...) } @@ -664,7 +666,7 @@ func lowerRequestBody(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorI "request body is not required; optionality kept under Unmodeled")) } // soa.RequestBody exposes no GetExtensions at this library version, so the - // field is read directly — as XMLHints already reads its own. The read sits + // field is read directly — as XMLHints already reads its own. Both reads sit // after the payload guard because ir.Payload is the body's only carrier: a // request body declaring no content lowers to nothing to hang them on, and // OpenAPI makes content REQUIRED there, so such a body is a defect in the @@ -672,6 +674,7 @@ func lowerRequestBody(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorI bodyExt, bodyExtDiags := schema.ExtensionsOf(c, rb.Extensions, bodyPtr) payload.Unmodeled = annotation.MergeUnmodeled(payload.Unmodeled, bodyExt) diags = append(diags, bodyExtDiags...) + diags = append(diags, annotation.UnknownKeysIn(&payload.Unmodeled, rb, c.SrcIndex, bodyPtr)...) op.Request = payload hb.RequestContentTypes = contentTypeKeys(rb.GetContent()) return diags diff --git a/compilers/openapi/internal/operation/operations.go b/compilers/openapi/internal/operation/operations.go index 698da45..e7c4405 100644 --- a/compilers/openapi/internal/operation/operations.go +++ b/compilers/openapi/internal/operation/operations.go @@ -378,6 +378,7 @@ func lowerOperation(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInd } op.Bindings = ir.OpBindings{HTTP: []ir.HTTPBinding{hb}} diags = append(diags, applyOperationExtensions(c, &op, src, decl)...) + diags = append(diags, annotation.UnknownKeysIn(&op.Unmodeled, src, c.SrcIndex, decl)...) // After the extensions are on the map, since that is what it reads. diags = append(diags, c.PromoteDeprecation(op.Unmodeled, op.Deprecation, &op.Provenance)...) diags = append(diags, applyOperationServers(c, &op, src, decl)...) @@ -652,9 +653,10 @@ func lowerResponse(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInde } // preserveResponseExtras keeps what a Response Object declares that has no home -// on the node it lowered to: its links map, and its own x-* extensions. +// on the node it lowered to: its links map, its own x-* extensions, and the keys +// the specification does not define at all. // -// One helper for both branches on purpose. ir.Response and ir.ErrorCase are two +// One helper for all three on purpose. ir.Response and ir.ErrorCase are two // lowerings of the same source object, and each construct kept on only one of // them makes a declaration survive or vanish on nothing but its status code: // links were kept on a 2xx and dropped on a 4xx, and extensions were read at @@ -668,7 +670,8 @@ func preserveResponseExtras(c lowering.Ctx, p *ir.Unmodeled, r *soa.Response, rp annotation.RawChildNode(r.GetRootNode(), "links"), ir.ReasonNoIRHome, rptr+ids.Ptr("links")) ext, extDiags := schema.ExtensionsOf(c, r.GetExtensions(), rptr) *p = annotation.MergeUnmodeled(*p, ext) - return append(diags, extDiags...) + diags = append(diags, extDiags...) + return append(diags, annotation.UnknownKeysIn(p, r, c.SrcIndex, rptr)...) } // responseName builds a success response's neutral naming. OpenAPI names no diff --git a/compilers/openapi/internal/operation/params.go b/compilers/openapi/internal/operation/params.go index 37a3102..d434237 100644 --- a/compilers/openapi/internal/operation/params.go +++ b/compilers/openapi/internal/operation/params.go @@ -209,7 +209,10 @@ func fillParamSchemaAnnotations(c lowering.Ctx, ts *compile.Types, param *ir.Par diags = append(diags, preserveParamXML(c, param, s, pointer)...) } param.Unmodeled = annotation.MergeUnmodeled(param.Unmodeled, a.Unmodeled) - return diags + // Last, per schema.PreserveUnknownKeywords: it keeps only the keywords no + // reader above it kept, and everything annotation.Read recorded is already on + // the parameter by this line. + return append(diags, schema.PreserveUnknownKeywords(c, ¶m.Unmodeled, s, pointer)...) } // preserveParamXML keeps a parameter schema's xml hints instead of dropping @@ -288,6 +291,7 @@ func fillParamDetail(c lowering.Ctx, param *ir.Parameter, p *soa.Parameter, pptr pExt, extDiags := schema.ExtensionsOf(c, p.GetExtensions(), pptr) diags = append(diags, extDiags...) param.Unmodeled = annotation.MergeUnmodeled(param.Unmodeled, pExt) + diags = append(diags, annotation.UnknownKeysIn(¶m.Unmodeled, p, c.SrcIndex, pptr)...) return append(diags, preserveAllowEmptyValue(c, param, p, pptr)...) } diff --git a/compilers/openapi/internal/schema/accumulate.go b/compilers/openapi/internal/schema/accumulate.go index 34f0262..ea05630 100644 --- a/compilers/openapi/internal/schema/accumulate.go +++ b/compilers/openapi/internal/schema/accumulate.go @@ -73,6 +73,20 @@ func PreserveSchemaKeyword(c lowering.Ctx, p *ir.Unmodeled, s *oas3.Schema, keyw return PreserveNode(c, p, "openapi:"+keyword, annotation.RawPropertyNode(s, keyword), reason, pointer) } +// PreserveUnknownKeywords records every keyword s writes that no field of the +// schema model names and no reader above it already kept (GitHub #297). +// +// It is the last thing an attachment does, which is the contract +// annotation.UnknownKeywordsIn states and the reason it is called here rather +// than from inside annotation.Read with the other readers: $dynamicRef has no +// field in the schema model either and is decided by recordUnexpandedDynamicRef, +// which runs after Read and deliberately keeps nothing once the reference has +// expanded. A census running before it could not tell that from an unread +// keyword. +func PreserveUnknownKeywords(c lowering.Ctx, p *ir.Unmodeled, s *oas3.Schema, pointer string) []ir.Diagnostic { + return annotation.UnknownKeywordsIn(p, s, pointer, c.SrcIndex) +} + // preserveKeyword records a validation-only keyword's raw payload under key in // p and returns the one info diagnostic naming it at declPtr, the schema that // wrote it. An absent or unconvertible payload records nothing and returns diff --git a/compilers/openapi/internal/schema/schema.go b/compilers/openapi/internal/schema/schema.go index f344944..0729d1a 100644 --- a/compilers/openapi/internal/schema/schema.go +++ b/compilers/openapi/internal/schema/schema.go @@ -1145,7 +1145,8 @@ func fillPropertyAnnotations(c lowering.Ctx, ts *compile.Types, anchors *AnchorI // nil node: this arm runs only when the schema lowered to no node of its own, // so nothing here can be carrying an Encoding. diags = append(diags, recordUnplacedContent(c, &p.Unmodeled, ref, nil, pointer)...) - return append(diags, recordUnexpandedDynamicRef(c, anchors, &p.Unmodeled, ref, pointer)...) + diags = append(diags, recordUnexpandedDynamicRef(c, anchors, &p.Unmodeled, ref, pointer)...) + return append(diags, PreserveUnknownKeywords(c, &p.Unmodeled, ref, pointer)...) } // LoweredToOwnNode reports whether the declaration at pointer lowered to a type @@ -1232,7 +1233,8 @@ func attachDeclaredAnnotations(c lowering.Ctx, ts *compile.Types, anchors *Ancho common.Examples = a.Examples } diags = append(diags, recordUnplacedContent(c, &common.Unmodeled, s, td, pointer)...) - return append(diags, recordUnexpandedDynamicRef(c, anchors, &common.Unmodeled, s, pointer)...) + diags = append(diags, recordUnexpandedDynamicRef(c, anchors, &common.Unmodeled, s, pointer)...) + return append(diags, PreserveUnknownKeywords(c, &common.Unmodeled, s, pointer)...) } // fillAdditional lowers additionalProperties, patternProperties, and diff --git a/compilers/openapi/meta.go b/compilers/openapi/meta.go index a282d88..d3618cc 100644 --- a/compilers/openapi/meta.go +++ b/compilers/openapi/meta.go @@ -42,12 +42,85 @@ type docMeta struct { // that lowers to no node of its own (ir-design §10, §12). func lowerMeta(c lowering.Ctx) (docMeta, []ir.Diagnostic) { m := lowerInfo(c) + ext, diags := documentExtensions(c) m.Unmodeled = ext servers, serverDiags := lowerServers(c) m.Servers = servers - return m, append(diags, serverDiags...) + diags = append(diags, serverDiags...) + + return m, append(diags, documentUnknownKeys(c, &m.Unmodeled)...) +} + +// documentUnknownKeys collects the keys the OpenAPI model names no field for +// from every object around the document metadata that lowers to no node of its +// own: the document root, the info block and the contact and license inside it, +// the root externalDocs, the components object, and each declared tag. +// +// ir.Document is the nearest node with an Unmodeled map for all of them, so each +// object's keys are scoped by the source path they were written at. One unscoped +// "openapi:status" would be a single key for six objects, and the entry that +// survived would be whichever site ran last. +func documentUnknownKeys(c lowering.Ctx, p *ir.Unmodeled) []ir.Diagnostic { + sites := append(rootUnknownSites(c), tagUnknownSites(c)...) + diags := make([]ir.Diagnostic, 0, len(sites)) + for _, site := range sites { + diags = append(diags, + annotation.UnknownKeysUnder(p, site.model, c.SrcIndex, site.owner, site.scope)...) + } + return diags +} + +// unknownSite is one object's census: what it keys under on the carrier holding +// it, the object's own source pointer, and the parsed object itself. +type unknownSite struct { + scope string + owner string + model any +} + +// rootUnknownSites returns the census sites a document has exactly one of. The +// root's keys take no scope, since ir.Document stands for the OpenAPI Object +// itself; the rest are keyed by the path from it down to the object that wrote +// them. +// +// The components object is one of them, unlike the maps beneath it. `paths`, +// `responses` and a callback each embed a sequenced map, so every key they hold +// is a valid entry and an unrecognized one is not a thing they have; the +// Components Object is a fixed-field struct beside them, and a key it does not +// define is as undeclared as one on any other object here. +func rootUnknownSites(c lowering.Ctx) []unknownSite { + info := c.Doc.GetInfo() + infoPtr := ids.Ptr("info") + return []unknownSite{ + {"", "", c.Doc}, + {"info", infoPtr, info}, + {"info/contact", infoPtr + ids.Ptr("contact"), info.GetContact()}, + {"info/license", infoPtr + ids.Ptr("license"), info.GetLicense()}, + {"externalDocs", ids.Ptr("externalDocs"), c.Doc.GetExternalDocs()}, + {"components", ids.Ptr("components"), c.Doc.GetComponents()}, + } +} + +// tagUnknownSites returns one census site per declared tag, since ir.TagDef +// holds no Unmodeled map for a tag's own keys to land on. +// +// Scoped by index rather than name, which is the pointer a tag is written at. A +// name would read better, but OpenAPI's requirement that tag names be unique is +// the document's to keep and not this compiler's to rely on: two tags spelled +// alike would silently leave one entry. +func tagUnknownSites(c lowering.Ctx) []unknownSite { + tags := c.Doc.GetTags() + out := make([]unknownSite, 0, len(tags)) + for i, t := range tags { + if t == nil { + continue + } + index := strconv.Itoa(i) + out = append(out, unknownSite{"tags/" + index, ids.Ptr("tags", index), t}) + } + return out } // documentExtensions collects the x-* of every object that lowers to no IR node @@ -163,13 +236,15 @@ func lowerServers(c lowering.Ctx) ([]ir.Server, []ir.Diagnostic) { func lowerServer(c lowering.Ctx, s *soa.Server, sptr string) (ir.Server, []ir.Diagnostic) { vars, diags := serverVariables(c, s, sptr) ext, extDiags := annotation.ExtensionsFrom(s.GetExtensions(), c.SrcIndex, sptr) - return ir.Server{ + diags = append(diags, extDiags...) + out := ir.Server{ Name: serverName(s), URLTemplate: s.GetURL(), Description: ir.Docs{Description: s.GetDescription()}, Variables: vars, Unmodeled: ext, - }, append(diags, extDiags...) + } + return out, append(diags, annotation.UnknownKeysIn(&out.Unmodeled, s, c.SrcIndex, sptr)...) } // serverName builds a server's neutral naming: the declared name when the source @@ -211,18 +286,20 @@ func serverVariables(c lowering.Ctx, s *soa.Server, sptr string) ([]ir.ServerVar if v == nil { continue } + vptr := sptr + ids.Ptr("variables", name) // ServerVariable exposes no GetExtensions at this library version, so the // field is read directly — as XMLHints already reads its own. - ext, extDiags := annotation.ExtensionsFrom(v.Extensions, c.SrcIndex, - sptr+ids.Ptr("variables", name)) + ext, extDiags := annotation.ExtensionsFrom(v.Extensions, c.SrcIndex, vptr) diags = append(diags, extDiags...) - out = append(out, ir.ServerVariable{ + one := ir.ServerVariable{ Name: name, Default: v.GetDefault(), Enum: v.GetEnum(), Docs: ir.Docs{Description: v.GetDescription()}, Unmodeled: ext, - }) + } + diags = append(diags, annotation.UnknownKeysIn(&one.Unmodeled, v, c.SrcIndex, vptr)...) + out = append(out, one) } return out, diags } diff --git a/compilers/openapi/meta_test.go b/compilers/openapi/meta_test.go index d9f8fb6..6fde511 100644 --- a/compilers/openapi/meta_test.go +++ b/compilers/openapi/meta_test.go @@ -85,6 +85,21 @@ func TestTagExtensions_NilEntrySkipped(t *testing.T) { assert.Equal(t, "/tags/1", got[0].Owner) } +// TestTagUnknownSites_NilEntrySkipped is TestTagExtensions_NilEntrySkipped's +// twin on the census side: the two walks of the tag list keep the same guard, so +// neither can be the one that dereferences a nil entry or keys a site at an +// index holding no tag. +func TestTagUnknownSites_NilEntrySkipped(t *testing.T) { + t.Parallel() + doc := &soa.OpenAPI{Tags: []*soa.Tag{nil, {Name: "kept"}}} + + got := tagUnknownSites(lowering.Ctx{Doc: doc}) + + require.Len(t, got, 1, "only the surviving tag contributes a census site") + assert.Equal(t, "tags/1", got[0].scope, "the site is keyed at the tag's own index, not its position") + assert.Equal(t, "/tags/1", got[0].owner) +} + func TestMeta_NoInfoNoServers(t *testing.T) { t.Parallel() // With no info block the title is empty; with no servers the library injects diff --git a/compilers/openapi/unknownkeys_test.go b/compilers/openapi/unknownkeys_test.go new file mode 100644 index 0000000..6f67b8a --- /dev/null +++ b/compilers/openapi/unknownkeys_test.go @@ -0,0 +1,236 @@ +// This file covers one property: a key the OpenAPI model names no field for +// reaches the IR rather than vanishing between the parser and the lowering. It +// is separate from annotations_test.go because the subject is the complement of +// what the model names, not any one annotation. +package openapi_test // external test package — exercises only the public API + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" +) + +// specFile reads a census fixture the corpus sweeps also drive, so the +// assertions below and the six oracles run over the same bytes: putting one +// under testdata is what gets it compiled in both declaration orders, +// round-tripped and verified, none of which a spec written inline reaches. +func specFile(t *testing.T, name string) string { + t.Helper() + data, err := os.ReadFile(filepath.Join("..", "..", "testdata", "openapi", name)) + require.NoError(t, err) + return string(data) +} + +// unknownKeysSpec is the census fixture writing one undeclared key per object. +func unknownKeysSpec(t *testing.T) string { + t.Helper() + return specFile(t, "unknown_keys.yaml") +} + +// TestUnknownKeys_KeptAtEveryObject holds the whole rule rather than the +// positions that happened to be noticed. A key the model does not name reached +// no IR field, no Unmodeled entry and no diagnostic at every one of these +// objects, so two documents differing only in it compiled to the same IR +// (GitHub #297). +// +// Carriers are derived from the value graph rather than named: each row says +// which Unmodeled map the entry must land on by the path the walk reaches it at, +// so an entry written to the wrong carrier fails here rather than passing +// because the assertion looked only where it expected. +func TestUnknownKeys_KeptAtEveryObject(t *testing.T) { + t.Parallel() + doc, diags := compileAnnotationSpec(t, "unknown-keys", unknownKeysSpec(t)) + requireNoErrorDiagnostics(t, diags) + sites := unmodeledSites(doc) + + for _, tc := range []struct { + object string + key string + want string + carrier string + }{ + {"openapi root", "openapi:basePath", `"ROOT"`, "doc.Unmodeled"}, + {"info", "openapi:info/contactEmail", `"INFO"`, "doc.Unmodeled"}, + {"contact", "openapi:info/contact/slack", `"CONTACT"`, "doc.Unmodeled"}, + {"license", "openapi:info/license/spdx", `"LICENSE"`, "doc.Unmodeled"}, + {"externalDocs", "openapi:externalDocs/title", `"EXTERNALDOCS"`, "doc.Unmodeled"}, + {"server", "openapi:host", `"SERVER"`, "doc.Servers[0].Unmodeled"}, + {"server variable", "openapi:example", `"SERVERVARIABLE"`, "doc.Servers[0].Variables[0].Unmodeled"}, + {"tag", "openapi:tags/0/color", `"TAG"`, "doc.Unmodeled"}, + {"operation", "openapi:operationid", `"OPERATION"`, ".Unmodeled"}, + {"parameter", "openapi:collectionFormat", `"PARAMETER"`, ".Params[0].Unmodeled"}, + {"request body", "openapi:schema", `"REQUESTBODY"`, ".Request.Unmodeled"}, + {"media type", "openapi:format", `"MEDIATYPE"`, ".Contents[0].Unmodeled"}, + {"response", "openapi:status", `"RESPONSE"`, ".Responses[0].Unmodeled"}, + {"error response", "openapi:status", `"ERRORRESPONSE"`, ".Errors[0].Unmodeled"}, + {"header", "openapi:in", `"HEADER"`, ".Headers[0].Unmodeled"}, + {"components", "openapi:components/definitions", `"COMPONENTS"`, "doc.Unmodeled"}, + {"security scheme", "openapi:tokenUrl", `"SECURITYSCHEME"`, + "doc.Auth[auth/openapi/components/securitySchemes/k].Unmodeled"}, + {"schema", "openapi:additionalItems", `"SCHEMA"`, + "doc.Types[t/openapi/components/schemas/S].Unmodeled"}, + // The property's schema reduced to a shared primitive, so it owns no node + // and its keywords stay on the declaring property — the carrier rule + // fillPropertyAnnotations already applies to every other annotation. + {"property schema", "openapi:divisibleBy", `"PROPERTYSCHEMA"`, + "doc.Types[t/openapi/components/schemas/S].Properties[0].Unmodeled"}, + } { + site, found := findUnmodeled(sites, tc.key, tc.want) + if !assert.True(t, found, "%s drops %s = %s: it is nowhere in the document", + tc.object, tc.key, tc.want) { + continue + } + assert.Contains(t, site.path, tc.carrier, "%s key lands on the wrong carrier", tc.object) + } +} + +// TestUnknownKeys_SchemaAndObjectAreGradedApart pins the one distinction the +// census turns on, at the two keys the fixture picks for it: a draft-07 +// additionalItems in a schema, and an operationId with the case wrong on an +// operation. +// +// JSON Schema states that an implementation must ignore a keyword it does not +// recognize, so an unrecognized keyword in a schema is legal input and may carry +// meaning for other tooling; OpenAPI states that an extension key must be +// prefixed x-, so an undefined key on one of its objects is not an extension but +// a defect in the document. +// +// Both are kept — invariant 2 does not bend for invalid input — and both carry +// the same reason, because "no IR node is coming" is true of each. What differs +// is what the document did, which is the diagnostic channel's subject: an +// unrecognized keyword records a decision at info, and an undefined key reports +// a fault at warning. +func TestUnknownKeys_SchemaAndObjectAreGradedApart(t *testing.T) { + t.Parallel() + doc, diags := compileAnnotationSpec(t, "unknown-keys", unknownKeysSpec(t)) + requireNoErrorDiagnostics(t, diags) + + schema, ok := doc.Types[ir.TypeID("t/openapi/components/schemas/S")] + require.True(t, ok) + keyword := unmodeledEntry(t, schema.Common().Unmodeled, "openapi:additionalItems") + assert.Equal(t, ir.ReasonOutOfScope, keyword.Reason, + "no IR node is coming for a keyword this compiler does not model") + assert.Equal(t, "/components/schemas/S/additionalItems", keyword.Provenance.Pointer) + assert.Equal(t, []ir.Severity{ir.SeverityInfo}, + diagsAt(diags, "openapi/unknown-schema-keyword", "/components/schemas/S/additionalItems")) + + op, ok := opByName(doc, "listWidgets") + require.True(t, ok) + key := unmodeledEntry(t, op.Unmodeled, "openapi:operationid") + assert.Equal(t, ir.ReasonOutOfScope, key.Reason, + "OpenAPI defines no such key, so no IR node is coming for it either") + assert.Equal(t, "/paths/~1widgets/get/operationid", key.Provenance.Pointer) + assert.Equal(t, []ir.Severity{ir.SeverityWarning}, + diagsAt(diags, "openapi/unknown-object-key", "/paths/~1widgets/get/operationid")) +} + +// TestUnknownKeys_SpellingDecidesNeitherEntryNorCarrier holds the census to the +// two spellings that used to lose a key outright, each of which put a document +// writing it and a document writing nothing at all into the same IR. +// +// An aliased key is reported by the parser under the name it resolves to, while +// the mapping node it was written on still holds an alias whose own value is the +// anchor. Searching that mapping for the resolved name found nothing, and a key +// with no node kept nothing and said nothing. +// +// A key holding a "/" collided with the scope of the object that path names: +// entries for the objects with no Unmodeled map of their own are keyed by the +// path from the carrier down, so a root key spelled "info/contact/slack" was the +// same entry as the contact object's own "slack", and the site that reached the +// carrier second found it taken and dropped its key in silence. Escaping the key +// as one segment is what separates them, per ids.Scope. +func TestUnknownKeys_SpellingDecidesNeitherEntryNorCarrier(t *testing.T) { + t.Parallel() + doc, diags := compileAnnotationSpec(t, "spellings", specFile(t, "unknown_key_spellings.yaml")) + requireNoErrorDiagnostics(t, diags) + sites := unmodeledSites(doc) + + for _, tc := range []struct { + spelling string + key string + want string + }{ + {"a key written as an alias", "openapi:info/license/spdx", `"LICENSE_ALIASED"`}, + {"a root key spelling another object's scope", "openapi:info~1contact~1slack", `"ROOT_SLASHED"`}, + {"the scope that key spells", "openapi:info/contact/slack", `"CONTACT_PLAIN"`}, + } { + site, found := findUnmodeled(sites, tc.key, tc.want) + if !assert.True(t, found, "%s drops %s = %s: it is nowhere in the document", + tc.spelling, tc.key, tc.want) { + continue + } + assert.Equal(t, ir.ReasonOutOfScope, site.entry.Reason, + "%s is the census's, not the extension reader's", tc.spelling) + } + assert.Len(t, diagsAt(diags, "openapi/unknown-object-key", "/info/license/spdx"), 1, + "an aliased key is announced at the name it resolves to") +} + +// TestUnknownKeys_ClashingCarrierEntryIsReportedNotDropped covers the one +// carrier the census cannot key its way out of. A parameter and its schema are +// two objects at two pointers whose entries share one unscoped map, so a key both +// of them write is one entry, and the schema's reaches it first. +// +// The key that loses is announced with the pointer of the construct holding the +// entry, rather than skipped by the branch meant for a keyword another reader +// already said better. It is still in the IR in no form at all: separating the +// namespaces moves keys #345, #348 and the validation-only reader already +// publish, which is GitHub #396 and not this census's to settle. +func TestUnknownKeys_ClashingCarrierEntryIsReportedNotDropped(t *testing.T) { + t.Parallel() + doc, diags := compileAnnotationSpec(t, "clash", specFile(t, "unknown_key_carrier_clash.yaml")) + requireNoErrorDiagnostics(t, diags) + sites := unmodeledSites(doc) + + for _, tc := range []struct{ carrier, held, lost string }{ + {"parameter", "/paths/~1widgets/get/parameters/0/schema/divisibleBy", + "/paths/~1widgets/get/parameters/0/divisibleBy"}, + {"header", "/paths/~1widgets/get/responses/200/headers/X-Trace/schema/divisibleBy", + "/paths/~1widgets/get/responses/200/headers/X-Trace/divisibleBy"}, + } { + assert.Equal(t, []ir.Severity{ir.SeverityWarning}, + diagsAt(diags, "openapi/unknown-key-entry-taken", tc.lost), + "the %s key that lost the entry is announced at its own pointer", tc.carrier) + _, found := findUnmodeled(sites, "openapi:divisibleBy", `"FROM_`+strings.ToUpper(tc.carrier)+`_OBJECT"`) + assert.False(t, found, "the %s's own key is in the IR in no form at all, as reported", tc.carrier) + } +} + +// TestUnknownKeys_WellFormedDocumentRecordsNothing is the control the census +// needs: a document writing only what the model names keeps no entry and reports +// no finding, so the rows above are evidence of the keys they name rather than +// of a sweep that fires on everything. +func TestUnknownKeys_WellFormedDocumentRecordsNothing(t *testing.T) { + t.Parallel() + doc, diags := compileAnnotationSpec(t, "clean", `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /widgets: + get: + operationId: listWidgets + responses: {"200": {description: ok}} +components: + schemas: + S: {type: object, properties: {a: {type: string}}} +`) + requireNoErrorDiagnostics(t, diags) + + assert.Empty(t, unmodeledSites(doc), "nothing undeclared, so nothing kept") + for _, d := range diags { + assert.NotContains(t, d.Code, "unknown-", "a well-formed document reports no unknown key: %+v", d) + } +} + +// requireNoErrorDiagnostics fails the test on the first error-severity +// diagnostic, naming it. +func requireNoErrorDiagnostics(t *testing.T, diags []ir.Diagnostic) { + t.Helper() + d, ok := ir.FirstError(diags) + require.False(t, ok, "unexpected error diagnostic: %+v", d) +} diff --git a/testdata/conformance/openapi/allof-oneof-cooccurrence.golden.json b/testdata/conformance/openapi/allof-oneof-cooccurrence.golden.json index 1e943d3..4a1d4d5 100644 --- a/testdata/conformance/openapi/allof-oneof-cooccurrence.golden.json +++ b/testdata/conformance/openapi/allof-oneof-cooccurrence.golden.json @@ -44,7 +44,7 @@ }, "anonymous": true, "docs": { - "description": "named by position" + "description": "named by position, not by target" }, "sensitive": false, "provenance": { @@ -366,7 +366,7 @@ "eventPayload": false, "secret": false, "docs": { - "description": "named by position" + "description": "named by position, not by target" }, "provenance": { "source": 0, @@ -548,7 +548,7 @@ { "format": "openapi@3.1", "path": "allof-oneof-cooccurrence.yaml", - "hash": "cb7fb6bc613222b0211fc363b9028e7739b44a1d14b04f74cf709b8d603655f2" + "hash": "580c2ffd45b8b24e175b4c5b7f1af6c237a920de7d1942d84b10a00ad2dc1e33" } ] } diff --git a/testdata/conformance/openapi/allof-oneof-cooccurrence.yaml b/testdata/conformance/openapi/allof-oneof-cooccurrence.yaml index f085da9..042727d 100644 --- a/testdata/conformance/openapi/allof-oneof-cooccurrence.yaml +++ b/testdata/conformance/openapi/allof-oneof-cooccurrence.yaml @@ -56,5 +56,5 @@ components: branch: {$ref: '#/components/schemas/InlineHost/oneOf/0'} InlineHost: oneOf: - - {type: integer, description: named by position, not by target} + - {type: integer, description: 'named by position, not by target'} - {type: string} diff --git a/testdata/openapi/unknown_key_carrier_clash.yaml b/testdata/openapi/unknown_key_carrier_clash.yaml new file mode 100644 index 0000000..7665922 --- /dev/null +++ b/testdata/openapi/unknown_key_carrier_clash.yaml @@ -0,0 +1,30 @@ +# A parameter and a header each share their Unmodeled map with their schema: the +# object's own keys and the keywords the schema had no home for both land on it +# unscoped, so one key spelled by both objects is one entry between them. +# +# The schema's reaches it first, and the object's own is reported under +# openapi/unknown-key-entry-taken rather than dropped without a word. Settling +# which namespace each side owns moves keys three merged mechanisms already +# publish, so it is GitHub #396 rather than part of the census. +# +# `divisibleBy` is a draft-04 keyword: undeclared on a Parameter, undeclared on a +# Header, and no field of the schema model either, which is what lets one key be +# written on both objects at once. +openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /widgets: + get: + operationId: listWidgets + parameters: + - name: shape + in: query + divisibleBy: FROM_PARAMETER_OBJECT + schema: {type: string, divisibleBy: FROM_PARAMETER_SCHEMA} + responses: + "200": + description: ok + headers: + X-Trace: + divisibleBy: FROM_HEADER_OBJECT + schema: {type: string, divisibleBy: FROM_HEADER_SCHEMA} diff --git a/testdata/openapi/unknown_key_spellings.yaml b/testdata/openapi/unknown_key_spellings.yaml new file mode 100644 index 0000000..dc6e09c --- /dev/null +++ b/testdata/openapi/unknown_key_spellings.yaml @@ -0,0 +1,35 @@ +# Undeclared keys whose *spelling* is the subject, rather than the object they +# were written on (that is unknown_keys.yaml). Both spellings below reached the +# census but no Unmodeled entry, so a document writing one compiled to the same +# IR as a document writing neither. +# +# - An aliased key. YAML resolves `*anchor` to the name it stands for and the +# parser reports the key under that name, while the mapping node still holds +# an alias whose own value is the anchor. Searching the mapping for the +# resolved name found nothing, and a key with no node kept nothing. +# - A key holding a "/". Entries for the objects with no Unmodeled map of their +# own are keyed by the path from the carrier down, so a root key spelling one +# of those paths landed on the very entry that path's own object writes, and +# whichever site ran second was dropped without a word. Here the root writes +# `info/contact/slack` while the contact object writes `slack`: two keys, two +# entries, or the fixture is not testing what it says. +openapi: 3.1.0 +"info/contact/slack": ROOT_SLASHED +info: + # Anchored to a name with no "x-" prefix on purpose: an aliased x-* key + # resolves to one the extension reader claims, which would witness that reader + # rather than this census. + title: &aliased spdx + version: "1" + contact: + name: n + slack: CONTACT_PLAIN + license: + name: MIT + *aliased : LICENSE_ALIASED +paths: + /widgets: + get: + operationId: listWidgets + responses: + "200": {description: ok} diff --git a/testdata/openapi/unknown_keys.yaml b/testdata/openapi/unknown_keys.yaml new file mode 100644 index 0000000..1fd4b00 --- /dev/null +++ b/testdata/openapi/unknown_keys.yaml @@ -0,0 +1,64 @@ +# One key the OpenAPI model names no field for at every object the compiler takes +# a census from, each valued with the object it was written on so an entry found +# at the wrong carrier cannot pass for the right one. +# +# Every key here is one real documents carry rather than invented nonsense: a +# Swagger 2.0 field with no OpenAPI 3 equivalent (basePath, host, a body +# parameter's schema, definitions where components.schemas goes), a keyword from +# an older JSON Schema draft (additionalItems, divisibleBy), a field belonging to +# a neighbouring object (a flow's tokenUrl on the scheme, a parameter's `in` on a +# header), or operationId with the case wrong. None of them reached an IR field, +# an Unmodeled entry or a diagnostic. +# +# How a key is *spelled* is a separate property with a fixture of its own; see +# unknown_key_spellings.yaml. +openapi: 3.1.0 +basePath: ROOT +info: + title: T + version: "1" + contactEmail: INFO + contact: {name: n, slack: CONTACT} + license: {name: MIT, spdx: LICENSE} +externalDocs: {url: 'https://d.example', title: EXTERNALDOCS} +servers: + - url: 'https://a.example/{region}' + host: SERVER + variables: + region: {default: us, example: SERVERVARIABLE} +tags: + - {name: t1, color: TAG} +paths: + /widgets: + get: + operationId: listWidgets + tags: [t1] + operationid: OPERATION + parameters: + - {name: shape, in: query, schema: {type: string}, collectionFormat: PARAMETER} + requestBody: + required: true + schema: REQUESTBODY + content: + application/json: + schema: {$ref: '#/components/schemas/S'} + format: MEDIATYPE + responses: + "200": + description: ok + status: RESPONSE + headers: + X-Trace: {schema: {type: string}, in: HEADER} + "404": + description: gone + status: ERRORRESPONSE +components: + definitions: COMPONENTS + securitySchemes: + k: {type: apiKey, in: header, name: X-Key, tokenUrl: SECURITYSCHEME} + schemas: + S: + type: object + additionalItems: SCHEMA + properties: + a: {type: string, divisibleBy: PROPERTYSCHEMA}