Fix/deterministic expansion - #307
Merged
Merged
Conversation
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) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
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) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
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) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
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 go-openapi#93 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #307 +/- ##
==========================================
+ Coverage 93.05% 93.08% +0.02%
==========================================
Files 30 30
Lines 2463 2500 +37
==========================================
+ Hits 2292 2327 +35
- Misses 171 173 +2 ☔ View full report in Codecov by Harness. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change type
Please select: 🆕 New feature or enhancement|🔧 Bug fix'|📃 Documentation update
Short description
Fixes
Full description
Checklist