Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sotteson1
Copy link

@sotteson1 sotteson1 commented Nov 13, 2024

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):

Test-Json: The JSON is not valid with the schema: Expected 1 matching subschema but found 0 at '/Devices/3'
Test-Json: The JSON is not valid with the schema: Required properties ["os"] are not present at '/Devices/0'
Test-Json: The JSON is not valid with the schema: Required properties ["arch"] are not present at '/Devices/1'
Test-Json: The JSON is not valid with the schema: Required properties ["os"] are not present at '/Devices/2'
Test-Json: The JSON is not valid with the schema: Required properties ["arch"] are not present at '/Devices/3'
Test-Json: The JSON is not valid with the schema: Expected "\u0022smartphone\u0022" at '/Devices/0/deviceType'
Test-Json: The JSON is not valid with the schema: Expected "\u0022laptop\u0022" at '/Devices/1/deviceType'
Test-Json: The JSON is not valid with the schema: Expected "\u0022smartphone\u0022" at '/Devices/2/deviceType'
Test-Json: The JSON is not valid with the schema: Value should match one of the values specified by the enum at '/Devices/3/os'
Test-Json: The JSON is not valid with the schema: Expected "\u0022laptop\u0022" at '/Devices/3/deviceType'

After the fix it correctly only shows errors from /Devices/3:

Test-Json: The JSON is not valid with the schema: Expected 1 matching subschema but found 0 at '/Devices/3'
Test-Json: The JSON is not valid with the schema: Value should match one of the values specified by the enum at '/Devices/3/os'
Test-Json: The JSON is not valid with the schema: Required properties ["arch"] are not present at '/Devices/3'
Test-Json: The JSON is not valid with the schema: Expected "\u0022laptop\u0022" at '/Devices/3/deviceType'

PR Checklist

@sotteson1 sotteson1 changed the title Fix results to only show real errors and avoid false positives Fix results to avoid false positives for OneOf or AnyOf Nov 13, 2024
@sotteson1 sotteson1 changed the title Fix results to avoid false positives for OneOf or AnyOf try-json: Fix results to avoid false positives for constructs like OneOf or AnyOf Nov 13, 2024
@@ -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);
Copy link
Collaborator

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.

Copy link
Author

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?

Copy link
Collaborator

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.

Copy link
Author

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.

Copy link
Collaborator

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.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Nov 21, 2024
@venkat1017
Copy link

@iSazonov Any status update on this issue.. will this be merged ?

@iSazonov
Copy link
Collaborator

@venkat1017 I see my style comments was not addressed. Then we need more depth/functional review.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Jan 31, 2025
Copy link
Contributor

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Feb 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review - Needed The PR is being reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test-Json has false positives when using anyof and allof statements in JSON schema
3 participants