json-refs icon indicating copy to clipboard operation
json-refs copied to clipboard

Resolving Recursive References

Open dillonredding opened this issue 4 years ago • 13 comments

This might already be a known issue, but I haven't found anything exact. There seems to be an issue resolving external references that are recursive. Here's an example (a bit contrived, but narrowed down to the essentials):

foo.yaml:

Foo:
  type: object
  properties:
    bar:
      type: object
      properties:
        foo:
          $ref: '#/Bar'

bar.yaml:

Bar:
  type: object
  properties:
    foo:
      $ref: '#/Bar'

But when resolving through the CLI with json-refs resolve foo.yaml, I get a non-existent reference:

Foo:
  type: object
  properties:
    bar:
      type: object
      properties:
        foo:
          $ref: '#/Bar'

This might be expected behavior (I'm not well versed in the JSON Reference specification); however, one workaround that comes to mind would be to update the reference to where it's actually resolved:

Foo:
  type: object
  properties:
    bar:
      type: object
      properties:
        foo:
          $ref: '#/Foo/properties/bar'

Version of json-refs: 3.0.13

dillonredding avatar Jan 20 '20 21:01 dillonredding

This is intended behavior, and is documented here: https://github.com/whitlockjc/json-refs/tree/master/docs#resolution Long story short, circular references are a problem for many tools and so we will leave circular references as-is in the provided document. There is some grey area here, as documented. If this explains things, please feel free to close the issue.

whitlockjc avatar Jan 20 '20 22:01 whitlockjc

I think this behavior is fine for local circular references. The issue is when there's a local circular reference in an externally referenced document. As seen in my example, the reference from the external document is copied over and now references nothing. I would think the value of the $ref could be updated to point to the where it is in the file being resolved.

dillonredding avatar Jan 21 '20 16:01 dillonredding

Reading back over the example I gave, I preemptively resolved the reference in foo.yaml. That should be:

Foo:
  type: object
  properties:
    bar:
      $ref: bar.yaml#/Bar

dillonredding avatar Jan 21 '20 19:01 dillonredding

That's how it should work. Maybe we're not doing that and this would in fact be a bug. I'll get this fixed ASAP!

whitlockjc avatar Jan 23 '20 03:01 whitlockjc

Just curious, is there an ETA on this?

dillonredding avatar Feb 15 '20 21:02 dillonredding

I'll get it out today. Sorry, priorities didn't let me get to it sooner.

whitlockjc avatar Feb 24 '20 17:02 whitlockjc

I have this fixed locally, but I need to upgrade gulp so that TravisCI passes. My plan is to update all dependencies, and gulp, commit and once the build is passing, I'll cut the release.

Good news is that while working on this, I found two other circular-reference resolution bugs and fixed them too.

whitlockjc avatar Feb 25 '20 06:02 whitlockjc

I just went ahead and cut a release. [email protected] fixes this issue.

whitlockjc avatar Feb 25 '20 07:02 whitlockjc

@dillonredding Do you mind verifying this implemented what you were requesting?

whitlockjc avatar Feb 25 '20 19:02 whitlockjc

Not quite. Here's what I get:

foo.yaml:

Foo:
  type: object
  properties:
    bar:
      $ref: 'bar.yaml#/Bar'

bar.yaml:

Bar:
  type: object
  properties:
    foo:
      $ref: '#/Bar'

Command:

json-refs resolve foo.yaml

Expected Output:

Foo:
  type: object
  properties: 
    bar:      
      type: object
      properties:
        foo:
          $ref: '#/Foo/properties/bar'

Actual Output:

Foo:
  type: object
  properties: 
    bar:      
      type: object
      properties:
        foo:
          $ref: '\bar.yaml#/Bar'

dillonredding avatar Feb 25 '20 19:02 dillonredding

From a consistency perspective, what was implemented makes sense. The idea is that when you have a circular reference, we leave it as-is and let whatever tooling is there to resolve it. The bug you reported is that for locally-circular references in nested documents, doing that (keeping the local reference as-is) meant that the resolved value was left in a state where it's unresolvable in the resolved document. That problem is fixed now, as locally-circular references in nested documents are fully-qualified when injected into the resolved document, allowing the resolved document to reference the documents where the circular chain was introduced.

That being said, I almost took a path similar to what you are wanting back in the day and was worried it was too much magic. Manipulating $ref values wasn't something I wanted to do...I either wanted to replace/resolve them or leave them as-is. But I guess now, this new approach, does in fact manipulate $ref values by making them full-qualified in the aforementioned case. (Good news though, the reference metadata retains the original definition as expected.)

I would definitely say that in one way, maybe not exactly as you wanted, the original description of the issue is fixed. But let me give this some thought, to see if something jumps out at me in the circular processing that is making this behave different than intended.

whitlockjc avatar Feb 25 '20 20:02 whitlockjc

Yeah, what I'm asking for might be pretty specific. Specifically, I'm wanting to remove the references outside the file and have it be completely self-contained. I have no issues with manipulating $refs as long as the file remains "equivalent", but I might not be considering every scenario.

dillonredding avatar Feb 27 '20 23:02 dillonredding

I get what you mean, and it might be easier than I thought. I'm going to reopen this to avoid it falling through the cracks.

whitlockjc avatar Mar 05 '20 19:03 whitlockjc