Hang when expanding circular $ref #79
Conversation
| @@ -224,19 +235,23 @@ func init() { | |||
| func defaultSchemaLoader( | |||
| root interface{}, | |||
| expandOptions *ExpandOptions, | |||
| cache ResolutionCache) (*schemaLoader, error) { | |||
| cache ResolutionCache, | |||
| context *resolverContext) (*schemaLoader, error) { | |||
There was a problem hiding this comment.
defaultSchemaLoader - result 1 (error) is always nil
Codecov Report
@@ Coverage Diff @@
## master #79 +/- ##
==========================================
+ Coverage 40.27% 40.96% +0.69%
==========================================
Files 18 19 +1
Lines 1959 1982 +23
==========================================
+ Hits 789 812 +23
Misses 995 995
Partials 175 175
Continue to review full report at Codecov.
|
* Fixes go-openapi#76 (absolute path left behind in resolved circular $ref) * Fixes go-openapi#74 (isCircular no more checks on basePath!="") * Contributes go-openapi#75 (hang is solved, but the (always correct) expanded result may differ from one run to another) * Contributes go-swagger/go-swagger#957 (hang on circular spec) No other way to pass golangci than disabling some linters
|
This looks good to me, I am going to check out your changes and test this against some problematic specs we run into at Stoplight. |
|
How about the unstable aspect of this? I mean, if you expand to validate, this is okay (all cycles are expanded in a somewhat correct way), but if you want, say to get documentation, that's no good. Is that okay with you? |
|
For our use case at Stoplight and Prism, that is fine. We are expanding the spec to do contract testing of request/responses. It would take a bigger refactor to make it stable imo, and it is just nice that it doesn't hang anymore. |
|
We have a problem with that one. When I attempted to integrate with go-swagger, I could see that this breaks validate. Validate expands JSON-schema meta schema and this is broken. |
|
I reverted this one |
| return ref | ||
| } | ||
| // strip relativeBase from URI | ||
| r, _ := NewRef(strings.TrimPrefix(ref.String(), relativeBase)) |
There was a problem hiding this comment.
This line is causing the issue in the validation test. It is creating an invalid JSON Ref, if I change it to this:
// strip relativeBase from URI
relativeBaseURL, _ := url.Parse(relativeBase)
relativeBaseURL.Fragment = ""
r, _ := NewRef(strings.TrimPrefix(ref.String(), relativeBaseURL.String()))
Then all of the test in the validate package pass except one. It should be really easy to fix, I have to run, but will try to get to it later if someone else hasn't.
|
I am fixing this |
|
@pytlesk4 . Oh just realized you push a fix yourself. I am trying to test it with complete go-swagger integration then |
This PR is not fully satisfying in the sense that it does not fix once and for all the
circular ref story. It improves spec expansion which no more hangs on some complex
circular ref patterns. However, it is not able to produce stable expanded schemas when it
used to hang.
All produced expansions are correct, but may vary in the way they expand cycles,
when several cycles do collide. Since we walk the spec at random and this spec is mutating
during the expansion process, we stand no chance to come out with a clean resolution
(i.e. resolve shortest cycles first)
What it does:
Even though you chose not to merge this, and maybe rightly so, there are some advances with
this problem, like a minimal reproducible test case.
Cheers,
Fred