fix(flatten): import the target of a $ref held by an unmapped keyword

A keyword the Swagger 2.0 model does not map - propertyNames, contains,
if/then/else, $defs - lands in spec.Schema.ExtraProps as raw JSON, and
analyzeSchema never looked there. Flatten imported the subtree holding such
a $ref and left the pointer untouched, so "#/definitions/leaf" ended up
naming a definition of the root document: nothing at all when the root had
no definition of that name, and an unrelated schema when it had one. The
second case validates, wrongly, and reports no error.

analyzeSchema now walks ExtraProps and records what it finds in
referenceAnalysis.unmappedRefs, keyed by the node holding the $ref
("#/definitions/deep/if/anyOf/0"). The index is deliberately kept out of
allRefs and schemas: AllRefs, AllReferences and AllDefinitionReferences
address schemas through the model, and a key naming a raw JSON node is of no
use to them or to what go-swagger builds on them. Their contents are
unchanged.

Flatten consults the new index where it has to: normalizeRef strips the
absolute base from those $ref too, importExternalReferences indexes them
alongside the mapped ones so the target is imported and named through
uniqifyName, importNewRef rebases them onto the document they came from, and
removeUnusedSinglePass counts them as a use - without that last one, the
definition just imported would be pruned again.

replace.UpdateRef gains two cases for raw JSON nodes. No spec-side change is
needed: jsonpointer hands back a copy of the any that holds the map, and a
copied map shares its storage, so writing to it reaches the document.

A document where a mapped $ref happens to pull the same target in was
already flattened correctly, by coincidence. Its output is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Frederic BIDON <fredbi@yahoo.com>

Author: fredbi

analyzer.go

@@ -30,6 +30,15 @@ type referenceAnalysis struct {
 	parameterItems map[string]spec.Ref
 	allRefs        map[string]spec.Ref
 	pathItems      map[string]spec.Ref
+
+	// unmappedRefs holds the $ref found under keywords the Swagger 2.0 model does not map, which
+	// land in [spec.Schema.ExtraProps] as raw JSON: propertyNames, contains, if/then/else, $defs.
+	//
+	// They are kept apart from allRefs and schemas on purpose. Flatten needs them to import their
+	// target and rewrite the pointer; every other consumer of this analysis - AllRefs,
+	// AllReferences, AllDefinitionReferences and what go-swagger builds on them - addresses schemas
+	// through the model, and a key naming a raw JSON node is of no use there.
+	unmappedRefs map[string]spec.Ref
 }
 
 func (r *referenceAnalysis) addRef(key string, ref spec.Ref) {
@@ -48,6 +57,11 @@ func (r *referenceAnalysis) addItemsRef(key string, items *spec.Items, location
 	}
 }
 
+// addUnmappedRef records a $ref held by a keyword the model does not map.
+func (r *referenceAnalysis) addUnmappedRef(key string, ref spec.Ref) {
+	r.unmappedRefs["#"+key] = ref
+}
+
 func (r *referenceAnalysis) addSchemaRef(key string, ref SchemaRef) {
 	r.schemas["#"+key] = ref.Schema.Ref
 	r.addRef(key, ref.Schema.Ref)
@@ -739,6 +753,7 @@ func (s *Spec) reset() {
 	s.references.headerItems = make(map[string]spec.Ref, allocLargeMap)
 	s.references.parameterItems = make(map[string]spec.Ref, allocLargeMap)
 	s.references.allRefs = make(map[string]spec.Ref, allocLargeMap)
+	s.references.unmappedRefs = make(map[string]spec.Ref, allocSmallMap)
 	s.patterns.parameters = make(map[string]string, allocLargeMap)
 	s.patterns.headers = make(map[string]string, allocLargeMap)
 	s.patterns.items = make(map[string]string, allocLargeMap)
@@ -972,6 +987,42 @@ func (s *Spec) analyzeResponse(prefix string, k int, res spec.Response) {
 	}
 }
 
+// analyzeUnmapped records the $ref held by the keywords of a schema that the Swagger 2.0 model
+// does not map, which json.Unmarshal leaves in ExtraProps as raw JSON.
+//
+// The keys it produces address the node holding the $ref, so "#/definitions/deep/propertyNames"
+// or "#/definitions/deep/if/anyOf/0". [replace.UpdateRef] writes to them through the same
+// jsonpointer call every other key goes through.
+func (s *Spec) analyzeUnmapped(prefix string, extra map[string]any) {
+	for key := range extra {
+		s.analyzeUnmappedNode(slashpath.Join(prefix, jsonpointer.Escape(key)), extra[key])
+	}
+}
+
+func (s *Spec) analyzeUnmappedNode(refURI string, node any) {
+	switch value := node.(type) {
+	case map[string]any:
+		if raw, ok := value["$ref"].(string); ok {
+			ref, err := spec.NewRef(raw)
+			if err != nil {
+				return // a string under a "$ref" key is not necessarily a reference
+			}
+
+			s.references.addUnmappedRef(refURI, ref)
+
+			return // a $ref makes its siblings irrelevant
+		}
+
+		for key := range value {
+			s.analyzeUnmappedNode(slashpath.Join(refURI, jsonpointer.Escape(key)), value[key])
+		}
+	case []any:
+		for i := range value {
+			s.analyzeUnmappedNode(slashpath.Join(refURI, strconv.Itoa(i)), value[i])
+		}
+	}
+}
+
 func (s *Spec) analyzeSchema(name string, schema *spec.Schema, prefix string) {
 	refURI := slashpath.Join(prefix, jsonpointer.Escape(name))
 	schRef := SchemaRef{
@@ -995,6 +1046,8 @@ func (s *Spec) analyzeSchema(name string, schema *spec.Schema, prefix string) {
 		s.enums.addSchemaEnum(refURI, schema.Enum)
 	}
 
+	s.analyzeUnmapped(refURI, schema.ExtraProps)
+
 	for k, v := range schema.Definitions {
 		s.analyzeSchema(k, &v, slashpath.Join(refURI, "definitions"))
 	}

flatten.go

@@ -5,6 +5,7 @@ package analysis
 
 import (
 	"log"
+	"maps"
 	"path"
 	"slices"
 	"sort"
@@ -179,18 +180,20 @@ func normalizeRef(opts *FlattenOpts) error {
 	debugLog("normalizeRef")
 
 	altered := false
-	for k, w := range opts.Spec.references.allRefs {
-		if !strings.HasPrefix(w.String(), opts.BasePath+definitionsPath) { // may be a mix of / and \, depending on OS
-			continue
-		}
+	for _, refs := range []map[string]spec.Ref{opts.Spec.references.allRefs, opts.Spec.references.unmappedRefs} {
+		for k, w := range refs {
+			if !strings.HasPrefix(w.String(), opts.BasePath+definitionsPath) { // may be a mix of / and \, depending on OS
+				continue
+			}
 
-		altered = true
-		debugLog("stripping absolute path for: %s", w.String())
+			altered = true
+			debugLog("stripping absolute path for: %s", w.String())
 
-		// strip the base path from definition
-		if err := replace.UpdateRef(opts.Swagger(), k,
-			spec.MustCreateRef(path.Join(definitionsPath, path.Base(w.String())))); err != nil {
-			return err
+			// strip the base path from definition
+			if err := replace.UpdateRef(opts.Swagger(), k,
+				spec.MustCreateRef(path.Join(definitionsPath, path.Base(w.String())))); err != nil {
+				return err
+			}
 		}
 	}
 
@@ -276,6 +279,10 @@ func removeUnusedSinglePass(opts *FlattenOpts) (hasRemoved bool) {
 		delete(expected, k)
 	}
 
+	for _, ref := range opts.Spec.references.unmappedRefs {
+		delete(expected, ref.String())
+	}
+
 	for k := range expected {
 		hasRemoved = true
 		debugLog("removing unused definition %s", path.Base(k))
@@ -327,9 +334,11 @@ func importNewRef(entry sortref.RefRevIdx, refStr string, opts *FlattenOpts) err
 	partialAnalyzer.analyzeSchema("", sch, "/")
 
 	// now rewrite those refs with rebase
-	for key, ref := range partialAnalyzer.references.allRefs {
-		if err := replace.UpdateRef(sch, key, spec.MustCreateRef(normalize.RebaseRef(entry.Ref.String(), ref.String()))); err != nil {
-			return ErrRewriteRef(key, entry.Ref.String(), err)
+	for _, refs := range []map[string]spec.Ref{partialAnalyzer.references.allRefs, partialAnalyzer.references.unmappedRefs} {
+		for key, ref := range refs {
+			if err := replace.UpdateRef(sch, key, spec.MustCreateRef(normalize.RebaseRef(entry.Ref.String(), ref.String()))); err != nil {
+				return ErrRewriteRef(key, entry.Ref.String(), err)
+			}
 		}
 	}
 
@@ -374,10 +383,20 @@ func importNewRef(entry sortref.RefRevIdx, refStr string, opts *FlattenOpts) err
 // At every iteration, new remotes may be found when digging deeper: they are rebased to the current schema before being imported.
 //
 // This returns true when no more remote references can be found.
+// importableRefs returns every $ref that flatten has to bring into the root document: the ones the
+// model maps, and the ones held by keywords it does not.
+func importableRefs(sp *Spec) map[string]spec.Ref {
+	refs := make(map[string]spec.Ref, len(sp.references.schemas)+len(sp.references.unmappedRefs))
+	maps.Copy(refs, sp.references.schemas)
+	maps.Copy(refs, sp.references.unmappedRefs)
+
+	return refs
+}
+
 func importExternalReferences(opts *FlattenOpts) (bool, error) {
 	debugLog("importExternalReferences")
 
-	groupedRefs := sortref.ReverseIndex(opts.Spec.references.schemas, opts.BasePath)
+	groupedRefs := sortref.ReverseIndex(importableRefs(opts.Spec), opts.BasePath)
 	sortedRefStr := make([]string, 0, len(groupedRefs))
 	if opts.flattenContext == nil {
 		opts.flattenContext = newContext()

flatten_test.go

@@ -1463,3 +1463,92 @@ func testFlattenWithDefaults(t *testing.T, bp string) *Spec {
 
 	return an
 }
+
+func TestFlatten_UnmappedKeywordRef(t *testing.T) {
+	// A $ref under a keyword the Swagger 2.0 model does not map - propertyNames, if - lands in
+	// spec.Schema.ExtraProps as raw JSON. Flatten used to leave it alone while importing the
+	// subtree that held it, so the pointer named a definition of the root document, or nothing.
+	for _, testCase := range []struct {
+		name     string
+		fixture  string
+		expected string
+		assert   func(t *testing.T, sp *spec.Swagger)
+	}{
+		{
+			name:     "the target is imported once, and shared",
+			fixture:  "root.json",
+			expected: "#/definitions/leaf",
+			assert: func(t *testing.T, sp *spec.Swagger) {
+				// the mapped $ref and the unmapped ones name the same imported definition
+				require.MapContainsT(t, sp.Definitions, "leaf")
+				mapped := sp.Definitions["deep"].Properties["mapped"]
+				assert.EqualT(t, "#/definitions/leaf", mapped.Ref.String())
+			},
+		},
+		{
+			name:     "the target is imported although nothing mapped points at it",
+			fixture:  "orphan-root.json",
+			expected: "#/definitions/leaf",
+			assert: func(t *testing.T, sp *spec.Swagger) {
+				require.MapContainsT(t, sp.Definitions, "leaf")
+				assert.TrueT(t, sp.Definitions["leaf"].Type.Contains("string"))
+			},
+		},
+		{
+			name:     "a name conflict with the root does not capture the pointer",
+			fixture:  "collide-root.json",
+			expected: "#/definitions/leafOAIGen",
+			assert: func(t *testing.T, sp *spec.Swagger) {
+				// the root keeps its own, unrelated "leaf"
+				assert.TrueT(t, sp.Definitions["leaf"].Type.Contains("integer"))
+				assert.TrueT(t, sp.Definitions["leafOAIGen"].Type.Contains("string"))
+			},
+		},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			bp := filepath.Join(".", "testdata", "unmapped", testCase.fixture)
+			sp := antest.LoadOrFail(t, bp)
+
+			require.NoError(t, Flatten(FlattenOpts{Spec: New(sp), BasePath: bp, Minimal: true}))
+
+			deep := sp.Definitions["deep"]
+			assert.EqualT(t, testCase.expected, unmappedRef(t, deep.ExtraProps["propertyNames"]))
+			testCase.assert(t, sp)
+
+			t.Run("the flattened document is self-contained", func(t *testing.T) {
+				for _, ref := range New(sp).AllRefs() {
+					assert.TrueT(t, strings.HasPrefix(ref.String(), "#/"), "expected a local $ref, got %q", ref)
+				}
+				assertUnmappedRefsResolve(t, sp)
+			})
+		})
+	}
+}
+
+// unmappedRef returns the $ref held by a raw JSON node.
+func unmappedRef(t testing.TB, node any) string {
+	t.Helper()
+
+	asMap, ok := node.(map[string]any)
+	require.TrueT(t, ok)
+	ref, ok := asMap["$ref"].(string)
+	require.TrueT(t, ok)
+
+	return ref
+}
+
+// assertUnmappedRefsResolve checks that every $ref under an unmapped keyword names a definition
+// that the flattened document actually holds.
+func assertUnmappedRefsResolve(t *testing.T, sp *spec.Swagger) {
+	t.Helper()
+
+	an := New(sp)
+	require.NotEmpty(t, an.references.unmappedRefs)
+
+	for key := range an.references.unmappedRefs {
+		ref := an.references.unmappedRefs[key]
+		resolved, err := spec.ResolveRef(sp, &ref)
+		require.NoErrorf(t, err, "$ref %q at %s resolves to nothing", ref.String(), key)
+		require.NotNil(t, resolved)
+	}
+}

internal/flatten/replace/replace.go

@@ -19,6 +19,7 @@ import (
 
 const (
 	definitionsPath = "#/definitions"
+	jsonRef         = "$ref"
 	allocMediumMap  = 64
 )
 
@@ -217,6 +218,17 @@ func UpdateRef(sp any, key string, ref spec.Ref) error {
 	switch refable := value.(type) {
 	case *spec.Schema:
 		refable.Ref = ref
+	case map[string]any:
+		// a keyword the Swagger 2.0 model does not map: the node is raw JSON, and the map it
+		// holds is the one in the document - writing to it reaches the document
+		refable[jsonRef] = ref.String()
+	case *any:
+		raw, ok := (*refable).(map[string]any)
+		if !ok {
+			return ErrNoSchemaWithRef(key, value)
+		}
+
+		raw[jsonRef] = ref.String()
 	case *spec.SchemaOrArray:
 		if refable.Schema != nil {
 			refable.Schema.Ref = ref

testdata/unmapped/collide-other.json

@@ -0,0 +1,10 @@
+{
+  "definitions": {
+    "deep": {
+      "type": "object",
+      "properties": {"plain": {"type": "boolean"}},
+      "propertyNames": {"$ref": "#/definitions/leaf"}
+    },
+    "leaf": {"type": "string"}
+  }
+}

testdata/unmapped/collide-root.json

@@ -0,0 +1,9 @@
+{
+  "swagger": "2.0",
+  "info": {"title": "the root has its own definition of that name", "version": "1.0.0"},
+  "paths": {},
+  "definitions": {
+    "holder": {"$ref": "collide-other.json#/definitions/deep"},
+    "leaf": {"type": "integer", "description": "unrelated to the leaf of the other document"}
+  }
+}

testdata/unmapped/orphan-other.json

@@ -0,0 +1,10 @@
+{
+  "definitions": {
+    "deep": {
+      "type": "object",
+      "properties": {"plain": {"type": "boolean"}},
+      "propertyNames": {"$ref": "#/definitions/leaf"}
+    },
+    "leaf": {"type": "string"}
+  }
+}

testdata/unmapped/orphan-root.json

@@ -0,0 +1,6 @@
+{
+  "swagger": "2.0",
+  "info": {"title": "nothing mapped points at the target", "version": "1.0.0"},
+  "paths": {},
+  "definitions": {"holder": {"$ref": "orphan-other.json#/definitions/deep"}}
+}

testdata/unmapped/other.json

@@ -0,0 +1,11 @@
+{
+  "definitions": {
+    "deep": {
+      "type": "object",
+      "properties": {"mapped": {"$ref": "#/definitions/leaf"}},
+      "propertyNames": {"$ref": "#/definitions/leaf"},
+      "if": {"anyOf": [{"$ref": "#/definitions/leaf"}]}
+    },
+    "leaf": {"type": "string"}
+  }
+}

testdata/unmapped/root.json

@@ -0,0 +1,14 @@
+{
+  "swagger": "2.0",
+  "info": {"title": "a $ref under a keyword the model does not map", "version": "1.0.0"},
+  "paths": {
+    "/things": {
+      "get": {
+        "responses": {
+          "200": {"description": "a thing", "schema": {"$ref": "#/definitions/holder"}}
+        }
+      }
+    }
+  },
+  "definitions": {"holder": {"$ref": "other.json#/definitions/deep"}}
+}