Skip to content

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

Closed
Ilya (iSazonov) wants to merge 4 commits into
PowerShell:masterfrom
iSazonov:test-json-to-core
Closed

Migrate Test-Json to System.Text.Json API#11397
Ilya (iSazonov) wants to merge 4 commits into
PowerShell:masterfrom
iSazonov:test-json-to-core

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Dec 18, 2019

Copy link
Copy Markdown
Collaborator

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 Ilya (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 Ilya (iSazonov) added this to the 7.1.0-preview.1 milestone Dec 18, 2019
@iSazonov Ilya (iSazonov) self-assigned this Dec 18, 2019
@iSazonov

Copy link
Copy Markdown
Collaborator Author

Michael Klement (@mklement0) jochenvanwylick Dave Carroll (@thedavecarroll) Rain Sallow (/u/ta11ow) (@vexx32) The PR was moved to .Net Runtime 5.0 Preview1. Please play with compiled artifacts and feedback.

Comment on lines 109 to 162

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.

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

Ilya (iSazonov) commented Jun 11, 2020

Copy link
Copy Markdown
Collaborator Author

Rebase to resolve merge conflicts.

@ghost ghost removed the Review - Needed The PR is being reviewed label Jun 11, 2020
@iSazonov Ilya (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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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

@ghost ghost removed the Review - Needed The PR is being reviewed label Apr 16, 2022
@iSazonov
Ilya (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