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

fix: swapping out the $ref parsing engine of flattenSchema #292

Merged
merged 2 commits into from
Oct 20, 2020

Conversation

erunion
Copy link
Member

@erunion erunion commented Oct 20, 2020

🧰 What's being changed?

This completely swaps out the $ref parsing engine behind flattenSchema with json-schema-ref-parser to allow us to properly, easily, handle circular references without infinitely recusing them.

How it works is we'll funnel an incoming schema and API definition into json-schema-ref-parser to dereference it but allowing it to ignore circular refs. If a $ref is circular, it'll remain in the schema as $ref, enabling us to quickly detect and ignore circular refs when flattening a schema.

With this change, flattenSchema is now an async function.

@erunion erunion added bug Something isn't working dependencies Pull requests that update a dependency file refactor Issues about tackling technical debt labels Oct 20, 2020
@erunion erunion requested review from domharrington and dok October 20, 2020 23:10
@dok
Copy link
Contributor

dok commented Oct 20, 2020

How does this work in conjunction with https://github.com/readmeio/react-jsonschema-form? Are they related?

I'm a little bit out of the loop on this one

@erunion
Copy link
Member Author

erunion commented Oct 20, 2020

@dok This is completely unrelated to that. This flattenSchema method is only utilized when we flatten a response schema.

Ideally we'd dereference these schemas before they're passed into the explorer from the beginning so we wouldn't need to have any $ref fetching or parsing at all, but we're ways away from there as it'll require some reconfiguration in our backend models and potentially how we're storing data.

dereference: {
// If circular `$refs` are ignored they'll remain in `derefSchema` as `$ref: String`, otherwise `$ref‘ just
// won't exist. This allows us to do easy circular reference detection.
circular: 'ignore',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the way we would detect whether a dereferenced object after this function call? Whether a $ref: String exists within the dereferenced object?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If $ref exists in derefSchema at all, then it's circular.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Are there any other times where a $ref cannot be satisfied?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's an external ref (like a URL to another spec) it'll remain unsatisfied if it's inaccessible.

That said, we shouldn't be attempting to resolve external refs at this point in the application cycle so I'm going to add resolve.external: false into the options here to prevent that as well as a test for that.

value = findSchemaDefinition(value.$ref, oas);
// If we have a $ref present then it's circular and mark the type as such so we at least have something to
// identify it as to the user.
value.type = 'circular';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the type here to this is so that in the frontend it'll show up as:

Screen Shot 2020-10-20 at 4 19 53 PM

@dok
Copy link
Contributor

dok commented Oct 20, 2020

@erunion oh I see. Thanks for the clarification. Is this easier to work with than the isCyclic functionality? It does seem to have a similar format to that (it looks recursive).

@erunion
Copy link
Member Author

erunion commented Oct 20, 2020

@dok Yeah this work could likely be built into either our form module, or when we build up JSON Schema with tooling/lib/parametersToJsonSchema to do this resolving work then.

Reworking that is all is a much larger project though and should probably be done further up the stack so we aren't processing the same $ref n times.

Copy link
Contributor

@dok dok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-approving with this in mind

@dok
Copy link
Contributor

dok commented Oct 20, 2020

@erunion looks good!

@erunion erunion merged commit 682d91d into master Oct 20, 2020
@erunion erunion deleted the fix/flatten-schema-recursion branch October 20, 2020 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file refactor Issues about tackling technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants