-
Notifications
You must be signed in to change notification settings - Fork 7.6k
try-json: Fix results to avoid false positives for constructs like OneOf or AnyOf #24577
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
base: master
Are you sure you want to change the base?
Conversation
@@ -264,19 +264,11 @@ protected override void ProcessRecord() | |||
|
|||
if (_jschema != null) | |||
{ | |||
EvaluationResults evaluationResults = _jschema.Evaluate(parsedJson, new EvaluationOptions { OutputFormat = OutputFormat.List }); | |||
EvaluationResults evaluationResults = _jschema.Evaluate(parsedJson, new EvaluationOptions { OutputFormat = OutputFormat.Hierarchical }); | |||
result = evaluationResults.IsValid; | |||
if (!result) | |||
{ | |||
HandleValidationErrors(evaluationResults); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the if
since the check is in HandleValidationErrors() now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it save us a jump to the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No weird, this is often used in Runtime. We can even remove this method and inline the code.
src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs
Show resolved
Hide resolved
@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. |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
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
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.- [ ] Issue filed:
(which runs in a different PS Host).