Skip to content

Fix idSchema issue with dependencies#1005

Merged
glasserc merged 1 commit intorjsf-team:masterfrom
jaminthorns:fix/id-schema
Oct 5, 2018
Merged

Fix idSchema issue with dependencies#1005
glasserc merged 1 commit intorjsf-team:masterfrom
jaminthorns:fix/id-schema

Conversation

@jaminthorns
Copy link
Copy Markdown
Contributor

@jaminthorns jaminthorns commented Aug 16, 2018

Reasons for making this change

This fixes an issue where an idSchema would not be generated for property fields of objects that are in a dependency. It also adds a test to validate the fix.

This PR actually fixes the same issue as a part of its changes (here), but it looks like it's been sitting idle for the past month, and I'd like to get this fix in. My PR touches much less code, so I figure it should be easy to get this through.

The change in UnsupportedField.js is just because I ran the cs-format script. I suppose it was missed before, and I figured I'd commit it.

Checklist

  • I'm updating documentation
    • I've checked the rendering of the Markdown text I've added
    • If I'm adding a new section, I've updated the Table of Content
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@glasserc
Copy link
Copy Markdown
Contributor

Thanks for opening the PR. The changes seem straightforward to me, but I get the sense that there's a larger problem. It seems like all calls to toIdSchema are called on schemas that have already been "retrieved" using retrieveSchema, so I would have naïvely thought that the schema in the test would "work" (i.e. without this change, toIdSchema(retrieveSchema(...), ...) would provide the same thing as the new code after this change). Is there a user-facing bug that motivates this change? I don't object to the change in principle, but I feel like I don't have a good sense of where it's coming from.

@jaminthorns
Copy link
Copy Markdown
Contributor Author

Looking through the code base, you're mostly right about the usage of retrieveSchema and toIdSchema. toIdSchema is used in the Form, SchemaField, and ArrayField components, each time being passed a schema that's been run through retrieveSchema, but it's also being called within itself recursively.

The key thing to note is that retrieveSchema does not "resolve" schema deeply. It will resolve multiple levels of dependencies and $refs with recursive calls, but it does not actually dig into the structure of schema (the sub-schemas of children objects and arrays). On the other hand, toIdSchema does navigate schema deeply through recursion, and because the sub-schemas wouldn't have had retrieveSchema called on them, that's why it needs to (possibly) call retrieveSchema before doing any work.

Now, at the time toIdSchema was added (in this PR), dependencies weren't even supported in react-jsonschema-form, so there was no check for them in that first if statement. The fact that the "dependencies" in schema wasn't added to toIdSchema when support for dependencies were later added for react-jsonschema-form as a whole was an oversight (and what this PR addresses).

You're correct about the test working if I had done toIdSchema(retrieveSchema(...), ...), but the test is accounting for all calls to toIdSchema, those from the components mentioned above and from recursive calls from within toIdSchema itself.

As far as user-facing bugs, this change was motived by one that I encountered while using the top-level onBlur handler (which returns a field's id) to keep track of which fields have been blurred. Because dependencies weren't being resolved in toIdSchema, dependent fields didn't have IDs, which led to some issues.

@glasserc glasserc merged commit 05ea3fc into rjsf-team:master Oct 5, 2018
@glasserc
Copy link
Copy Markdown
Contributor

glasserc commented Oct 5, 2018

Thanks for the explanation!

@jaminthorns jaminthorns deleted the fix/id-schema branch October 5, 2018 15:27
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

Successfully merging this pull request may close these issues.

2 participants