Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolving Recursive References #175

Open
dillonredding opened this issue Jan 20, 2020 · 13 comments
Open

Resolving Recursive References #175

dillonredding opened this issue Jan 20, 2020 · 13 comments

Comments

@dillonredding
Copy link

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

@whitlockjc
Copy link
Owner

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.

@dillonredding
Copy link
Author

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
Copy link
Author

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

@whitlockjc
Copy link
Owner

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!

@dillonredding
Copy link
Author

Just curious, is there an ETA on this?

@whitlockjc
Copy link
Owner

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

@whitlockjc
Copy link
Owner

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
Copy link
Owner

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

@whitlockjc
Copy link
Owner

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

@dillonredding
Copy link
Author

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'

@whitlockjc
Copy link
Owner

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.

@dillonredding
Copy link
Author

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.

@whitlockjc
Copy link
Owner

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 whitlockjc reopened this Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants