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

Add clearer error when more than one oneOf rule matched #184

Merged
merged 1 commit into from Feb 7, 2020

Conversation

karenetheridge
Copy link
Contributor

fixes #164.

Copy link
Owner

@jhthorsen jhthorsen left a comment

Choose a reason for hiding this comment

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

Ah. I think I misunderstood #164. Thanks for clearing it up with this PR.

Would it be possible to let the user know which rules that failed? Let’s say something like “oneOf rules 0, 2, 5 match.”.

@karenetheridge
Copy link
Contributor Author

Would it be possible to let the user know which rules that failed?

Yes, that should be possible with a bit of juggling. I'll push an update.

@jhthorsen
Copy link
Owner

Don’t do it if it’s a lot of hassle/lines of code.

@karenetheridge
Copy link
Contributor Author

It would be a few lines of code -- we can get all the failure rules numbers by doing

-    push @expected, $schema_type;
+    push @expected, [$i, $schema_type];

and then @failed = sort {$a <=> $b} map $_->[0], @errors, @expected, but then we need to subtract that list from 0..@rules-1 to get the rules that passed. I don't see how to do it without at least one extra local variable :)

@karenetheridge
Copy link
Contributor Author

I pushed another commit that uses the indexes of the matching oneOf rules, which you can squash if you like it, or ignore if you don't :)

@karenetheridge
Copy link
Contributor Author

I force-pushed a much nicer change for adding passing indexes. :)

Copy link
Owner

@jhthorsen jhthorsen left a comment

Choose a reason for hiding this comment

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

Can you do this instead?

@@ -731,8 +731,7 @@ sub _validate_one_of {
   $self->_report_schema($path, 'oneOf', $rules) if REPORT;
   local $self->{grouped} = $self->{grouped} + 1;

-  my $i = 0;
-  my @passed;
+  my ($i, @passed) = (0);
   for my $rule (@$rules) {
     my @e = $self->_validate($_[1], $path, $rule) or push @passed, $i and next;
     my $schema_type = _guess_schema_type($rule);
@@ -751,13 +750,11 @@ sub _validate_one_of {
     $self->_report_errors($path, 'oneOf', \@e);
   }

-  return if @errors + @expected + 1 == @$rules;
-  my $expected = join '/', _uniq(@expected);
+  return if @passed == 1;
   return E $path, [oneOf => 'all_rules_match'] unless @errors + @expected;
-  return E $path, [oneOf => 'n_rules_match', join(', ', @passed)]
-    if @passed > 1;
-  return E $path, [oneOf => type => $expected => $type] unless @errors;
-  return _add_path_to_error_messages(oneOf => @errors);
+  return E $path, [oneOf => n_rules_match => join ', ', @passed] if @passed;
+  return _add_path_to_error_messages(oneOf => @errors) if @errors;
+  return E $path, [oneOf => type => join('/', _uniq(@expected)), $type];

@jhthorsen jhthorsen changed the title use clearer error when more than one oneOf rule matched Add clearer error when more than one oneOf rule matched Feb 7, 2020
@karenetheridge
Copy link
Contributor Author

and done!

Copy link
Owner

@jhthorsen jhthorsen left a comment

Choose a reason for hiding this comment

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

Awesome! 👍

@jhthorsen jhthorsen merged commit 92f86cb into jhthorsen:master Feb 7, 2020
jhthorsen pushed a commit that referenced this pull request Feb 7, 2020
 - Add clearer error when more than one oneOf rule matched #184
   Contributor: Karen Etheridge
 - Improved validation of numeric minimum and maximum values
@karenetheridge karenetheridge deleted the ether/fix-oneOf-message branch February 7, 2020 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

handling of 'oneOf' rules less than optimal
2 participants