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

Super deep references found but not resolved #177

Open
kenisteward opened this issue Feb 28, 2020 · 13 comments
Open

Super deep references found but not resolved #177

kenisteward opened this issue Feb 28, 2020 · 13 comments

Comments

@kenisteward
Copy link

kenisteward commented Feb 28, 2020

I have 2 documents

doc one
address field has $ref that points to common.json#definitions address

doc 2
has a list of definitions and those definitions can refer to each other or other documents

definitions address has state field that has a $ref that points to a local definition state
local definition state has a $ref that points to a server defined enum of states

Currently, all of the refs are properly found and are in the details.refs list. when looking at details.resolved for the base doc address.state points to the unresolved definition (that has a $ref in it) instead of the properly gotten data from the server. Again the schema from the server is in details.refs however was never replaced during the setvalue process.

It is not a circular reference.

If more details are required i can try to create a bare bones schema that has our stuff ripped out that causes the issue. Right now after res.resolve I'm checking to see if there are any "unset" references and setting them. The expectation is that they would already be set.

graphical representation

doc1.addres.$ref => doc2.definitions.commonAddress
doc2.definitions.commonAddress.state.$ref => doc2.definitions.commonState
doc2.definitions.commonState.$ref => serverResolvedSchemaWithEnumerationOfStates

All values are in their respective refs. The server state is not properly in place in the resolved doc1.

EDIT: If you take out the commonState in the equation, the refs are resolved as expected. It is only when commonState is introduced in this process that commonState doesn't get resolved in the chain. this makes me assume that it only resolves 2 level's deep.
I also have other fields directly pointing to commonState and they resolve just fine. It is only in this case that it doesn't resolve to the expected server values.
thanks in advance

@whitlockjc
Copy link
Owner

Which version of json-refs are you using? Can you give me a real reproduction recipe so that I can debug locally?

@kenisteward
Copy link
Author

@whitlockjc sure thing.

Using version 3.0.13 and I'll get you a stripped down version of the schema that still bugs it.

@kenisteward
Copy link
Author

kenisteward commented Mar 9, 2020

@whitlockjc i finally got some time to sit down and try to replicate with as little data as possible the issue . You can find and debug in this stackblitz

https://stackblitz.com/edit/angular-2loodr?file=src%2Fapp%2Fapp.component.ts

It looks like some how order matters for how you define the references. If you look in the resolved object the addresses state code and country code don't get resolved when the address in the base is last on the list.

If you put it first everything is resolved as expected. This gets worse if there are more references in between.

In the stack blitz all of the connected schemas are there. It is important to do them as all external as that is the only way I'm able to get the bug to come up. You can fork the stackblitz if you wanna put the json-refs code in instead of the minified code so you can debug better.

@kenisteward
Copy link
Author

@whitlockjc any news?

@kenisteward
Copy link
Author

@whitlockjc As a workaround, I've set up a recursive function that checks to see if there are any $refs that still exist int he r.resolved object (that should not have any). If it exists I simply call resolveRefs on the resolved object and do the check again. So far, only two calls have been required. This has worked thus far as well.

@kenisteward
Copy link
Author

@whitlockjc have you per chance had a second to take a look here?

@whitlockjc
Copy link
Owner

@kenisteward Sorry about that, I've not done much OSS lately. But I did look into this and I think it's because the unresolved JSON References are invalid. According to the JSON References specification, any properties alongside the $ref are disallowed (but handling this is tooling specific) and we will not resolve them. If you remote the title properties alongside the $refs, they should resolve.

Give that a shot and let me know. If this is the case, we may need to figure out how we want to handle cases like this. For example, what should we do with title when we replace it's surrounding object with the value of $ref?

@whitlockjc
Copy link
Owner

I've verified this is indeed the case by using the json-refs CLI (json-refs resolve -w will treat warnings as errors. Otherwise, warnings are ignored but resolution still doesn't occur):

json-refs resolve test.json -w

  error: Document has invalid references:

  #/properties/Inner1: Extra JSON Reference properties will be ignored: title
  #/properties/inner2: Extra JSON Reference properties will be ignored: title
  #/properties/sub/properties/countryCode: Extra JSON Reference properties will be ignored: title
  #/properties/sub/properties/stateCode: Extra JSON Reference properties will be ignored: title

@kenisteward
Copy link
Author

@whitlockjc thanks for this explanation.

I had assumed that it would still resolve them and just use the title given since it's not a validation field.

Give that a shot and let me know. If this is the case, we may need to figure out how we want to handle cases like this. For example, what should we do with title when we replace it's surrounding object with the value of $ref?

What I've done in other frameworks when I contributed to them is set stitle/description/comments as they are in the schema implementing the $ref so that those all stay as overwritten values. Based on the spec those fields are meant to help hydrate UI's anyway so this seemed logical to me.

@whitlockjc
Copy link
Owner

How would you like this to work? I'm all ears on making this better, and more intuitive.

@kenisteward
Copy link
Author

I'm currently just using our work around until we hit production but I'd love to see if I could find a viable solution you can agree to that fits the lib. :)

@whitlockjc
Copy link
Owner

I agree. I was thinking of a flag, like options.resolveRefsWithProperties that has a couple different options:

  • false (default): Do not replace the values (this is the current mode)
  • true: Resolves as expected, replacing the surrounding object containing the $ref with the resolved value

We could get more complex, like merging whenever the resolved value is an object but that would need to be a feature request because it can get quite problematic quickly. (For example, the depth at which you resolve, handling conflicts, etc.)

@kenisteward
Copy link
Author

@whitlockjc half a year later and we are in production with my current work around. I can finally get some time to contribute a change here :)

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