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 bug where matching anyOf branch is not selected correctly #1129

Merged
merged 1 commit into from
Jan 22, 2019
Merged

Fix bug where matching anyOf branch is not selected correctly #1129

merged 1 commit into from
Jan 22, 2019

Conversation

LucianBuzzo
Copy link
Collaborator

@LucianBuzzo LucianBuzzo commented Jan 16, 2019

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:

{
  "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:

{
  "type": "object",
  "anyOf": [
    {
      "properties": {
        "foo": {
          "type": "string"
        }
      },
      "anyOf": [
        { "required": [ "foo" ] }
      ]
    },
    {
      "properties": {
        "bar": {
          "type": "string"
        }
      },
      "anyOf": [
        { "required": [ "bar" ] }
      ]
    }
  ]
}

Signed-off-by: Lucian lucian.buzzo@gmail.com

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

@LucianBuzzo
Copy link
Collaborator Author

@glasserc If you have time a review would be great! This fixes an annoying bug with my original anyOf PR.

src/components/fields/AnyOfField.js Outdated Show resolved Hide resolved
src/components/fields/AnyOfField.js Outdated Show resolved Hide resolved
src/components/fields/AnyOfField.js Outdated Show resolved Hide resolved
src/components/fields/AnyOfField.js Outdated Show resolved Hide resolved
target: { value: "Lorem ipsum dolor sit amet" },
});

expect($select.value).eql("1");
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

@LucianBuzzo
Copy link
Collaborator Author

@glasserc Thanks for the detailed review 👍 . I've addressed your comments and the PR should be good to go now.

@epicfaace
Copy link
Member

@LucianBuzzo I'm wondering, why did you wrap the required attribute in another anyOf -- couldn't you have created an augmented schema like this?:

{
  "type": "object",
  "anyOf": [
    {
      "properties": {
        "foo": {
          "type": "string"
        }
      },
      "required": [ "foo" ]
    },
    {
      "properties": {
        "bar": {
          "type": "string"
        }
      },
      "required": [ "bar" ]
    }
  ]
}

@LucianBuzzo
Copy link
Collaborator Author

@epicfaace Wrapping the required in anyOf is a trick to allow you to require at least one of a list of keys, which is what you need to do if the subschema is a more complex object, for example:

{
  "type": "object",
  "anyOf": [
    {
      "properties": {
        "foo": {
          "type": "string"
        },
        "bar": {
          "type": "string"
        }
      },
      "anyOf": [
        { "required": [ "foo" ] },
        { "required": [ "bar" ] }
      ]
    },
    {
      "properties": {
        "buzz": {
          "type": "string"
        }
      },
      "anyOf": [
        { "required": [ "buzz" ] }
      ]
    }
  ]
}

If you just use a plain required statement, then the first object wouldn't match if you start typing into the foo field, but the bar field is still empty.

src/components/fields/MultiSchemaField.js Show resolved Hide resolved
test/anyOf_test.js Show resolved Hide resolved
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 <lucian.buzzo@gmail.com>
@epicfaace epicfaace merged commit 2de683e into rjsf-team:master Jan 22, 2019
KeroVieux added a commit to KeroVieux/react-jsonschema-form that referenced this pull request Jan 25, 2019
* '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
LucianBuzzo added a commit that referenced this pull request Feb 7, 2019
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 <lucian.buzzo@gmail.com>
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.

3 participants