search

APIDevTools / json-schema-ref-parser

Parse, Resolve, and Dereference JSON Schema $ref pointers in Node and browsers
https://apitools.dev/json-schema-ref-parser
MIT License
952 stars 227 forks source link

Fix dereference recursion #277

Closed wparad closed 12 months ago

wparad commented 2 years ago

Fixes a bunch of unnecessary complexity in recursion and two issues that should be resolved.

No idea why the browser tests are failing, I thought it was bad coveralls configuration, but after fixing that, it is not. The output says success, but github action says nope.

jonluca commented 1 year ago

I don't believe the new behavior is correct.

For https://github.com/APIDevTools/json-schema-ref-parser/pull/277/files#diff-e17696fc5b8288ef1f42c8848360d8b90d660f7b3606465f458c25b0174b1e0aL66 when circular is set to ignore, it should not be re-creating a partial definition - it should keep the $ref as is

wparad commented 1 year ago

I don't believe the new behavior is correct.

For https://github.com/APIDevTools/json-schema-ref-parser/pull/277/files#diff-e17696fc5b8288ef1f42c8848360d8b90d660f7b3606465f458c25b0174b1e0aL66 when circular is set to ignore, it should not be re-creating a partial definition - it should keep the $ref as is

I think there is a lot to debate here. I'm not changing the behavior, I'm changing where the "circle" is detected. Right now it is all the way at the beginning, and as such we lose out on all the awesome data we collected. For instance if a cycle is 10 $ref deep,

0 => 1 => 2 => 3 => 4 => 5 => 6 => 7 => 8 => 9 => 0 => 1. Then the current result of the library is to show:

0 => $ref

That's terrible, it's far better to show 0 => 1 => 2 => 3 => 4 => 5 => 6 => 7 => 8 => 9 => 0 => $ref

Without this, it makes dereference next to useless, and requires anyone that wants to show valuable circular references to use bundle and build the real dereference solution themselves.

If we really don't want to change how ignore works, we can create a new property value, but that's honestly really dumb, since no one would want to use it and the "show ignore but also include all the valuable information" is so much better. There are also at least two other issues in this repo attempting to deal with this problem (although they never understood what the real problem was).

Also, the recursion is just broken. Nested and deep property overrides don't work. The caching incorrectly stores the $ref values instead of using the overrides.

I really want to merge this/improve it. How can we achieve that?

jonluca commented 12 months ago

That's very fair. I'm closing stale PRs - happy to merge in a new PR if it's rebased

We'll release it as a breaking change. We should also have an option to keep the old behavior