Circular refs are hoisted when using: `dereference.circular="ignore"`

[RomanHotsiy]

If I use dereference with dereference.circular option set to "ignore" circular refs are hoisted to the first usage. For example, given the following input

A:
  props:
    prop:
      $ref: "#/A"
B:
  $ref: "#/A"

I receive the following result:

A:
  props:
    prop:
      $ref: "#/A"
B:
  $ref: "#/A" # <-- B here is not replaced by the ref, it is hoisted

While I would expect the following result:

A:
  props:
    prop:
      $ref: "#/A"
B:
  props:
    prop:
      $ref: "#/A"

where in memory, A and B props are the same object. The latter result makes much more sense for ReDoc use case and getting from former to latter requires reimplementing part of this lib.

I believe changing the current behaviour would be a breaking change. So, @JamesMessinger, what do you think of introducing a new value for circular option "ignore-nohoist"?

[JamesMessinger]

Actually... I think the expected behavior that you described should be the default. No need for a new option. Just need to fix the circular-dereferencing logic.

[RomanHotsiy]

I've spent some time trying to fix it and faced a few issues I'm not sure what to do about.

Here are my findings:

The current implementation of dereference$Ref can be simplified for our needs as:

1. resolve $ref
2. crawl resolved value
3. if the resolved value was circular and ignore=true return the ref unchanged, else return value

2 and 3 are the root causes of the hoisting issue: because we are crawling the value deep before circular check we hoist circular $ref to the start of $ref chain.

To fix the issue steps 2 and 3 have to be reordered:

1. resolve ref
2. if the resolved value was circular and ignore=true return the ref unchanged
3. else return crawl(value)

I swapped the order and can verify the issue is fixed but there another issue arised: in some cases when using ignore=true the resulting object is circular.

The cause of this is:

https://github.com/APIDevTools/json-schema-ref-parser/blob/9db2e2b6eef765ce5235e341a6bfaf2187bd9b14/lib/dereference.js#L107

If the ref is an "extended ref" the underlying value gets replaced by a new object with merged extra props, and this object is not in the parents stack. This causes circularity detection miss. This was not an issue with the original implementation as circularity was detected on the inner object and $ref was hoisted anyway.

@JamesMessinger, if you managed to understand my mind-dump, do you have any ideas? You input would be appreciated here 🙌!

[JamesMessinger]

Copying my DM'ed response here...

Just read your comments. Not sure how to fix it off-hand. Will require some experimenting.

One option might be to drop support for "extended refs", since it's not spec-compliant (neither OpenAPI nor JSON Schema allows $refs to contain additional properties)

But if there's a way to avoid dropping support, I'd prefer it, because I know that many people rely on the "extended refs" behavior.

[goldingdamien]

What happened to this issue? Did the commit by @wparad that failed checks fix this issue completely? It seems the failed parts of the checks are minor. Regards.

[GlennButera]

Is there a timeline at all to merge in the above change or for another solution to this issue? A "best effort" style circular reference algorithm for dereferencing would be greatly appreciated for many use cases with JSON Schemas that have circular references nested inside of them.

[wparad]

@GlennButera I don't know if our fork would what you are looking for, but after waiting much too long for this PR to be merged, we went straight for a fork. If that's interesting, it is over at openapi-resolver.

[jonluca]

This library was semi maintained for a while and communication was fairly slow, but it should be better now. We're open for PRs and will respond promptly!