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.

Sign up for GitHub

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

Closed
sotteson1 wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
505a3eb
Fix results to only show real errors
sotteson1 Nov 13, 2024
3d889c6
Merge branch 'master' into user/sotteson/fix-try-json
sotteson1 Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 17 additions & 14 deletions src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy Markdown
Collaborator

@iSazonov iSazonov Nov 13, 2024

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

@sotteson1 sotteson1 Nov 13, 2024

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

@iSazonov iSazonov Nov 14, 2024

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

@sotteson1 sotteson1 Nov 14, 2024

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

@iSazonov iSazonov Nov 14, 2024

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.


if (evaluationResults.HasDetails)
{
foreach (var nestedResult in evaluationResults.Details)
{
HandleValidationErrors(nestedResult);
}
}
}
}
}
Expand All @@ -300,16 +292,27 @@ protected override void ProcessRecord()

private void HandleValidationErrors(EvaluationResults evaluationResult)
{
if (!evaluationResult.HasErrors)
if (evaluationResult.IsValid)
{
return;
}

foreach (var error in evaluationResult.Errors!)
if (evaluationResult.HasErrors)
Comment thread
sotteson1 marked this conversation as resolved.
{
foreach (var error in evaluationResult.Errors!)
{
Exception exception = new(string.Format(TestJsonCmdletStrings.InvalidJsonAgainstSchemaDetailed, error.Value, evaluationResult.InstanceLocation));
ErrorRecord errorRecord = new(exception, "InvalidJsonAgainstSchemaDetailed", ErrorCategory.InvalidData, null);
WriteError(errorRecord);
}
}

if (evaluationResult.HasDetails)
{
Exception exception = new(string.Format(TestJsonCmdletStrings.InvalidJsonAgainstSchemaDetailed, error.Value, evaluationResult.InstanceLocation));
ErrorRecord errorRecord = new(exception, "InvalidJsonAgainstSchemaDetailed", ErrorCategory.InvalidData, null);
WriteError(errorRecord);
foreach (var nestedResult in evaluationResult.Details)
{
HandleValidationErrors(nestedResult);
}
}
}
}
Expand Down
104 changes: 104 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Utility/Test-Json.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,95 @@ Describe "Test-Json" -Tags "CI" {
}
'@

$oneOfSchemaJson = @'
{
"type": "object",
"properties" : {
"Devices": {
"type": "array",
"items": {
"type": "object",
"oneOf": [
{
"properties": {
"id": { "type": "string" },
"deviceType": { "const": "smartphone" },
"os": { "type": "string", "enum": ["iOS", "Android" ] }
},
"required": ["deviceType", "os"]
},
{
"properties": {
"id": { "type": "string" },
"deviceType": { "const": "laptop" },
"arch": { "type": "string", "enum": ["x86", "x64", "arm64"] }
},
"required": ["deviceType", "arch"]
}
]
}
}
},
"required": [ "Devices"]
}

'@

$jsonValidOneOf = @'
{
"Devices": [
{
"id": "0",
"deviceType": "laptop",
"arch": "x64"
},
{
"id": "1",
"deviceType": "smartphone",
"os": "iOS"
},
{
"id": "2",
"deviceType": "laptop",
"arch": "arm64"
},
{
"id": "3",
"deviceType": "smartphone",
"os": "Android"
}
]
}
'@

$jsonInvalidOneOf = @'
{
"Devices": [
{
"id": "0",
"deviceType": "laptop",
"arch": "x64"
},
{
"id": "1",
"deviceType": "smartphone",
"os": "iOS"
},
{
"id": "2",
"deviceType": "laptop",
"arch": "arm64"
},
{
"id": "3",
"deviceType": "smartphone",
"os": "WindowsPhone"
}
]
}
'@


$validJsonPath = Join-Path -Path $TestDrive -ChildPath 'validJson.json'
$validLiteralJsonPath = Join-Path -Path $TestDrive -ChildPath "[valid]Json.json"
$invalidNodeInJsonPath = Join-Path -Path $TestDrive -ChildPath 'invalidNodeInJson.json'
Expand Down Expand Up @@ -181,6 +270,21 @@ Describe "Test-Json" -Tags "CI" {
Test-Json -Path $invalidNodeInJsonPath -ErrorAction SilentlyContinue | Should -BeFalse
}

It "OneOf invalid json only reports real errors" {
$errVar = @()
Test-Json -Json $jsonInvalidOneOf -Schema $oneOfSchemaJson -ErrorAction SilentlyContinue -ErrorVariable errVar

$falseErrors = $errVar | select-string -pattern "Devices\/(0|1|2)"
$falseErrors.Count | Should -Be 0

$realErrors = $errVar | select-string -pattern "Devices\/3"
$realErrors.Count | Should -BeGreaterThan 0
}

It "OneOf valid json should succeed" {
Test-Json -Json $jsonValidOneOf -Schema $oneOfSchemaJson -ErrorAction SilentlyContinue | Should -BeTrue
}

It "Json file is invalid against a valid schema from string" {
Test-Json -Path $invalidTypeInJson2Path -Schema $validSchemaJson -ErrorAction SilentlyContinue | Should -BeFalse
Test-Json -Path $invalidNodeInJsonPath -Schema $validSchemaJson -ErrorAction SilentlyContinue | Should -BeFalse
Expand Down