diff --git a/compilers/openapi/internal/resolve/resolve.go b/compilers/openapi/internal/resolve/resolve.go index 22c21ec..a936f5a 100644 --- a/compilers/openapi/internal/resolve/resolve.go +++ b/compilers/openapi/internal/resolve/resolve.go @@ -20,6 +20,7 @@ import ( "strings" oas3 "github.com/speakeasy-api/openapi/jsonschema/oas3" + "github.com/speakeasy-api/openapi/references" "github.com/dexpace/morphic/compilers/compile" "github.com/dexpace/morphic/compilers/openapi/internal/annotation" @@ -72,6 +73,19 @@ func (s Scope) sameFile(doc string) bool { // file (an OpenAPI self-reference) is treated as internal — Milestone 1 interns // only same-file targets; genuinely external ones are diagnosed and dropped. // +// The split into document and pointer is the resolver's own (references.Reference, +// v1.24.0) rather than a hand-rolled one: a $ref is a URI, so its fragment is +// percent-encoded, and `#/components/schemas/Foo%2DBar` names the component +// "Foo-Bar". Comparing the raw fragment against declared names instead reported a +// reference the resolver had resolved as unresolved, degrading the position to +// `any` and dropping any discriminator mapping that spelled its target that way. +// The pointer returned here is also an ID source, so the quieter half cost more: +// an encoded pointer interned a second node for a position an unencoded pointer +// already named, leaving one coordinate with two types and no diagnostic either +// side of it (GitHub #40). Asking the resolver is what stops the answer drifting +// from it again; nodeview.InternalPointer mirrors the same two methods for the +// cycle scan, and records what a dependency bump should re-check. +// // A fragment that is not a JSON pointer is refused here rather than passed on. // `#addr` names a JSON Schema `$anchor`, not a coordinate, and Milestone 1 // resolves no anchors; letting it through returned "addr" as though it were a @@ -80,11 +94,15 @@ func (s Scope) sameFile(doc string) bool { // that puts the refusal outside this compiler, where a library that started // resolving anchors would silently reinstate the malformed derivation. func (s Scope) InternalPointer(ref string) (string, bool) { - doc, pointer, found := strings.Cut(ref, "#") - if !found || !strings.HasPrefix(pointer, "/") { + r := references.Reference(ref) + if !r.HasJSONPointer() { + return "", false + } + pointer := string(r.GetJSONPointer()) + if !strings.HasPrefix(pointer, "/") { return "", false } - if doc != "" && !s.sameFile(doc) { + if doc := r.GetURI(); doc != "" && !s.sameFile(doc) { return "", false } return pointer, true diff --git a/compilers/openapi/internal/resolve/resolve_internal_test.go b/compilers/openapi/internal/resolve/resolve_internal_test.go index 9ac7ced..1662072 100644 --- a/compilers/openapi/internal/resolve/resolve_internal_test.go +++ b/compilers/openapi/internal/resolve/resolve_internal_test.go @@ -40,6 +40,49 @@ func TestInternalPointer(t *testing.T) { assert.Equal(t, tc.want, got, tc.ref) } } + +// TestInternalPointer_MatchesTheResolversNormalization pins that the fragment is +// read the way the resolver reads it, which is what makes a reference this +// compiler calls unresolved one the resolver also failed to resolve. A $ref is a +// URI, so `%2D` in the fragment is a hyphen; comparing the raw text against +// declared names failed every spec-correct escape (GitHub #40). It carries the +// same name as nodeview's test of the same two accessors, so the pair is one +// grep apart — a dependency bump has to satisfy both. +func TestInternalPointer_MatchesTheResolversNormalization(t *testing.T) { + t.Parallel() + sc := Scope{SelfPath: "m.yaml", Declares: func(string) bool { return false }} + tests := []struct { + name, ref, want string + internal bool + }{ + {name: "hyphen", ref: "#/components/schemas/Foo%2DBar", want: "/components/schemas/Foo-Bar", internal: true}, + {name: "underscore", ref: "#/components/schemas/Foo%5FBar", want: "/components/schemas/Foo_Bar", internal: true}, + {name: "dot", ref: "#/components/schemas/Foo%2EBar", want: "/components/schemas/Foo.Bar", internal: true}, + {name: "space", ref: "#/components/schemas/A%20B", want: "/components/schemas/A B", internal: true}, + {name: "percent", ref: "#/components/schemas/A%25B", want: "/components/schemas/A%B", internal: true}, + // %2F decodes to a separator, so it deepens the pointer rather than naming + // a component with a slash in it — RFC 6901 spells that one `~1`. + {name: "encoded separator deepens", ref: "#/components/schemas/A%2FB", want: "/components/schemas/A/B", internal: true}, + {name: "undecodable escape kept raw", ref: "#/components/schemas/A%zzB", want: "/components/schemas/A%zzB", internal: true}, + {name: "trailing space", ref: "#/components/schemas/A ", want: "/components/schemas/A", internal: true}, + {name: "leading space", ref: " #/components/schemas/A", want: "/components/schemas/A", internal: true}, + {name: "second hash ends the pointer", ref: "#/a#b", want: "/a", internal: true}, + {name: "self-document part still internal", ref: "m.yaml#/components/schemas/A%2DB", want: "/components/schemas/A-B", internal: true}, + // The document half is not decoded, because the resolver does not decode it + // either: GetURI trims and stops. A self-reference has to be spelled the way + // the file is named. + {name: "document half is not decoded", ref: "m%2Eyaml#/components/schemas/A", internal: false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, internal := sc.InternalPointer(tc.ref) + assert.Equal(t, tc.internal, internal) + assert.Equal(t, tc.want, got) + }) + } +} + func TestResolveComponentRef(t *testing.T) { t.Parallel() sc := Scope{Declares: func(n string) bool { return n == "User" }} diff --git a/compilers/openapi/resolve_internal_test.go b/compilers/openapi/resolve_internal_test.go index 7483cfb..95478a7 100644 --- a/compilers/openapi/resolve_internal_test.go +++ b/compilers/openapi/resolve_internal_test.go @@ -138,3 +138,128 @@ components: require.NotNil(t, f.Examples[0].Value) assert.Equal(t, ir.Value{Kind: ir.ValueString, Str: "at-reference"}, *f.Examples[0].Value) } + +// TestLowerComponentSchemas_PercentEncodedRefResolves pins that a $ref whose +// fragment is percent-encoded reaches the component it names. A $ref is a URI, so +// `#/components/schemas/Foo%2DBar` addresses "Foo-Bar", and the resolver reads it +// that way; comparing the raw fragment against declared names instead called a +// resolved reference unresolved and left the property as `any`, losing the type +// from a spec-correct document (GitHub #40). Each name here is legal under +// OpenAPI's own component-name rule (^[a-zA-Z0-9.\-_]+$), so the escape is the +// only thing under test. +func TestLowerComponentSchemas_PercentEncodedRefResolves(t *testing.T) { + t.Parallel() + cases := []struct{ name, decl, encoded string }{ + {"hyphen", "Foo-Bar", "Foo%2DBar"}, + {"underscore", "Foo_Bar", "Foo%5FBar"}, + {"dot", "Foo.Bar", "Foo%2EBar"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + doc, diags := lowerSpec(t, componentSpec( + " "+tc.decl+": {type: string}\n"+ + " User: {type: object, properties: {x: {$ref: '#/components/schemas/"+tc.encoded+"'}}}\n")) + requireNoErrorDiags(t, diags) + + user, ok := typeByName(doc, "User").(*ir.Model) + require.True(t, ok, "User must own a Model node") + require.Len(t, user.Properties, 1) + assert.Equal(t, componentID(tc.decl), user.Properties[0].Type.Target, + "the encoded fragment names the declared component") + }) + } +} + +// TestLowerComponentSchemas_PercentEncodedRefHoistsAtTheDeclaredCoordinate pins +// the identity half of the same fix, which the resolution half hides: a pointer +// *through* an encoded component name addresses a sub-schema an unencoded pointer +// also addresses, so the two must intern one node. Reading the fragment raw +// hoisted a second one at `.../Foo%2DBar/properties/inner` — a path no source +// coordinate spells, the derivation GitHub #141 refused for anchors — so one +// position became two types, silently: both references resolved, no diagnostic +// was emitted, and the duplicate is a node irverify has no reason to call +// dangling. +// +// Both spellings appear here because that is what makes the duplicate observable +// at all; the encoded ref alone lands on one node either way, and only its name +// is wrong. +func TestLowerComponentSchemas_PercentEncodedRefHoistsAtTheDeclaredCoordinate(t *testing.T) { + t.Parallel() + doc, diags := lowerSpec(t, componentSpec( + " Foo-Bar: {type: object, properties: {inner: {type: object, properties: {n: {type: integer}}}}}\n"+ + " User:\n type: object\n properties:\n"+ + " a: {$ref: '#/components/schemas/Foo-Bar/properties/inner'}\n"+ + " b: {$ref: '#/components/schemas/Foo%2DBar/properties/inner'}\n")) + requireNoErrorDiags(t, diags) + + user, ok := typeByName(doc, "User").(*ir.Model) + require.True(t, ok, "User must own a Model node") + props := propsByWire(user.Properties) + require.Len(t, props, 2) + + const want = ir.TypeID("t/anon/components/schemas/Foo-Bar/properties/inner") + assert.Equal(t, want, props["a"].Type.Target) + assert.Equal(t, want, props["b"].Type.Target, + "the encoded spelling addresses the position the declaration spells, not one of its own") + assert.Contains(t, doc.Types, want, "and that ID is backed by a node") + for id := range doc.Types { + assert.NotContains(t, string(id), "%", + "no node is interned at an encoded path: nothing in this document is named with one") + } +} + +// TestLowerService_PercentEncodedEntryRefKeepsTheDeclaredCoordinate covers the +// same identity defect on the components that are not schemas, where it was +// wholly silent. Their entries resolve through the library, so the value arrived +// intact and no unresolved-ref was ever emitted; only the pointer the entry +// carried stayed encoded, and every ID hoisted beneath it inherited the +// encoding — here the response's own content schema. +func TestLowerService_PercentEncodedEntryRefKeepsTheDeclaredCoordinate(t *testing.T) { + t.Parallel() + doc, _, diags := lowerServiceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /a: + get: + responses: + '200': {$ref: '#/components/responses/My%2DResp'} +components: + responses: + My-Resp: + description: ok + content: + application/json: + schema: {type: object, properties: {q: {type: string}}} +`) + requireNoErrorDiags(t, diags) + + const want = ir.TypeID("t/anon/components/responses/My-Resp/content/application~1json/schema") + assert.Contains(t, doc.Types, want, + "the response body is hoisted at the coordinate the component declares") + for id := range doc.Types { + assert.NotContains(t, string(id), "%", + "no node is interned at an encoded path") + } +} + +// TestLowerComponentSchemas_PercentEncodedDiscriminatorMapping covers the third +// consumer of the pointer, and the one whose failure is loudest: a mapping entry +// whose target does not resolve is dropped, so an encoded target silently cost +// the union a branch of its polymorphic dispatch rather than merely degrading a +// position's type. InternalPointer's contract has always named discriminator +// mappings alongside $ref; nothing exercised that half. +func TestLowerComponentSchemas_PercentEncodedDiscriminatorMapping(t *testing.T) { + t.Parallel() + doc, diags := lowerSpec(t, componentSpec( + " Cat-A: {type: object, properties: {kind: {type: string}}}\n"+ + " Pet:\n oneOf: [{$ref: '#/components/schemas/Cat-A'}]\n"+ + " discriminator: {propertyName: kind, mapping: {cat: '#/components/schemas/Cat%2DA'}}\n")) + requireNoErrorDiags(t, diags) + + pet, ok := typeByName(doc, "Pet").(*ir.Union) + require.True(t, ok, "Pet must own a Union node") + require.NotNil(t, pet.Discriminator, "the discriminator survives lowering") + assert.Equal(t, map[string]ir.TypeID{"cat": componentID("Cat-A")}, pet.Discriminator.Mapping, + "the encoded mapping target names the declared component, and the entry is kept") +}