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

Remove external ref resolving #1750

Closed
sdirix opened this issue May 10, 2021 · 3 comments · Fixed by #1762 or #1829
Closed

Remove external ref resolving #1750

sdirix opened this issue May 10, 2021 · 3 comments · Fixed by #1762 or #1829
Assignees
Milestone

Comments

@sdirix
Copy link
Member

sdirix commented May 10, 2021

Overview

Refs (i.e. $ref) are a core part of the JSON Schema specification. At the moment JSON Forms tackles it with different concepts.

  • UI Schema scopes (which are defined as JSON pointers) are manually resolved against the JSON Schema
  • When the resolving ends in a $ref, JSON Forms manually resolves the $ref

But JSON Forms also supports ref-resolving via json-schema-ref-parser. As a concept it is implemented in @jsonforms/core, however only the React renderers are actually using it.

The implemented functionality is:

  • The React Dispatch renderer checks whether there are any $refs in its schema. This is checked via copied code from json-refs which adds the dependencies uri-js and uuid to @jsonforms-core.
  • If there are any, the json-schema-ref-parser is tasked with resolving all of them. The json-schema-ref-parser can be configured via options which allows configuring resolving even to external paths, e.g. https://whatever.com/example.json. The resulting JSON Schema is then actually used.

Problems

  • The check for $refs in the dispatch renderer is relatively slow.
  • json-schema-ref-parser is really slow, and especially so for circular JSON Schemas. Also it's run unconditionally in React, even when the integrated manually resolving would be enough.
  • We have three additional dependencies for this in the @jsonforms/core although only one binding is actually using it.
  • The dependencies of json-schema-ref-parser are actually Node dependencies which can't be run in the Browser. Any application using JSON Forms therefore also needs to add polyfills to actually run it. Up until Webpack 4 this happened automatically. With Webpack 5 this was removed, leading to build errors when using JSON Forms. It doesn't make a good impression when using JSON Forms requires Node polyfills.

Overall there is hardly any benefit to all of this. Yes, we allow resolving external refs, however as the actual resolving method has to be handed in anyway AND it is run unconditionally in most cases, the respective developer could just resolve them beforehand and then hand them in to JSON Forms. This is actually what we recommend for example for the Angular renderers and demonstrate in the Angular seed.

Downsides of removing ref resolving

Potential downsides when removing this without replacement:

  • There could be cases where the implemented manually resolving was aided by json-schema-ref-parser, i.e. handed in JSON Schemas were prepared in a way which allowed the manually resolving to actually work. This worry is mitigated a bit by the Angular renderers as they never used this resolving. Still the React renderers support more complicated schemas, e.g. oneOf etc.
  • Some users requested this feature, so they will certainly rely on it. Their setups will break and they'll need to resolve their schemas before handing them over to JSON Forms.

Mitigation idea

To still allow external refs to be resolved in general, we could offer a resolving customization method. If the user hands in such a function, it will be called whenever something needs to be resolved. The user can then implement arbitrary behavior here, including resolving to external paths.

Tl;dr

The current external ref resolving approach using json-schema-ref-parser has too many downsides to keep it like it is in the current form. Removing it will break some users, but they should be mostly fine by resolving the schemas themselves before handing it to JSON Forms. We can offer a customization method which would allow for all previously supported use cases (and many more) but this requires work by the user.

@sdirix sdirix added this to the 3.0 milestone May 10, 2021
@sdirix
Copy link
Member Author

sdirix commented Jun 15, 2021

Additional requirements for a release

Adapt migration guide

Add a small motivation why we performed this change, with a link to this issue.
Mention that there is no difference for Angular and Vue users.
Most React users should also not be affected.
Problematic cases are mostly complex $ref setups, often in combination with anyOf, allOf, oneOf.
The old behavior can be restored by resolving the references before handing the schema over to JSON Forms.
Add an example how to resolve the schema with json-schema-ref-parser and one with json-refs.

Website

We should introduce versioning in our Website for the 3.0 release.
Move current content to an (unlinked) ref-resolving-legacy page and add an info box on the new ref-resolving.
Explain in the ref-resolving section that JSON Forms only supports basic ref resolving out of the box and that complex cases might fail.
Showcase a complex example, you can use anyOf-oneOf-allOf-resolve as a basis.
However if that is a use case, it can be supported by resolving the schema before handing it over to JSON Forms.
Explain in same or more detail as in the migration guide how schemas can be resolved before handing them over to JSON Forms.

@sdirix sdirix linked a pull request Jun 16, 2021 that will close this issue
@sdirix
Copy link
Member Author

sdirix commented Jun 16, 2021

PR #1762 removes json-schema-ref-parser from JSON Forms. However to fully close this issue we need to look into the crashing examples and make the resolving code more robust.

@sdirix
Copy link
Member Author

sdirix commented May 6, 2022

Completed with #1829

@sdirix sdirix closed this as completed May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants