fix: oneOf anyOf default value always using first option #2198

QingqiShi · 2021-01-18T14:39:54Z

Reasons for making this change

When generating default values, currently we're always assuming the first option is matched. This default value will then be merged with existing data - so if you have the second option selected, you can end up with both values from the first and the second options.

#2183

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
I'm adding a new feature
- I've updated the playground with an example use of the feature

Canas · 2021-02-11T19:25:04Z

Whatever happened to this fix? This is still happening in the playground

QingqiShi · 2021-02-11T19:30:41Z

Still waiting for a review and merge.

exevil · 2021-03-23T10:42:29Z

+1 here. Would be great to merge this.

epicfaace

Can you add a test that specifically covers this playground example (from #2183)? Meaning, the case in which neither option has a default, and when you switch from option 1 to option 2.

chenghu3 · 2021-04-01T19:58:48Z

I think @QingqiShi addressed the comment, could we pls review and merge this? thanks

QingqiShi · 2021-04-02T12:38:35Z

Yes indeed. I added a test to cover the example from #2183

tappleby · 2021-04-06T21:38:36Z

Just been tracking down some issues with an upgrade and found this fix. I noticed that for any schemas that make use of refs, this fix may also depend on #2272

QingqiShi · 2021-04-20T19:57:48Z

Need some help with this one. Build is failing after merging with master. It seems to be an unrelated test that's failing.

epicfaace · 2021-04-21T02:12:56Z

@QingqiShi looks like this is the line that is failing. Maybe you could try debugging the test by printing out onChange and onChange.lastCall? I'm wondering if perhaps onChange isn't being called properly with the changes in this PR.

QingqiShi · 2021-04-21T19:47:33Z

I realised that after the fix it no longer generate formData if you haven't selected any of the options in a oneOf or anyOf yet.

To make this fix backwards compatible, I've added a check to see if formData is an empty object, if it is then continue to pass undefined to getMatchingOption when generating defaults. This maintains the current behaviour before the user selects an option, and will start using formData after the user selects an option.

@epicfaace tests fixed, please instruct on how to proceed.

epicfaace

Thanks for your changes! A few more comments.

packages/core/src/utils.js

epicfaace · 2021-04-21T20:23:34Z

packages/core/src/utils.js

-      schema.anyOf[getMatchingOption(undefined, schema.anyOf, rootSchema)];
+      schema.anyOf[
+        getMatchingOption(
+          JSON.stringify(formData) !== "{}" ? formData : undefined,


Also, should we move this logic out of this file and directly inside the getMatchingOption function instead?

Finally, getMatchingOption is also used a few times in MultiSchemaField.js -- should we apply this fix there as well, and if we do, perhaps we could add a test that covers that change as well?

I'm a bit scared to widen this change.

I know that getMatchingOption is used for a couple of different purposes, such as generating defaults (as we're doing here), and for loading the initial selected multi-schema options (as you mentioned in MultiSchemaField), we also use it in resolveDependencies.

These use cases have subtle differences - in the computeDefault function, formData gets defaulted to {} (for reasons I don't understand), and that's why we need this check here so it goes back to generating defaults. Whereas in other places it may be necessary to keep the empty object as the object type may be matched to a particular sub-schema.

If I may, I will restrain myself from potentially breaking the world. I'll leave it to your judgement as you are the expert on this.

@epicfaace is this a blocking comment? It would be great to get this pr merged.

cc @nickgros (I see you on an active pr)

Stankk · 2021-12-16T21:01:30Z

Hello, any news on the merge ?

Akifhan · 2022-03-24T08:18:49Z

Hello, any updates on the merge?

rjsf-team#2198

davidpett · 2022-06-16T19:49:43Z

Can we get this merged, please?

rjsf-team#2198

eliyaoo32 · 2022-08-11T12:36:12Z

Hi, are they plans to merge this?

heath-freenome · 2022-09-07T18:12:35Z

@QingqiShi Do you have the bandwidth to refactor this code into the new @rjsf/utils package in the v5 beta?

- The fix in rjsf-team#2198 needed to be refactored into the new `@rjsf/utils` package - Replicated the fix into the `getDefaultFormState.ts` file - Replicated the additional tests into `getDefaultFormStateTest.ts` and `oneOf_test.js` - Updated the `CHANGELOG.md` with the note for this fix along with the notes for PR rjsf-team#3094

* Reimplemented fix in #2198 in @rjsf/utils package - The fix in #2198 needed to be refactored into the new `@rjsf/utils` package - Replicated the fix into the `getDefaultFormState.ts` file - Replicated the additional tests into `getDefaultFormStateTest.ts` and `oneOf_test.js` - Updated the `CHANGELOG.md` with the note for this fix along with the notes for PR #3094 * - Updated `CHANGELOG.md` for PR #2925 * - Fixed up `CHANGELOG.md` to move note from #3049 into the proper location

heath-freenome · 2022-09-08T21:54:40Z

@QingqiShi closing as I took your excellent work and refactored it into the new @rjsf/utils library code in #3099

QingqiShi · 2022-09-08T21:58:21Z

Thank you very much ❤️ @heath-freenome

epicfaace reviewed Mar 23, 2021

View reviewed changes

epicfaace self-assigned this Mar 23, 2021

QingqiShi force-pushed the fix/multi-schema-default branch from 11a9101 to b2eb250 Compare March 23, 2021 17:10

fix: oneOf anyOf default value always using first option

8d8c915

QingqiShi force-pushed the fix/multi-schema-default branch from b2eb250 to 8d8c915 Compare March 23, 2021 17:12

QingqiShi requested a review from epicfaace March 23, 2021 17:13

Merge branch 'master' into fix/multi-schema-default

755bb12

Merge branch 'master' into fix/multi-schema-default

5e52ae8

epicfaace assigned QingqiShi and unassigned epicfaace Apr 21, 2021

fix: use undefined when formData is empty object

14e75b1

epicfaace reviewed Apr 21, 2021

View reviewed changes

QingqiShi added 2 commits April 21, 2021 22:29

Use lodash isEmpty instead of JSON.stringify

2536be0

Fix boo boo

440132e

ubik2 added a commit to ubik2/react-jsonschema-form that referenced this pull request Jun 5, 2022

Fix type selection for oneOf and anyOf

32349fd

rjsf-team#2198

davidpett added a commit to davidpett/react-jsonschema-form that referenced this pull request Jun 17, 2022

update utils.js

c448c26

rjsf-team#2198

heath-freenome added the p1 Important to fix soon label Aug 26, 2022

heath-freenome added awaiting changes v5 refactor Needs refactor due to v5 breaking changes labels Sep 7, 2022

heath-freenome mentioned this pull request Sep 8, 2022

Reimplemented fix in #2198 in @rjsf/utils package #3099

Merged

8 tasks

heath-freenome closed this Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: oneOf anyOf default value always using first option #2198

fix: oneOf anyOf default value always using first option #2198

QingqiShi commented Jan 18, 2021

Canas commented Feb 11, 2021

QingqiShi commented Feb 11, 2021

exevil commented Mar 23, 2021

epicfaace left a comment

chenghu3 commented Apr 1, 2021

QingqiShi commented Apr 2, 2021

tappleby commented Apr 6, 2021

QingqiShi commented Apr 20, 2021

epicfaace commented Apr 21, 2021

QingqiShi commented Apr 21, 2021

epicfaace left a comment

epicfaace Apr 21, 2021

QingqiShi Apr 21, 2021

QingqiShi Apr 21, 2021

logancool Aug 26, 2022

logancool Aug 30, 2022

Stankk commented Dec 16, 2021

Akifhan commented Mar 24, 2022

davidpett commented Jun 16, 2022

eliyaoo32 commented Aug 11, 2022

heath-freenome commented Sep 7, 2022

heath-freenome commented Sep 8, 2022

QingqiShi commented Sep 8, 2022

fix: oneOf anyOf default value always using first option #2198

fix: oneOf anyOf default value always using first option #2198

Conversation

QingqiShi commented Jan 18, 2021

Reasons for making this change

Checklist

Canas commented Feb 11, 2021

QingqiShi commented Feb 11, 2021

exevil commented Mar 23, 2021

epicfaace left a comment

Choose a reason for hiding this comment

chenghu3 commented Apr 1, 2021

QingqiShi commented Apr 2, 2021

tappleby commented Apr 6, 2021

QingqiShi commented Apr 20, 2021

epicfaace commented Apr 21, 2021

QingqiShi commented Apr 21, 2021

epicfaace left a comment

Choose a reason for hiding this comment

epicfaace Apr 21, 2021

Choose a reason for hiding this comment

QingqiShi Apr 21, 2021

Choose a reason for hiding this comment

QingqiShi Apr 21, 2021

Choose a reason for hiding this comment

logancool Aug 26, 2022

Choose a reason for hiding this comment

logancool Aug 30, 2022

Choose a reason for hiding this comment

Stankk commented Dec 16, 2021

Akifhan commented Mar 24, 2022

davidpett commented Jun 16, 2022

eliyaoo32 commented Aug 11, 2022

heath-freenome commented Sep 7, 2022

heath-freenome commented Sep 8, 2022

QingqiShi commented Sep 8, 2022