From ba4dd044d4ef67182955f0c91ee0860929f18c09 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Mon, 24 Aug 2026 13:06:23 +0200 Subject: [PATCH 1/4] test(expander): serve the remote circular id fixture on a free port TestExpandCircular_RemoteCircularID bound localhost:1234 from a goroutine that panicked when the bind failed, and never closed the server, so the package could not run under -count>1 - which is what measuring expansion determinism needs. testdata/more_circulars/remote/tree and with-id.json both spell the schema id as http://localhost:1234, so a random port needs the id to follow it. rewritingFixtureServer serves the embedded FS with that origin replaced by the address the listener holds; rewriteFixture writes the rewritten spec into t.TempDir(). The fixtures on disk are unchanged. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- circular_test.go | 22 ++++++++++------------ helpers_test.go | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/circular_test.go b/circular_test.go index 54dcbe3..8231c31 100644 --- a/circular_test.go +++ b/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://...) diff --git a/helpers_test.go b/helpers_test.go index 06ab941..29558e0 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -9,6 +9,7 @@ import ( "io/fs" "net/http" "net/http/httptest" + "os" "path/filepath" "regexp" "strings" @@ -37,6 +38,52 @@ func fixtureServer(t testing.TB, dir string) *httptest.Server { return server } +// rewritingFixtureServer serves a subdirectory of the embedded fixtureAssets FS, +// replacing every occurrence of placeholder with the address the server listens on. +// +// Fixtures that carry an absolute schema id cannot be served from a random port +// unless the id follows the port. The handler is wired before Start, so the URL +// it captures is the one the listener already holds. +func rewritingFixtureServer(t testing.TB, dir, placeholder string) *httptest.Server { + t.Helper() + + sub, err := fs.Sub(fixtureAssets, filepath.ToSlash(dir)) + require.NoError(t, err) + + server := httptest.NewUnstartedServer(nil) + url := "http://" + server.Listener.Addr().String() + server.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + data, err := fs.ReadFile(sub, strings.TrimPrefix(r.URL.Path, "/")) + if err != nil { + http.NotFound(w, r) + + return + } + + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(strings.ReplaceAll(string(data), placeholder, url))) //nolint:gosec // serves a fixture from the embedded FS + }) + server.Start() + t.Cleanup(server.Close) + + return server +} + +// rewriteFixture copies a fixture into the test's temporary directory, replacing +// every occurrence of placeholder with replacement, and returns the copy's path. +func rewriteFixture(t testing.TB, path, placeholder, replacement string) string { + t.Helper() + + data, err := os.ReadFile(path) + require.NoError(t, err) + + target := filepath.Join(t.TempDir(), filepath.Base(path)) + //nolint:gosec // writes a fixture into the test's own temporary directory + require.NoError(t, os.WriteFile(target, []byte(strings.ReplaceAll(string(data), placeholder, replacement)), 0o600)) + + return target +} + func jsonDoc(path string) (json.RawMessage, error) { data, err := loading.LoadFromFileOrHTTP(path) if err != nil { From af1d328e4b421e52aa75117eb824a27cbeca8909 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Mon, 24 Aug 2026 13:06:23 +0200 Subject: [PATCH 2/4] chore(normalizer): correct the build constraint of normalizer_windows.go The file opened with "// -build windows", which is not a build constraint in any syntax. The _windows suffix already restricts the file, so nothing changes; the line now reads //go:build windows, like the !windows one in normalizer_nonwindows.go. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- normalizer_windows.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/normalizer_windows.go b/normalizer_windows.go index 61515c9..a439f43 100644 --- a/normalizer_windows.go +++ b/normalizer_windows.go @@ -1,4 +1,4 @@ -// -build windows +//go:build windows // SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers // SPDX-License-Identifier: Apache-2.0 From ee01904a7bb26e8a22611a97a69597af4a333314 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Mon, 24 Aug 2026 13:06:34 +0200 Subject: [PATCH 3/4] fix(expander): rebase a $ref found under an unmapped keyword 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 Schema.ExtraProps as raw JSON, and inlining that subtree from another document copied its $ref into the root verbatim: "#/definitions/leaf" then named a definition of the root document rather than the one it came from, and resolving it raised "has no key". rebaseExtraRefs walks the raw JSON and rewrites every string under a "$ref" key with normalizeURI then denormalizeRef, the pair the SkipSchemas branch already uses for a mapped $ref, so "#/definitions/leaf" becomes "other.json#/definitions/leaf". The pointer is correct; it is not expanded, and the output is no more self-contained than before. Empty and unparseable strings are left alone: an unmapped keyword may hold any JSON, and a string under a "$ref" key is not necessarily a reference. Two alternatives were rejected: refusing to expand a $ref whose target the model cannot represent breaks documents that work today, and expanding the ExtraProps subtree properly needs a raw-JSON walker with its own cycle detection over untyped values. FuzzExpandSpec gains the property that found this - every local $ref surviving expansion must still name something - guarded by the same check on the input, so a document that already dangles proves nothing. Its stub loader used to answer every path, including the root document's own URL, so a $ref spelled "." inlined a foreign document; it now returns the document's own bytes for that path. No cycle is involved: the cut point of shared-node-cycles.json is unchanged over 40 expansions, with the same three outcomes as before. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- expander.go | 63 +++++++++++++++++++++ expander_fuzz_test.go | 76 ++++++++++++++++++++++++-- expander_test.go | 54 ++++++++++++++++++ testdata/expansion/unmapped/other.json | 25 +++++++++ testdata/expansion/unmapped/root.json | 26 +++++++++ 5 files changed, 239 insertions(+), 5 deletions(-) create mode 100644 testdata/expansion/unmapped/other.json create mode 100644 testdata/expansion/unmapped/root.json diff --git a/expander.go b/expander.go index 00eb5b5..06d1210 100644 --- a/expander.go +++ b/expander.go @@ -312,6 +312,8 @@ func expandSchema(target Schema, parentRefs []string, resolver *schemaLoader, ba return &target, nil } + rebaseExtraRefs(target.ExtraProps, resolver, basePath) + for k := range target.Definitions { tt, err := expandSchema(target.Definitions[k], parentRefs, resolver, basePath) if resolver.shouldStopOnError(err) { @@ -424,6 +426,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 diff --git a/expander_fuzz_test.go b/expander_fuzz_test.go index cc15249..2c5a337 100644 --- a/expander_fuzz_test.go +++ b/expander_fuzz_test.go @@ -5,6 +5,8 @@ package spec import ( "encoding/json" + "sort" + "strings" "testing" "github.com/go-openapi/testify/v2/require" @@ -15,10 +17,17 @@ import ( // It answers any path, so the loader never touches the filesystem or the network, and it holds a // $ref back out to another document: since every path yields this same stub, that $ref chains // forever unless cycle detection and the expansion budget stop it. +// +// "unmapped" carries a $ref under propertyNames, a keyword this model does not map: inlining +// that subtree must rebase the $ref on the stub, not copy it into the root. +// fuzzBase is where the document being expanded is taken to live. +const fuzzBase = "file:///fuzz/spec.json" + const remoteFuzzStub = `{"definitions":{` + `"remote":{"type":"string"},` + `"cycle":{"$ref":"other.json#/definitions/cycle"},` + - `"deep":{"properties":{"a":{"$ref":"#/definitions/remote"},"b":{"$ref":"other.json#/definitions/deep"}}}` + + `"deep":{"properties":{"a":{"$ref":"#/definitions/remote"},"b":{"$ref":"other.json#/definitions/deep"}}},` + + `"unmapped":{"propertyNames":{"$ref":"#/definitions/remote"}}` + `}}` // FuzzExpandSpec expands arbitrary documents. @@ -27,9 +36,10 @@ const remoteFuzzStub = `{"definitions":{` + // $refs across documents and rewrites the tree as it goes. It is also where a hostile document // costs the most: see ErrExpandTooManyNodes and the confinement notes on ExpandOptions. // -// The properties are that expansion terminates, and that what it produces is still a document - +// The properties are that expansion terminates, that what it produces is still a document - // an expansion that succeeds but leaves behind something we can no longer read is a silent -// corruption of the caller's spec. +// corruption of the caller's spec - and that the local pointers it leaves behind still name +// something. That last one is what found the ExtraProps defect fixed by rebaseExtraRefs. // // The document loader is stubbed, so nothing here reads a file or opens a socket. func FuzzExpandSpec(f *testing.F) { @@ -43,9 +53,23 @@ func FuzzExpandSpec(f *testing.F) { return // not a document we claim to accept } + // what the input claims before anything is expanded: an input that already + // dangles cannot say whether expansion broke a pointer + original, err := json.Marshal(doc) + if err != nil { + return + } + soundBefore := len(unresolvedLocalRefs(&doc, string(original))) == 0 + opts := &ExpandOptions{ - RelativeBase: "file:///fuzz/spec.json", - PathLoader: func(string) (json.RawMessage, error) { + RelativeBase: fuzzBase, + PathLoader: func(path string) (json.RawMessage, error) { + if normalizeBase(path) == normalizeBase(fuzzBase) { + // a $ref spelled "." names the document being expanded: a real loader + // hands back the bytes it was read from, not another document + return original, nil + } + return json.RawMessage(remoteFuzzStub), nil }, MaxExpansionNodes: 500, @@ -61,9 +85,50 @@ func FuzzExpandSpec(f *testing.F) { var reread Swagger require.NoErrorf(t, json.Unmarshal(expanded, &reread), "an expanded document no longer parses: %s", expanded) + + if !soundBefore { + return // a document whose own pointers were already dangling proves nothing + } + + require.Emptyf(t, unresolvedLocalRefs(&reread, string(expanded)), + "expansion left a local $ref pointing at nothing.\ninput: %s\nexpanded: %s", data, expanded) }) } +// unresolvedLocalRefs returns the local $ref of jazon that name nothing in doc. +// +// Only "#/..." pointers are looked at: a $ref into another document is the loader's business, +// and the loader is a stub here. The document is walked with the same jsonpointer call the +// resolver makes, so "has no key" here is "has no key" there. +func unresolvedLocalRefs(doc any, jazon string) []string { + var unresolved []string + seen := make(map[string]struct{}) + + for _, matched := range rex.FindAllStringSubmatch(jazon, -1) { + refString := matched[1] + if !strings.HasPrefix(refString, "#/") { + continue + } + if _, already := seen[refString]; already { + continue + } + seen[refString] = struct{}{} + + ref, err := NewRef(refString) + if err != nil { + continue // not a reference this package claims to understand + } + + if _, _, err := ref.GetPointer().Get(doc); err != nil { + unresolved = append(unresolved, refString) + } + } + + sort.Strings(unresolved) + + return unresolved +} + // expanderSeeds are the documents the fuzzer starts from. // // They aim at the shapes expansion has to walk: $refs in every position that accepts one, @@ -78,6 +143,7 @@ func expanderSeeds() []string { `{"definitions":{"A":{"$ref":"other.json#/definitions/cycle"}}}`, `{"definitions":{"A":{"properties":{"a":{"$ref":"#/definitions/A"}},"items":{"$ref":"#/definitions/A"}}}}`, `{"definitions":{"A":{"allOf":[{"$ref":"#/definitions/A"}],"additionalProperties":{"$ref":"#/definitions/A"}}}}`, + `{"definitions":{"A":{"$ref":"other.json#/definitions/unmapped"}}}`, `{"paths":{"/x":{"$ref":"other.json#/definitions/deep"}}}`, `{"paths":{"/x":{"get":{"parameters":[{"$ref":"#/parameters/p"}],` + `"responses":{"200":{"$ref":"#/responses/r"}}}}},` + diff --git a/expander_test.go b/expander_test.go index 16f8a26..2bef2ea 100644 --- a/expander_test.go +++ b/expander_test.go @@ -1082,3 +1082,57 @@ func TestExpand_Issue145(t *testing.T) { }) }) } + +func TestExpand_UnmappedSubtreeRef(t *testing.T) { + // A $ref under a keyword the Swagger 2.0 model does not map (propertyNames, if) + // lands in Schema.ExtraProps as raw JSON. Inlining the subtree from other.json used + // to carry those $ref into the root verbatim, where "#/definitions/leaf" names + // nothing. They are rebased on the document they came from instead. + fixturePath := filepath.Join("testdata", "expansion", "unmapped", "root.json") + jazon, doc := expandThisOrDieTrying(t, fixturePath) + require.NotEmpty(t, jazon) + + holder := doc.Definitions["holder"] + + t.Run("the mapped $ref is expanded", func(t *testing.T) { + mapped := holder.Properties["mapped"] + assert.EqualT(t, "", mapped.Ref.String()) + assert.TrueT(t, mapped.Type.Contains("string")) + }) + + t.Run("the unmapped $ref are rebased, not expanded", func(t *testing.T) { + assert.EqualT(t, "other.json#/definitions/leaf", rawRef(t, holder.ExtraProps["propertyNames"])) + + cond, ok := holder.ExtraProps["if"].(map[string]any) + require.TrueT(t, ok) + anyOf, ok := cond["anyOf"].([]any) + require.TrueT(t, ok) + require.EqualT(t, 1, len(anyOf)) + assert.EqualT(t, "other.json#/definitions/leaf", rawRef(t, anyOf[0])) + }) + + t.Run("every surviving $ref resolves against the expanded document", func(t *testing.T) { + opts := &ExpandOptions{RelativeBase: fixturePath} + assertRefResolve(t, jazon, "", doc, opts) + }) + + t.Run("and against a document read back from the expanded bytes", func(t *testing.T) { + reloaded := new(Swagger) + require.NoError(t, json.Unmarshal([]byte(jazon), reloaded)) + + opts := &ExpandOptions{RelativeBase: fixturePath} + assertRefResolve(t, jazon, "", reloaded, opts) + }) +} + +// rawRef returns the $ref held by a raw JSON node. +func rawRef(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 +} diff --git a/testdata/expansion/unmapped/other.json b/testdata/expansion/unmapped/other.json new file mode 100644 index 0000000..b2b9310 --- /dev/null +++ b/testdata/expansion/unmapped/other.json @@ -0,0 +1,25 @@ +{ + "definitions": { + "deep": { + "type": "object", + "properties": { + "mapped": { + "$ref": "#/definitions/leaf" + } + }, + "propertyNames": { + "$ref": "#/definitions/leaf" + }, + "if": { + "anyOf": [ + { + "$ref": "#/definitions/leaf" + } + ] + } + }, + "leaf": { + "type": "string" + } + } +} diff --git a/testdata/expansion/unmapped/root.json b/testdata/expansion/unmapped/root.json new file mode 100644 index 0000000..951c81a --- /dev/null +++ b/testdata/expansion/unmapped/root.json @@ -0,0 +1,26 @@ +{ + "swagger": "2.0", + "info": { + "title": "a $ref under a keyword this 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" + } + } +} From 663f784283f71d8e217bb63c70bc2063ec15b10c Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Mon, 24 Aug 2026 19:28:49 +0200 Subject: [PATCH 4/4] fix(expander): walk maps in sorted order so expansion is reproducible 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 ends up holding the $ref. Nine walks in expander.go ranged a map straight and handed that decision to Go's map iteration, so the same document expanded differently from one run to the next: fixture-957.json produced 40 different documents in 40 runs, bitbucket.json 14, and shared-node-cycles.json 4. They now walk through sortedKeys, which yields keys in sorted order. The result is one of the documents the expander already produced - sorting pins an outcome rather than inventing one - and it is now the only one. sortedKeys yields straight from the map when it holds fewer than two keys, since there is then only one order. Most nodes of a large document are schemata with a single property or definition, and sorting them cost a slice each: without that fast path expansion was 18% slower overall, with it the geomean is 9.5% and bitbucket.json is 4% faster than before, its deterministic order being a better one for the cycle memo. The residual cost comes from the order, not from the sorting: where the sorted walk lands on a costlier cut than the average random one it does more work, +39% on shared-node-cycles.json, and where it lands on a cheaper one it does less. In exchange the cost stops varying - expanding fixture-957.json was swinging 44% in time and 46% in allocations between runs of identical input. BenchmarkExpandSpec and BenchmarkExpandSpecSkipSchemas measure both paths; the second is the one analysis takes for minimal and full flattening, where schemata are not inlined and cycles are never walked. analysis and validate are green against this, their real-spec corpus included: their assertions read the form of a surviving $ref and never which definition holds it. Fixes #93 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- determinism_test.go | 59 +++++++++++++ expander.go | 55 ++++++++++-- expander_bench_test.go | 85 +++++++++++++++++++ .../expansion/multi-entry-cycles/remote.json | 12 +++ .../expansion/multi-entry-cycles/root.json | 20 +++++ 5 files changed, 222 insertions(+), 9 deletions(-) create mode 100644 determinism_test.go create mode 100644 expander_bench_test.go create mode 100644 testdata/expansion/multi-entry-cycles/remote.json create mode 100644 testdata/expansion/multi-entry-cycles/root.json diff --git a/determinism_test.go b/determinism_test.go new file mode 100644 index 0000000..8bedcd2 --- /dev/null +++ b/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) +} diff --git a/expander.go b/expander.go index 06d1210..c9ffed5 100644 --- a/expander.go +++ b/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,7 +128,7 @@ 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(¶meter, resolver, specBasePath); resolver.shouldStopOnError(err) { return err @@ -131,7 +136,7 @@ func ExpandSpec(spec *Swagger, options *ExpandOptions) error { 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 { @@ -314,7 +351,7 @@ func expandSchema(target Schema, parentRefs []string, resolver *schemaLoader, ba rebaseExtraRefs(target.ExtraProps, resolver, basePath) - for k := range target.Definitions { + for k := range sortedKeys(target.Definitions) { tt, err := expandSchema(target.Definitions[k], parentRefs, resolver, basePath) if resolver.shouldStopOnError(err) { return &target, err @@ -372,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 @@ -392,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 @@ -402,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) { @@ -591,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 diff --git a/expander_bench_test.go b/expander_bench_test.go new file mode 100644 index 0000000..7432100 --- /dev/null +++ b/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) + } + } + }) + } +} diff --git a/testdata/expansion/multi-entry-cycles/remote.json b/testdata/expansion/multi-entry-cycles/remote.json new file mode 100644 index 0000000..2b23dab --- /dev/null +++ b/testdata/expansion/multi-entry-cycles/remote.json @@ -0,0 +1,12 @@ +{ + "definitions": { + "a": {"type": "object", "properties": { + "toB": {"$ref": "#/definitions/b"}, + "toC": {"$ref": "#/definitions/c"}}}, + "b": {"type": "object", "properties": { + "backToA": {"$ref": "#/definitions/a"}}}, + "c": {"type": "object", "properties": { + "backToA": {"$ref": "#/definitions/a"}, + "alsoB": {"$ref": "#/definitions/b"}}} + } +} diff --git a/testdata/expansion/multi-entry-cycles/root.json b/testdata/expansion/multi-entry-cycles/root.json new file mode 100644 index 0000000..0b5f1a5 --- /dev/null +++ b/testdata/expansion/multi-entry-cycles/root.json @@ -0,0 +1,20 @@ +{ + "swagger": "2.0", + "info": { + "title": "several entry points into one group of remote cycles", + "version": "1.0.0", + "description": "reproduces the trigger of #93: which entry the walk visits first decides where the cycle is cut" + }, + "paths": {}, + "definitions": { + "entryA": { + "$ref": "remote.json#/definitions/a" + }, + "entryC": { + "$ref": "remote.json#/definitions/c" + }, + "entryB": { + "$ref": "remote.json#/definitions/b" + } + } +} \ No newline at end of file