Repository navigation
fix: only treat schemas on a reference loop as circular - #667
Open
josephaxisa wants to merge 1 commit into
Open
josephaxisa wants to merge 1 commit into
josephaxisa wants to merge 1 commit into
Conversation
CheckSchemaProxyForCircularRefs returned true for every schema proxy as soon as a document had one circular reference: the journey check kept a single dup map across all results, and every journey repeats its loop point. Inline schemas then hashed to 0 with UseSchemaQuickHash, so their changes were skipped, and CompareSchemas dropped $ref changes. Check membership of the loop instead: the part of each journey from the loop point on (all of it when the loop point only ends the journey, as at the resolver's depth limit), matched by the node the reference resolves to, since a relative reference such as 'b.yaml#/B' never matches the journey's absolute definitions. Inline schemas are not references and are never circular. Fixes pb33f#662
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #667 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 309 309
Lines 39178 39197 +19
=========================================
+ Hits 39178 39197 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Fixes #662 .
CheckSchemaProxyForCircularRefsreturned true for every schema proxy once a document had a single circular reference anywhere: the journey check kept onedupmap across all results, and every journey repeats its loop point. That had two effects:CompareSchemasreturned nil in its$refbranches, so$refchanges to schemas that are not on any loop were dropped (the repro in $ref changes are dropped when a document has any circular reference #662).UseSchemaQuickHash(which openapi-changes turns on), every inline schema proxy hashed to 0, so changes inside inline schemas were skipped. For example, an inlinenamechanging fromstringtointegeris not reported at all if the document also containsA -> B -> A.The check now asks whether this schema is on a loop: the part of each journey from its loop point on, matched against the node the proxy's reference resolves to. Matching by node is needed because a relative reference such as
b.yaml#/Bnever equals the journey's absolute definitions. Inline schemas are not references, so they are never circular. If a loop point only appears at the end of a journey (the resolver's depth limit), the whole journey counts, as before. The existing LoopPoint check and the string match against journey entries are kept, soTestCheckSchemaProxyForCircularRefs_JourneyCheckpasses unchanged.Comparing the
$refstring only withref.Journeywould hang on cross-file cycles, because the quick hash path recurses through any reference the check misses.TestCompareOpenAPIDocuments_CrossFileCircularReferencecovers that case. The resolver records one loop per loop point, so inA -> B -> AplusA -> C -> Aonly one is listed.Cisn't reported as circular, but anything that followsCreachesA, which is, so recursion still stops there(covered by
TestCompareOpenAPIDocuments_UnrelatedCircularReference).This PR doesn't change what
CompareSchemasdoes with a schema that really is on a loop: its$refchange is still dropped by thereturn nilin those branches. The PR for #663 changes that to keep the recorded change. I kept it out of this PR so the two don't edit the same lines.Tests (each fails on
main):TestCheckSchemaProxyForCircularRefs_JourneyCheck_LeadIn: a schema that only leads into a loop.TestCheckSchemaProxyForCircularRefs_JourneyCheck_DepthLimit: the loop point only ends the journey.TestCheckSchemaProxyForCircularRefs_ResolvesReference: references and child proxies resolved against absolute journey definitions.TestCheckSchemaProxyForCircularRefs_OnlyLoopMembers: on a real document.TestCompareSchemas_RefToInline_UnrelatedCircularReference: the $ref changes are dropped when a document has any circular reference #662 repro.TestCompareOpenAPIDocuments_UnrelatedCircularReference: inline change with quick hash, in a document with two loops through the same schema.TestCompareOpenAPIDocuments_CrossFileCircularReference:a.yaml<->b.yamlwith quick hash, new fixtures intest_specs/circular_cross_file.circ_check.gois at 100% coverage. The full suite passes apart fromFuzzAppendJSONString, which also fails onmainwith Go 1.27. openapi-changes' tests pass against this branch.Stripe's
spec3.json(it has circular references), compared with quick hash on:mainmain, quick hash offWithout a file path, results now match quick hash off: identical on three of five version pairs, and on the other two the only difference is 1 and 47 extra non-breaking
$refentries, which come from the quick hash itself. When the document is loaded withSpecFilePathset, as openapi-changes does, quick hash reports a change behind an identical$refat each place it's referenced. Documents without cycles already work this way onmain, and Stripe now does too: 7,799 changes for the first pair, and about 1 s and 1.3 GB instead of 0.26 s and 320 MB. Whether that per-reference reporting is wanted is a separate question, so I didn't change it here.Related: #663, which needs this to work in documents that contain a cycle.