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

Expand anyOf support to also support oneOf #1133

Merged
merged 1 commit into from
Jan 18, 2019
Merged

Expand anyOf support to also support oneOf #1133

merged 1 commit into from
Jan 18, 2019

Conversation

LucianBuzzo
Copy link
Collaborator

@LucianBuzzo LucianBuzzo commented Jan 17, 2019

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

Reasons for making this change

This is a follow-up to #1118 that expands the code to support oneOf as well.

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

Signed-off-by: Lucian <lucian.buzzo@gmail.com>
@LucianBuzzo
Copy link
Collaborator Author

@glasserc If you have time a review would be great!

Copy link
Contributor

@glasserc glasserc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to merge this, but I guess there's a larger question about support for anyOf and whether it should be different from that of oneOf. I guess conceptually anyOf could allow a user to match several of the options? That sounds like it would be hard to produce a good UI for.

@glasserc glasserc merged commit b068705 into rjsf-team:master Jan 18, 2019
@jericmason
Copy link

jericmason commented Jan 19, 2019

I'm trying to understand how this will work? I saw the previous PR for anyOf and it looks similar to what is already supported via schema dependencies. There is currently a problem with schema dependencies in that data will stick around when you change selections, which in combination with additionalProperties: false will result in invalid schemas. I feel that is a pretty big bug as the schema form should never generate invalid schemas. That behaviour is more relevant with anyOf and seems to work correctly, however I find the UI a bit confusing as anyOf should be a multi-select that allows input supporting one or more of the sub-schemas at a given time.

With respect to oneOf, I believe this solution is correct, but I'm curious as to whether it too will generate an invalid schema if you update more than one of the possible options. If it behaves exactly like anyOf there's a bug as it will generate invalid schemas.

@epicfaace
Copy link
Member

@jericmason I'm not clear as to what you're referring to; can you make an issue and give an example schema that doesn't work well as you mention?

@jericmason
Copy link

@epicfaace I just checked the updated examples and oneOf does indeed work as expected. It won't generate invalid json for the schema. It would be nice if the current implementation of schema dependencies used the same oneOf functionality (I've added a ticket with examples for that).

I'm still a little confused about anyOf; my understanding is that anyOf should support one or more of the provided sub-schemas, but in this implementation it will support the base schema and exactly one sub-schema. Maybe I'm missing something here. I really do appreciated these features and feel like they will be powerful additions to this project.

@epicfaace
Copy link
Member

epicfaace commented Jan 21, 2019

@jericmason thanks for the ticket, I've added my thoughts to that.

You are exactly right about oneOf and anyOf. Note how an example for the playground for oneOf will error when both schemas are the same (because if both schemas are the same, then the form data can't just match one of those two schemas), while the playground for anyOf will not error. Additionally, anyOf can be used with arrays.

@LucianBuzzo
Copy link
Collaborator Author

@epicfaace @jericmason The difference between anyOf and oneOf are in validation, if you want to be able to use multiple options from anyOf you need to model your data as an array.

@alex-pex
Copy link

@glasserc Can you release a new version with oneOf support ?

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.

5 participants