-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 bug where matching anyOf branch is not selected correctly #1129
Fix bug where matching anyOf branch is not selected correctly #1129
Conversation
@glasserc If you have time a review would be great! This fixes an annoying bug with my original |
target: { value: "Lorem ipsum dolor sit amet" }, | ||
}); | ||
|
||
expect($select.value).eql("1"); |
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.
Do we need a test this complicated? Can't we just render it with {idCode: "something"}
and see if the select shows the right value?
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.
This test specifically checks to see that the selected value remains as expected after the user inputs a value, which is subtly different to checking that the correct value is selected when data is already present. The other validations in the test are sanity checks to make sue the form is in the state we expect before inputting a value.
It might seem a little verbose, but the verbosity will help ensure that there are no future regressions or that the test reports false positives.
@glasserc Thanks for the detailed review 👍 . I've addressed your comments and the PR should be good to go now. |
@LucianBuzzo I'm wondering, why did you wrap the required attribute in another
|
@epicfaace Wrapping the
If you just use a plain |
This change improves the logic that selects a matching anyOf branch based on form data. Previously the behaviour was to just check if the form data was valid against an anyOf branch, however due to the permissive nature of JSON schema this has unexpected behaviour. For example, given the following schema: ```json { "type": "object", "anyOf": [ { "properties": { "foo": { "type": "string" } } }, { "properties": { "bar": { "type": "string" } } } ] } ``` The form data `{ bar: 'baz' }` will actually match the first branch. To mitigate this, when doing the matching, the branch schema is augmented to require at least one of the keys in the branch. For example the schema above would become: ```json { "type": "object", "anyOf": [ { "properties": { "foo": { "type": "string" } }, "anyOf": [ { "required": [ "foo" ] } ] }, { "properties": { "bar": { "type": "string" } }, "anyOf": [ { "required": [ "bar" ] } ] } ] } ``` Signed-off-by: Lucian <[email protected]>
* 'master' of github.com:KeroVieux/react-jsonschema-form: updated replace submit button paragraph tag with div (rjsf-team#766) Fixes rjsf-team#824 (rjsf-team#1147) Fix bug where matching anyOf branch is not selected correctly (rjsf-team#1129) Document a gotcha with `additionalProperties` (rjsf-team#1149) doc: add permalinks and fix internal hyperlinks in documentation doc: add doc build instructions, update PR/issue templates with doc links Add onBlur and onFocus events for radio and checkbox widgets (rjsf-team#1143) add ui:help tips (rjsf-team#1145) doc: fix browserstack logo size docs: remove docs from README and link to readthedocs rjsf-team#1138 doc: organize docs into separate files rjsf-team#1138 doc: add browserstack logo rjsf-team#990 Add test and update documentation for using anyOf inside array items (rjsf-team#1131) # Conflicts: # src/components/Form.js
This change improves the logic that selects a matching anyOf branch based on form data. Previously the behaviour was to just check if the form data was valid against an anyOf branch, however due to the permissive nature of JSON schema this has unexpected behaviour. For example, given the following schema: ```json { "type": "object", "anyOf": [ { "properties": { "foo": { "type": "string" } } }, { "properties": { "bar": { "type": "string" } } } ] } ``` The form data `{ bar: 'baz' }` will actually match the first branch. To mitigate this, when doing the matching, the branch schema is augmented to require at least one of the keys in the branch. For example the schema above would become: ```json { "type": "object", "anyOf": [ { "properties": { "foo": { "type": "string" } }, "anyOf": [ { "required": [ "foo" ] } ] }, { "properties": { "bar": { "type": "string" } }, "anyOf": [ { "required": [ "bar" ] } ] } ] } ``` Signed-off-by: Lucian <[email protected]>
This change improves the logic that selects a matching anyOf branch
based on form data. Previously the behaviour was to just check if the
form data was valid against an anyOf branch, however due to the
permissive nature of JSON schema this has unexpected behaviour. For
example, given the following schema:
The form data
{ bar: 'baz' }
will actually match the first branch. Tomitigate this, when doing the matching, the branch schema is augmented
to require at least one of the keys in the branch. For example the
schema above would become:
Signed-off-by: Lucian [email protected]
Checklist
npm run cs-format
on my branch to conform my code to prettier coding style