From e4f4069a19751b1973c2b4342598b1fece3428c3 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 08:02:38 +0300 Subject: [PATCH 1/3] fix(compilers/openapi): keep a path item's undeclared keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every OpenAPI object the compiler reads records the keys the specification does not define for it, kept verbatim under Unmodeled and announced at warning. A Path Item Object was the one exception, because the library takes no census for it: the unmarshaller folds a key it does not recognize into the item's embedded operations map rather than recording it as undeclared, so GetUnknownProperties is empty however much the document wrote. The value written under such a key reached the IR in no form. For a scalar the key was at least named, by the type mismatch the fold produces when something that is not an operation object is unmarshalled as one; for a well-formed operation under an undefined key there was no finding at all, and two documents differing only in it compiled to the same IR. Read what is left of the operations map once the HTTP methods are taken out. Everything else a path item may write — summary, description, servers, parameters, additionalOperations, x-* — is a field of the library's model, and a $ref is consumed by the reference wrapper, so nothing else reaches that map. The predicate is the library's own IsStandardMethod rather than this compiler's httpMethods: the two answer different questions, and reading the map against httpMethods would report a valid OpenAPI 3.2 `query` operation as an undeclared key, since the compiler does not lower that method yet. Whether it should is a separate question and stays open. Entries land on every operation of the item, like its servers and its x-*, under the same key spelling, reason and severity the census already uses. All three routes that lower a path item — a path, a webhook, a callback expression — reach it through the one entry point that already applies the other two. --- .../openapi/internal/annotation/unknown.go | 57 ++++++++--- .../openapi/internal/operation/operations.go | 78 +++++++++------ .../internal/operation/operations_test.go | 62 ++++++++++++ compilers/openapi/unknownkeys_test.go | 95 +++++++++++++++++++ testdata/openapi/unknown_keys.yaml | 14 ++- 5 files changed, 262 insertions(+), 44 deletions(-) diff --git a/compilers/openapi/internal/annotation/unknown.go b/compilers/openapi/internal/annotation/unknown.go index bab5acc..56db979 100644 --- a/compilers/openapi/internal/annotation/unknown.go +++ b/compilers/openapi/internal/annotation/unknown.go @@ -55,7 +55,8 @@ var DecidedKeywords = []string{"$comment", "$dynamicAnchor", "$dynamicRef"} // 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{ + keys, root := undeclaredKeys(s) + return census(p, keys, root, srcIndex, pointer, "", keyClass{ code: diag.UnknownSchemaKeyword, severity: ir.SeverityInfo, skip: DecidedKeywords, @@ -88,7 +89,28 @@ func UnknownKeysIn(p *ir.Unmodeled, model any, srcIndex int, owner string) []ir. // 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{ + keys, root := undeclaredKeys(model) + return UnknownKeysNamed(p, keys, root, srcIndex, owner, scope) +} + +// UnknownKeysNamed is UnknownKeysUnder for an object whose model keeps no census +// of its own, so the caller names the keys and hands over the mapping node they +// were written on. +// +// One object needs it: a Path Item Object, whose core model embeds the map of +// its operations. The unmarshaller folds a key it does not recognize into that +// embedded map rather than recording it as undeclared, so the object's own +// census is empty however much the document wrote (speakeasy-api/openapi +// v1.24.0). Its leftovers are still an undeclared key of the path item, graded +// as one — same code, same severity, same reason — because which reader found +// them is not a property of the source. +// +// It delegates rather than duplicating the grading, so the two can only be +// announced alike. +func UnknownKeysNamed(p *ir.Unmodeled, keys []string, root *yaml.Node, + srcIndex int, owner, scope string, +) []ir.Diagnostic { + return census(p, keys, root, 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 " + @@ -112,18 +134,28 @@ type keyClass struct { 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. +// census records on p every key in keys, read off the mapping node root they +// were written on, each under its own key beneath scope. +// +// It sorts, and on a copy. Neither source of keys hands over the order the +// document wrote them in: a core model's census is filled by a parallel walk of +// the mapping under a mutex, and a path item's leftovers are what a filter left +// of a map. Both slices belong to the model they came from, so sorting one in +// place would reorder it under its owner; an unsorted read would order this +// compiler's diagnostics by something the source does not decide, which +// invariant 7 forbids. // // 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) +func census(p *ir.Unmodeled, keys []string, root *yaml.Node, + srcIndex int, owner, scope string, cl keyClass, +) []ir.Diagnostic { if len(keys) == 0 { return nil } + keys = slices.Sorted(slices.Values(keys)) var diags []ir.Diagnostic if len(keys) > MaxUnknownKeys { diags = append(diags, budgetDiag(len(keys), owner, srcIndex)) @@ -178,14 +210,9 @@ type parsedObject interface { // 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. +// undeclaredKeys returns the keys model's source object wrote that its model +// names no field for, and the mapping node they were written on. The order is +// the library's; census is what puts it in one the source decides. // // 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 @@ -203,5 +230,5 @@ func undeclaredKeys(model any) ([]string, *yaml.Node) { if !ok { return nil, nil } - return slices.Sorted(slices.Values(core.GetUnknownProperties())), obj.GetRootNode() + return core.GetUnknownProperties(), obj.GetRootNode() } diff --git a/compilers/openapi/internal/operation/operations.go b/compilers/openapi/internal/operation/operations.go index 583b964..54c03ad 100644 --- a/compilers/openapi/internal/operation/operations.go +++ b/compilers/openapi/internal/operation/operations.go @@ -410,39 +410,63 @@ func fillOperationDocs(d *ir.Docs, src *soa.Operation) { } // applyPathItem keeps what a path item declares that its operations have no -// home for: its servers and its own x-* extensions. Both belong to the path -// item rather than to any one operation on it, so both are written onto every -// operation the item declares. +// home for: its servers, its own x-* extensions, and the keys the specification +// does not define for it. All three belong to the path item rather than to any +// one operation on it, so all three are written onto every operation the item +// declares. // -// The two are applied together, through this one entry point, because every -// route that lowers a path item — a path, a webhook, a callback expression — -// 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). +// They are applied together, through this one entry point, because every route +// that lowers a path item — a path, a webhook, a callback expression — must +// reach each, 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. +// The census reaches this object through its own keys rather than through the +// library's, which reports none for it: the unmarshaller folds a key it does not +// recognize into the item's embedded operations map, so GetUnknownProperties is +// empty however much the document wrote (speakeasy-api/openapi v1.24.0). What is +// left of that map once the HTTP methods are taken out is the same set the +// census would have reported, which is what undeclaredPathItemKeys reads. 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") op.Unmodeled = annotation.MergeUnmodeled(op.Unmodeled, ext) - return append(diags, extDiags...) + diags = append(diags, extDiags...) + return append(diags, annotation.UnknownKeysNamed(&op.Unmodeled, undeclaredPathItemKeys(pi), + pi.GetRootNode(), c.SrcIndex, declPtr, "pathItem")...) +} + +// undeclaredPathItemKeys returns the keys of a path item's operations map that +// name no HTTP method, which are the keys the Path Item Object does not define. +// Everything else the object may write is taken out before that map is filled: +// summary, description, servers, parameters, additionalOperations and every x-* +// are fields of the library's model, and a $ref is consumed by the reference +// wrapper around it — pi is the referent it named by the time this runs. +// +// The predicate is the library's IsStandardMethod, deliberately, and not +// httpMethods. The two are different vocabularies for different questions: +// httpMethods says what this compiler lowers, while this asks only whether a key +// names a method at all. Reading the map against httpMethods would report a +// valid OpenAPI 3.2 `query` operation as an undeclared key, since the compiler +// does not lower it yet — and whether it should is GitHub #293, which this +// leaves exactly where it stands. +// +// A method spelled in any case but lowercase is undeclared and reported here. +// OpenAPI fixes the field names, so `GET` is no more a path item's key than +// `bogusPathItem` is, and neither is lowered. +// +// pi is never nil: every caller of applyPathItem has already lowered an +// operation off it. An uninitialized map is still tolerated, since the iterator +// is nil-safe. +func undeclaredPathItemKeys(pi *soa.PathItem) []string { + var keys []string + for method := range pi.All() { + if soa.IsStandardMethod(string(method)) { + continue + } + keys = append(keys, string(method)) + } + return keys } // applyPathServers preserves path-item-level servers verbatim under Unmodeled diff --git a/compilers/openapi/internal/operation/operations_test.go b/compilers/openapi/internal/operation/operations_test.go index b621fdb..531b281 100644 --- a/compilers/openapi/internal/operation/operations_test.go +++ b/compilers/openapi/internal/operation/operations_test.go @@ -1493,6 +1493,68 @@ func TestOperations_PathItemServersKeptOnEveryRoute(t *testing.T) { } } +// pathItemUnknownKeySpec writes one key the Path Item Object does not define on +// each of the three path items a document can declare, valued with the route it +// sits on so an entry recovered from the wrong item cannot pass for the right +// one. +// +// The value is a whole operation because that is what the position accepts: +// the library folds a key it does not recognize into the item's operations map +// and unmarshals it as an Operation, so a scalar there is a validation error +// rather than a key with a value to keep. It is also the case that used to be +// lost in silence — a well-formed operation under an undefined key raised no +// finding at all. +const pathItemUnknownKeySpec = `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /p: + onPath: {responses: {"200": {description: PATH}}} + post: + operationId: postP + callbacks: + onEvent: + '{$request.body#/url}': + onCallback: {responses: {"200": {description: CALLBACK}}} + post: + operationId: onEvent + responses: {"200": {description: ok}} + responses: {"200": {description: ok}} +webhooks: + hooked: + onWebhook: {responses: {"200": {description: WEBHOOK}}} + post: + operationId: onHook + responses: {"200": {description: ok}} +` + +// TestOperations_PathItemUnknownKeyKeptOnEveryRoute holds the path item's census +// to all three routes that lower one, for the reason recorded above the servers +// case beside it: the same object has three parents, and that half of +// applyPathItem was live on the paths walk and missing on the other two +// (GitHub #39). Both halves now run from the one entry point, and this is what +// says so rather than assuming it. +func TestOperations_PathItemUnknownKeyKeptOnEveryRoute(t *testing.T) { + t.Parallel() + doc, diags := parseFull(t, pathItemUnknownKeySpec) + requireNoErrorDiags(t, diags) + + for _, tc := range []struct{ op, key, marker, at string }{ + {"postP", "openapi:pathItem/onPath", "PATH", "/paths/~1p/onPath"}, + {"onHook", "openapi:pathItem/onWebhook", "WEBHOOK", "/webhooks/hooked/onWebhook"}, + {"onEvent", "openapi:pathItem/onCallback", "CALLBACK", + "/paths/~1p/post/callbacks/onEvent/{$request.body#~1url}/onCallback"}, + } { + entry, ok := findOp(t, doc, tc.op).Unmodeled[tc.key] + require.True(t, ok, "%s keeps the key its own path item wrote", tc.op) + assert.Equal(t, ir.ReasonOutOfScope, entry.Reason) + assert.JSONEq(t, `{"responses":{"200":{"description":"`+tc.marker+`"}}}`, string(entry.Value), + "%s keeps the value its own path item declared", tc.op) + assert.Equal(t, tc.at, entry.Provenance.Pointer) + assert.True(t, hasDiagCodeAt(diags, diag.UnknownObjectKey, tc.at), + "%s announces the key at the key's own pointer", tc.op) + } +} + // TestErrorCase_SingleMediaTypeKeepsContentMap pins the arity-independent half of // error-content preservation. ir.ErrorCase holds a TypeRef and no media type, so // an error declared only as application/problem+json reached the IR diff --git a/compilers/openapi/unknownkeys_test.go b/compilers/openapi/unknownkeys_test.go index 34f5269..8350ff3 100644 --- a/compilers/openapi/unknownkeys_test.go +++ b/compilers/openapi/unknownkeys_test.go @@ -59,6 +59,11 @@ func TestUnknownKeys_KeptAtEveryObject(t *testing.T) { {"components", "openapi:components/definitions", `"COMPONENTS"`, "doc.Unmodeled"}, {"tag externalDocs", "openapi:tags/0/externalDocs/title", `"TAGEXTERNALDOCS"`, "doc.Unmodeled"}, {"operation", "openapi:operationid", `"OPERATION"`, ".Unmodeled"}, + // On each operation of the item, like its servers and its x-*: the path + // item lowers to no node of its own, so the scope is what tells its keys + // from the operation's own on the one map they share. + {"path item", "openapi:pathItem/GET", `{"responses":{"200":{"description":"PATHITEM"}}}`, + "Operations[0].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. @@ -218,6 +223,96 @@ components: } } +// TestUnknownKeys_PathItemKeyKeepsItsValue compiles the reproducer from GitHub +// #377 and holds the half of it that used to go missing. +// +// The other half never did: folding the key into the item's operations map makes +// the library reject the scalar sitting where an Operation should be, so the +// error below is the diagnostic that named the key all along. What no channel +// carried is the 1 it was written with, which is what the entry keeps — and the +// warning beside it is the census's own, graded like every other undeclared key. +func TestUnknownKeys_PathItemKeyKeepsItsValue(t *testing.T) { + t.Parallel() + doc, diags := compileAnnotationSpec(t, "path-item-key", `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /x: + bogusPathItem: 1 + get: {operationId: getX, responses: {"200": {description: ok}}} +`) + op, ok := opByName(doc, "getX") + require.True(t, ok) + + entry := unmodeledEntry(t, op.Unmodeled, "openapi:pathItem/bogusPathItem") + assert.JSONEq(t, `1`, string(entry.Value), "the key keeps the value the source wrote") + assert.Equal(t, ir.ReasonOutOfScope, entry.Reason, + "OpenAPI defines no such key on a path item, so no IR node is coming for it") + assert.Equal(t, "/paths/~1x/bogusPathItem", entry.Provenance.Pointer) + assert.Equal(t, []ir.Severity{ir.SeverityWarning}, + diagsAt(diags, "openapi/unknown-object-key", "/paths/~1x/bogusPathItem")) + _, isError := ir.FirstError(diags) + assert.True(t, isError, "the library still rejects the scalar the key was written with") +} + +// TestUnknownKeys_PathItemDeclaredFieldsAreNotUndeclared is the control the path +// item's census needs, and the one that decides whether reading its operations +// map is sound at all. +// +// `query` is the case that decides it. OpenAPI 3.2 adds the method, and the +// library puts it in the same map as every other operation, so a census taken +// against the eight methods this compiler lowers would report a valid operation +// as a key the specification does not define. Taking it against the library's own +// method vocabulary is what keeps the two questions apart: whether a key names a +// method, and whether this compiler lowers it — the second being GitHub #293. +// +// The rest of a path item's fields are here because a census is only evidence +// about the keys it leaves alone. Each is a field of the library's model and so +// never reaches the operations map, which is the property being pinned: +// additionalOperations included, since #293 has it dropped rather than +// undeclared. +func TestUnknownKeys_PathItemDeclaredFieldsAreNotUndeclared(t *testing.T) { + t.Parallel() + doc, diags := compileAnnotationSpec(t, "path-item-fields", `openapi: 3.2.0 +info: {title: T, version: "1"} +paths: + /x: + $ref: '#/components/pathItems/P' + /y: + summary: s + description: d + servers: [{url: 'https://a.example'}] + parameters: [{name: p, in: query, schema: {type: string}}] + additionalOperations: + PURGE: {operationId: purgeY, responses: {"200": {description: ok}}} + get: {operationId: getY, responses: {"200": {description: ok}}} + query: {operationId: queryY, responses: {"200": {description: ok}}} +components: + pathItems: + P: + summary: ps + description: pd + get: {operationId: getP, responses: {"200": {description: ok}}} +`) + requireNoErrorDiagnostics(t, diags) + + for _, d := range diags { + assert.NotEqual(t, "openapi/unknown-object-key", d.Code, + "every key here is one a path item declares: %+v", d) + } + for _, site := range unmodeledSites(doc) { + assert.NotContains(t, site.key, "openapi:pathItem/", + "nothing a path item declares is kept as a key it does not: %s at %s", site.key, site.path) + } + // An absence is only evidence where the reader ran, and the census runs once + // per operation lowered off a path item. Both items here have to reach it, + // the $ref'd one included, or the rows above hold over a walk that never + // looked. + for _, name := range []string{"getP", "getY"} { + _, ok := opByName(doc, name) + assert.True(t, ok, "%s lowers, so the census ran on the path item declaring it", name) + } +} + // 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 ba1668b..0aaf80e 100644 --- a/testdata/openapi/unknown_keys.yaml +++ b/testdata/openapi/unknown_keys.yaml @@ -8,14 +8,23 @@ # 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. +# case or the number wrong (operationid, scope, GET). 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. # +# The path item's key holds a whole operation where the rest hold a scalar. A key +# the library does not recognize there is folded into the item's operations map +# and unmarshalled as an Operation, so anything that is not one draws a +# validation error, and an error stops harness.Check before the oracles this +# fixture exists to reach. `GET` is the key for the same reason: a method with the +# case wrong is what a document really writes at a position that accepts an +# operation. It is also the shape the compiler lost most quietly — folding a +# well-formed operation raises no validation error at all, so nothing named it. +# # 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 @@ -41,6 +50,7 @@ tags: externalDocs: {url: 'https://t.example', title: TAGEXTERNALDOCS} paths: /widgets: + GET: {responses: {"200": {description: PATHITEM}}} get: operationId: listWidgets tags: [t1] From 3d4c899e4b54e93f486f86cde55c46c562cb2725 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 08:16:29 +0300 Subject: [PATCH 2/3] fix(compilers/openapi): keep an unmounted path item's declarations applyPathItem runs once per operation an item produces, so an item that produces none reached it through nothing. Its servers, its extensions and its undeclared keys were dropped whole, and no diagnostic named the loss -- the census added above included, since it had no carrier to write to. An item produces no operation when it declares none, when every method it declares is one this compiler does not lower yet (#293), or when the only keys it holds are ones the Path Item Object does not define. They go on the service, which is where the Paths Object's own extensions already go for the same reason: a path item lowers to no node, so the nearest node holding an Unmodeled map is what holds them. The key carries the item's own pointer, because one service holds every such item and a bare prefix would let two collide -- the survivor decided by iteration order. A warning says why they are there rather than on an operation. Both loops that mount operations off a path item are swept, paths and webhooks. What is kept is what applyPathItem keeps anywhere; a path item's summary and description are dropped here as they are dropped on a mounted item, and the message does not claim otherwise. --- .../openapi/internal/operation/operations.go | 124 +++++++++++++++--- .../operation/operations_internal_test.go | 3 +- .../internal/operation/operations_test.go | 62 +++++++++ compilers/openapi/unknownkeys_test.go | 64 +++++++++ 4 files changed, 234 insertions(+), 19 deletions(-) diff --git a/compilers/openapi/internal/operation/operations.go b/compilers/openapi/internal/operation/operations.go index 54c03ad..2f638c4 100644 --- a/compilers/openapi/internal/operation/operations.go +++ b/compilers/openapi/internal/operation/operations.go @@ -67,8 +67,8 @@ func LowerService(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorIndex svc.Unmodeled = annotation.MergeUnmodeled(svc.Unmodeled, pathsExt) diags = append(diags, pathsDiags...) groups := newServiceGroups() - diags = append(diags, lowerPaths(c, ts, anchors, operationIDs, groups)...) - diags = append(diags, lowerWebhooks(c, ts, anchors, operationIDs, groups)...) + diags = append(diags, lowerPaths(c, ts, anchors, operationIDs, groups, &svc)...) + diags = append(diags, lowerWebhooks(c, ts, anchors, operationIDs, groups, &svc)...) svc.Groups = groups.finalize() return svc, lowerTagDefs(c), diags } @@ -100,7 +100,7 @@ func tagDocsFrom(t *soa.Tag) ir.Docs { } // lowerPaths lowers every path operation in source order into groups. -func lowerPaths(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorIndex, operationIDs map[string]string, groups *serviceGroups) []ir.Diagnostic { +func lowerPaths(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorIndex, operationIDs map[string]string, groups *serviceGroups, svc *ir.Service) []ir.Diagnostic { paths := c.Doc.GetPaths() if paths == nil { return nil @@ -111,7 +111,7 @@ func lowerPaths(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorIndex, if pi == nil { continue } - diags = append(diags, lowerPathItem(c, ts, anchors, operationIDs, groups, path, pi, declPtr)...) + diags = append(diags, lowerPathItem(c, ts, anchors, operationIDs, groups, svc, path, pi, declPtr)...) } return diags } @@ -122,9 +122,10 @@ func lowerPaths(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorIndex, // path item, or a referenced path item's component pointer (issue #107) — // shared parameters and bodies lower from there, while each operation keeps // its mount pointer (under path) as its identity. -func lowerPathItem(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorIndex, operationIDs map[string]string, groups *serviceGroups, path string, pi *soa.PathItem, declPtr string) []ir.Diagnostic { +func lowerPathItem(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorIndex, operationIDs map[string]string, groups *serviceGroups, svc *ir.Service, path string, pi *soa.PathItem, declPtr string) []ir.Diagnostic { var diags []ir.Diagnostic pathPtr := ids.Ptr("paths", path) + var mounted int for _, m := range httpMethods { src := m.get(pi) if src == nil { @@ -142,17 +143,21 @@ func lowerPathItem(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInde } op, extra, opDiags := lowerOperation(c, ts, anchors, operationIDs, src, opCtx) diags = append(diags, opDiags...) - diags = append(diags, applyPathItem(c, &op, pi, declPtr)...) + diags = append(diags, applyPathItem(c, onOperation(&op), pi, declPtr)...) grp := groups.group(key, func() ir.OperationGroup { return ir.OperationGroup{Name: name, Docs: docs} }) grp.Operations = append(grp.Operations, op) grp.Operations = append(grp.Operations, extra...) + mounted++ + } + if mounted == 0 { + return append(diags, preserveUnmountedPathItem(c, svc, pi, pathPtr, declPtr)...) } return diags } // lowerWebhooks lowers webhook path items into the dedicated "webhooks" group; // each webhook operation carries IsWebhook on its HTTP binding. -func lowerWebhooks(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorIndex, operationIDs map[string]string, groups *serviceGroups) []ir.Diagnostic { +func lowerWebhooks(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorIndex, operationIDs map[string]string, groups *serviceGroups, svc *ir.Service) []ir.Diagnostic { hooks := c.Doc.GetWebhooks() if hooks == nil || hooks.Len() == 0 { return nil @@ -164,6 +169,7 @@ func lowerWebhooks(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInde if pi == nil { continue } + var mounted int for _, m := range httpMethods { src := m.get(pi) if src == nil { @@ -180,7 +186,7 @@ func lowerWebhooks(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInde } op, extra, opDiags := lowerOperation(c, ts, anchors, operationIDs, src, opCtx) diags = append(diags, opDiags...) - diags = append(diags, applyPathItem(c, &op, pi, declPtr)...) + diags = append(diags, applyPathItem(c, onOperation(&op), pi, declPtr)...) grp := groups.group("webhook", func() ir.OperationGroup { // A hint, not a source name: no document declares this group. The // compiler synthesizes it to hold webhook operations, exactly as it @@ -190,6 +196,10 @@ func lowerWebhooks(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInde }) grp.Operations = append(grp.Operations, op) grp.Operations = append(grp.Operations, extra...) + mounted++ + } + if mounted == 0 { + diags = append(diags, preserveUnmountedPathItem(c, svc, pi, hookPtr, declPtr)...) } } return diags @@ -427,13 +437,91 @@ func fillOperationDocs(d *ir.Docs, src *soa.Operation) { // empty however much the document wrote (speakeasy-api/openapi v1.24.0). What is // left of that map once the HTTP methods are taken out is the same set the // census would have reported, which is what undeclaredPathItemKeys reads. -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") - op.Unmodeled = annotation.MergeUnmodeled(op.Unmodeled, ext) +func applyPathItem(c lowering.Ctx, into carrier, pi *soa.PathItem, declPtr string) []ir.Diagnostic { + diags := applyPathServers(c, into, pi, declPtr) + ext, extDiags := schema.ExtensionsIn(c, pi.GetExtensions(), declPtr, into.scope) + *into.unmodeled = annotation.MergeUnmodeled(*into.unmodeled, ext) diags = append(diags, extDiags...) - return append(diags, annotation.UnknownKeysNamed(&op.Unmodeled, undeclaredPathItemKeys(pi), - pi.GetRootNode(), c.SrcIndex, declPtr, "pathItem")...) + return append(diags, annotation.UnknownKeysNamed(into.unmodeled, undeclaredPathItemKeys(pi), + pi.GetRootNode(), c.SrcIndex, declPtr, into.scope)...) +} + +// carrier is where a path item's own declarations are kept, and under what key +// scope. A path item lowers to no node of its own — its entries become +// operations — so what it writes beside them has to be held by something else. +// +// Normally that is each operation the item produced. An item that produces none +// has no such node, and used to lose everything it wrote: its servers, its +// extensions and its undeclared keys reached the IR in no form and nothing +// reported it. The service is the nearest node holding an Unmodeled map, which +// is where the Paths Object's own extensions already go for the same reason, so +// an item with no operation is kept there instead. +// +// scope carries the item's own pointer in that case, because one service holds +// every such item and a bare "pathItem" prefix would make two of them collide on +// one key — the survivor decided by iteration order. +type carrier struct { + unmodeled *ir.Unmodeled + provenance ir.Provenance + // scope prefixes the keys built from a keyword name, as the extension and + // undeclared-key writers do. + scope string + // serversKey is spelled in full rather than derived from scope, because the + // servers entry has always been keyed by the bare keyword on an operation and + // this must not move it. + serversKey string +} + +// onOperation carries a path item's declarations on an operation it produced. +func onOperation(op *ir.Operation) carrier { + return carrier{ + unmodeled: &op.Unmodeled, + provenance: op.Provenance, + scope: "pathItem", + serversKey: "openapi:servers", + } +} + +// onService carries them on the service, for an item that produced no operation +// to hold them. mountPtr is the item's own pointer, which keys it apart from +// every other item kept there. +func onService(svc *ir.Service, mountPtr string) carrier { + return carrier{ + unmodeled: &svc.Unmodeled, + provenance: svc.Provenance, + scope: "pathItem" + mountPtr, + serversKey: "openapi:pathItem" + mountPtr + "/servers", + } +} + +// preserveUnmountedPathItem keeps what an item that produced no operation wrote, +// and says so: the item is not lowered, so a reader seeing its declarations on +// the service needs to know why they are there rather than on an operation. +// +// An item produces no operation when it declares none, when every method it +// declares is one this compiler does not lower yet (GitHub #293), or when the +// only keys it holds are ones the Path Item Object does not define. +// +// What is kept is what applyPathItem keeps anywhere: servers, extensions and +// undeclared keys. A path item's summary and description are dropped here as +// they are dropped on a mounted item — that is a separate gap, and the message +// below does not claim otherwise. +func preserveUnmountedPathItem(c lowering.Ctx, svc *ir.Service, pi *soa.PathItem, mountPtr, declPtr string) []ir.Diagnostic { + diags := applyPathItem(c, onService(svc, mountPtr), pi, declPtr) + if len(diags) == 0 && !pathItemDeclaresAnything(pi) { + return nil // nothing was written beside the operations it does not have + } + return append(diags, c.DiagAt(ir.SeverityWarning, diag.DegradedConstruct, declPtr, + "path item declares no operation this compiler lowers; its servers, extensions and "+ + "undeclared keys are kept on the service, having no operation to hold them")) +} + +// pathItemDeclaresAnything reports whether pi wrote anything applyPathItem would +// have had to find a home for. +func pathItemDeclaresAnything(pi *soa.PathItem) bool { + return len(pi.GetServers()) > 0 || + pi.GetExtensions().Len() > 0 || + len(undeclaredPathItemKeys(pi)) > 0 } // undeclaredPathItemKeys returns the keys of a path item's operations map that @@ -482,16 +570,16 @@ func undeclaredPathItemKeys(pi *soa.PathItem) []string { // // This is the path-item half of the pair; applyOperationServers keeps the // operation's own list, which overrides this one, under its own key. -func applyPathServers(c lowering.Ctx, op *ir.Operation, pi *soa.PathItem, declPtr string) []ir.Diagnostic { +func applyPathServers(c lowering.Ctx, into carrier, pi *soa.PathItem, declPtr string) []ir.Diagnostic { if len(pi.GetServers()) == 0 { return nil } - kept, diags := schema.PreserveNode(c, &op.Unmodeled, "openapi:servers", + kept, diags := schema.PreserveNode(c, into.unmodeled, into.serversKey, annotation.RawChildNode(pi.GetRootNode(), "servers"), ir.ReasonNoIRHome, declPtr+ids.Ptr("servers")) if !kept { return diags } - return append(diags, diag.Newf(ir.SeverityInfo, diag.DegradedConstruct, op.Provenance, + return append(diags, diag.Newf(ir.SeverityInfo, diag.DegradedConstruct, into.provenance, "path-item servers kept under Unmodeled; an operation has no server-scope list to bind them to")) } @@ -762,7 +850,7 @@ func lowerCallbackOps(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorI } op, _, opDiags := lowerOperation(c, ts, anchors, operationIDs, src, opCtx) diags = append(diags, opDiags...) - diags = append(diags, applyPathItem(c, &op, pi, cb.decl)...) + diags = append(diags, applyPathItem(c, onOperation(&op), pi, cb.decl)...) opIDs = append(opIDs, op.ID) ops = append(ops, op) } diff --git a/compilers/openapi/internal/operation/operations_internal_test.go b/compilers/openapi/internal/operation/operations_internal_test.go index 2004025..3767d9d 100644 --- a/compilers/openapi/internal/operation/operations_internal_test.go +++ b/compilers/openapi/internal/operation/operations_internal_test.go @@ -234,7 +234,8 @@ func TestApplyPathServers_WithoutRootNode(t *testing.T) { t.Parallel() l := newRawLowerer(&soa.OpenAPI{}) op := &ir.Operation{} - diags := applyPathServers(l.ctx, op, &soa.PathItem{Servers: []*soa.Server{{URL: "https://x"}}}, "/paths/~1a") + diags := applyPathServers(l.ctx, onOperation(op), + &soa.PathItem{Servers: []*soa.Server{{URL: "https://x"}}}, "/paths/~1a") assert.Nil(t, op.Unmodeled, "servers with no raw node are not preserved") assert.Empty(t, diags) } diff --git a/compilers/openapi/internal/operation/operations_test.go b/compilers/openapi/internal/operation/operations_test.go index 531b281..8caf57b 100644 --- a/compilers/openapi/internal/operation/operations_test.go +++ b/compilers/openapi/internal/operation/operations_test.go @@ -1733,3 +1733,65 @@ func TestOperations_OwnServersSurviveBesideExtensions(t *testing.T) { assert.Contains(t, op.Unmodeled, "openapi:x-vendor", "and the extensions survive beside them, so neither overwrote the other") } + +// TestPathItem_WithNoOperationKeepsWhatItWroteOnTheService covers the one path +// item shape that reached applyPathItem through nothing. +// +// applyPathItem runs once per operation an item produces, so an item producing +// none lost its servers, its extensions and its undeclared keys whole, with no +// diagnostic naming the loss. The service is where they go now — the same node +// the Paths Object's own extensions go to, and for the same reason: a path item +// lowers to no node of its own. +func TestPathItem_WithNoOperationKeepsWhatItWroteOnTheService(t *testing.T) { + t.Parallel() + svc, diags := serviceWithGrouping(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /mounted: + get: {operationId: getX, responses: {"200": {description: ok}}} + /unmounted: + servers: [{url: 'https://a.example'}] + x-kept: {a: 1} +webhooks: + hook: + x-hook-kept: {b: 2} +`, lowering.GroupByTags) + + require.Contains(t, svc.Unmodeled, "openapi:pathItem/paths/~1unmounted/x-kept") + require.Contains(t, svc.Unmodeled, "openapi:pathItem/paths/~1unmounted/servers") + require.Contains(t, svc.Unmodeled, "openapi:pathItem/webhooks/hook/x-hook-kept") + assert.Equal(t, ir.ReasonNoIRHome, svc.Unmodeled["openapi:pathItem/paths/~1unmounted/servers"].Reason) + + // The key carries the item's own pointer because one service holds every such + // item: two of them under a bare prefix would collide, and the survivor would + // be whichever the walk reached last. + assert.NotContains(t, svc.Unmodeled, "openapi:servers", + "the unqualified servers key is the one an operation uses") + + var announced int + for _, d := range diags { + if strings.Contains(d.Message, "declares no operation this compiler lowers") { + announced++ + } + } + assert.Equal(t, 2, announced, "one per unmounted item: the path and the webhook") +} + +// TestPathItem_WithNoOperationAndNothingToKeepIsSilent holds the other half: an +// item that produces no operation and wrote nothing beside it has lost nothing, +// so there is nothing to keep and nothing to announce. +func TestPathItem_WithNoOperationAndNothingToKeepIsSilent(t *testing.T) { + t.Parallel() + svc, diags := serviceWithGrouping(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /empty: {} + /described: {summary: s, description: d} +`, lowering.GroupByTags) + + assert.Empty(t, svc.Unmodeled) + for _, d := range diags { + assert.NotContains(t, d.Message, "declares no operation this compiler lowers", + "an item with nothing beside its operations announces nothing") + } +} diff --git a/compilers/openapi/unknownkeys_test.go b/compilers/openapi/unknownkeys_test.go index 8350ff3..028d942 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" @@ -320,3 +321,66 @@ func requireNoErrorDiagnostics(t *testing.T, diags []ir.Diagnostic) { d, ok := ir.FirstError(diags) require.False(t, ok, "unexpected error diagnostic: %+v", d) } + +// TestUnknownKeys_PathItemWithNoOperationKeepsWhatItWrote covers the one place +// the census had no carrier at all. +// +// applyPathItem runs once per operation an item produces, so an item producing +// none — because it declares no method this compiler lowers, or because the only +// keys it holds are ones the Path Item Object does not define — reached the +// census through nothing, and its servers, extensions and undeclared keys were +// dropped whole with no diagnostic naming the loss. +// +// The service is where they go, which is where the Paths Object's own extensions +// already go for the same reason: a path item lowers to no node, so the nearest +// node holding an Unmodeled map is what holds them. The key carries the item's +// own pointer, because one service holds every such item and a bare prefix would +// let two of them collide. +func TestUnknownKeys_PathItemWithNoOperationKeepsWhatItWrote(t *testing.T) { + t.Parallel() + doc, diags := compileAnnotationSpec(t, "path-item-unmounted", `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /mounted: + x-on-mounted: {a: 1} + get: {operationId: getX, responses: {"200": {description: ok}}} + /unmounted: + servers: [{url: 'https://a.example'}] + x-on-unmounted: {b: 2} + undeclaredKey: {c: 3} +webhooks: + hook: + x-on-hook: {d: 4} +`) + // Not requireNoErrorDiagnostics: an object-valued undeclared key folds into + // the operations map as an operation with no responses, so the library reports + // a required-field error naming it. That is the fold this census exists to see + // past, not a problem with the fixture. + + keys := map[string]bool{} + for _, site := range unmodeledSites(doc) { + keys[site.key] = true + } + assert.True(t, keys["openapi:pathItem/x-on-mounted"], + "an item with an operation still keeps its own on that operation") + for _, want := range []string{ + "openapi:pathItem/paths/~1unmounted/x-on-unmounted", + "openapi:pathItem/paths/~1unmounted/undeclaredKey", + "openapi:pathItem/paths/~1unmounted/servers", + "openapi:pathItem/webhooks/hook/x-on-hook", + } { + assert.True(t, keys[want], "kept on the service: %s (have %v)", want, keys) + } + + // And the loss is announced rather than merely repaired: a reader finding + // these on the service needs to know why they are not on an operation. + var announced int + for _, d := range diags { + if d.Code == "openapi/degraded-construct" && + strings.Contains(d.Message, "declares no operation this compiler lowers") { + announced++ + assert.Equal(t, ir.SeverityWarning, d.Severity) + } + } + assert.Equal(t, 2, announced, "one per unmounted item: the path and the webhook") +} From 06cd9d4b249fa3f9080e8f2f6363526536f631eb Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 08:17:47 +0300 Subject: [PATCH 3/3] docs(compilers/openapi): cite the path-item docs gap at the code --- compilers/openapi/internal/operation/operations.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/compilers/openapi/internal/operation/operations.go b/compilers/openapi/internal/operation/operations.go index 2f638c4..67932f5 100644 --- a/compilers/openapi/internal/operation/operations.go +++ b/compilers/openapi/internal/operation/operations.go @@ -504,8 +504,9 @@ func onService(svc *ir.Service, mountPtr string) carrier { // // What is kept is what applyPathItem keeps anywhere: servers, extensions and // undeclared keys. A path item's summary and description are dropped here as -// they are dropped on a mounted item — that is a separate gap, and the message -// below does not claim otherwise. +// they are dropped on a mounted item — that is a separate gap (GitHub #383, +// where the question is how path-item docs relate to an operation's own rather +// than where to put a payload), and the message below names only what it keeps. func preserveUnmountedPathItem(c lowering.Ctx, svc *ir.Service, pi *soa.PathItem, mountPtr, declPtr string) []ir.Diagnostic { diags := applyPathItem(c, onService(svc, mountPtr), pi, declPtr) if len(diags) == 0 && !pathItemDeclaresAnything(pi) {