jhthorsen / json-validator

:cop: Validate data against a JSON schema
https://metacpan.org/release/JSON-Validator
56 stars 58 forks source link

fix edge cases from PR#201 where error types are misidentified #203

Closed karenetheridge closed 4 years ago

karenetheridge commented 4 years ago

Summary

These new test cases expose cases where the [ $i, @errors ] structures have more than one error in them, and consequently the detection of 'type' errors was incorrect. A change to JSON::Validator::Error was required to preserve the details field that we rely on here (prefix_errors was removing that field, resulting in the errors appearing to have generic type.)

References

as discussed on irc from the last few days.

karenetheridge commented 4 years ago

I thought of more test cases that exposed more edge cases ( 😃 and 😲 ) which I added in a separate commit. It may be better to squash these together. Also, it may be preferred now to invert the (or..not or not..) conditionals in the _validate_*_of subs, so we do the collapse-types-together path first for readability. I kept it the way it is now because the diffs are smaller, and so you can see what I changed, but I'm happy to make that change too and squash it all together.

karenetheridge commented 4 years ago

For the second commit, this definitely fixes a bug, but I wonder if it exposes another one...

this test case:

validate_ok {foo => 'not an arrayref'},
  {
  anyOf => [
    {type => 'object', properties => {foo => {type => 'array'}}},
    {type => 'boolean'},
  ]
  },
  E('/foo', '/anyOf/0 Expected array - got string.'),
  E('/',    '/anyOf/1 Expected boolean - got object.');

without the fix, we got a totally nonsensical error message.. but I wonder if we should only return the first error message (the one that doesn't pertain to an incorrect type at the top level of the data), so behaviour would be the same as if we had said type => ['object','boolean'].

To fix that, I can just discard the errors where the error type is 'type' and the path eq $path, which is already the check being done to see how to fold errors together. (I can't promise the code will be very pretty though!) 😛

jhthorsen commented 4 years ago

I had to merge this manually, since there was some commits that does not seem to be rebased correctly.