Skip to content
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

[ WIP ] Test-Json: add parameter 'SchemaFile' #11934

Open
wants to merge 19 commits into
base: master
from

Conversation

@beatcracker
Copy link

beatcracker commented Feb 23, 2020

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

  1. I don't have much experience with C#, so please, bear with me.
  2. 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?
  3. @iSazonov is the author of the original Test-Json implementation and there is #11397 that should be taken into account.

PR Checklist

@msftclas

This comment has been minimized.

Copy link

msftclas commented Feb 23, 2020

CLA assistant check
All CLA requirements met.

@beatcracker

This comment has been minimized.

Copy link
Author

beatcracker commented Feb 24, 2020

Signed CLA, added tests.

@beatcracker beatcracker requested a review from iSazonov Feb 24, 2020
beatcracker and others added 3 commits Feb 24, 2020
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.
@@ -15,9 +12,12 @@ namespace Microsoft.PowerShell.Commands
/// <summary>
/// This class implements Test-Json command.
/// </summary>
[Cmdlet(VerbsDiagnostic.Test, "Json", HelpUri = "")]
[Cmdlet(VerbsDiagnostic.Test, "Json", DefaultParameterSetName = ParameterAttribute.AllParameterSets, HelpUri = "")]

This comment has been minimized.

Copy link
@iSazonov

iSazonov Feb 27, 2020

Collaborator

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

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

This comment has been minimized.

Copy link
Collaborator

vexx32 commented Mar 4, 2020

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

@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Mar 4, 2020

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

@vexx32

This comment has been minimized.

Copy link
Collaborator

vexx32 commented Mar 5, 2020

@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

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Mar 5, 2020

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

This comment has been minimized.

Copy link
Collaborator

vexx32 commented Mar 5, 2020

@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.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Mar 5, 2020

I don't think that changing behaviours between interactive use and script is ever a good idea.

I wonder you say this after investigating in Test-Connection :-)

@vexx32

This comment has been minimized.

Copy link
Collaborator

vexx32 commented Mar 5, 2020

@iSazonov If you see a different behaviour between script and interactive use there, let me know. 🙂

@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Mar 5, 2020

@vexx32 I say that Test-Json probably should works like Test-Connection.
Test-Connection returns some results, Test-Connection -Quite returns only true/false. Test-Json returns some results. Test-Json -Quite returns only true/false.

@vexx32

This comment has been minimized.

Copy link
Collaborator

vexx32 commented Mar 5, 2020

@iSazonov if it wasn't a significant breaking change I'd recommend heavily for Test-Connection to be only true/false output to align with things like Test-Path, and add a new cmdlet for the full information (e.g., Get-ConnectionInfo or something along those lines).

But in practice as long as there is a way to get both the rich result and the quiet one, whether it's a switch or another cmdlet entirely, I think it'll be OK overall. I do think there's more value in a rich output object than outputting errors, though. 🙂

@beatcracker

This comment has been minimized.

Copy link
Author

beatcracker commented Mar 7, 2020

My 2 cents: I use Test-Json in Pester tests and I would prefer if there was a way to display validation errors or at least get them all from underlying exception without too much hassle. I'm not sure if implementing -Detailed parameter is in the scope of this PR, but I could take shot at it if needed.

@vexx32

This comment has been minimized.

Copy link
Collaborator

vexx32 commented Mar 7, 2020

If you'd like to take a stab at it in this PR, or even as a follow up PR, it'd be greatly appreciated either way! 😊

@beatcracker

This comment has been minimized.

Copy link
Author

beatcracker commented Mar 14, 2020

Sorry, folks haven't been able to work on this lately.

If you'd like to take a stab at it in this PR, or even as a follow up PR, it'd be greatly appreciated either way! 😊

I'd prefer the -Detailed switch to be a separate PR, since I don't have a clear idea of how this should work and I'll need help to figure it out. Meanwhile this PR is small backwards compatible change which could be merged as soon as everybody is happy with my code.

What do you say?

@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Mar 14, 2020

@beatcracker Please ignore discussion about additional parameters and keep current behavior.

@beatcracker

This comment has been minimized.

Copy link
Author

beatcracker commented Mar 14, 2020

@iSazonov, sure we can talk about it later. Anything else I could do to improve this PR?

{
if (e is TargetInvocationException)
{
ExceptionDispatchInfo.Capture(e.InnerException).Throw();

This comment has been minimized.

Copy link
@iSazonov

iSazonov Mar 16, 2020

Collaborator

I think we could avoid re-throw and write an error here.

// Do we really need to wrap exception? Not doing this provides more clear error message upfront.
// E.g.: "'{}'|Test-Json -SchemaFile c:" results in "Test-Json : Access to the path 'C:\' is denied".
Exception exception = new Exception("JSON schema file open failure", e); // TODO: Add resource string
ThrowTerminatingError(new ErrorRecord(exception, "JsonSchemaFileOpenFailure", ErrorCategory.OpenError, null));

This comment has been minimized.

Copy link
@iSazonov

iSazonov Mar 16, 2020

Collaborator

Please write non-terminating error.

This comment has been minimized.

Copy link
@beatcracker

beatcracker Mar 23, 2020

Author

I'm not sure how non-terminating error (WriteError ?) is supposed to work. If we don't terminate here, then we need to somehow handle this in the ProcessRecord, and it doesn't make sense to run ProcessRecord at all since the scheme file can't be accessed.

P.S> There is infinitesimal that file will become accessible on the subsequent pipeline iterations, but...

This comment has been minimized.

Copy link
@iSazonov

iSazonov Mar 24, 2020

Collaborator

Do WriteError end return false.

This comment has been minimized.

Copy link
@vexx32

vexx32 Mar 24, 2020

Collaborator

This cmdlet's verb is Test - the primary output mode should be true / false, not throwing errors. We're not asserting that the json is valid and erroring otherwise, we're testing it. 😕

This comment has been minimized.

Copy link
@iSazonov

iSazonov Mar 24, 2020

Collaborator

If we have a problem with schema file we should write a terminating error.
For JSON test we should return true/false but in interactive session I'd expect to see non-terminating error by default. It is less typing in interactive session to see useful result. In script mode user can suppress errors with -ErrorActoion SilentlyContinue - more typing but it is not too important as for interactive session.

This comment has been minimized.

Copy link
@beatcracker

beatcracker Mar 24, 2020

Author

@iSazonov Ok, I see how it can be done, thanks.

@vexx32 Returning true/false is fine when we're talking about the resource itself (JSON), but if schema is invalid (or not found) shouldn't we treat it as an invalid input and throw?

E.g.: Test-Connection will throw if it can't resolve host, Test-ModuleManifest throws if path is invalid or not pointing to the psd1 file..

This comment has been minimized.

Copy link
@vexx32

vexx32 Mar 24, 2020

Collaborator

Oh, this is if the schema is totally invalid / can't be found?

Then yeah, ignore me, this is totally appropriate. Sorry! 😊

catch (Exception e)
{
Exception exception = new Exception(TestJsonCmdletStrings.InvalidJsonSchema, e);
ThrowTerminatingError(new ErrorRecord(exception, "InvalidJsonSchema", ErrorCategory.InvalidData, null));

This comment has been minimized.

Copy link
@iSazonov

iSazonov Mar 16, 2020

Collaborator

Please write non-terminating error.

This comment has been minimized.

Copy link
@beatcracker

beatcracker Mar 23, 2020

Author

The same as above (how WriteError is supposed to work here) + there is no chance at all that schema passed via string will ever become valid. Could you please explain in more detail?

beatcracker and others added 4 commits Mar 17, 2020
Co-Authored-By: Ilya <darpa@yandex.ru>
Co-Authored-By: Ilya <darpa@yandex.ru>
Co-Authored-By: Ilya <darpa@yandex.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.