Skip to content

try-json: Fix results to avoid false positives for constructs like OneOf or AnyOf#24577

Closed
sotteson1 wants to merge 2 commits into
PowerShell:masterfrom
sotteson1:user/sotteson/fix-try-json
Closed

try-json: Fix results to avoid false positives for constructs like OneOf or AnyOf#24577
sotteson1 wants to merge 2 commits into
PowerShell:masterfrom
sotteson1:user/sotteson/fix-try-json

Conversation

@sotteson1
Copy link
Copy Markdown

@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
result = evaluationResults.IsValid;
if (!result)
{
HandleValidationErrors(evaluationResults);
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

@iSazonov
Copy link
Copy Markdown
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
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Review - Needed The PR is being reviewed label Feb 8, 2025
@skaravos
Copy link
Copy Markdown

It would be really nice to have this fixed.

I'm really not sure how users are supposed to make effective use of Test-Json without this change.

@jakauppila
Copy link
Copy Markdown

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?

@iSazonov
Copy link
Copy Markdown
Collaborator

Someone can take this work away if the author has lost interest.

@microsoft-github-policy-service microsoft-github-policy-service Bot removed the Review - Needed The PR is being reviewed label Nov 11, 2025
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Review - Needed The PR is being reviewed label Nov 18, 2025
@iSazonov
Copy link
Copy Markdown
Collaborator

@yotsuda Have you an interest to grab this?

@iSazonov
Copy link
Copy Markdown
Collaborator

Implemented in #26618

@iSazonov iSazonov closed this Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

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

5 participants