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 oneOf #431

Closed
wants to merge 7 commits into from
Closed

fix oneOf #431

wants to merge 7 commits into from

Conversation

znewsham
Copy link

@znewsham znewsham commented Apr 5, 2021

This ended up being quite a lot larger than I initially planned. Initially it looked like it was going to be fairly simple, but alas not. All tests are passing and I added a handful of others.

I believe this fixes #428 and #410

There are a few small changes that look large, and a couple of actual large ones:

  1. In checkObj - move everything after the validate call into a new function called recurse - this is called by checkObj in the case that we don't want to validate a key (or initially when we dont have a key) but also by validate which may also change the "schema" used from the root schema to the current branch schema.
  2. Rather than accumulating errors, everything now returns an array of errors - these are recursively concatenated until a def.type.some completes with multiple options. At which point it either discards the entire array (in the case of a pass) or continues to concat.
  3. subSchema subSchemaAffectedKey and subSchemaAffectedKeyGeneric(sometimes) are now additionally passed around - these represent the current sub schema and the affected key relative to the sub schema affectedKey and affectedKeyGeneric refer to the current key relative to the root schema.
  4. If none of the provided schemas match, the entire set of accumulated errors are thrown. This behaviour is debatable - it isn't useful to be told that "a.b" and "a.c" are missing when you provided "a.d" and "a.e" - but I'm not sure how else to handle this. Perhaps a new error type that preempts all the others for a given schema '"a" did not match any of the provided schemas'?
  5. add oneOfKeys() and recursiveKeys() functions to accumulate all keys in a schema (including child schemas) and those that are inside of a oneOf.
  6. change checkModifier to check if a key is inside a oneOf and if so run the check against each subSchema

Thanks for providing such a vast library of test cases - it made implementing this very safe. I'm going to include this in my project where I've been hacking around this limitation for ages, and will ensure that all my schemas work, as I make heavy use of oneOf(schema1, schema2, ...)

@znewsham
Copy link
Author

znewsham commented Apr 5, 2021

I just realised this will fall over on modifiers :'(

@znewsham znewsham marked this pull request as draft April 5, 2021 03:46
@znewsham znewsham marked this pull request as ready for review April 5, 2021 19:08
@aldeed
Copy link
Collaborator

aldeed commented Apr 8, 2021

@znewsham Welcome to the complexity of SimpleSchema! Thanks for giving it a shot. This looks like a good start. I will do some careful review and see if we can get it through.

@znewsham
Copy link
Author

znewsham commented Apr 8, 2021

@aldeed Ha! Yes, I've been "hacking" around this bug for a while in my project, thought I'd give it a go. These changes + one other change that probably doesn't make sense to be in the class (a function that specifies WHICH schema defined by oneOf to use, evaluated at validation time) has removed almost all the hacks I put in place.

It's currently in use on a fairly complicated project involving nested, schemas that change based on fields within the document - so I'm relatively sure this is going to work in the majority of cases!

@aldeed
Copy link
Collaborator

aldeed commented Apr 8, 2021

@znewsham An option to narrow the accepted one-ofs at validation time sounds interesting. It should be a separate future PR, but seems like something others would benefit from.

@znewsham znewsham mentioned this pull request Apr 9, 2021
@znewsham
Copy link
Author

znewsham commented Apr 9, 2021

closed in favour of #435 - this PR was against my main branch, which causes trouble.

@znewsham znewsham closed this Apr 9, 2021
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.

oneOf(Object, Number) somehow blocks all updates
2 participants