Fix/deterministic expansion

circular_test.go

@@ -6,11 +6,10 @@ package spec
 import (
 	"encoding/json"
 	"fmt"
-	"net/http"
 	"os"
 	"path/filepath"
+	"regexp"
 	"testing"
-	"time"
 
 	"github.com/go-openapi/testify/v2/assert"
 	"github.com/go-openapi/testify/v2/require"
@@ -263,16 +262,13 @@ func TestExpandCircular_SpecExpansion(t *testing.T) {
 }
 
 func TestExpandCircular_RemoteCircularID(t *testing.T) {
-	go func() {
-		err := http.ListenAndServe("localhost:1234", http.FileServer(http.Dir("testdata/more_circulars/remote"))) //#nosec
-		if err != nil {
-			panic(err.Error())
-		}
-	}()
-	time.Sleep(100 * time.Millisecond)
+	// tree and with-id.json both spell the schema id as http://localhost:1234:
+	// rewrite it to wherever the test server listens, so the package runs under -count>1.
+	const fixtureOrigin = "http://localhost:1234"
+	server := rewritingFixtureServer(t, "testdata/more_circulars/remote", fixtureOrigin)
 
 	// from json-schema test suite testcase for remote with circular ID
-	fixturePath := "http://localhost:1234/tree"
+	fixturePath := server.URL + "/tree"
 	jazon, root := expandThisSchemaOrDieTrying(t, fixturePath)
 	assertRefResolve(t, jazon, "", root, &ExpandOptions{RelativeBase: fixturePath})
 	assertRefExpand(t, jazon, "", root, &ExpandOptions{RelativeBase: fixturePath})
@@ -281,10 +277,12 @@ func TestExpandCircular_RemoteCircularID(t *testing.T) {
 
 	jazon = asJSON(t, root)
 
-	assertRefInJSONRegexp(t, jazon, "^http://localhost:1234/tree$") // $ref now point to the root doc
+	assertRefInJSONRegexp(t, jazon, "^"+regexp.QuoteMeta(fixturePath)+"$") // $ref now point to the root doc
 
 	// a spec using the previous circular schema
-	fixtureSpecPath := filepath.Join("testdata", "more_circulars", "with-id.json")
+	fixtureSpecPath := rewriteFixture(t,
+		filepath.Join("testdata", "more_circulars", "with-id.json"), fixtureOrigin, server.URL,
+	)
 	jazon, doc := expandThisOrDieTrying(t, fixtureSpecPath)
 
 	assertRefInJSON(t, jazon, fixturePath) // all remaining $ref's point to the circular ID (http://...)

determinism_test.go

@@ -0,0 +1,59 @@
+// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers
+// SPDX-License-Identifier: Apache-2.0
+
+package spec
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/go-openapi/testify/v2/assert"
+	"github.com/go-openapi/testify/v2/require"
+)
+
+// TestExpand_IsReproducible expands the same document repeatedly and requires the same bytes
+// every time.
+//
+// Expansion inlines the first branch that reaches a cycle and leaves a $ref on the others, so
+// the order siblings are visited in decides which node holds the $ref. Ranging a map straight
+// gave that decision to Go's map iteration: fixture-957.json produced 40 different documents in
+// 40 runs. See go-openapi/spec#93.
+func TestExpand_IsReproducible(t *testing.T) {
+	const runs = 10
+
+	for _, fixture := range []struct{ name, path string }{
+		{"cycles sharing a node", filepath.Join("testdata", "expansion", "shared-node-cycles.json")},
+		{"several entries into remote cycles", filepath.Join("testdata", "expansion", "multi-entry-cycles", "root.json")},
+		{"a cycle in the root", filepath.Join("testdata", "expansion", "circularSpec.json")},
+		{"issue 957", filepath.Join("testdata", "bugs", "957", "fixture-957.json")},
+		{"bitbucket", filepath.Join("testdata", "more_circulars", "bitbucket.json")},
+	} {
+		t.Run(fixture.name, func(t *testing.T) {
+			t.Parallel()
+
+			data, err := os.ReadFile(fixture.path)
+			require.NoError(t, err)
+
+			first := expandOnce(t, data, fixture.path)
+			for range runs - 1 {
+				assert.EqualT(t, first, expandOnce(t, data, fixture.path),
+					"expanding %s twice gave two different documents", fixture.path)
+			}
+		})
+	}
+}
+
+func expandOnce(t *testing.T, data []byte, basePath string) string {
+	t.Helper()
+
+	doc := new(Swagger)
+	require.NoError(t, json.Unmarshal(data, doc))
+	require.NoError(t, ExpandSpec(doc, &ExpandOptions{RelativeBase: basePath}))
+
+	expanded, err := json.Marshal(doc)
+	require.NoError(t, err)
+
+	return string(expanded)
+}

expander.go

@@ -4,8 +4,12 @@
 package spec
 
 import (
+	"cmp"
 	"encoding/json"
 	"fmt"
+	"iter"
+	"maps"
+	"slices"
 
 	"github.com/go-openapi/swag/loading"
 )
@@ -109,7 +113,8 @@ func ExpandSpec(spec *Swagger, options *ExpandOptions) error {
 	specBasePath := options.RelativeBase
 
 	if !options.SkipSchemas {
-		for key, definition := range spec.Definitions {
+		for key := range sortedKeys(spec.Definitions) {
+			definition := spec.Definitions[key]
 			parentRefs := make([]string, 0, smallPrealloc)
 			parentRefs = append(parentRefs, "#/definitions/"+key)
 
@@ -123,15 +128,15 @@ func ExpandSpec(spec *Swagger, options *ExpandOptions) error {
 		}
 	}
 
-	for key := range spec.Parameters {
+	for key := range sortedKeys(spec.Parameters) {
 		parameter := spec.Parameters[key]
 		if err := expandParameterOrResponse(&parameter, resolver, specBasePath); resolver.shouldStopOnError(err) {
 			return err
 		}
 		spec.Parameters[key] = parameter
 	}
 
-	for key := range spec.Responses {
+	for key := range sortedKeys(spec.Responses) {
 		response := spec.Responses[key]
 		if err := expandParameterOrResponse(&response, resolver, specBasePath); resolver.shouldStopOnError(err) {
 			return err
@@ -140,7 +145,7 @@ func ExpandSpec(spec *Swagger, options *ExpandOptions) error {
 	}
 
 	if spec.Paths != nil {
-		for key := range spec.Paths.Paths {
+		for key := range sortedKeys(spec.Paths.Paths) {
 			pth := spec.Paths.Paths[key]
 			if err := expandPathItem(&pth, resolver, specBasePath); resolver.shouldStopOnError(err) {
 				return err
@@ -278,6 +283,38 @@ func expandItems(target Schema, parentRefs []string, resolver *schemaLoader, bas
 	return &target, nil
 }
 
+// sortedKeys walks the keys of a map in a fixed order.
+//
+// Expansion inlines the first branch that reaches a cycle and leaves a $ref on the others, so
+// the order the walk visits siblings in decides which node ends up holding the $ref. Ranging a
+// map straight gives that decision to Go's map iteration, and the same document then expands
+// differently from one run to the next - see go-openapi/spec#93.
+//
+// A map of fewer than two keys has only one order, so it is yielded without sorting: schemata
+// with a single property or definition are most of what a walk of a large document visits, and
+// the slice this would otherwise allocate is paid at every node.
+func sortedKeys[K cmp.Ordered, V any](m map[K]V) iter.Seq[K] {
+	const alreadyOrdered = 2 // a map of fewer keys than this has only one order
+
+	return func(yield func(K) bool) {
+		if len(m) < alreadyOrdered {
+			for key := range m {
+				yield(key)
+
+				return
+			}
+
+			return
+		}
+
+		for _, key := range slices.Sorted(maps.Keys(m)) {
+			if !yield(key) {
+				return
+			}
+		}
+	}
+}
+
 //nolint:gocognit,gocyclo,cyclop // complex but well-tested $ref expansion logic; refactoring deferred to dedicated PR
 func expandSchema(target Schema, parentRefs []string, resolver *schemaLoader, basePath string) (*Schema, error) {
 	if err := resolver.context.countNode(); err != nil {
@@ -312,7 +349,9 @@ func expandSchema(target Schema, parentRefs []string, resolver *schemaLoader, ba
 		return &target, nil
 	}
 
-	for k := range target.Definitions {
+	rebaseExtraRefs(target.ExtraProps, resolver, basePath)
+
+	for k := range sortedKeys(target.Definitions) {
 		tt, err := expandSchema(target.Definitions[k], parentRefs, resolver, basePath)
 		if resolver.shouldStopOnError(err) {
 			return &target, err
@@ -370,7 +409,7 @@ func expandSchema(target Schema, parentRefs []string, resolver *schemaLoader, ba
 		}
 	}
 
-	for k := range target.Properties {
+	for k := range sortedKeys(target.Properties) {
 		t, err := expandSchema(target.Properties[k], parentRefs, resolver, basePath)
 		if resolver.shouldStopOnError(err) {
 			return &target, err
@@ -390,7 +429,7 @@ func expandSchema(target Schema, parentRefs []string, resolver *schemaLoader, ba
 		}
 	}
 
-	for k := range target.PatternProperties {
+	for k := range sortedKeys(target.PatternProperties) {
 		t, err := expandSchema(target.PatternProperties[k], parentRefs, resolver, basePath)
 		if resolver.shouldStopOnError(err) {
 			return &target, err
@@ -400,7 +439,7 @@ func expandSchema(target Schema, parentRefs []string, resolver *schemaLoader, ba
 		}
 	}
 
-	for k := range target.Dependencies {
+	for k := range sortedKeys(target.Dependencies) {
 		if target.Dependencies[k].Schema != nil {
 			t, err := expandSchema(*target.Dependencies[k].Schema, parentRefs, resolver, basePath)
 			if resolver.shouldStopOnError(err) {
@@ -424,6 +463,67 @@ func expandSchema(target Schema, parentRefs []string, resolver *schemaLoader, ba
 	return &target, nil
 }
 
+// rebaseExtraRefs rewrites the $ref held by keywords this model does not map, so that
+// they still point at their target once the schema is inlined into another document.
+//
+// expandSchema walks the fields of [Schema] and stops there. A keyword the Swagger 2.0
+// model predates - propertyNames, contains, if/then/else, $defs - lands in ExtraProps as
+// raw JSON, and a $ref inside it is copied into the root verbatim: "#/definitions/leaf"
+// then names a definition of the root document instead of the one it came from.
+//
+// Rebasing makes the pointer correct. It does not expand it, and it does not make the
+// expanded document self-contained: a $ref that came from another document keeps pointing
+// there.
+func rebaseExtraRefs(extra map[string]any, resolver *schemaLoader, basePath string) {
+	for key := range extra {
+		rebaseRawRefs(extra[key], resolver, basePath)
+	}
+}
+
+// rebaseRawRefs walks raw JSON and rebases every "$ref" string value it finds.
+func rebaseRawRefs(node any, resolver *schemaLoader, basePath string) {
+	switch value := node.(type) {
+	case map[string]any:
+		for key := range value {
+			if key == jsonRef {
+				if ref, ok := value[key].(string); ok {
+					if rebased, ok := rebaseRawRef(ref, resolver, basePath); ok {
+						value[key] = rebased
+					}
+
+					continue
+				}
+			}
+
+			rebaseRawRefs(value[key], resolver, basePath)
+		}
+	case []any:
+		for i := range value {
+			rebaseRawRefs(value[i], resolver, basePath)
+		}
+	}
+}
+
+// rebaseRawRef resolves a $ref against basePath, then spells it relative to the document
+// being expanded, like the SkipSchemas branch of [expandSchema] does for a mapped $ref.
+//
+// It reports false when the $ref is empty or does not parse: an unmapped keyword may hold
+// any JSON, and a string under a "$ref" key is not necessarily a reference.
+func rebaseRawRef(ref string, resolver *schemaLoader, basePath string) (string, bool) {
+	if ref == "" {
+		return "", false
+	}
+
+	rebased, err := NewRef(normalizeURI(ref, basePath))
+	if err != nil {
+		return "", false
+	}
+
+	denormalized := denormalizeRef(&rebased, resolver.context.basePath, resolver.context.rootID)
+
+	return denormalized.String(), true
+}
+
 func expandSchemaRef(target Schema, parentRefs []string, resolver *schemaLoader, basePath string) (*Schema, error) {
 	// if a Ref is found, all sibling fields are skipped
 	// Ref also changes the resolution scope of children expandSchema
@@ -528,7 +628,7 @@ func expandOperation(op *Operation, resolver *schemaLoader, basePath string) err
 		return err
 	}
 
-	for code := range responses.StatusCodeResponses {
+	for code := range sortedKeys(responses.StatusCodeResponses) {
 		response := responses.StatusCodeResponses[code]
 		if err := expandParameterOrResponse(&response, resolver, basePath); resolver.shouldStopOnError(err) {
 			return err

expander_bench_test.go

@@ -0,0 +1,85 @@
+// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers
+// SPDX-License-Identifier: Apache-2.0
+
+package spec
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+// expandBenchmarks are the documents the expansion benchmarks run on, ordered by how much cycle
+// detection they exercise.
+//
+// fixture-957.json is the one PR #121 had to disable for slowness, so it is the case any change
+// to how cycles are remembered has to answer for. bitbucket.json is the largest cyclic document
+// here, 300 $ref; clickmeter.json is the largest acyclic one, and says what expansion costs when
+// cycles are not the subject.
+func expandBenchmarks() []struct{ name, path string } {
+	return []struct{ name, path string }{
+		{"petstore-acyclic", filepath.Join("testdata", "expansion", "petstore2.0.json")},
+		{"circular-spec", filepath.Join("testdata", "expansion", "circularSpec.json")},
+		{"shared-node-cycles", filepath.Join("testdata", "expansion", "shared-node-cycles.json")},
+		{"issue-957", filepath.Join("testdata", "bugs", "957", "fixture-957.json")},
+		{"bitbucket", filepath.Join("testdata", "more_circulars", "bitbucket.json")},
+		{"clickmeter", filepath.Join("testdata", "expansion", "clickmeter.json")},
+	}
+}
+
+// BenchmarkExpandSpec measures expansion alone: the document is unmarshalled again for every
+// iteration, since expansion rewrites it, and that unmarshalling is not timed.
+func BenchmarkExpandSpec(b *testing.B) {
+	for _, bench := range expandBenchmarks() {
+		data, err := os.ReadFile(bench.path)
+		if err != nil {
+			b.Fatal(err)
+		}
+
+		b.Run(bench.name, func(b *testing.B) {
+			b.ReportAllocs()
+
+			for b.Loop() {
+				b.StopTimer()
+				doc := new(Swagger)
+				if err := json.Unmarshal(data, doc); err != nil {
+					b.Fatal(err)
+				}
+				b.StartTimer()
+
+				if err := ExpandSpec(doc, &ExpandOptions{RelativeBase: bench.path}); err != nil {
+					b.Fatal(err)
+				}
+			}
+		})
+	}
+}
+
+// BenchmarkExpandSpecSkipSchemas measures the path flatten takes for its minimal and full modes,
+// where schemata are not inlined and only $ref are rebased.
+func BenchmarkExpandSpecSkipSchemas(b *testing.B) {
+	for _, bench := range expandBenchmarks() {
+		data, err := os.ReadFile(bench.path)
+		if err != nil {
+			b.Fatal(err)
+		}
+
+		b.Run(bench.name, func(b *testing.B) {
+			b.ReportAllocs()
+
+			for b.Loop() {
+				b.StopTimer()
+				doc := new(Swagger)
+				if err := json.Unmarshal(data, doc); err != nil {
+					b.Fatal(err)
+				}
+				b.StartTimer()
+
+				if err := ExpandSpec(doc, &ExpandOptions{RelativeBase: bench.path, SkipSchemas: true}); err != nil {
+					b.Fatal(err)
+				}
+			}
+		})
+	}
+}