Add parameter `SchemaFile` to `Test-Json` cmdlet #11934

beatcracker · 2020-02-23T22:37:25Z

PR Summary

Add parameter SchemaFile to the Test-Json cmdlet.

PR Context

Current implementation allows JSON schema to be only passed as string. This makes it impossible to validate JSON files against the schema that includes definitions from separate files: https://json-schema.org/understanding-json-schema/structuring.html#reuse

{ "$ref": "definitions.json#/address" }

The new parameter accepts literal path to the JSON schema file and allows JSON files to be validated against such schemas.

Notes

I don't have much experience with C#, so please, bear with me.
~~This PR currently lacks tests. I think that I need to extend Test-Json.Tests.ps1, but I'm not sure where to put JSON schema files with includes. Would assets folder be ok?~~
@iSazonov is the author of the original Test-Json implementation and there is Migrate Test-Json to System.Text.Json API #11397 that should be taken into account.

PR Checklist

msftclas · 2020-02-23T22:37:38Z

All CLA requirements met.

beatcracker · 2020-02-24T16:32:57Z

Signed CLA, added tests.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs

test/powershell/Modules/Microsoft.PowerShell.Utility/assets/invalid_schema_definitions.json

Co-Authored-By: Ilya <darpa@yandex.ru>

Seems that "ResolveFilePath" doesn't throw in any conditions I could test. NJsonSchema will throw "System.IO.IOException" so we catch that.

iSazonov · 2020-02-27T16:53:14Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs

    /// This class implements Test-Json command.
    /// </summary>
-    [Cmdlet(VerbsDiagnostic.Test, "Json", HelpUri = "")]
+    [Cmdlet(VerbsDiagnostic.Test, "Json", DefaultParameterSetName = ParameterAttribute.AllParameterSets, HelpUri = "")]


@sdwheeler @SteveL-MSFT I wonder that we haven't HelpUri for the cmdlet.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs

vexx32 · 2020-03-04T18:34:10Z

We may also want to consider #11999 in tandem with this PR and the overall behaviour when a JSON input fails validation / schema.

iSazonov · 2020-03-04T19:01:43Z

My initial thoughts was and I still believe that users want to see errors of the JSON validation.

vexx32 · 2020-03-05T04:34:40Z

@iSazonov I would agree that there is potential value in being able to see that, but I think that having that as an explicit opt-in would be more useful. There's a lot of cases where you just want to test whether some input JSON is valid or not, and not do much else.

For the cases where the full feedback is asked for, I would expect we output a rich object indicating success status and a proper list of structured data indicating any validation failures that were encountered, all on the one output object. The current state of having two separate and simultaneous modes of outputting data across two separate streams is, I think, much more difficult for folks to work with.

iSazonov · 2020-03-05T15:54:16Z

I think that having that as an explicit opt-in would be more useful. There's a lot of cases where you just want to test whether some input JSON is valid or not, and not do much else.

I don't know what is a primary scenario. In interactive scenario users could get verbose output without extra switches and in script scenario we could do Test-Json -Quite like we do in Test-Connection cmdlet - it would be non-breaking change.

vexx32 · 2020-03-05T17:58:51Z

@iSazonov I don't think that changing behaviours between interactive use and script is ever a good idea. It should always be an option for the user to specify. Any kind of black magic like that will simply confuse users and make it more difficult to appropriately diagnose issues.

Cmdlet behaviours should be consistent.

ghost · 2020-05-27T02:01:38Z

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

adityapatwardhan · 2020-05-28T19:03:00Z

@beatcracker If the PR is ready for review, please remove the WIP from the title. That helps me prioritize things.

beatcracker · 2020-05-28T19:52:38Z

@adityapatwardhan I think it is. I've removed [ WIP ], thanks.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs

adityapatwardhan · 2020-05-28T20:06:37Z

@beatcracker Can you also respond to comment about not rethrowing and writing error instead.

Co-authored-by: Aditya Patwardhan <adityap@microsoft.com>

beatcracker · 2020-05-28T20:31:13Z

@adityapatwardhan, Sure, I just did that. As far as I understand, in that case we're supposed to throw. because this is invalid input.

adityapatwardhan · 2020-05-29T20:14:08Z

@PoshChan Please remind me in 1 hour

PoshChan · 2020-05-29T21:14:44Z

@adityapatwardhan, this is the reminder you requested 1 hour ago

adityapatwardhan · 2020-06-01T20:19:30Z

@PoshChan please rebuild windows

PoshChan · 2020-06-01T20:20:00Z

@adityapatwardhan, successfully started rebuild of PowerShell-CI-Windows

adityapatwardhan · 2020-06-02T20:52:12Z

@beatcracker Thank you for your contribution!

vexx32 · 2020-06-06T21:24:27Z

@adityapatwardhan @beatcracker did we get an issue filed in the docs repo to ensure this is documented in time for 7.1.0?

beatcracker · 2020-06-06T21:36:44Z

@vexx32 I don't think so. If you point me in the right direction, I could do that. Are we talking about PR to update Test-Json.md?

vexx32 · 2020-06-06T21:52:06Z

@beatcracker yep! You can either submit a PR to add the information directly or file an issue so the docs team are aware and can sort it out as they're able. 🙂

* Test-Json: document SchemaFile parameter See PowerShell/PowerShell#11934 for details * Update reference/7.1/Microsoft.PowerShell.Utility/Test-Json.md Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com> * Add SchemaFile example * Reformatted to fix build error Fixed formatting to fit PlatyPS schema requirements * Fix duplicate JSON in SchemaFile example Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>

ghost · 2020-06-25T19:06:56Z

🎉v7.1.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

beatcracker added 2 commits February 23, 2020 23:57

Add SchemaPath parameter

3a6f1fd

Remove unnecessary usings

3d5e834

ghost assigned adityapatwardhan Feb 23, 2020

Add tests

576866b

beatcracker requested a review from iSazonov February 24, 2020 16:39

iSazonov reviewed Feb 24, 2020

View reviewed changes

beatcracker and others added 3 commits February 25, 2020 00:19

Add newline at the EOF

0b27f48

Use named argument for 'ResolveFilePath'

f07bec0

Co-Authored-By: Ilya <darpa@yandex.ru>

Remove "ReportFileOpenFailure" usage

8b4f4d5

Seems that "ResolveFilePath" doesn't throw in any conditions I could test. NJsonSchema will throw "System.IO.IOException" so we catch that.

iSazonov reviewed Feb 27, 2020

View reviewed changes

beatcracker added 2 commits March 1, 2020 22:22

Handle AggregateException from async methods

1e991d2

Add tests for JSON schema file open failure

5a183bc

mklement0 reviewed Mar 1, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs Outdated Show resolved Hide resolved

beatcracker added 2 commits March 1, 2020 23:59

Replace "An JSON" with "A JSON"

77a24af

Replace "A JSON" with "A JSON string"

c86c47d

iSazonov reviewed Mar 2, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs Outdated Show resolved Hide resolved

beatcracker added 2 commits March 2, 2020 22:52

Replace "SchemaPath" with "SchemaFile"

5b5ef80

Rename inner exception variable: "i" -> "ie"

11a2d4f

beatcracker changed the title ~~[ WIP ] Test-Json: add parameter 'SchemaPath'~~ [ WIP ] Test-Json: add parameter 'SchemaFile' Mar 2, 2020

beatcracker added 3 commits March 3, 2020 23:59

Convert delegates to private method

0899edb

Remove top-level declaration of the "resolvedpath"

7b064f7

Sort usings

34f8473

iSazonov mentioned this pull request Mar 4, 2020

Test-Json returns Error before result #11999

Closed

TravisEz13 added this to the 7.1.0-preview.4 milestone May 14, 2020

ghost added the Review - Needed The PR is being reviewed label May 27, 2020

beatcracker changed the title ~~[ WIP ] Test-Json: add parameter 'SchemaFile'~~ Test-Json: add parameter 'SchemaFile' May 28, 2020

adityapatwardhan requested changes May 28, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs Outdated Show resolved Hide resolved

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 28, 2020

ghost removed this from the 7.1.0-preview.4 milestone May 28, 2020

ghost removed the Review - Needed The PR is being reviewed label May 28, 2020

Add HelpUri

8429e2f

Co-authored-by: Aditya Patwardhan <adityap@microsoft.com>

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 28, 2020

adityapatwardhan approved these changes May 29, 2020

View reviewed changes

adityapatwardhan changed the title ~~Test-Json: add parameter 'SchemaFile'~~ Add parameter SchemaFile to Test-Json cmdlet May 29, 2020

adityapatwardhan merged commit c22ccbe into PowerShell:master Jun 2, 2020

adityapatwardhan added this to the 7.1.0-preview.4 milestone Jun 2, 2020

vexx32 mentioned this pull request Jun 6, 2020

Test-Json should have a -JsonFile parameter #12882

Closed

beatcracker mentioned this pull request Jun 17, 2020

Test-Json: document SchemaFile parameter MicrosoftDocs/PowerShell-Docs#6134

Merged

18 tasks

Add parameter SchemaFile to Test-Json cmdlet #11934

Add parameter SchemaFile to Test-Json cmdlet #11934

Uh oh!

Conversation

beatcracker commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

Notes

PR Checklist

Uh oh!

msftclas commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beatcracker commented Feb 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vexx32 commented Mar 4, 2020

Uh oh!

iSazonov commented Mar 4, 2020

Uh oh!

vexx32 commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Mar 5, 2020

Uh oh!

vexx32 commented Mar 5, 2020

Uh oh!

ghost commented May 27, 2020

Uh oh!

adityapatwardhan commented May 28, 2020

Uh oh!

beatcracker commented May 28, 2020

Uh oh!

Uh oh!

adityapatwardhan commented May 28, 2020

Uh oh!

beatcracker commented May 28, 2020

Uh oh!

adityapatwardhan commented May 29, 2020

Uh oh!

PoshChan commented May 29, 2020

Uh oh!

adityapatwardhan commented Jun 1, 2020

Uh oh!

PoshChan commented Jun 1, 2020

Uh oh!

adityapatwardhan commented Jun 2, 2020

Uh oh!

vexx32 commented Jun 6, 2020

Uh oh!

beatcracker commented Jun 6, 2020

Uh oh!

vexx32 commented Jun 6, 2020

Uh oh!

ghost commented Jun 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Add parameter `SchemaFile` to `Test-Json` cmdlet #11934

Add parameter `SchemaFile` to `Test-Json` cmdlet #11934

beatcracker commented Feb 23, 2020 •

edited

Loading

msftclas commented Feb 23, 2020 •

edited

Loading

vexx32 commented Mar 5, 2020 •

edited

Loading