X Tutup
Skip to content

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

Closed
iSazonov wants to merge 4 commits intoPowerShell:masterfrom
iSazonov:test-json-to-core
Closed

Migrate Test-Json to System.Text.Json API#11397
iSazonov wants to merge 4 commits intoPowerShell:masterfrom
iSazonov:test-json-to-core

Conversation

@iSazonov
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov commented Dec 18, 2019 •
edited
Loading

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 iSazonov mentioned this pull request Dec 19, 2019
Closed
@beatcracker beatcracker mentioned this pull request Feb 23, 2020
Merged
14 tasks
@mklement0 mklement0 mentioned this pull request Mar 4, 2020
Closed
@iSazonov iSazonov force-pushed the test-json-to-core branch from f63a0ee to 4280157 Compare March 29, 2020 07:15
@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Mar 29, 2020

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

@iSazonov iSazonov force-pushed the test-json-to-core branch from 4280157 to 142aa60 Compare April 8, 2020 16:50
@iSazonov iSazonov force-pushed the test-json-to-core branch from 142aa60 to daf3271 Compare April 30, 2020 11:42
vexx32
vexx32 reviewed Apr 30, 2020
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/Test-Json.Tests.ps1 Outdated
Comment on lines 109 to 162
Copy link
Copy Markdown
Collaborator

@vexx32 vexx32 Apr 30, 2020

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

@iSazonov iSazonov Apr 30, 2020

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

@vexx32 vexx32 Apr 30, 2020

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

@iSazonov iSazonov Apr 30, 2020

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

@iSazonov iSazonov force-pushed the test-json-to-core branch from 5fe5849 to 8e403e7 Compare May 6, 2020 06:24
@iSazonov iSazonov force-pushed the test-json-to-core branch from 8e403e7 to cfdb351 Compare May 15, 2020 12:58
@ghost ghost added the Review - Needed The PR is being reviewed label May 27, 2020
@ghost
Copy link
Copy Markdown

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 iSazonov mentioned this pull request May 27, 2020
Closed
14 tasks
@iSazonov iSazonov force-pushed the test-json-to-core branch from cfdb351 to d9d8936 Compare June 11, 2020 11:31
@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Jun 11, 2020 •
edited
Loading

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
Merged
14 tasks
@ghost ghost added the Review - Needed The PR is being reviewed label Jun 18, 2020
@ghost
Copy link
Copy Markdown

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

@adityapatwardhan adityapatwardhan removed this from the 7.1.0-preview.4 milestone Jun 29, 2020
@iSazonov iSazonov force-pushed the test-json-to-core branch from d9d8936 to 7122071 Compare August 7, 2020 11:50
@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Oct 23, 2020

Rebased to move 5.0 RC2.

@ghost ghost removed the Review - Needed The PR is being reviewed label Oct 23, 2020
@iSazonov iSazonov mentioned this pull request Oct 26, 2020
Closed
@ghost ghost added the Review - Needed The PR is being reviewed label Nov 3, 2020
@ghost
Copy link
Copy Markdown

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

hilari0n commented Feb 16, 2021

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
@dkaszews dkaszews mentioned this pull request Jul 21, 2022
Merged
22 tasks
@iSazonov iSazonov mentioned this pull request Sep 6, 2022
Closed
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

3 more reviewers

@vexx32 vexx32 vexx32 left review comments

@KalleOlaviNiemitalo KalleOlaviNiemitalo KalleOlaviNiemitalo left review comments

@PoshChan PoshChan PoshChan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Test-Json doesn't recognize JSON arrays or primitives

7 participants

@iSazonov @hilari0n @vexx32 @KalleOlaviNiemitalo @PoshChan @TravisEz13 @adityapatwardhan
X Tutup