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..67932f5 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 @@ -410,39 +420,142 @@ 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 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, 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(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. // -// - 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. +// 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. // -// 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") - op.Unmodeled = annotation.MergeUnmodeled(op.Unmodeled, ext) - return append(diags, extDiags...) +// 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 (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) { + 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 +// 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 @@ -458,16 +571,16 @@ func applyPathItem(c lowering.Ctx, op *ir.Operation, pi *soa.PathItem, declPtr s // // 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")) } @@ -738,7 +851,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 b621fdb..8caf57b 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 @@ -1671,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 34f5269..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" @@ -59,6 +60,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 +224,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) { @@ -225,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") +} 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]