Issue535: OneOf validation gives unnecessary errors #537
Conversation
|
Hmm - I fear this could probably still be a bit too simplistic..!? What about a scenario where we have a oneOf where both alternatives require a specific, but disjunct set of properties, and the message to be validated does not contain any single of all these required properties. I fear that removing any errors of ValidatorTypeCode.REQUIRED could/would lead to a scenario where such a message would then be validated fine!? Of course, I'd need to prove this with a test case, but maybe can you already see what I mean from your scenario... Also, would you agree that we have a similar scenario in the anyOf case, where we might in the same way as in your scenario get validation errors about other branches that would neither intended nor needed for the anyOf to validate? |
|
I don't know if i understand you correctly, but if none of the required oneOf properties are present, then there could only be REQUIRED errors in that oneOf, in which case they will not be removed. If that's not what you meant, maybe you could provide a test case? I agree that anyOf is similar in that regard, and could also be fixed |
|
Sorry, I see: yes - looks like I missed/misunderstood an important check: As you are replacing the full set of messages by the filtered set not containing the REQUIRED messages if and only if there has been at least one non-REQUIRED validation error message, I now understand that your approach seems to be working as intended... 😄 Would you be willing to do an equivalent change also to the "AnyOfValidator" and maybe even add it to this PR? Otherwise I would be planning to do this myself as time permits (very likely only around Easter), because I had the exact same/similiar type of issue just with "anyOf" (instead of "oneOf"), and have currently worked around it by adapting my OpenAPI yaml - but solving it along the lines of your "oneOf" fix would be the much better way indeed... 👍 |
|
Sure, I'm going to try to do it as soon as possible and add it here :) |
|
Version 1.0.68 (released a few days ago) introduced a nasty memory leak. There were only two significant PRs included in that release, including this one. Might be worth running some regression tests on this code with big JSON files, and schemas involving |
|
Hello @TJC could you please provide a heap dump showing the memory leak you are referring to (hope you know how to do that)? Looking into a likely memory leak based on a heap dump is white box analysis, compared to wildly guessing and looking for a needle in a haystack if you don't have neither a reproducer nor a heap dump... Thanks a million! :-) |
|
I did some git bisecting, and narrowed it down to another commit - 5007242 |
|
I'll take the discussion to the issue I raised #546 rather than keep hassling the poor renegade wizard that I falsely accused :) |
|
HI @ksychla why was the decision being made to filter out with required errors? We are seeing the use case with additional properties: false in both schemas not showing required errors is giving the wrong impression of the errors. cc: @prashanthjos |
|
@rahultokase I assume you are mixing up issues here: I might have seen a similar issue to what you describe regarding a lack of validation errors when required properties are missing (outside oneOf, anyOf, allOf), but that's a completely different issue. If you haven't done so, please open a new issue and provide an example where validation is going wrong for you wrt required properties... thx! 😄 |
|
Thanks for the reply created the following bug |
No description provided.