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

Migrate Test-Json to System.Text.Json API #11397

Closed
wants to merge 4 commits into from

Conversation

iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Dec 18, 2019

PR Summary

Before the change we used Newtonsoft Json.NET JObject.Parse() method to validate Json. The method is not best choice to validate Json because it doesn't parse valid primitives.

As result we:

  • migrate the cmdlet to new Core API - System.Text.Json. JsonDocument.Parse()
  • change old tests to follow JSON standard (use double quotes as delimiters instead of single quotes)
  • add new tests
  • enable nullable reference types to follow best practice

Formally it is a breaking change because Newtonsoft Json.NET JObject.Parse() accepted single quote delimiters.

PR Context

Fix #11384

Need review #5797

.Net docs https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-migrate-from-newtonsoft-how-to

PR Checklist

@iSazonov iSazonov added CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Dec 18, 2019
@iSazonov iSazonov added this to the 7.1.0-preview.1 milestone Dec 18, 2019
@iSazonov iSazonov self-assigned this Dec 18, 2019
@iSazonov
Copy link
Collaborator Author

@mklement0 @jochenvanwylick @thedavecarroll @vexx32 The PR was moved to .Net Runtime 5.0 Preview1. Please play with compiled artifacts and feedback.

Comment on lines 109 to 162
@{ name = '"true"'; value = '"true"' }
@{ name = '"false"'; value = '"false"' }
@{ name = '"null"'; value = '"null"' }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these values be unquoted? Doesn't JSON accept true/false/null as primitive values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you thing about ConvertTo-Json. Test-Json accepts string input.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but that's already tested in the case below. Is this not intended to verify that it accepts the true/false/null literals?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see your point.
Add new tests

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

ghost commented May 27, 2020

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

@iSazonov
Copy link
Collaborator Author

iSazonov commented Jun 11, 2020

Rebase to resolve merge conflicts.

@ghost ghost removed the Review - Needed The PR is being reviewed label Jun 11, 2020
@iSazonov iSazonov mentioned this pull request Jun 11, 2020
14 tasks
@ghost ghost added the Review - Needed The PR is being reviewed label Jun 18, 2020
@ghost
Copy link

ghost commented Jun 18, 2020

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

@iSazonov
Copy link
Collaborator Author

Rebased to move 5.0 RC2.

@ghost ghost added the Review - Needed The PR is being reviewed label Nov 3, 2020
@ghost
Copy link

ghost commented Nov 3, 2020

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

@hilari0n
Copy link

Doesn't this change actually cause a performance drop as it causes the validated JSON to be parsed twice in ProcessRecord?
I.e. it's parsed first time explicitly, using JsonDocument.Parse(Json) (System.Text.Json parser), and then when _jschema.Validate(Json) is called (from NJsonSchema.JsonSchema, which internally parses it - I'm not sure, if it uses System.Text.Json parser or the Newtonsoft.Json parser).

@iSazonov iSazonov closed this Apr 16, 2022
@ghost ghost removed the Review - Needed The PR is being reviewed label Apr 16, 2022
@iSazonov iSazonov deleted the test-json-to-core branch April 16, 2022 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test-Json doesn't recognize JSON arrays or primitives
7 participants