try-json: Fix results to avoid false positives for constructs like OneOf or AnyOf#24577
try-json: Fix results to avoid false positives for constructs like OneOf or AnyOf#24577sotteson1 wants to merge 2 commits into
Conversation
| result = evaluationResults.IsValid; | ||
| if (!result) | ||
| { | ||
| HandleValidationErrors(evaluationResults); |
There was a problem hiding this comment.
Please remove the if since the check is in HandleValidationErrors() now.
There was a problem hiding this comment.
Doesn't it save us a jump to the method?
There was a problem hiding this comment.
If you say about a performance now we call the small method once so the method will be inlined.
There was a problem hiding this comment.
I also think it looks weird to call "HandleValidationErrors" when we just learned there aren't any errors.
There was a problem hiding this comment.
No weird, this is often used in Runtime. We can even remove this method and inline the code.
|
@iSazonov Any status update on this issue.. will this be merged ? |
|
@venkat1017 I see my style comments was not addressed. Then we need more depth/functional review. |
|
It would be really nice to have this fixed. I'm really not sure how users are supposed to make effective use of |
|
I’ve been looking to migrate a process to use Test-Json for validating a complex JSON Schema, but this issue makes that impractical at the moment. Is there any plan to review and merge this PR? |
|
Someone can take this work away if the author has lost interest. |
|
@yotsuda Have you an interest to grab this? |
|
Implemented in #26618 |
PR Summary
Fixes #21471
When calling the API to get the results against the schema, ask for the results in hierarchical format. Then walk down the tree and skip any nodes and its children if IsValid is true.
PR Context
See #21471
When using a construct like OneOf, each json node that uses it will have results nodes like this:
OneOf - true
--Choice1 - false
--Choice2- true
OneOf - true
--Choice1 - true
--Choice2- false
The 'false' items aren't actually failures. They're just the result of checking each OneOf choice. But if you then add a node where the OneOf actually fails:
OneOf - false
--Choice1 - false
--Choice2- false
When you call try-json with the existing code, it includes all the "false" choices even if the parent was "true". This leads to a bunch of false positives and can make so much noise as to make try-json unsuable. The fix is to ask for the results in hierarchical format instead of list format. Then walk down the tree and skip any node and its children if IsValid is true.
Some real-world output:
Before the fix (the only invalid json node is /Devices/3):
After the fix it correctly only shows errors from /Devices/3:
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.- [ ] Issue filed:
(which runs in a different PS Host).