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

oneOf/anyOf array item incorrectly selected despite not valid against data, once it has at least two oneOf/anyOf properties itself #3693

Closed
4 tasks done
michal-kurz opened this issue May 24, 2023 · 4 comments · Fixed by #3720
Labels
any-one-all-of Related to fixing anyOf, oneOf or allOf bug help wanted

Comments

@michal-kurz
Copy link
Contributor

michal-kurz commented May 24, 2023

Prerequisites

What theme are you using?

core

Version

5.7.2

Current Behavior

Schema:

{
  "type": "array",
  "title": "Test",
  "items": {
    "oneOf": [
      {
        "type": "object",
        "title": "Foo",
        "properties": {
          "type": {
            "type": "string",
            "enum": ["foo"],
            "default": "foo"
          },
          "aaa": {
            "type": "object",
            "anyOf": []
          },
          "bbb": {
            "type": "object",
            "anyOf": []
          }
        }
      },
      {
        "type": "object",
        "title": "Bar",
        "properties": {
          "type": {
            "type": "string",
            "enum": ["bar"],
            "default": "bar"
          }
        }
      }
    ]
  }
}

Data:

[{  "type": "bar" }]

Result: "Foo" item is selected in UI

Expected Behavior

"Bar" item being selected in UI

Steps To Reproduce

Open this sandbox: https://rjsf-team.github.io/react-jsonschema-form/#eyJmb3JtRGF0YSI6W3sidHlwZSI6ImJhciJ9XSwic2NoZW1hIjp7InR5cGUiOiJhcnJheSIsInRpdGxlIjoiVGVzdCIsIml0ZW1zIjp7Im9uZU9mIjpbeyJ0eXBlIjoib2JqZWN0IiwidGl0bGUiOiJGb28iLCJwcm9wZXJ0aWVzIjp7InR5cGUiOnsidHlwZSI6InN0cmluZyIsImVudW0iOlsiZm9vIl0sImRlZmF1bHQiOiJmb28ifSwiYWFhIjp7InR5cGUiOiJvYmplY3QiLCJhbnlPZiI6W119LCJiYmIiOnsidHlwZSI6Im9iamVjdCIsImFueU9mIjpbXX19fSx7InR5cGUiOiJvYmplY3QiLCJ0aXRsZSI6IkJhciIsInByb3BlcnRpZXMiOnsidHlwZSI6eyJ0eXBlIjoic3RyaW5nIiwiZW51bSI6WyJiYXIiXSwiZGVmYXVsdCI6ImJhciJ9fX1dfX0sInVpU2NoZW1hIjp7ImZpcnN0TmFtZSI6eyJ1aTphdXRvZm9jdXMiOnRydWUsInVpOmVtcHR5VmFsdWUiOiIiLCJ1aTpwbGFjZWhvbGRlciI6InVpOmVtcHR5VmFsdWUgY2F1c2VzIHRoaXMgZmllbGQgdG8gYWx3YXlzIGJlIHZhbGlkIGRlc3BpdGUgYmVpbmcgcmVxdWlyZWQiLCJ1aTphdXRvY29tcGxldGUiOiJmYW1pbHktbmFtZSJ9LCJsYXN0TmFtZSI6eyJ1aTphdXRvY29tcGxldGUiOiJnaXZlbi1uYW1lIn0sImFnZSI6eyJ1aTp3aWRnZXQiOiJ1cGRvd24iLCJ1aTp0aXRsZSI6IkFnZSBvZiBwZXJzb24iLCJ1aTpkZXNjcmlwdGlvbiI6IihlYXJ0aCB5ZWFyKSJ9LCJiaW8iOnsidWk6d2lkZ2V0IjoidGV4dGFyZWEifSwicGFzc3dvcmQiOnsidWk6d2lkZ2V0IjoicGFzc3dvcmQiLCJ1aTpoZWxwIjoiSGludDogTWFrZSBpdCBzdHJvbmchIn0sInRlbGVwaG9uZSI6eyJ1aTpvcHRpb25zIjp7ImlucHV0VHlwZSI6InRlbCJ9fX0sInRoZW1lIjoiZGVmYXVsdCIsImxpdmVTZXR0aW5ncyI6eyJzaG93RXJyb3JMaXN0IjoidG9wIiwidmFsaWRhdGUiOmZhbHNlLCJkaXNhYmxlZCI6ZmFsc2UsIm5vSHRtbDVWYWxpZGF0ZSI6ZmFsc2UsInJlYWRvbmx5IjpmYWxzZSwib21pdEV4dHJhRGF0YSI6ZmFsc2UsImxpdmVPbWl0IjpmYWxzZSwiZXhwZXJpbWVudGFsX2RlZmF1bHRGb3JtU3RhdGVCZWhhdmlvciI6eyJhcnJheU1pbkl0ZW1zIjoicG9wdWxhdGUifX19

Environment

No response

Anything else?

"foo" must have two or more anyOf/oneOf properties for this to happen

@michal-kurz michal-kurz added bug needs triage Initial label given, to be assigned correct labels and assigned labels May 24, 2023
@heath-freenome
Copy link
Member

@michal-kurz Another good catch. You'd think we'd have oneOf/anyOf nailed down by now, but nope. Turns out it's a hard nut to crack. Have you figured out where the issue is occurring? I noticed you have already dug into the performance issue. This one is simpler and could be fixed more easily.

@heath-freenome heath-freenome added help wanted any-one-all-of Related to fixing anyOf, oneOf or allOf and removed needs triage Initial label given, to be assigned correct labels and assigned labels May 26, 2023
@cwendtxealth
Copy link
Contributor

I believe I found the issue but haven't come up with the best solution yet. The root cause is in AnyOfField.getMatchingOption. Basically getClosestMatchingOption is returning the matched schema index of 0 which is the first anyOf/oneOf option, however that check will only take any index over 0. So then it goes and defaults to the currently selectedOption which is still the index of 1 ignoring the formData change.

The logic is assuming that index 0 means it didn't match a schema but in reality it does. I have been playing around with a solution removing that check and always taking the index returned by getClosestMatchingOption however I am running into a issue when none of the schemas match and we want to default to the selectedOption it still returns the first index 0.

@heath-freenome
Copy link
Member

@cwendtxealth @michal-kurz Wow, what a subtle little bug. I think that making the following change would solve this issue:

getMatchingOption(selectedOption: number, formData: T | undefined, options: S[]) {
    const {
      schema,
      registry: { schemaUtils },
    } = this.props;

    const discriminator = getDiscriminatorFieldFromSchema<S>(schema);
    const option = schemaUtils.getClosestMatchingOption(formData, options, -1, discriminator);
    if (option >= 0) {
      return option;
    }
    // If the form data matches none of the options, use the currently selected
    // option, assuming it's available; otherwise use the first option
    return selectedOption || 0;
  }

Feel free to test it out and if it does work provide a PR! Thanks!

@cwendtxealth
Copy link
Contributor

@heath-freenome I tried out that solution and its still happening. It is still returning the valid option of index 0 but will default to the selectedOption of 1. I believe I have a fix and will have a PR soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
any-one-all-of Related to fixing anyOf, oneOf or allOf bug help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants