GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Remove workaround for fixed invalid json array deserializing bug by adamgauthier · Pull Request #8346 · PowerShell/PowerShell

Conversation

Adam Gauthier (adamgauthier) commented Nov 28, 2018

Copy link

Copy Markdown

Contributor

PR Summary

This PR removes a workaround that was added because of a bug in the underlying json deserializing library Newtonsoft.Json.

It is no longer required as the bug has been fixed in Newtonsoft.Json 10.0.3, see fix here.

This workaround was added in #3823 and involves using a regex to detect a json array and calling JArray.Parse before deserialization to throw an exception on an invalid array. As a result, removing it will increase ConvertFrom-Json & Invoke-RestMethod performance.

Current behavior

PS D:\> ConvertFrom-Json '["1",'
ConvertFrom-Json : Conversion from JSON failed with error: Unexpected end of content while loading JArray. Path '[0]', line 1, position 5.
At line:1 char:1
+ ConvertFrom-Json '["1",'
+ ~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [ConvertFrom-Json], ArgumentException
+ FullyQualifiedErrorId : System.ArgumentException,Microsoft.PowerShell.Commands.ConvertFromJsonCommand

After workaround removal

PS D:\> ConvertFrom-Json '["1",'
ConvertFrom-Json : Conversion from JSON failed with error: Unexpected end when reading token. Path ''.
At line:1 char:1
+ ConvertFrom-Json '["1",'
+ ~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [ConvertFrom-Json], ArgumentException
+ FullyQualifiedErrorId : System.ArgumentException,Microsoft.PowerShell.Commands.ConvertFromJsonCommand

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
User-facing changes
- Not Applicable
Testing - New and feature
- Not Applicable or can only be tested interactively

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Remove workaround for fixed invalid json array deserializing bug

fe0836d

Adam Gauthier (adamgauthier) requested review from James Truher (JamesWTruher) and Paul Higinbotham (PaulHigin) as code owners

November 28, 2018 09:33

Paul Higinbotham (PaulHigin) suggested changes

Nov 28, 2018

View reviewed changes

Paul Higinbotham (PaulHigin) left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add a test to ensure the expected exception is thrown on invalid array.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          [fixup!] Add ConvertFrom-Json test to cover incomplete array exception

ac76a36

Adam Gauthier (adamgauthier) commented Nov 29, 2018

Copy link

Copy Markdown

Contributor Author

Paul Higinbotham (@PaulHigin) done!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) approved these changes

Nov 29, 2018

View reviewed changes

Ilya (iSazonov) self-assigned this

Nov 29, 2018

Ilya (iSazonov) added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label

Nov 29, 2018

Paul Higinbotham (PaulHigin) approved these changes

Nov 29, 2018

View reviewed changes

Paul Higinbotham (PaulHigin) left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) merged commit 60a4e2f into PowerShell:master

Nov 30, 2018

Ilya (iSazonov) commented Nov 30, 2018

Copy link

Copy Markdown

Collaborator

Adam Gauthier (@louistio) Thanks for your contribution!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) commented Nov 30, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

It looks to me this PR causes the following feature-level test to fail:

PowerShell/test/powershell/Modules/Microsoft.PowerShell.Utility/Pester.Commands.Cmdlets.Json.Tests.ps1

Lines 1446 to 1459 in 3794a5e

	It "ConvertFrom-Json deserializes an array of PSObjects (in multiple lines) as a single string." {

	# Create an array of PSCustomObjects, and serialize it
	$array = [pscustomobject]@{ objectName = "object1Name"; objectValue = "object1Value" },
	[pscustomobject]@{ objectName = "object2Name"; objectValue = "object2Value" }

	# Serialize the array to a text file
	$filePath = Join-Path $TESTDRIVE test.json
	$array \| ConvertTo-Json \| Out-File $filePath -Encoding utf8

	# Read the object as an array of PSObjects and deserialize it.
	$result = Get-Content $filePath \| ConvertFrom-Json
	$result.Count \| Should -Be 2
	}

This PR didn't have [feature] tag in commit message, so the feature-level tests didn't run.

Here is the failure:

2018-11-30T19:46:25.1215536Z     [-] ConvertFrom-Json deserializes an array of PSObjects (in multiple lines) as a single string. 32ms
2018-11-30T19:46:25.1604605Z       JsonSerializationException: Unexpected end when reading JSON. Path '', line 1, position 3.
2018-11-30T19:46:25.1617847Z       ArgumentException: Conversion from JSON failed with error: Unexpected end when reading JSON. Path '', line 1, position 3.
2018-11-30T19:46:25.1618346Z       at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Modules/Microsoft.PowerShell.Utility/Pester.Commands.Cmdlets.Json.Tests.ps1: line 1457

Affected CIs:

Linux CI: https://powershell.visualstudio.com/PowerShell/_build/results?buildId=7147
macOS CI: https://powershell.visualstudio.com/PowerShell/_build/results?buildId=7146
Windows CI: https://powershell.visualstudio.com/PowerShell/_build/results?buildId=7148

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) mentioned this pull request

Nov 30, 2018

Improve powershell startup time #8341

Merged

8 of 11 tasks

Travis Plunk (TravisEz13) commented Nov 30, 2018

Copy link

Copy Markdown

Member

We already had feature tests for this case and they are failing after the removal of the workaround. I'm reverting the change.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Travis Plunk (TravisEz13) mentioned this pull request

Nov 30, 2018

Revert "Remove workaround for fixed invalid json array deserializing bug (#8346)" #8375

Merged

7 of 11 tasks

Travis Plunk (TravisEz13) added a commit to TravisEz13/PowerShell that referenced this pull request

Nov 30, 2018


          [Feature] Revert "Remove workaround for fixed invalid json array dese…

d37e871

…rializing bug (PowerShell#8346)"

This reverts commit 60a4e2f.

Adam Gauthier (adamgauthier) commented Nov 30, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

I'm sorry about this inconvenience, I was not aware of the existence of this particular test and should have been.

I debugged this issue locally as soon as I could, the new error is caused by yet another Newtonsoft.Json bug, which I have reported to their repository here. This means the workaround is still relevant to cover that other bug.

Sorry again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Travis Plunk (TravisEz13) added a commit that referenced this pull request

Nov 30, 2018


          Revert "Remove workaround for fixed invalid json array deserializing …

2a469b7

…bug (#8346)" (#8375)

Revert "Remove workaround for fixed invalid json array deserializing bug (#8346)"

This reverts commit 60a4e2f.

This change caused the test which verified the functionality this workaround enabled, to start failing

Travis Plunk (TravisEz13) commented Dec 1, 2018

Copy link

Copy Markdown

Member

Adam Gauthier (@louistio) No problem, things happen. To automatically run the tests, prefix the last commit with [feature].
I would be nice if the fix actually covered the scenario.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) commented Dec 1, 2018

Copy link

Copy Markdown

Collaborator

Oh, seems we need re-pack our tests or at least update the code comment. Adam Gauthier (@louistio) Could you please make PR and update the comment with new information?

adamgauthier reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Adam Gauthier (adamgauthier) mentioned this pull request

Dec 1, 2018

Change Newtonsoft deserializing bug comment to link to the new issue #8377

Merged

6 tasks done

Ilya (iSazonov) removed the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label

Dec 3, 2018

Travis Plunk (TravisEz13) pushed a commit that referenced this pull request

Dec 3, 2018


          Change Newtonsoft deserializing bug comment to link to the new issue (#…

d150871

…8377)

This is a follow up to reverted PR #8346. I removed a workaround that was used to avoid [a Newtonsoft.Json bug](JamesNK/Newtonsoft.Json#1321) that caused some incomplete json array input to successfully be deserialized.

This bug being fixed, I thought the workaround was unnecessary, but after the merge some feature tests started to fail (which I did not run prior to merging).

It was discovered that the workaround is still needed because while the first Newtonsoft.Json bug was fixed, some cases of incomplete json array input still behave unexpectedly, namely json input `[`. 

This PR is just to update the comment explaining the workaround so it links to the newly created issue [here](JamesNK/Newtonsoft.Json#1930).

Aditya Patwardhan (adityapatwardhan) added the CL-NotInBuild Indicates that a PR is reverted and not part of the build. label

Dec 3, 2018

Dongbo Wang (daxian-dbw) mentioned this pull request

Jul 25, 2022

Remove pre-parse for array-based JSON #15684

Merged

8 of 22 tasks

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Remove workaround for fixed invalid json array deserializing bug (Pow…

fbe7090

…erShell#8346)

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Revert "Remove workaround for fixed invalid json array deserializing …

d7c69ef

…bug (PowerShell#8346)" (PowerShell#8375)

Revert "Remove workaround for fixed invalid json array deserializing bug (PowerShell#8346)"

This reverts commit fbe7090.

This change caused the test which verified the functionality this workaround enabled, to start failing

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Change Newtonsoft deserializing bug comment to link to the new issue (P…

a9dc28d

…owerShell#8377)

This is a follow up to reverted PR PowerShell#8346. I removed a workaround that was used to avoid [a Newtonsoft.Json bug](JamesNK/Newtonsoft.Json#1321) that caused some incomplete json array input to successfully be deserialized.

This bug being fixed, I thought the workaround was unnecessary, but after the merge some feature tests started to fail (which I did not run prior to merging).

It was discovered that the workaround is still needed because while the first Newtonsoft.Json bug was fixed, some cases of incomplete json array input still behave unexpectedly, namely json input `[`. 

This PR is just to update the comment explaining the workaround so it links to the newly created issue [here](JamesNK/Newtonsoft.Json#1930).

Reviewers

Ilya (iSazonov)

iSazonov approved these changes

James Truher (JamesWTruher)

Awaiting requested review from JamesWTruher

+1 more reviewer

Paul Higinbotham (PaulHigin)

PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Ilya (iSazonov)

Labels

CL-NotInBuild

Indicates that a PR is reverted and not part of the build.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Adam Gauthier (adamgauthier) commented Nov 28, 2018

PR Summary

Current behavior

After workaround removal

PR Checklist

Uh oh!

Paul Higinbotham (PaulHigin) left a comment

Choose a reason for hiding this comment

Uh oh!

Adam Gauthier (adamgauthier) commented Nov 29, 2018

Uh oh!

Paul Higinbotham (PaulHigin) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Nov 30, 2018

Uh oh!

Dongbo Wang (daxian-dbw) commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Travis Plunk (TravisEz13) commented Nov 30, 2018

Uh oh!

Adam Gauthier (adamgauthier) commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Travis Plunk (TravisEz13) commented Dec 1, 2018

Uh oh!

Ilya (iSazonov) commented Dec 1, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Dongbo Wang (daxian-dbw) commented Nov 30, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Adam Gauthier (adamgauthier) commented Nov 30, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.