-
-
Notifications
You must be signed in to change notification settings - Fork 99
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
refactor: handle circular references in schemas #591
refactor: handle circular references in schemas #591
Conversation
b6cc0c9
to
cfea5d7
Compare
if (pathOfData.startsWith(refPath)) { // starts by given path | ||
return retrieveDeepData(spec, splitPath(refPath)) || data; | ||
} else if (pathOfData.includes(refPath)) { // circular path in substring of path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong but given:
pathOfData
= /components/channels/test
refPath
= /channels/test
Would it match (pathOfData.includes(refPath)
when it shouldn't?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string.includes
in JS can be used to check if a string has a substring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. What I want to raise here is that '/components/channels/test'.includes('/channels/test')
returns true
but I think it is not an expected behavior here since both are completely different paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Circular references you can only have with Schema Objects, so '/components/channels/test'.includes('/channels/test')
is not a good example.
You can create circular reference only when you point to the schema which is already used in parent path(s), so schema like:
type: object
properties:
deepProperty:
type: object
properties:
circular:
$ref: '#/properties/deepProperty'
is circular in properties/deepProperty/properties/circular
path. That case is handled by first if
in mentioned function
Another case is related to Spectral and how it resolves circular references. We can have two two schemas, main one and external (from file):
type: object
properties:
fileProperty:
$ref: './fromfile.yaml'
# fromfile.yaml
type: object
properties:
deepProperty:
type: object
properties:
circular:
$ref: '#/properties/deepProperty'
but in this case the circular reference after dereferencing has $ref
field still with '#/properties/deepProperty'
value, so it means that if we will check path we will have '#/properties/deepProperty'
from file pov, not from main file pov, like '#/properties/fileProperty/properties/deepProperty'
and due to this fact we need to also check (by second if) the subpath. I hope it's clear now. Above logic is dictated by Spectral "problems".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I fixed conflicts. @smoya Do you have any other concerns?
cfea5d7
to
22265ce
Compare
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I think we should create a new issue for keep working on a final solution regarding handling circular references; could be doing work on Spectral side, json-schema-ref-parser or whatever.
/rtm |
Description
Handle circular references, which are not handled by Spectral internal ref-resolver. The solution in PR doesn't support the really strange cases related with circular references, but for the moment it is sufficient. When we release the first alpha/beta of the new API, then I will try to support all the cases with custom ref-resolver.
Also, that PR move some utils from
./utils
file to the new./document
to prevent circular references between TS files - sometimes we need utils from./utils
file in model file, and due to fact that we have reference to the AsyncAPIDocument model, we can case with circular ref between TS files - example withSchema
model andMessageTrait
.cc @smoya
Related issue(s)
Part of #481
Part of #482