fix(compilers/openapi): resolve a percent-encoded $ref fragment

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

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" }}

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")
+}