From 3a0bc4e584428333070c6424bfb7b46f20b984c9 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 05:16:14 +0300 Subject: [PATCH 1/3] fix(compilers/openapi): keep keys the source model does not name --- .../openapi/internal/annotation/unknown.go | 207 +++++++++++++++++ .../annotation/unknown_internal_test.go | 212 ++++++++++++++++++ compilers/openapi/internal/auth/auth.go | 8 +- compilers/openapi/internal/diag/diag.go | 34 +++ compilers/openapi/internal/diag/diag_test.go | 1 + .../openapi/internal/operation/content.go | 10 +- .../openapi/internal/operation/operations.go | 19 +- .../openapi/internal/operation/params.go | 6 +- .../openapi/internal/schema/accumulate.go | 14 ++ compilers/openapi/internal/schema/schema.go | 6 +- compilers/openapi/meta.go | 108 +++++++-- compilers/openapi/meta_test.go | 24 +- compilers/openapi/unknownkeys_test.go | 200 +++++++++++++++++ .../allof-oneof-cooccurrence.golden.json | 6 +- .../openapi/allof-oneof-cooccurrence.yaml | 2 +- .../openapi/unwitnessed.golden.txt | 2 - testdata/openapi/unknown_keys.yaml | 59 +++++ 17 files changed, 878 insertions(+), 40 deletions(-) create mode 100644 compilers/openapi/internal/annotation/unknown.go create mode 100644 compilers/openapi/internal/annotation/unknown_internal_test.go create mode 100644 compilers/openapi/unknownkeys_test.go create mode 100644 testdata/openapi/unknown_keys.yaml diff --git a/compilers/openapi/internal/annotation/unknown.go b/compilers/openapi/internal/annotation/unknown.go new file mode 100644 index 00000000..bab5accd --- /dev/null +++ b/compilers/openapi/internal/annotation/unknown.go @@ -0,0 +1,207 @@ +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" +) + +// 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. +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 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. +func census(p *ir.Unmodeled, model any, srcIndex int, owner, scope string, cl keyClass) []ir.Diagnostic { + keys, root := undeclaredKeys(model) + if len(keys) == 0 { + return nil + } + var diags []ir.Diagnostic + if len(keys) > MaxUnknownKeys { + diags = append(diags, budgetDiag(len(keys), owner, srcIndex)) + keys = keys[:MaxUnknownKeys] + } + for _, key := range keys { + entry := "openapi:" + scoped(scope, key) + if _, recorded := (*p)[entry]; recorded || slices.Contains(cl.skip, key) { + continue + } + at := owner + ids.Ptr(key) + kept, keptDiags := PreserveNodeInto(p, entry, RawChildNode(root, key), + ir.ReasonOutOfScope, at, srcIndex) + diags = append(diags, keptDiags...) + if !kept { + continue + } + diags = append(diags, diag.Newf(cl.severity, cl.code, + ir.Provenance{Source: srcIndex, Pointer: at}, cl.message, key)) + } + return diags +} + +// scoped spells one entry's key on the carrier holding it. +func scoped(scope, key string) string { + if scope == "" { + return key + } + return 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, 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 00000000..764a402b --- /dev/null +++ b/compilers/openapi/internal/annotation/unknown_internal_test.go @@ -0,0 +1,212 @@ +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_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 b04dbe96..b7287f59 100644 --- a/compilers/openapi/internal/auth/auth.go +++ b/compilers/openapi/internal/auth/auth.go @@ -134,7 +134,13 @@ func lowerSecurityScheme(c lowering.Ctx, name string, ss *soa.SecurityScheme, diags = preserveUnreadFields(c, &scheme, ss, decl) ext, extDiags := annotation.ExtensionsFrom(ss.GetExtensions(), c.SrcIndex, decl) scheme.Unmodeled = annotation.MergeUnmodeled(scheme.Unmodeled, ext) - return scheme, true, append(diags, extDiags...) + diags = append(diags, extDiags...) + // 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. + return scheme, true, append(diags, + annotation.UnknownKeysIn(&scheme.Unmodeled, ss, c.SrcIndex, decl)...) } // mechanismRefusalDiag reports a securitySchemes entry that declares a scheme diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index 59027084..5a9a12b8 100644 --- a/compilers/openapi/internal/diag/diag.go +++ b/compilers/openapi/internal/diag/diag.go @@ -191,6 +191,40 @@ 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" ) // 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 26e351ac..8b172081 100644 --- a/compilers/openapi/internal/diag/diag_test.go +++ b/compilers/openapi/internal/diag/diag_test.go @@ -138,6 +138,7 @@ func codes() []string { diag.AliasAmplification, diag.UnattachableRequired, diag.InternalInvariant, diag.DuplicateOperationID, diag.IncompleteSecurityScheme, diag.ReservedHeaderName, diag.UnpreservableConstruct, + diag.UnknownSchemaKeyword, diag.UnknownObjectKey, diag.UnknownKeyBudget, } } diff --git a/compilers/openapi/internal/operation/content.go b/compilers/openapi/internal/operation/content.go index c202731a..94bb76fa 100644 --- a/compilers/openapi/internal/operation/content.go +++ b/compilers/openapi/internal/operation/content.go @@ -88,7 +88,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 @@ -512,7 +513,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) - return diags + return append(diags, annotation.UnknownKeysIn(&p.Unmodeled, h, c.SrcIndex, hdecl)...) } // exampleList lowers a single example node and a plural example map into value @@ -604,6 +605,11 @@ func lowerRequestBody(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorI diags = append(diags, c.DiagAt(ir.SeverityInfo, diag.DegradedConstruct, bodyPtr, "request body is not required; optionality kept under Unmodeled")) } + // 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 + // document rather than a shape this compiler has to place. + 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 351fb057..0c3bf09b 100644 --- a/compilers/openapi/internal/operation/operations.go +++ b/compilers/openapi/internal/operation/operations.go @@ -293,6 +293,7 @@ func lowerOperation(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInd op.Unmodeled = ext } // After the extensions assignment, which would otherwise overwrite the map. + diags = append(diags, annotation.UnknownKeysIn(&op.Unmodeled, src, c.SrcIndex, decl)...) diags = append(diags, applyOperationServers(c, &op, src, decl)...) return op, extra, append(diags, checkOperationIDUnique(c, operationIDs, op, mount)...) } @@ -465,7 +466,20 @@ func lowerResponse(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInde resp.Docs.Description = r.GetDescription() _, linkDiags := schema.PreserveNode(c, &resp.Unmodeled, "openapi:links", annotation.RawChildNode(r.GetRootNode(), "links"), ir.ReasonNoIRHome, rptr+ids.Ptr("links")) - return resp, append(diags, linkDiags...) + diags = append(diags, linkDiags...) + return resp, append(diags, preserveResponseUnknownKeys(c, &resp.Unmodeled, r, rptr)...) +} + +// preserveResponseUnknownKeys keeps the keys a Response Object writes that the +// specification does not define. +// +// One helper for both branches on purpose. ir.Response and ir.ErrorCase are two +// lowerings of the same source object, and a construct kept on only one of them +// makes a declaration survive or vanish on nothing but its status code — which +// is how a response's links came to be kept on a 2xx and dropped on a 4xx +// (GitHub #275). +func preserveResponseUnknownKeys(c lowering.Ctx, p *ir.Unmodeled, r *soa.Response, rptr string) []ir.Diagnostic { + return annotation.UnknownKeysIn(p, r, c.SrcIndex, rptr) } // responseName builds a success response's neutral naming. OpenAPI names no @@ -497,7 +511,8 @@ func lowerErrorCase(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInd } ec.Docs.Description = r.GetDescription() diags := fillErrorType(c, ts, anchors, &ec, r, rptr) - return ec, append(diags, preserveErrorHeaders(c, &ec, r, rptr)...) + diags = append(diags, preserveErrorHeaders(c, &ec, r, rptr)...) + return ec, append(diags, preserveResponseUnknownKeys(c, &ec.Unmodeled, r, rptr)...) } // preserveErrorHeaders keeps an error response's headers from being dropped: diff --git a/compilers/openapi/internal/operation/params.go b/compilers/openapi/internal/operation/params.go index e0875ee3..c7bcc3a6 100644 --- a/compilers/openapi/internal/operation/params.go +++ b/compilers/openapi/internal/operation/params.go @@ -201,7 +201,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 @@ -274,6 +277,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 bf2a5fac..836687c2 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 d06d1eb9..b5cc509f 100644 --- a/compilers/openapi/internal/schema/schema.go +++ b/compilers/openapi/internal/schema/schema.go @@ -894,7 +894,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 @@ -978,7 +979,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 23191d28..b46de471 100644 --- a/compilers/openapi/meta.go +++ b/compilers/openapi/meta.go @@ -1,10 +1,13 @@ package openapi import ( + "strconv" + soa "github.com/speakeasy-api/openapi/openapi" "github.com/dexpace/morphic/compilers/compile" "github.com/dexpace/morphic/compilers/openapi/internal/annotation" + "github.com/dexpace/morphic/compilers/openapi/internal/ids" "github.com/dexpace/morphic/compilers/openapi/internal/lowering" "github.com/dexpace/morphic/ir" ) @@ -38,11 +41,74 @@ type docMeta struct { // service graph: info, servers, and top-level extensions (ir-design §10, §12). func lowerMeta(c lowering.Ctx) (docMeta, []ir.Diagnostic) { m := lowerInfo(c) - m.Servers = lowerServers(c) + + servers, serverDiags := lowerServers(c) + m.Servers = servers ext, diags := annotation.ExtensionsFrom(c.Doc.GetExtensions(), c.SrcIndex, "") m.Unmodeled = ext - return m, diags + 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, 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. +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()}, + } +} + +// 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 { + index := strconv.Itoa(i) + out = append(out, unknownSite{"tags/" + index, ids.Ptr("tags", index), t}) + } + return out } // lowerInfo maps info onto the document identity, docs, contact, and license. @@ -79,31 +145,37 @@ func infoDocs(c lowering.Ctx, info *soa.Info) ir.Docs { // template, description, and templated variables (ir-design §10). It returns nil // rather than an empty slice when every entry was skipped, so a document // declaring no usable server leaves the field unset. -func lowerServers(c lowering.Ctx) []ir.Server { +func lowerServers(c lowering.Ctx) ([]ir.Server, []ir.Diagnostic) { // GetServers never returns an empty slice — it injects a default "/" server // when none are declared — so the loop always runs at least once. servers := c.Doc.GetServers() out := make([]ir.Server, 0, len(servers)) - for _, s := range servers { + var diags []ir.Diagnostic + for i, s := range servers { if s == nil { continue } - out = append(out, lowerServer(s)) + one, serverDiags := lowerServer(c, s, ids.Ptr("servers", strconv.Itoa(i))) + diags = append(diags, serverDiags...) + out = append(out, one) } if len(out) == 0 { - return nil + return nil, diags } - return out + return out, diags } -// lowerServer lowers one server, named by serverName. -func lowerServer(s *soa.Server) ir.Server { - return ir.Server{ +// lowerServer lowers one server, named by serverName; sptr is the server's own +// pointer in the servers list. +func lowerServer(c lowering.Ctx, s *soa.Server, sptr string) (ir.Server, []ir.Diagnostic) { + vars, diags := serverVariables(c, s, sptr) + out := ir.Server{ Name: serverName(s), URLTemplate: s.GetURL(), Description: ir.Docs{Description: s.GetDescription()}, - Variables: serverVariables(s), + Variables: vars, } + 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 @@ -134,22 +206,26 @@ func serverName(s *soa.Server) ir.Naming { // serverVariables lowers a server's URL template variables in source order, or // nil when it declares none. -func serverVariables(s *soa.Server) []ir.ServerVariable { +func serverVariables(c lowering.Ctx, s *soa.Server, sptr string) ([]ir.ServerVariable, []ir.Diagnostic) { vars := s.GetVariables() if vars == nil || vars.Len() == 0 { - return nil + return nil, nil } out := make([]ir.ServerVariable, 0, vars.Len()) + var diags []ir.Diagnostic for name, v := range vars.All() { if v == nil { continue } - out = append(out, ir.ServerVariable{ + one := ir.ServerVariable{ Name: name, Default: v.GetDefault(), Enum: v.GetEnum(), Docs: ir.Docs{Description: v.GetDescription()}, - }) + } + diags = append(diags, annotation.UnknownKeysIn(&one.Unmodeled, v, c.SrcIndex, + sptr+ids.Ptr("variables", name))...) + out = append(out, one) } - return out + return out, diags } diff --git a/compilers/openapi/meta_test.go b/compilers/openapi/meta_test.go index 528f6243..212936e6 100644 --- a/compilers/openapi/meta_test.go +++ b/compilers/openapi/meta_test.go @@ -103,7 +103,7 @@ func TestServerName_DerivedFromURLWhenUnnamed(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, tc.want, lowerServer(&soa.Server{URL: tc.url}).Name) + assert.Equal(t, tc.want, serverName(&soa.Server{URL: tc.url})) }) } } @@ -114,9 +114,9 @@ func TestServerName_DerivedFromURLWhenUnnamed(t *testing.T) { // both servers the same. func TestServerName_DistinguishesServersDifferingOnlyInPath(t *testing.T) { t.Parallel() - v1 := lowerServer(&soa.Server{URL: "https://api.example.com/v1"}) - v2 := lowerServer(&soa.Server{URL: "https://api.example.com/v2"}) - assert.NotEqual(t, v1.Name.Hint, v2.Name.Hint, "two distinct servers get two distinct hints") + v1 := serverName(&soa.Server{URL: "https://api.example.com/v1"}) + v2 := serverName(&soa.Server{URL: "https://api.example.com/v2"}) + assert.NotEqual(t, v1.Hint, v2.Hint, "two distinct servers get two distinct hints") } // TestServerName_CollidesOnPunctuationAlone bounds that claim, which must not be @@ -126,17 +126,18 @@ func TestServerName_DistinguishesServersDifferingOnlyInPath(t *testing.T) { // known bound rather than a later discovery. func TestServerName_CollidesOnPunctuationAlone(t *testing.T) { t.Parallel() - dotted := lowerServer(&soa.Server{URL: "https://api.example.com/v1"}) - dashed := lowerServer(&soa.Server{URL: "https://api.example.com/v-1"}) - assert.Equal(t, dotted.Name.Hint, dashed.Name.Hint, + dotted := serverName(&soa.Server{URL: "https://api.example.com/v1"}) + dashed := serverName(&soa.Server{URL: "https://api.example.com/v-1"}) + assert.Equal(t, dotted.Hint, dashed.Hint, "neutral words carry no punctuation, so these two collide") } func TestLowerServers_NilEntrySkipped(t *testing.T) { t.Parallel() doc := &soa.OpenAPI{Servers: []*soa.Server{nil, {URL: "https://x.example.com"}}} - got := lowerServers(lowering.Ctx{Doc: doc}) + got, diags := lowerServers(lowering.Ctx{Doc: doc}) + assert.Empty(t, diags) require.Len(t, got, 1, "nil server entry skipped, valid one lowered") assert.Equal(t, "https://x.example.com", got[0].URLTemplate) } @@ -147,7 +148,8 @@ func TestServerVariables_NilEntrySkipped(t *testing.T) { sequencedmap.NewElem("skip", (*soa.ServerVariable)(nil)), sequencedmap.NewElem("keep", &soa.ServerVariable{}), ) - srv := lowerServer(&soa.Server{URL: "https://x", Variables: vars}) + srv, diags := lowerServer(lowering.Ctx{}, &soa.Server{URL: "https://x", Variables: vars}, "/servers/0") + assert.Empty(t, diags) require.Len(t, srv.Variables, 1, "nil variable entry skipped") assert.Equal(t, "keep", srv.Variables[0].Name) } @@ -159,5 +161,7 @@ func TestLowerServers_EveryEntrySkippedIsNil(t *testing.T) { t.Parallel() doc := &soa.OpenAPI{Servers: []*soa.Server{nil, nil}} - assert.Nil(t, lowerServers(lowering.Ctx{Doc: doc})) + got, diags := lowerServers(lowering.Ctx{Doc: doc}) + assert.Nil(t, got) + assert.Empty(t, diags) } diff --git a/compilers/openapi/unknownkeys_test.go b/compilers/openapi/unknownkeys_test.go new file mode 100644 index 00000000..ba0d55a0 --- /dev/null +++ b/compilers/openapi/unknownkeys_test.go @@ -0,0 +1,200 @@ +// 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" + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" +) + +// unknownKeysSpec reads the census fixture the corpus sweeps also drive, so the +// assertions below and the six oracles run over the same bytes: putting it under +// testdata is what gets it compiled in both declaration orders, round-tripped and +// verified, none of which a spec written inline reaches. +func unknownKeysSpec(t *testing.T) string { + t.Helper() + data, err := os.ReadFile(filepath.Join("..", "..", "testdata", "openapi", "unknown_keys.yaml")) + require.NoError(t, err) + return string(data) +} + +// 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 := unmodeledSitesOf(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"}, + {"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 := findUnmodeledSite(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_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, unmodeledSitesOf(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) + } +} + +// unmodeledSite is one Unmodeled entry paired with the walk path of the map +// holding it. +type unmodeledSite struct { + key string + path string + entry ir.UnmodeledEntry +} + +// unmodeledSitesOf returns every Unmodeled entry the document holds, found by +// walking the value graph rather than by naming the carriers a test expects. +func unmodeledSitesOf(doc *ir.Document) []unmodeledSite { + unmodeledType := reflect.TypeOf(ir.Unmodeled(nil)) + var out []unmodeledSite + ir.WalkValues(doc, ir.DocumentPath, func(v reflect.Value, path string) bool { + if v.Type() != unmodeledType || !v.CanInterface() { + return true + } + u, ok := v.Interface().(ir.Unmodeled) + if !ok { + return true + } + for key, entry := range u { + out = append(out, unmodeledSite{key: key, path: path, entry: entry}) + } + return true + }) + return out +} + +// findUnmodeledSite returns the site holding key with the given JSON value. The +// value is part of the match because one key spelling occurs at several +// carriers — "openapi:status" is written on two responses in this fixture — so +// matching on the key alone would find another object's entry and call it a +// pass. +func findUnmodeledSite(sites []unmodeledSite, key, wantJSON string) (unmodeledSite, bool) { + for _, site := range sites { + if site.key == key && string(site.entry.Value) == wantJSON { + return site, true + } + } + return unmodeledSite{}, false +} + +// 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 c43e9937..3218cb4f 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 f085da90..042727df 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/conformance/openapi/unwitnessed.golden.txt b/testdata/conformance/openapi/unwitnessed.golden.txt index 0ef3569a..ca1e200c 100644 --- a/testdata/conformance/openapi/unwitnessed.golden.txt +++ b/testdata/conformance/openapi/unwitnessed.golden.txt @@ -195,8 +195,6 @@ Server.Bindings Server.Protocol Server.ProtocolVersion Server.Tags -Server.Unmodeled -ServerVariable.Unmodeled Service.CommonErrors Service.Extends Service.Namespace diff --git a/testdata/openapi/unknown_keys.yaml b/testdata/openapi/unknown_keys.yaml new file mode 100644 index 00000000..7a49ac00 --- /dev/null +++ b/testdata/openapi/unknown_keys.yaml @@ -0,0 +1,59 @@ +# 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), 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. +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: + securitySchemes: + k: {type: apiKey, in: header, name: X-Key, tokenUrl: SECURITYSCHEME} + schemas: + S: + type: object + additionalItems: SCHEMA + properties: + a: {type: string, divisibleBy: PROPERTYSCHEMA} From 6fcfd8ab701619102e8df194f9425b56abf1b900 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 11 Aug 2026 13:51:10 +0300 Subject: [PATCH 2/3] fix(compilers/openapi): census components and odd key spellings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways an undeclared key still reached the IR in no form at all, each leaving two documents that differ compiling to the same document. The Components Object took no census. It was excluded as one of the maps whose every key is a valid entry, which `paths`, `responses` and a callback are — each embeds a sequenced map — while Components is a fixed-field struct beside them, so a key it does not define is as undeclared as one anywhere else. It is a census site now, keyed under "components" on the carrier #345 already gives its extensions. A key written as an alias was reported by the parser under the name it resolves to, while the mapping still holds an alias node whose own value is the anchor; searching it raw found nothing, and a key with no node kept nothing and said nothing. RawChildNode now compares the resolved name, which is the name every caller asks by. A key holding a "/" spelled the scope of the object that path names, so a root key "info/contact/slack" was the same entry as the contact object's own "slack" and the second site to reach the carrier dropped its key in silence. The key is escaped as one segment, per the rule ids.Scope already records for the scopes a document chooses the segments of. Beside them: a key the raw mapping does not present at all is announced under openapi/unknown-key-unreachable instead of passed over. The one class that reaches it is a key merged in through a `<<`, which needs the merge-expanded view this package cannot reach today (#395); the bound now applies to what the census contributes rather than to keys another reader already kept, and tagUnknownSites skips a nil entry the way tagExtensions does. --- .../openapi/internal/annotation/annotation.go | 17 +++- .../annotation/readers_internal_test.go | 18 +++++ .../openapi/internal/annotation/unknown.go | 80 +++++++++++++++---- .../annotation/unknown_internal_test.go | 50 ++++++++++++ compilers/openapi/internal/diag/diag.go | 11 +++ compilers/openapi/internal/diag/diag_test.go | 1 + compilers/openapi/meta.go | 12 ++- compilers/openapi/meta_test.go | 15 ++++ compilers/openapi/unknownkeys_test.go | 61 ++++++++++++-- testdata/openapi/unknown_key_spellings.yaml | 35 ++++++++ testdata/openapi/unknown_keys.yaml | 13 ++- 11 files changed, 286 insertions(+), 27 deletions(-) create mode 100644 testdata/openapi/unknown_key_spellings.yaml diff --git a/compilers/openapi/internal/annotation/annotation.go b/compilers/openapi/internal/annotation/annotation.go index f222497e..b7af1661 100644 --- a/compilers/openapi/internal/annotation/annotation.go +++ b/compilers/openapi/internal/annotation/annotation.go @@ -514,13 +514,28 @@ func RawChildNode(root *yaml.Node, key string) *yaml.Node { return nil } for i := 0; i+1 < len(root.Content); i += 2 { - if root.Content[i].Value == key { + if keyName(root.Content[i]) == key { return root.Content[i+1] } } return nil } +// 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 // one themselves. Obtaining a referent is reference resolution — walk work — // and the two available resolutions are not interchangeable: Site.Referent is diff --git a/compilers/openapi/internal/annotation/readers_internal_test.go b/compilers/openapi/internal/annotation/readers_internal_test.go index e0ee66fb..48d41639 100644 --- a/compilers/openapi/internal/annotation/readers_internal_test.go +++ b/compilers/openapi/internal/annotation/readers_internal_test.go @@ -515,6 +515,24 @@ 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") +} + // 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 index bab5accd..e8c10ec9 100644 --- a/compilers/openapi/internal/annotation/unknown.go +++ b/compilers/openapi/internal/annotation/unknown.go @@ -12,6 +12,28 @@ import ( "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) +} + // 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 @@ -19,6 +41,11 @@ import ( // 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 what this census contributes, not what the object wrote: a key +// 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. const MaxUnknownKeys = 64 // DecidedKeywords are the JSON Schema keywords the library's schema model names @@ -118,25 +145,26 @@ type keyClass struct { // A key p already holds 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. +// better. Those are filtered before the bound applies — see MaxUnknownKeys. func census(p *ir.Unmodeled, model any, srcIndex int, owner, scope string, cl keyClass) []ir.Diagnostic { keys, root := undeclaredKeys(model) - if len(keys) == 0 { + fresh := unrecorded(p, keys, scope, cl.skip) + if len(fresh) == 0 { return nil } var diags []ir.Diagnostic - if len(keys) > MaxUnknownKeys { - diags = append(diags, budgetDiag(len(keys), owner, srcIndex)) - keys = keys[:MaxUnknownKeys] + if len(fresh) > MaxUnknownKeys { + diags = append(diags, budgetDiag(len(fresh), owner, srcIndex)) + fresh = fresh[:MaxUnknownKeys] } - for _, key := range keys { - entry := "openapi:" + scoped(scope, key) - if _, recorded := (*p)[entry]; recorded || slices.Contains(cl.skip, key) { + for _, key := range fresh { + entry, at := "openapi:"+scoped(scope, key), owner+ids.Ptr(key) + node := RawChildNode(root, key) + if node == nil { + diags = append(diags, unreachableKeyDiag(entry, at, srcIndex)) continue } - at := owner + ids.Ptr(key) - kept, keptDiags := PreserveNodeInto(p, entry, RawChildNode(root, key), - ir.ReasonOutOfScope, at, srcIndex) + kept, keptDiags := PreserveNodeInto(p, entry, node, ir.ReasonOutOfScope, at, srcIndex) diags = append(diags, keptDiags...) if !kept { continue @@ -147,20 +175,42 @@ func census(p *ir.Unmodeled, model any, srcIndex int, owner, scope string, cl ke return diags } +// unrecorded returns the keys this census has to contribute: the ones no reader +// already kept on p, less the ones cl has decided about. +func unrecorded(p *ir.Unmodeled, keys []string, scope string, skip []string) []string { + out := make([]string, 0, len(keys)) + for _, key := range keys { + if _, recorded := (*p)["openapi:"+scoped(scope, key)]; recorded || slices.Contains(skip, 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 key + return ids.Scope(key) } - return 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, past the %d this compiler keeps; "+ - "the rest are represented in the IR in no form at all", total, MaxUnknownKeys) + "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 diff --git a/compilers/openapi/internal/annotation/unknown_internal_test.go b/compilers/openapi/internal/annotation/unknown_internal_test.go index 764a402b..75668824 100644 --- a/compilers/openapi/internal/annotation/unknown_internal_test.go +++ b/compilers/openapi/internal/annotation/unknown_internal_test.go @@ -154,6 +154,56 @@ func TestUnknownKeysIn_BudgetBoundsWhatOneObjectContributes(t *testing.T) { 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_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 diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index 53868f68..e42af0c6 100644 --- a/compilers/openapi/internal/diag/diag.go +++ b/compilers/openapi/internal/diag/diag.go @@ -279,6 +279,17 @@ const ( // 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" ) // 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 e14f9f19..5d9c7f03 100644 --- a/compilers/openapi/internal/diag/diag_test.go +++ b/compilers/openapi/internal/diag/diag_test.go @@ -142,6 +142,7 @@ func codes() []string { diag.DuplicateOperationID, diag.IncompleteSecurityScheme, diag.ReservedHeaderName, diag.UnpreservableConstruct, diag.UnknownSchemaKeyword, diag.UnknownObjectKey, diag.UnknownKeyBudget, + diag.UnknownKeyUnreachable, } } diff --git a/compilers/openapi/meta.go b/compilers/openapi/meta.go index 0083e9e9..d3618ccb 100644 --- a/compilers/openapi/meta.go +++ b/compilers/openapi/meta.go @@ -56,7 +56,7 @@ func lowerMeta(c lowering.Ctx) (docMeta, []ir.Diagnostic) { // 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, and each declared tag. +// 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 @@ -84,6 +84,12 @@ type unknownSite struct { // 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") @@ -93,6 +99,7 @@ func rootUnknownSites(c lowering.Ctx) []unknownSite { {"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()}, } } @@ -107,6 +114,9 @@ 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}) } diff --git a/compilers/openapi/meta_test.go b/compilers/openapi/meta_test.go index d9f8fb6e..6fde5113 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 index 784316ba..40a3cf76 100644 --- a/compilers/openapi/unknownkeys_test.go +++ b/compilers/openapi/unknownkeys_test.go @@ -15,17 +15,23 @@ import ( "github.com/dexpace/morphic/ir" ) -// unknownKeysSpec reads the census fixture the corpus sweeps also drive, so the -// assertions below and the six oracles run over the same bytes: putting it under -// testdata is what gets it compiled in both declaration orders, round-tripped and -// verified, none of which a spec written inline reaches. -func unknownKeysSpec(t *testing.T) string { +// 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", "unknown_keys.yaml")) + 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 @@ -63,6 +69,7 @@ func TestUnknownKeys_KeptAtEveryObject(t *testing.T) { {"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"`, @@ -122,6 +129,48 @@ func TestUnknownKeys_SchemaAndObjectAreGradedApart(t *testing.T) { 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_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 diff --git a/testdata/openapi/unknown_key_spellings.yaml b/testdata/openapi/unknown_key_spellings.yaml new file mode 100644 index 00000000..dc6e09c0 --- /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 index 7a49ac00..1fd4b003 100644 --- a/testdata/openapi/unknown_keys.yaml +++ b/testdata/openapi/unknown_keys.yaml @@ -4,10 +4,14 @@ # # 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), 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. +# 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: @@ -49,6 +53,7 @@ paths: description: gone status: ERRORRESPONSE components: + definitions: COMPONENTS securitySchemes: k: {type: apiKey, in: header, name: X-Key, tokenUrl: SECURITYSCHEME} schemas: From d359ca9ced4f4ed080af2c63f5d31dd4bf684791 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 11 Aug 2026 14:17:33 +0300 Subject: [PATCH 3/3] fix(compilers/openapi): report the entries the census cannot claim Re-reviewing the branch as it will be merged turned up two more ways a key reached the IR in no form at all, both silent. A parameter and its schema are two objects at two pointers whose Unmodeled entries share one unscoped map, and so are a header and its schema. A key both of them write is one entry between them, and the census skipped the loser through the branch meant for a keyword another reader had already said better. Sameness is now the entry's provenance rather than its presence: a reader recording at the pointer the census would use has nothing added to it and nothing said, while an entry held for a construct written elsewhere is announced under openapi/unknown-key-entry-taken, naming the holder. Keeping both needs a namespace three merged mechanisms already publish keys in, so that is #396 rather than a side effect of this census. RawChildNode returned the first pair spelling a key where the parser reads the last, which no document could reach until a key could be spelled two ways: an explicit pair and an aliased one are one key to the parser and two nodes here, so the reader described a mapping by a value nothing else in the compiler uses. --- .../openapi/internal/annotation/annotation.go | 11 ++- .../annotation/readers_internal_test.go | 27 ++++++ .../openapi/internal/annotation/unknown.go | 94 ++++++++++++++----- .../annotation/unknown_internal_test.go | 30 ++++++ compilers/openapi/internal/diag/diag.go | 9 ++ compilers/openapi/internal/diag/diag_test.go | 2 +- compilers/openapi/unknownkeys_test.go | 31 ++++++ .../openapi/unknown_key_carrier_clash.yaml | 30 ++++++ 8 files changed, 206 insertions(+), 28 deletions(-) create mode 100644 testdata/openapi/unknown_key_carrier_clash.yaml diff --git a/compilers/openapi/internal/annotation/annotation.go b/compilers/openapi/internal/annotation/annotation.go index b7af1661..8ba4b1e3 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,13 @@ 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 keyName(root.Content[i]) == key { - return root.Content[i+1] + found = root.Content[i+1] } } - return nil + return found } // keyName is the on-wire name a mapping key node spells, following an alias to diff --git a/compilers/openapi/internal/annotation/readers_internal_test.go b/compilers/openapi/internal/annotation/readers_internal_test.go index 48d41639..093de979 100644 --- a/compilers/openapi/internal/annotation/readers_internal_test.go +++ b/compilers/openapi/internal/annotation/readers_internal_test.go @@ -533,6 +533,33 @@ func TestRawChildNode_FindsAKeyWrittenAsAnAlias(t *testing.T) { 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 index e8c10ec9..bcb0326a 100644 --- a/compilers/openapi/internal/annotation/unknown.go +++ b/compilers/openapi/internal/annotation/unknown.go @@ -34,6 +34,24 @@ func unreachableKeyDiag(entry, at string, srcIndex int) ir.Diagnostic { "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 @@ -42,10 +60,15 @@ func unreachableKeyDiag(entry, at string, srcIndex int) ir.Diagnostic { // rather than merely sloppy, and what it discards is announced under // diag.UnknownKeyBudget rather than dropped in silence. // -// It bounds what this census contributes, not what the object wrote: a key +// 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. +// 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 @@ -142,13 +165,15 @@ type keyClass struct { // 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 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. +// 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, scope, cl.skip) + fresh := unrecorded(p, keys, owner, scope, cl.skip) if len(fresh) == 0 { return nil } @@ -158,29 +183,48 @@ func census(p *ir.Unmodeled, model any, srcIndex int, owner, scope string, cl ke fresh = fresh[:MaxUnknownKeys] } for _, key := range fresh { - entry, at := "openapi:"+scoped(scope, key), owner+ids.Ptr(key) - node := RawChildNode(root, key) - if node == nil { - diags = append(diags, unreachableKeyDiag(entry, at, srcIndex)) - continue - } - kept, keptDiags := PreserveNodeInto(p, entry, node, ir.ReasonOutOfScope, at, srcIndex) - diags = append(diags, keptDiags...) - if !kept { - continue - } - diags = append(diags, diag.Newf(cl.severity, cl.code, - ir.Provenance{Source: srcIndex, Pointer: at}, cl.message, key)) + diags = append(diags, keep(p, root, key, srcIndex, owner, scope, cl)...) } return diags } -// unrecorded returns the keys this census has to contribute: the ones no reader -// already kept on p, less the ones cl has decided about. -func unrecorded(p *ir.Unmodeled, keys []string, scope string, skip []string) []string { +// 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 _, recorded := (*p)["openapi:"+scoped(scope, key)]; recorded || slices.Contains(skip, key) { + 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) diff --git a/compilers/openapi/internal/annotation/unknown_internal_test.go b/compilers/openapi/internal/annotation/unknown_internal_test.go index 75668824..e92a1e68 100644 --- a/compilers/openapi/internal/annotation/unknown_internal_test.go +++ b/compilers/openapi/internal/annotation/unknown_internal_test.go @@ -204,6 +204,36 @@ func TestUnknownKeysUnder_KeyHoldingASeparatorIsItsOwnEntry(t *testing.T) { 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 diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index e42af0c6..80d7a0ec 100644 --- a/compilers/openapi/internal/diag/diag.go +++ b/compilers/openapi/internal/diag/diag.go @@ -290,6 +290,15 @@ const ( // 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 5d9c7f03..76fec0c9 100644 --- a/compilers/openapi/internal/diag/diag_test.go +++ b/compilers/openapi/internal/diag/diag_test.go @@ -142,7 +142,7 @@ func codes() []string { diag.DuplicateOperationID, diag.IncompleteSecurityScheme, diag.ReservedHeaderName, diag.UnpreservableConstruct, diag.UnknownSchemaKeyword, diag.UnknownObjectKey, diag.UnknownKeyBudget, - diag.UnknownKeyUnreachable, + diag.UnknownKeyUnreachable, diag.UnknownKeyEntryTaken, } } diff --git a/compilers/openapi/unknownkeys_test.go b/compilers/openapi/unknownkeys_test.go index 40a3cf76..6f67b8ac 100644 --- a/compilers/openapi/unknownkeys_test.go +++ b/compilers/openapi/unknownkeys_test.go @@ -7,6 +7,7 @@ package openapi_test // external test package — exercises only the public API import ( "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -171,6 +172,36 @@ func TestUnknownKeys_SpellingDecidesNeitherEntryNorCarrier(t *testing.T) { "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 diff --git a/testdata/openapi/unknown_key_carrier_clash.yaml b/testdata/openapi/unknown_key_carrier_clash.yaml new file mode 100644 index 00000000..76659228 --- /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}