diff --git a/compilers/openapi/internal/schema/resolve.go b/compilers/openapi/internal/schema/resolve.go index a340714..3c3322a 100644 --- a/compilers/openapi/internal/schema/resolve.go +++ b/compilers/openapi/internal/schema/resolve.go @@ -1,11 +1,14 @@ package schema import ( + "strings" + oas3 "github.com/speakeasy-api/openapi/jsonschema/oas3" "github.com/dexpace/morphic/compilers/compile" "github.com/dexpace/morphic/compilers/openapi/internal/annotation" "github.com/dexpace/morphic/compilers/openapi/internal/diag" + "github.com/dexpace/morphic/compilers/openapi/internal/ids" "github.com/dexpace/morphic/compilers/openapi/internal/lowering" "github.com/dexpace/morphic/compilers/openapi/internal/resolve" "github.com/dexpace/morphic/ir" @@ -161,19 +164,26 @@ func hoistSubSchema(c lowering.Ctx, ts *compile.Types, anchors *AnchorIndex, dep // subSchemaHint names the node a $ref'd sub-schema pointer owns: the target it // aliases when the sub-schema is itself a $ref carrying siblings, the branch -// hint when the pointer addresses a composition branch, the pointer's last -// segment otherwise. +// hint when the pointer addresses a composition branch, the structural hint when +// it addresses an inline structural position, the pointer's last segment +// otherwise. // -// The first two cases exist because a composition branch can own the same -// pointer and derives its hint that way (branchHint). Both lowerings reach the -// pointer — the branch through its composition, this one through an outside $ref -// naming it — and only the first to arrive interns the node, so a hint derived +// Every case but the last exists because another lowering can own the same +// pointer and derives its hint that way. Both lowerings reach the pointer — the +// enclosing schema through its own body, this one through an outside $ref naming +// it — and only the first to arrive interns the node, so a hint derived // differently here makes the document depend on declaration order. // -// The branch case is the second half of that agreement. Falling through to the -// last segment named an inline branch after its own ordinal — "0" — which is a -// hint an emitter cannot build an identifier from, and which disagreed with the -// composition's "variant_0" (GitHub #181). +// The branch case is the second half of that agreement for a composition branch. +// Falling through to the last segment named an inline branch after its own +// ordinal — "0" — which is a hint an emitter cannot build an identifier from, +// and which disagreed with the composition's "variant_0" (GitHub #181). +// +// The structural case is the same agreement for items, additionalProperties, a +// patternProperties entry and a prefixItems slot (GitHub #353), where the last +// segment named the node after the keyword that holds it — "items" — or after +// the pattern text or the slot ordinal, none of which distinguish it from the +// same position on any other schema. func subSchemaHint(decl *oas3.JSONSchema[oas3.Referenceable], pointer string) string { if decl != nil && decl.IsReference() { if name := refLastSegment(decl.GetRef().String()); name != "" { @@ -183,9 +193,99 @@ func subSchemaHint(decl *oas3.JSONSchema[oas3.Referenceable], pointer string) st if hint, ok := branchPointerHint(pointer); ok { return hint } + if hint, ok := structuralPointerHint(pointer); ok { + return hint + } return refLastSegment(pointer) } +// componentSchemasPrefix is the pointer root under which a structural position's +// enclosing hint is the enclosing pointer's own last segment. See +// structuralPointerHint for why the derivation is confined to it. +const componentSchemasPrefix = "/components/schemas/" + +// structuralPointerHint returns the hint the inline structural position at +// pointer takes, for a caller holding only the pointer, and whether the pointer +// addresses one it can answer. +// +// It is branchPointerHint's counterpart for the four positions whose hint is +// composed rather than positional: the structural lowering builds them as +// compile.SubHint(enclosing, role), so answering requires the enclosing node's +// hint, which a bare pointer walk does not carry. Under /components/schemas it +// does: the enclosing hint there is the enclosing pointer's own last segment — +// the component's name, or a property's key — so the composition can be replayed +// by peeling roles off the tail and rebuilding from what is left. +// +// It is confined to that root because the derivation is not total, and the +// remainder is a naming decision rather than a bug to paper over. A position +// under /paths takes its enclosing hint from an operationId, a response, or a +// media-type key, and the pointer records none of them: the same items position +// is "response_item" to the structural lowering and has no pointer spelling that +// reproduces it. Answering those with a pointer-derived name would replace one +// disagreement with a different one, so they keep the last-segment fallback. +// That leaves no order dependence there — components lower before paths, so a +// reference from one always interns first — but it does leave a name that +// depends on whether an unrelated schema points at the position. GitHub #372 +// holds that remainder. +// +// The walk is bounded by construction: each step consumes at least one segment +// and the loop runs only while segments remain. +func structuralPointerHint(pointer string) (string, bool) { + segments := strings.Split(pointer, "/") + + var roles []string // innermost first + for len(segments) > 1 { + role, consumed, ok := structuralRole(segments) + if !ok { + break + } + roles = append(roles, role) + segments = segments[:len(segments)-consumed] + } + if len(roles) == 0 { + return "", false + } + + enclosing := strings.Join(segments, "/") + if !strings.HasPrefix(enclosing, componentSchemasPrefix) { + return "", false + } + + hint := ids.UnescapeSegment(segments[len(segments)-1]) + for i := len(roles) - 1; i >= 0; i-- { + hint = compile.SubHint(hint, roles[i]) + } + return hint, true +} + +// structuralRole reports the role the structural lowering names the position at +// the tail of segments by, and how many segments that position spells. The roles +// are the suffixes the four compile.SubHint call sites pass, and a change to one +// of them has to be made here too — TestInlinePosition_HintIsTheSameInBothOrders +// is what fails when they drift. +// +// segments holds at least two entries: its only caller reads the tail of a +// pointer, which always starts with the empty segment before the first token, and +// stops looping once one segment is left. +func structuralRole(segments []string) (role string, consumed int, ok bool) { + last := segments[len(segments)-1] + switch last { + case "items": + return "item", 1, true + case "additionalProperties": + return "value", 1, true + } + switch segments[len(segments)-2] { + case "patternProperties": + return "pattern", 2, true + case "prefixItems": + if isDecimalIndex(last) { + return last, 2, true + } + } + return "", 0, false +} + // refNullable reports whether a $ref usage admits null: the reference site or // its resolved target admits null in any spelling. The ref site must recompute // this because a target interned at its own ID (a model, a union) discards the diff --git a/compilers/openapi/internal/schema/schema_test.go b/compilers/openapi/internal/schema/schema_test.go index f4b1c91..8d57c10 100644 --- a/compilers/openapi/internal/schema/schema_test.go +++ b/compilers/openapi/internal/schema/schema_test.go @@ -2094,6 +2094,103 @@ func stolenPositions() []stolenPosition { } } +// TestInlinePosition_HintIsTheSameInBothOrders pins the spelling each inline +// position takes, rather than only that the two orders agree (GitHub #353). +// +// Agreement alone is satisfied by both namers producing the weaker name, so the +// table is what says which one won: the structural spelling, composed from the +// enclosing node's hint and the position's role. The outside $ref used to name +// these after the keyword holding them ("items"), the pattern text ("^x") or the +// slot ordinal ("0") — none of which distinguish the position from the same +// position on any other schema. +func TestInlinePosition_HintIsTheSameInBothOrders(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + owner string + id ir.TypeID + hint string + }{ + {"items", " A: {type: array, items: " + inlineProbeBody + "}\n", + "t/anon/components/schemas/A/items", "A_item"}, + {"additionalProperties", " A: {type: object, additionalProperties: " + inlineProbeBody + "}\n", + "t/anon/components/schemas/A/additionalProperties", "A_value"}, + {"patternProperties", " A: {type: object, patternProperties: {\"^x\": " + inlineProbeBody + "}}\n", + "t/anon/components/schemas/A/patternProperties/^x", "A_pattern"}, + {"prefixItems", " A: {type: array, prefixItems: [" + inlineProbeBody + "]}\n", + "t/anon/components/schemas/A/prefixItems/0", "A_0"}, + // Nested, because the derivation replays the whole chain rather than one + // step: the outside $ref used to name this "items", losing both levels. + {"items under items", " A: {type: array, items: {type: array, items: " + inlineProbeBody + "}}\n", + "t/anon/components/schemas/A/items/items", "A_item_item"}, + // Rooted at a property rather than at the component, so the enclosing hint + // the walk rebuilds from is the property's key. + {"items under a property", " A: {type: object, properties: {p: {type: array, items: " + + inlineProbeBody + "}}}\n", + "t/anon/components/schemas/A/properties/p/items", "p_item"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + pos := stolenPosition{name: tc.name, owner: tc.owner, id: tc.id} + for _, order := range []struct { + name string + refFirst bool + }{{"owner declared first", false}, {"reference declared first", true}} { + doc, diags := parseFull(t, pos.spec(order.refFirst)) + requireNoErrorDiags(t, diags) + td, ok := doc.Types[tc.id] + require.True(t, ok, "%s: the position owns a node at %s", order.name, tc.id) + assert.Equal(t, tc.hint, td.Common().Name.Hint, order.name) + } + }) + } +} + +// TestInlinePosition_UnderPathsTakesTheWeakerName pins what GitHub #353 did not +// close, so the remainder is a recorded state rather than a silent one. +// +// The enclosing hint under /paths comes from a response, an operationId or a +// media-type key, none of which the pointer records, so the pointer walk cannot +// replay it and the position keeps the last-segment fallback. What is left is +// not an order dependence — components lower before paths, so the reference +// interns first in either spelling of the document — but a name that depends on +// whether an unrelated schema points at the position: "response_item" without +// the reference, "items" with it. GitHub #372 holds it. +func TestInlinePosition_UnderPathsTakesTheWeakerName(t *testing.T) { + t.Parallel() + const id = ir.TypeID("t/anon/paths/~1x/get/responses/200/content/application~1json/schema/items") + const op = `paths: + /x: + get: + operationId: getX + responses: + "200": + description: ok + content: + application/json: {schema: {type: array, items: ` + inlineProbeBody + `}} +` + const outsider = "components:\n schemas:\n" + + " Outsider: {$ref: '#/paths/~1x/get/responses/200/content/application~1json/schema/items'}\n" + + ownerFirst, diags := parseFull(t, "openapi: 3.1.0\ninfo: {title: O, version: \"1.0.0\"}\n"+op+outsider) + requireNoErrorDiags(t, diags) + refFirst, diags := parseFull(t, "openapi: 3.1.0\ninfo: {title: O, version: \"1.0.0\"}\n"+outsider+op) + requireNoErrorDiags(t, diags) + + assert.Equal(t, "items", ownerFirst.Types[id].Common().Name.Hint, + "the reference names it, whichever order the two blocks are written in") + assert.Equal(t, "items", refFirst.Types[id].Common().Name.Hint, + "so the document is deterministic; it is the name that is weak") + + // Without the reference the structural lowering names it, which is what the + // two above are being compared against: an unrelated $ref elsewhere in the + // document is what costs the position its enclosing context. + unreferenced, diags := parseFull(t, "openapi: 3.1.0\ninfo: {title: O, version: \"1.0.0\"}\n"+op) + requireNoErrorDiags(t, diags) + assert.Equal(t, "response_item", unreferenced.Types[id].Common().Name.Hint, + "the structural lowering composes the enclosing response's hint") +} + // TestInlinePosition_OutsideRefDoesNotMoveTheHome is the regression for the // second half of the pointer collision. A $ref naming an inline position hoists // that position's home before the position itself is reached, and the position @@ -2131,20 +2228,20 @@ func TestInlinePosition_OutsideRefDoesNotMoveTheHome(t *testing.T) { } } -// orderInvariantIR compares two whole IR documents, minus the two fields that -// differ by construction when the same components are declared in two orders. +// orderInvariantIR compares two whole IR documents, minus what differs by +// construction when the same components are declared in two orders. // // SourceInfo.Hash digests the source bytes, which are the thing being permuted. -// Naming.Hint is a live gap: the hint a node hoisted at a pointer carries is -// minted by whichever of the two namers reaches the pointer first — the -// declaration's context ("A_item") or the reference's last pointer segment -// ("items") — because intern keeps the first name it is given. That divergence -// is naming only, and predates the annotation work: a $ref to an object-bodied -// `items` shows it with no annotations involved at all. Everything else is -// compared. +// +// Naming.Hint used to be excluded too, and no longer is. The hint a node hoisted +// at a pointer carries is minted by whichever namer reaches the pointer first — +// the enclosing declaration's context or an outside $ref's pointer walk — +// because intern keeps the first name it is given, so the two had to agree and +// did not. They now do at every position either can reach (GitHub #181, #281, +// #353), which is what lets the field be compared: the tests permuting those +// positions are the regression only while nothing here hides the difference. func orderInvariantIR() []cmp.Option { return []cmp.Option{ - cmpopts.IgnoreFields(ir.Naming{}, "Hint"), cmpopts.IgnoreFields(ir.SourceInfo{}, "Hash"), } }