8000 Remove workaround for fixed invalid json array deserializing bug by adamgauthier · Pull Request #8346 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Remove workaround for fixed invalid json array deserializing bug #8346

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 30, 2018
Merged

Remove workaround for fixed invalid json array deserializing bug #8346

merged 2 commits into from
Nov 30, 2018

Conversation

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

PaulHigin
PaulHigin suggested changes Nov 28, 2018
View reviewed changes
Copy link
Contributor
@PaulHigin PaulHigin left a 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.

@adamgauthier
Copy link
Contributor Author

@PaulHigin done!

@iSazonov iSazonov self-assigned this Nov 29, 2018
@iSazonov 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
PaulHigin
PaulHigin approved these changes Nov 29, 2018
View reviewed changes
Copy link
Contributor
@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM

@iSazonov iSazonov merged commit 60a4e2f into PowerShell:master Nov 30, 2018
@iSazonov
Copy link
Collaborator

@louistio Thanks for your contribution!

@daxian-dbw
Copy link
Member
daxian-dbw commented Nov 30, 2018 •
edited
Loading

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

@daxian-dbw daxian-dbw mentioned this pull request Nov 30, 2018
Merged
11 tasks
@TravisEz13
Copy link
Member

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

@TravisEz13 TravisEz13 mentioned this pull request Nov 30, 2018
Merged
11 tasks
TravisEz13 added a commit to TravisEz13/PowerShell that referenced this pull request Nov 30, 2018
@TravisEz13
[Feature] Revert "Remove workaround for fixed invalid json array dese…
d37e871 
…rializing bug (PowerShell#8346)"

This reverts commit 60a4e2f.
@adamgauthier
Copy link
Contributor Author
adamgauthier commented Nov 30, 2018 •
edited
Loading

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.

TravisEz13 added a commit that referenced this pull request Nov 30, 2018
@TravisEz13
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
@TravisEz13
Copy link
Member

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

@iSazonov
Copy link
Collaborator
iSazonov commented Dec 1, 2018

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

@adamgauthier adamgauthier mentioned this pull request Dec 1, 2018
Merged
6 tasks
@iSazonov 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
TravisEz13 pushed a commit that referenced this pull request Dec 3, 2018
@adamgauthier @TravisEz13
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).
@adityapatwardhan adityapatwardhan added the CL-NotInBuild Indicates that a PR is reverted and not part of the build. label Dec 3, 2018
@daxian-dbw daxian-dbw mentioned this pull request Jul 25, 2022
Merged
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulHigin PaulHigin PaulHigin approved these changes

@iSazonov iSazonov iSazonov approved these changes

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

Assignees

@iSazonov 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.
6 participants
@adamgauthier @iSazonov @daxian-dbw @TravisEz13 @PaulHigin @adityapatwardhan
0