Remove workaround for fixed invalid json array deserializing bug by adamgauthier · Pull Request #8346 · PowerShell/PowerShell

adamgauthier · 2018-11-28T09:33:07Z

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

PaulHigin

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

adamgauthier · 2018-11-29T02:15:57Z

@PaulHigin done!

PaulHigin

LGTM

iSazonov · 2018-11-30T11:49:51Z

@louistio Thanks for your contribution!

daxian-dbw · 2018-11-30T20:15:43Z

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

TravisEz13 · 2018-11-30T20:56:59Z

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

…rializing bug (PowerShell#8346)" This reverts commit 60a4e2f.

adamgauthier · 2018-11-30T22:05:23Z

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.

…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 · 2018-12-01T00:44:57Z

@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 · 2018-12-01T09:02:24Z

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?

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

Remove workaround for fixed invalid json array deserializing bug

fe0836d

adamgauthier requested review from JamesWTruher and PaulHigin as code owners November 28, 2018 09:33

PaulHigin suggested changes Nov 28, 2018

View reviewed changes

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

ac76a36

iSazonov approved these changes Nov 29, 2018

View reviewed changes

iSazonov self-assigned this Nov 29, 2018

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 approved these changes Nov 29, 2018

View reviewed changes

iSazonov merged commit 60a4e2f into PowerShell:master Nov 30, 2018

daxian-dbw mentioned this pull request Nov 30, 2018

Improve powershell startup time #8341

Merged

11 tasks

TravisEz13 mentioned this pull request Nov 30, 2018

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

Merged

11 tasks

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.

adamgauthier mentioned this pull request Dec 1, 2018

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

Merged

6 tasks

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

adityapatwardhan added the CL-NotInBuild Indicates that a PR is reverted and not part of the build. label Dec 3, 2018

daxian-dbw mentioned this pull request Jul 25, 2022

Remove pre-parse for array-based JSON #15684

Merged

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove workaround for fixed invalid json array deserializing bug#8346

Remove workaround for fixed invalid json array deserializing bug#8346
iSazonov merged 2 commits intoPowerShell:masterfrom
adamgauthier:remove-jarray-bug-workaround

adamgauthier commented Nov 28, 2018

Uh oh!

PaulHigin left a comment

Uh oh!

adamgauthier commented Nov 29, 2018

Uh oh!

PaulHigin left a comment

Uh oh!

iSazonov commented Nov 30, 2018

Uh oh!

daxian-dbw commented Nov 30, 2018 •

edited

Loading

Uh oh!

TravisEz13 commented Nov 30, 2018

Uh oh!

adamgauthier commented Nov 30, 2018 •

edited

Loading

Uh oh!

TravisEz13 commented Dec 1, 2018

Uh oh!

iSazonov commented Dec 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

adamgauthier commented Nov 28, 2018

PR Summary

Current behavior

After workaround removal

PR Checklist

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

adamgauthier commented Nov 29, 2018

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Nov 30, 2018

Uh oh!

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

Uh oh!

Uh oh!

TravisEz13 commented Nov 30, 2018

Uh oh!

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

Uh oh!

Uh oh!

TravisEz13 commented Dec 1, 2018

Uh oh!

iSazonov commented Dec 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

daxian-dbw commented Nov 30, 2018 •

edited

Loading

adamgauthier commented Nov 30, 2018 •

edited

Loading