From 0b4678b347c840a2767cb0530e8d27cf2765fc62 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 07:09:23 +0300 Subject: [PATCH] fix(compilers/openapi): agree on an inline position's hint A node hoisted at an inline structural position -- items, additionalProperties, a patternProperties entry, a prefixItems slot -- took its Naming.Hint from whichever lowering interned it first. The structural lowering composes the enclosing node's hint with the position's role; hoistSubSchema, reached through an outside $ref naming that same pointer, fell through to the pointer's last segment. Both are valid hints and nothing compared them, so the same components compiled in two declaration orders produced two documents, silently. structuralPointerHint is branchPointerHint's counterpart for these four: it peels roles off the pointer's tail and rebuilds the composition from what is left. Under /components/schemas that reproduces the structural answer exactly, because the enclosing hint there is the enclosing pointer's own last segment -- a component's name, or a property's key -- and the walk replays the whole chain, so items under items composes as the lowering does. The structural spelling wins rather than the pointer one, which is the naming decision the agreement needed: the last segment named the node after the keyword holding it, the pattern text, or the slot ordinal, none of which distinguish it from the same position on any other schema. That is the direction #181 already chose in preferring variant_0 to 0. It is confined to /components/schemas because the derivation is not total. 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. That leaves no order dependence -- 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. #372 holds that remainder and a test pins all three values. orderInvariantIR no longer excludes ir.Naming.Hint at all, which is what turns TestInlinePosition_OutsideRefDoesNotMoveTheHome into this fix's regression: it already permuted all four positions, and the exclusion was the only thing standing between it and this bug. --- compilers/openapi/internal/schema/resolve.go | 120 ++++++++++++++++-- .../openapi/internal/schema/schema_test.go | 117 +++++++++++++++-- 2 files changed, 217 insertions(+), 20 deletions(-) diff --git a/compilers/openapi/internal/schema/resolve.go b/compilers/openapi/internal/schema/resolve.go index a3407143..3c3322a6 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 f4b1c910..8d57c102 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"), } }