From 3a0bc4e584428333070c6424bfb7b46f20b984c9 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 05:16:14 +0300 Subject: [PATCH 1/2] 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 053feb278bcac24fe7ffc8a9b3b9a883a6e9d9aa Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 07:32:41 +0300 Subject: [PATCH 2/2] fix(compilers/openapi): keep undeclared keys on nested objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A key the OpenAPI model names no field for was kept at the objects that lower to a node with an Unmodeled map of their own, and dropped in silence at the objects nested inside one: an example, an encoding, the oauth flows and each flow, a schema's xml, discriminator and externalDocs, an operation's and a tag's externalDocs, and the components object. Those had no Unmodeled map to land on until the carriers went in; this reads the census at each of them and keys the entries under the same scheme the extensions there already use, so an entry says which object wrote it and two objects reaching one map cannot collide. The components object was classified as a map with nothing to census. It is not: only the map under each of its keys is the document's to name, while its own key set is the fixed list of component kinds, which the library models as named fields and takes a census over. `paths`, `responses` and a callback are the real maps of that kind, and each is confirmed to fold an unrecognized key into itself rather than report it. Two objects are deliberately left out, both recorded at the code: - A path item. The library folds an unrecognized key into the item's embedded operations map, so there is no census to read; recovering the value needs a method vocabulary wider than the one this compiler owns, which is what #293 is about. Unlike every object above, the key is not lost in silence — folding it reports a type mismatch at error severity naming the key at its own pointer. Filed as #377. - A Link Object, following the same decision made for its extensions. This compiler lowers no Link Object anywhere: a response's links survive only as a verbatim node, and an unreferenced components link is dropped whole. A schema's xml, discriminator and externalDocs are censused but stay out of the corpus fixture. The OpenAPI dialect meta-schema closes all three to anything but an x- key, so an undeclared key there draws a library validation error, and an error diagnostic stops harness.Check before the oracles that fixture exists to reach. --- .../openapi/internal/annotation/annotation.go | 34 +++++---- compilers/openapi/internal/auth/auth.go | 32 +++++---- .../openapi/internal/operation/content.go | 14 ++-- .../openapi/internal/operation/operations.go | 52 +++++++++++--- compilers/openapi/meta.go | 21 ++++-- compilers/openapi/unknownkeys_test.go | 71 +++++++++++++++++++ testdata/openapi/unknown_keys.yaml | 48 +++++++++++-- 7 files changed, 221 insertions(+), 51 deletions(-) diff --git a/compilers/openapi/internal/annotation/annotation.go b/compilers/openapi/internal/annotation/annotation.go index 518973bc..a2125148 100644 --- a/compilers/openapi/internal/annotation/annotation.go +++ b/compilers/openapi/internal/annotation/annotation.go @@ -544,7 +544,7 @@ func Read(st Site, pointer string, srcIndex int) (Set, []ir.Diagnostic) { out.Examples = examples ext, extDiags := ExtensionsFrom(st.Node.GetExtensions(), srcIndex, pointer) - sub, subDiags := subObjectExtensions(st.Node, pointer, srcIndex) + sub, subDiags := subObjectKeys(st.Node, pointer, srcIndex) kept, keptDiags := unmodeledAt(st.Node, pointer, srcIndex) diags := make([]ir.Diagnostic, 0, len(exDiags)+len(extDiags)+len(subDiags)+len(keptDiags)) @@ -557,27 +557,37 @@ func Read(st Site, pointer string, srcIndex int) (Set, []ir.Diagnostic) { return out, diags } -// subObjectExtensions collects the x-* the sub-objects of a schema declare — -// its xml, its discriminator and its externalDocs. Each is an OpenAPI object -// that admits extensions, and none of ir.XMLHints, ir.Discriminator or ir.Link -// holds an Unmodeled map, so the entries ride on the node the schema itself -// lowers to; the keyword each was written under is what keeps three objects' -// extensions apart on that one map. -func subObjectExtensions(s *oas3.Schema, pointer string, srcIndex int) (ir.Unmodeled, []ir.Diagnostic) { +// subObjectKeys collects what the sub-objects of a schema declare that reaches +// no IR field — the x-* they carry and the keys the specification defines for +// none of them — over its xml, its discriminator and its externalDocs. +// +// Each is an OpenAPI object with its own closed key set, and none of +// ir.XMLHints, ir.Discriminator or ir.Link holds an Unmodeled map, so the +// entries ride on the node the schema itself lowers to; the keyword each was +// written under is what keeps three objects' entries apart on that one map. +// +// The census is graded as an OpenAPI object's rather than as a schema keyword's, +// even though these hang off a schema: the JSON Schema rule that an unrecognized +// keyword is legal governs the schema itself, and these three are OpenAPI +// objects that the schema vocabulary says nothing about. +func subObjectKeys(s *oas3.Schema, pointer string, srcIndex int) (ir.Unmodeled, []ir.Diagnostic) { subs := []struct { keyword string + obj any ext *extensions.Extensions }{ - {"xml", s.GetXML().GetExtensions()}, - {"discriminator", s.GetDiscriminator().GetExtensions()}, - {"externalDocs", s.GetExternalDocs().GetExtensions()}, + {"xml", s.GetXML(), s.GetXML().GetExtensions()}, + {"discriminator", s.GetDiscriminator(), s.GetDiscriminator().GetExtensions()}, + {"externalDocs", s.GetExternalDocs(), s.GetExternalDocs().GetExtensions()}, } var out ir.Unmodeled var diags []ir.Diagnostic for _, sub := range subs { - ext, extDiags := ExtensionsUnder(sub.ext, srcIndex, pointer+ids.Ptr(sub.keyword), sub.keyword) + owner := pointer + ids.Ptr(sub.keyword) + ext, extDiags := ExtensionsUnder(sub.ext, srcIndex, owner, sub.keyword) out = MergeUnmodeled(out, ext) diags = append(diags, extDiags...) + diags = append(diags, UnknownKeysUnder(&out, sub.obj, srcIndex, owner, sub.keyword)...) } return out, diags } diff --git a/compilers/openapi/internal/auth/auth.go b/compilers/openapi/internal/auth/auth.go index b24a3a5c..fdfc82af 100644 --- a/compilers/openapi/internal/auth/auth.go +++ b/compilers/openapi/internal/auth/auth.go @@ -132,7 +132,7 @@ func lowerSecurityScheme(c lowering.Ctx, name string, ss *soa.SecurityScheme, return ir.AuthScheme{}, false, []ir.Diagnostic{mechanismRefusalDiag(c, name, missing, entry)} } diags = preserveUnreadFields(c, &scheme, ss, decl) - diags = append(diags, applySchemeExtensions(c, &scheme, ss, decl)...) + diags = append(diags, applySchemeAnnotations(c, &scheme, ss, decl)...) // Distinct from preserveUnreadFields above it: that keeps the fields OpenAPI // defines for a securityScheme which this entry's own mechanism gives no // meaning to, while this keeps the keys OpenAPI defines for no securityScheme @@ -141,37 +141,43 @@ func lowerSecurityScheme(c lowering.Ctx, name string, ss *soa.SecurityScheme, annotation.UnknownKeysIn(&scheme.Unmodeled, ss, c.SrcIndex, decl)...) } -// applySchemeExtensions keeps the x-* of the securitySchemes entry and, for an -// oauth2 scheme, of the flows object and of each flow inside it. ir.OAuthFlow +// applySchemeAnnotations keeps what the securitySchemes entry and, for an oauth2 +// scheme, the flows object and each flow inside it declare that reaches no IR +// field: their x-*, and the keys OpenAPI defines for none of them. ir.OAuthFlow // carries an Unmodeled map of its own; the flows object does not lower to a node -// at all, so its own extensions are kept on the scheme under the keyword. +// at all, so its entries are kept on the scheme under the keyword. // // Only oauth2 reads inside `flows`: on any other type the whole node is kept // verbatim by preserveUnreadFields, extensions and all, and no ir.OAuthFlow was // lowered for a flow's own to land on. -func applySchemeExtensions(c lowering.Ctx, scheme *ir.AuthScheme, ss *soa.SecurityScheme, decl string) []ir.Diagnostic { +func applySchemeAnnotations(c lowering.Ctx, scheme *ir.AuthScheme, ss *soa.SecurityScheme, decl string) []ir.Diagnostic { ext, diags := annotation.ExtensionsFrom(ss.GetExtensions(), c.SrcIndex, decl) scheme.Unmodeled = annotation.MergeUnmodeled(scheme.Unmodeled, ext) if scheme.Kind != ir.AuthKindOAuth2 { return diags } + flows := ss.GetFlows() flowsPtr := decl + ids.Ptr("flows") - flowsExt, flowsDiags := annotation.ExtensionsUnder(ss.GetFlows().GetExtensions(), c.SrcIndex, flowsPtr, "flows") + flowsExt, flowsDiags := annotation.ExtensionsUnder(flows.GetExtensions(), c.SrcIndex, flowsPtr, "flows") scheme.Unmodeled = annotation.MergeUnmodeled(scheme.Unmodeled, flowsExt) diags = append(diags, flowsDiags...) - return append(diags, applyFlowExtensions(c, scheme.Flows, ss.GetFlows(), flowsPtr)...) + diags = append(diags, annotation.UnknownKeysUnder(&scheme.Unmodeled, flows, c.SrcIndex, flowsPtr, "flows")...) + return append(diags, applyFlowAnnotations(c, scheme.Flows, flows, flowsPtr)...) } -// applyFlowExtensions writes each declared flow's own x-* onto the ir.OAuthFlow -// it lowered to. Both lists are one ordered reading of the same object — -// scheme.Flows came from oauthFlows, which walks presentFlows — so the i-th -// lowered flow is the i-th declared one and the two cannot differ in length. -func applyFlowExtensions(c lowering.Ctx, lowered []ir.OAuthFlow, flows *soa.OAuthFlows, flowsPtr string) []ir.Diagnostic { +// applyFlowAnnotations writes each declared flow's own x-* and undeclared keys +// onto the ir.OAuthFlow it lowered to. Both lists are one ordered reading of the +// same object — scheme.Flows came from oauthFlows, which walks presentFlows — so +// the i-th lowered flow is the i-th declared one and the two cannot differ in +// length. +func applyFlowAnnotations(c lowering.Ctx, lowered []ir.OAuthFlow, flows *soa.OAuthFlows, flowsPtr string) []ir.Diagnostic { var diags []ir.Diagnostic for i, f := range presentFlows(flows) { - ext, extDiags := annotation.ExtensionsFrom(f.src.GetExtensions(), c.SrcIndex, flowsPtr+ids.Ptr(f.keyword)) + fptr := flowsPtr + ids.Ptr(f.keyword) + ext, extDiags := annotation.ExtensionsFrom(f.src.GetExtensions(), c.SrcIndex, fptr) lowered[i].Unmodeled = annotation.MergeUnmodeled(lowered[i].Unmodeled, ext) diags = append(diags, extDiags...) + diags = append(diags, annotation.UnknownKeysIn(&lowered[i].Unmodeled, f.src, c.SrcIndex, fptr)...) } return diags } diff --git a/compilers/openapi/internal/operation/content.go b/compilers/openapi/internal/operation/content.go index b2dbdd40..677f1109 100644 --- a/compilers/openapi/internal/operation/content.go +++ b/compilers/openapi/internal/operation/content.go @@ -355,9 +355,10 @@ func encodingConfig(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInd // encodingUnmodeled keeps what an Encoding Object declares that nothing in the // IR holds: `allowReserved`, which ir.PartEncoding has no field for even though -// its neighbours style and explode do, and the object's own x-*. Neither had -// reached an IR field, an Unmodeled entry or a diagnostic, so two documents -// differing only in them compiled to one IR (GitHub #291). +// its neighbours style and explode do, the object's own x-*, and the keys the +// specification defines for no encoding at all. Neither allowReserved nor the +// extensions had reached an IR field, an Unmodeled entry or a diagnostic, so two +// documents differing only in them compiled to one IR (GitHub #291). // // Both ride on the owning ir.Content, since PartEncoding carries no Unmodeled // map, keyed under scope — "encoding/" or "itemEncoding". One content can @@ -378,7 +379,8 @@ func encodingUnmodeled(c lowering.Ctx, enc *soa.Encoding, encPtr, scope string) } ext, extDiags := schema.ExtensionsIn(c, enc.GetExtensions(), encPtr, scope) out = annotation.MergeUnmodeled(out, ext) - return out, append(diags, extDiags...) + diags = append(diags, extDiags...) + return out, append(diags, annotation.UnknownKeysUnder(&out, enc, c.SrcIndex, encPtr, scope)...) } // lowerHeaders lowers a header map into Properties in source order. Each @@ -593,12 +595,14 @@ func exampleList(c lowering.Ctx, single *yaml.Node, plural *sequencedmap.Map[str func appendPluralExample(c lowering.Ctx, out []ir.Example, re *soa.ReferencedExample, pointer, name string) ([]ir.Example, []ir.Diagnostic) { // The declaration pointer, not the entry's, is where an Example Object's own // keywords are written: a $ref entry holds none of them. ir.Example carries an - // Unmodeled map, so the object's x-* need no scope. + // Unmodeled map, so neither the object's x-* nor its undeclared keys need a + // scope. ex, decl := resolve.ObjectAt[soa.Example](c.RefScope(), re, pointer+ids.Ptr("examples", name)) if ex == nil { return out, nil } ext, diags := schema.ExtensionsOf(c, ex.GetExtensions(), decl) + diags = append(diags, annotation.UnknownKeysIn(&ext, ex, c.SrcIndex, decl)...) proto := ir.Example{ Name: name, Summary: ex.GetSummary(), diff --git a/compilers/openapi/internal/operation/operations.go b/compilers/openapi/internal/operation/operations.go index 2e91a71a..583b964b 100644 --- a/compilers/openapi/internal/operation/operations.go +++ b/compilers/openapi/internal/operation/operations.go @@ -296,31 +296,38 @@ func lowerOperation(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInd diags = append(diags, cbDiags...) } op.Bindings = ir.OpBindings{HTTP: []ir.HTTPBinding{hb}} - diags = append(diags, applyOperationExtensions(c, &op, src, decl)...) - diags = append(diags, annotation.UnknownKeysIn(&op.Unmodeled, src, c.SrcIndex, decl)...) + diags = append(diags, applyOperationAnnotations(c, &op, src, decl)...) diags = append(diags, applyOperationServers(c, &op, src, decl)...) return op, extra, append(diags, checkOperationIDUnique(c, operationIDs, op, mount)...) } -// applyOperationExtensions keeps the operation's own x-* and those of the two -// objects beneath it that lower to no node of their own: its externalDocs, -// since ir.Link holds no Unmodeled map, and its Responses Object, whose -// extensions are the map's own rather than any one response's. +// applyOperationAnnotations keeps the operation's own x-* and undeclared keys, +// and those of the two objects beneath it that lower to no node of their own: +// its externalDocs, since ir.Link holds no Unmodeled map, and its Responses +// Object, whose extensions are the map's own rather than any one response's. +// +// The Responses Object contributes extensions but no census. Its key set is the +// status codes the document chooses, which the library models as a map, so an +// undeclared key there is read as one more response rather than reported as +// unknown — there is nothing for a census to say about it. // // It merges rather than assigns. The operation's map already carries whatever // its parameters or callbacks wrote by the time this runs, and an assignment // here would drop them — which is why the servers preservation used to have to // run after it. -func applyOperationExtensions(c lowering.Ctx, op *ir.Operation, src *soa.Operation, decl string) []ir.Diagnostic { +func applyOperationAnnotations(c lowering.Ctx, op *ir.Operation, src *soa.Operation, decl string) []ir.Diagnostic { + docsPtr := decl + ids.Ptr("externalDocs") ext, diags := annotation.ExtensionsAt(c.SrcIndex, annotation.ExtensionSite{Owner: decl, Ext: src.GetExtensions()}, - annotation.ExtensionSite{Scope: "externalDocs", Owner: decl + ids.Ptr("externalDocs"), + annotation.ExtensionSite{Scope: "externalDocs", Owner: docsPtr, Ext: src.GetExternalDocs().GetExtensions()}, annotation.ExtensionSite{Scope: "responses", Owner: decl + ids.Ptr("responses"), Ext: src.GetResponses().GetExtensions()}, ) op.Unmodeled = annotation.MergeUnmodeled(op.Unmodeled, ext) - return diags + diags = append(diags, annotation.UnknownKeysIn(&op.Unmodeled, src, c.SrcIndex, decl)...) + return append(diags, annotation.UnknownKeysUnder(&op.Unmodeled, + src.GetExternalDocs(), c.SrcIndex, docsPtr, "externalDocs")...) } // applyOperationServers preserves an operation's own `servers` verbatim under @@ -412,6 +419,25 @@ func fillOperationDocs(d *ir.Docs, src *soa.Operation) { // must reach both, and a second call beside the first is a second chance to // forget one on a route added later. That is exactly how the servers half came // to be missing on two of its three routes (GitHub #39). +// +// A path item takes no census, unlike every other object the compiler reads +// extensions from, and deliberately so. The library folds a key it does not +// recognize into the item's embedded operations map rather than recording it as +// undeclared, so GetUnknownProperties reports nothing and there is no census to +// read (speakeasy-api/openapi v1.24.0). Two consequences decide it: +// +// - The key is not lost in silence. Folding it produces a +// validation-type-mismatch at error severity naming the key at its own +// pointer, which is the losslessness property the census exists for; what is +// lost is the key's value, not the fact that it was written. +// - Recovering the value means reading the raw node against a path item's key +// vocabulary, and the only vocabulary this compiler owns is httpMethods, +// which is narrower than the library's — it has no `query`, the method +// OpenAPI 3.2 adds. A census over what httpMethods does not name would +// therefore report a valid 3.2 `query` operation as an undeclared key. +// +// That vocabulary is what GitHub #293 is about, so widening it here would settle +// that issue as a side effect of this one. Tracked separately in GitHub #377. func applyPathItem(c lowering.Ctx, op *ir.Operation, pi *soa.PathItem, declPtr string) []ir.Diagnostic { diags := applyPathServers(c, op, pi, declPtr) ext, extDiags := schema.ExtensionsIn(c, pi.GetExtensions(), declPtr, "pathItem") @@ -522,6 +548,14 @@ func lowerResponse(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInde // The links entry carries ReasonNoIRHome and no diagnostic, as it always has: // nothing is degraded, the map is in the document, and the gap is one the IR can // close by growing a links field. +// +// A Link Object inside that map gets no entry and no census of its own, which is +// the decision already recorded for its extensions. This compiler lowers no Link +// Object anywhere: a response's links survive only as the verbatim node above, +// and a components/links entry nothing references is dropped whole, so a keyed +// entry at one of the two positions would be the only trace of a construct the +// IR does not model — while duplicating, for the response position alone, a +// value the node above already carries. func preserveResponseExtras(c lowering.Ctx, p *ir.Unmodeled, r *soa.Response, rptr string) []ir.Diagnostic { _, diags := schema.PreserveNode(c, p, "openapi:links", annotation.RawChildNode(r.GetRootNode(), "links"), ir.ReasonNoIRHome, rptr+ids.Ptr("links")) diff --git a/compilers/openapi/meta.go b/compilers/openapi/meta.go index f8874e6d..ae6aafdb 100644 --- a/compilers/openapi/meta.go +++ b/compilers/openapi/meta.go @@ -109,7 +109,8 @@ func tagExtensions(c lowering.Ctx) []annotation.ExtensionSite { // 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 with its +// own externalDocs. // // 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 @@ -137,6 +138,11 @@ 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 rather than a map with nothing to say: +// its own key set is the fixed list of component kinds, which the library models +// as named fields and takes a census over, and only the map *under* each of +// those keys is the document's to name. func rootUnknownSites(c lowering.Ctx) []unknownSite { info := c.Doc.GetInfo() infoPtr := ids.Ptr("info") @@ -146,11 +152,13 @@ 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()}, } } -// tagUnknownSites returns one census site per declared tag, since ir.TagDef -// holds no Unmodeled map for a tag's own keys to land on. +// tagUnknownSites returns the census sites each declared tag contributes: its +// own, and its externalDocs object's. Neither ir.TagDef nor ir.Link holds an +// Unmodeled map, so both ride on the document. // // 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 @@ -158,10 +166,13 @@ func rootUnknownSites(c lowering.Ctx) []unknownSite { // alike would silently leave one entry. func tagUnknownSites(c lowering.Ctx) []unknownSite { tags := c.Doc.GetTags() - out := make([]unknownSite, 0, len(tags)) + out := make([]unknownSite, 0, 2*len(tags)) for i, t := range tags { index := strconv.Itoa(i) - out = append(out, unknownSite{"tags/" + index, ids.Ptr("tags", index), t}) + ptr := ids.Ptr("tags", index) + out = append(out, + unknownSite{"tags/" + index, ptr, t}, + unknownSite{"tags/" + index + "/externalDocs", ptr + ids.Ptr("externalDocs"), t.GetExternalDocs()}) } return out } diff --git a/compilers/openapi/unknownkeys_test.go b/compilers/openapi/unknownkeys_test.go index 784316ba..34f5269c 100644 --- a/compilers/openapi/unknownkeys_test.go +++ b/compilers/openapi/unknownkeys_test.go @@ -56,15 +56,31 @@ func TestUnknownKeys_KeptAtEveryObject(t *testing.T) { {"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"}, + {"components", "openapi:components/definitions", `"COMPONENTS"`, "doc.Unmodeled"}, + {"tag externalDocs", "openapi:tags/0/externalDocs/title", `"TAGEXTERNALDOCS"`, "doc.Unmodeled"}, {"operation", "openapi:operationid", `"OPERATION"`, ".Unmodeled"}, + // The carrier is named down to the operation rather than left at + // ".Unmodeled": the document writes this exact key too, and the row is only + // evidence of the operation's if it cannot match the document's. + {"operation externalDocs", "openapi:externalDocs/title", `"OPERATIONEXTERNALDOCS"`, + "Operations[0].Unmodeled"}, {"parameter", "openapi:collectionFormat", `"PARAMETER"`, ".Params[0].Unmodeled"}, + {"example", "openapi:name", `"EXAMPLE"`, ".Params[0].Examples[0].Unmodeled"}, {"request body", "openapi:schema", `"REQUESTBODY"`, ".Request.Unmodeled"}, {"media type", "openapi:format", `"MEDIATYPE"`, ".Contents[0].Unmodeled"}, + // On the content rather than on the part: ir.PartEncoding holds no + // Unmodeled map, and one content can carry an entry per part, so the part + // name is what tells two encodings' keys apart on that one map. + {"encoding", "openapi:encoding/part/contentEncoding", `"ENCODING"`, ".Contents[1].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"}, + {"oauth flows", "openapi:flows/application", `"OAUTHFLOWS"`, + "doc.Auth[auth/openapi/components/securitySchemes/o].Unmodeled"}, + {"oauth flow", "openapi:scope", `"OAUTHFLOW"`, + "doc.Auth[auth/openapi/components/securitySchemes/o].Flows[0].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 @@ -147,6 +163,61 @@ components: } } +// TestUnknownKeys_SchemaSubObjects covers the three objects that hang off a +// schema — its xml, its discriminator and its externalDocs — which the fixture +// above deliberately leaves out. +// +// It leaves them out because the OpenAPI dialect meta-schema closes all three to +// anything but an x- key, so the library reports a validation error on each and +// harness.Check returns at the first one, before the oracles that fixture exists +// to reach. The keys are kept and announced all the same, which is what this +// asserts: an invalid document is still not a document whose keys may vanish. +// +// Graded as an OpenAPI object's keys rather than as schema keywords, at warning +// rather than info: JSON Schema's rule that an unrecognized keyword is legal +// governs the schema, and these three are OpenAPI objects the schema vocabulary +// says nothing about. All three ride on the schema's own map, since none of +// ir.XMLHints, ir.Discriminator or ir.Link holds one, and the keyword each was +// written under is what keeps them apart there. +func TestUnknownKeys_SchemaSubObjects(t *testing.T) { + t.Parallel() + doc, diags := compileAnnotationSpec(t, "schema-sub-objects", `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: {} +components: + schemas: + S: + type: object + xml: {name: s, attribute2: XML} + discriminator: {propertyName: k, mapping2: DISCRIMINATOR} + externalDocs: {url: 'https://d.example', title: SCHEMAEXTERNALDOCS} + properties: {k: {type: string}} +`) + schema, ok := doc.Types[ir.TypeID("t/openapi/components/schemas/S")] + require.True(t, ok) + + // assert rather than require on the lookup, so a keyword missing from the map + // reports itself and leaves the other two still checked. The three share one + // reader, and a row that never runs is no evidence about the keyword it names. + for _, tc := range []struct{ keyword, key, want string }{ + {"xml", "openapi:xml/attribute2", `"XML"`}, + {"discriminator", "openapi:discriminator/mapping2", `"DISCRIMINATOR"`}, + {"externalDocs", "openapi:externalDocs/title", `"SCHEMAEXTERNALDOCS"`}, + } { + entry, found := schema.Common().Unmodeled[tc.key] + if !assert.True(t, found, "schema %s drops %s: it is nowhere on the schema", tc.keyword, tc.key) { + continue + } + assert.Equal(t, tc.want, string(entry.Value), "%s key keeps what the source wrote", tc.keyword) + assert.Equal(t, ir.ReasonOutOfScope, entry.Reason, "%s key reason", tc.keyword) + at := "/components/schemas/S/" + tc.keyword + "/" + tc.key[len("openapi:"+tc.keyword+"/"):] + assert.Equal(t, at, entry.Provenance.Pointer, "%s key provenance", tc.keyword) + assert.Equal(t, []ir.Severity{ir.SeverityWarning}, + diagsAt(diags, "openapi/unknown-object-key", at), + "%s key is announced as an object's, not as a schema keyword's", tc.keyword) + } +} + // requireNoErrorDiagnostics fails the test on the first error-severity // diagnostic, naming it. func requireNoErrorDiagnostics(t *testing.T, diags []ir.Diagnostic) { diff --git a/testdata/openapi/unknown_keys.yaml b/testdata/openapi/unknown_keys.yaml index 7a49ac00..ba1668b1 100644 --- a/testdata/openapi/unknown_keys.yaml +++ b/testdata/openapi/unknown_keys.yaml @@ -3,11 +3,24 @@ # 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. +# Swagger 2.0 field with no OpenAPI 3 equivalent (basePath, host, definitions, a +# body parameter's schema, a flow named `application`), 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, a documentation page's `title` on an externalDocs), or a field with the +# case or the number wrong (operationid, scope). None of them reached an IR field, +# an Unmodeled entry or a diagnostic. +# +# `title` is written on all three externalDocs objects and `scope` on the one +# flow, so the entries a document can write more than once are here in more than +# one copy: an unscoped key would leave a single entry and the survivor would +# depend on which lowering ran last. +# +# A schema's own xml, discriminator and externalDocs are censused too but are not +# here. The OpenAPI dialect meta-schema closes those three to undeclared keys, so +# one draws a library validation error, and an error diagnostic stops +# harness.Check before the oracles this fixture exists to reach — see +# TestUnknownKeys_SchemaSubObjects for where they are asserted instead. openapi: 3.1.0 basePath: ROOT info: @@ -23,15 +36,23 @@ servers: variables: region: {default: us, example: SERVERVARIABLE} tags: - - {name: t1, color: TAG} + - name: t1 + color: TAG + externalDocs: {url: 'https://t.example', title: TAGEXTERNALDOCS} paths: /widgets: get: operationId: listWidgets tags: [t1] operationid: OPERATION + externalDocs: {url: 'https://o.example', title: OPERATIONEXTERNALDOCS} parameters: - - {name: shape, in: query, schema: {type: string}, collectionFormat: PARAMETER} + - name: shape + in: query + schema: {type: string} + collectionFormat: PARAMETER + examples: + round: {value: circle, name: EXAMPLE} requestBody: required: true schema: REQUESTBODY @@ -39,6 +60,10 @@ paths: application/json: schema: {$ref: '#/components/schemas/S'} format: MEDIATYPE + multipart/form-data: + schema: {type: object, properties: {part: {type: string}}} + encoding: + part: {contentType: text/plain, contentEncoding: ENCODING} responses: "200": description: ok @@ -49,8 +74,17 @@ paths: description: gone status: ERRORRESPONSE components: + definitions: COMPONENTS securitySchemes: k: {type: apiKey, in: header, name: X-Key, tokenUrl: SECURITYSCHEME} + o: + type: oauth2 + flows: + application: OAUTHFLOWS + implicit: + authorizationUrl: 'https://auth.example' + scopes: {} + scope: OAUTHFLOW schemas: S: type: object