Make ConvertFrom-Json deserialize an array of objects with multiple lines. #3823

Francisco-Gamino · 2017-05-19T16:24:21Z

Addressing issue #3284: ConvertFrom-JSON fails to parse syntactically-correct JSON array

For PowerShell on CoreCLR, we use Json.Net. However, this deserialzer does not throw an exception if an invalid array is passed, so we need to add extra logic to handle this. An invalid array can occurred when the user reads a file via get-content as a collection of PSObjects. E.g.,

# test.json has the following content:
PS D:\> Get-Content .\test.json
[
    1,
    2
]

# The output of Get-Content is a collection of Object[]
PS D:\> (Get-Content .\test.json).GetType()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     True     Object[]                                 System.Array

# When we pipe the output of Get-Content to ConvertFrom-Json, the logic in the cmdlet
# sends the first object to be deserialized, and if an Argument exception 
# is thrown, then it sends the whole collection of objects as one single Json content. 
# For CoreCLR PowerShell, we use Json.Net which does not throw an error when an 
# invalid array is deserialize, so this PR addresses this issue. 

# Here is the output of ConvertFrom-Json after this code change:
PS D:\> Get-Content .\test.json | ConvertFrom-Json
1
2

Added tests for the following:

Deserialize a Json array of strings (in multiple lines)
Deserialize a Json array of PSObjects (in multiple lines)

adityapatwardhan · 2017-05-19T17:05:32Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

Should this be a && so we check for existence of [ and ] ? If any of those is missing it is an invalid array anyways.

No, this should be an '||' because we are trying to detect an invalid array. These are some examples:

'["a"'

'['

'"a"]'

adityapatwardhan · 2017-05-19T17:06:59Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

Trim will only remove newline characters at the start and end. To remove all newline characters use Replace instead. And .ToCharArray() is unnecessary.

adityapatwardhan · 2017-05-19T17:11:11Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

Use a String.Empty instead of "". More efficient as it does not create a new zero length string.

adityapatwardhan · 2017-05-19T17:16:21Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

Maybe a && here as well?

Same as the comment above.

adityapatwardhan · 2017-05-19T17:22:03Z

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

Use [Environment]::NewLine instead

Test code updated as per Jim's recommendation below.

adityapatwardhan · 2017-05-19T17:23:58Z

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

Can the variations be converted to use -TestCase for It? So if one of the Should fails the others are still run.

I've implemented two different test cases instead.

JamesWTruher · 2017-05-19T17:48:57Z

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

this should just be:

$array = [pscustomobject]@{ objectName = "object1Name"; objectValue = "object1Value" }, [pscustomobject]@{ objectName = "object2Name"; objectValue = "object2Value" }

Great suggestion. I will update the test code.

JamesWTruher · 2017-05-19T17:55:16Z

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

I'm wondering if this (or the above) needs to be done with a file at all. Does the following cover the case?

$result = "[1,","2,","3]" | convertfrom-json $result.count | Should be 3

Test code updated.

adityapatwardhan

LGTM

JamesWTruher · 2017-05-19T20:19:41Z

tests are much cleaner now - LGTM

SteveL-MSFT · 2017-05-19T20:37:27Z

@JamesWTruher can you mark your review as Approved?

Francisco-Gamino · 2017-05-19T21:49:42Z

@PowerShell/powershell-maintainers : This is ready to be merged, please let me know if you need anything else. Thanks.

lzybkr · 2017-05-19T22:27:45Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

The comment says contains, but the code says starts with/ends with.

Besides that, it just looks more complicated than it needs to be, e.g. 2 calls to IndexOf, then strip newlines then spaces. Why not just use a regex, like:

if (Regex.Match(input, "(^\s*\[)|(\s*\]$)")) { JArray.Parse(input); }

I'd also like to see some sort of note about whether or not this is considered a NewtonSoft bug - ideally a link to an issue so we can track it.

daxian-dbw · 2017-05-19T22:20:20Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

as these markers for

Do you mean these markers are for?

daxian-dbw · 2017-05-19T22:21:57Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

The comment doesn't match the code. Carriage return is \r, but Environemnt.NewLine is \r\n on windows and \n on Unix. And what if the json string is has \r\n but we are calling ConvertFrom-Json on Unix?

daxian-dbw · 2017-05-19T22:23:33Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

Do we care about other white spaces? For example what if there is \t?
By the way, new lines are white spaces too.

daxian-dbw · 2017-05-19T22:25:16Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

white spaces include new lines.

[char]::IsWhiteSpace([environment]::NewLine, 0) True

daxian-dbw · 2017-05-19T22:28:17Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

is can be

Typo

daxian-dbw · 2017-05-19T22:29:54Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

The comments here are confusing to me -- what is the buffer here? Could you please make the comments easier to understand?

daxian-dbw · 2017-05-19T22:34:02Z

@Francisco-Gamino One more question -- why do you have to remove all white spaces before calling JArray.Parse? Won't it be enough to just trim the leading and trailing white spaces?

TravisEz13 · 2017-05-19T22:34:51Z

Please update the title and description based on the instructions in Contributing - Pull Request Submission

Remove the issues number from the title and into the description (it's already in the description, so, just remove it from the title.)
Update the title to use "the present tense and imperative mood" to describe the change, not the issue you are fixing.

daxian-dbw · 2017-05-19T23:52:22Z

Chatted with @Francisco-Gamino offline, and it turns out the JsonException thrown by JArray.Parse will be captured and re-thrown as an ArgumentException, and then the enclosing code will capture the ArgumentException and assume that it's caused by partial input and hen keep accumulating pipeline input. Francisco will add more comments to explain this process.

…of strings which represent a JSON content.

…s as a single string.

Francisco-Gamino · 2017-05-24T20:25:05Z

@lzybkr: Thank you Jason for your suggestion. Using regular expressions makes the code much simpler.
@lzybkr, @daxian-dbw, and @TravisEz13 : Could you please take another look?

daxian-dbw · 2017-05-26T00:40:02Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

+                // To work around this, we need to identify when input is a Json array, and then try to parse it via JArray.Parse().
+
+                // If input starts with '[' or ends with ']' (ignoring white spaces).
+                if ((Regex.Match(input, @"(^\s*\[)|(\s*\]$)")).Success)


The reg expression used here seems not right ... @Francisco-Gamino let's discuss it tomorrow.

Francisco-Gamino requested review from JamesWTruher, adityapatwardhan and dantraMSFT May 19, 2017 16:24

msftclas added the cla-not-required label May 19, 2017

adityapatwardhan requested changes May 19, 2017

View reviewed changes

JamesWTruher reviewed May 19, 2017

View reviewed changes

adityapatwardhan approved these changes May 19, 2017

View reviewed changes

TravisEz13 self-assigned this May 19, 2017

lzybkr reviewed May 19, 2017

View reviewed changes

daxian-dbw reviewed May 19, 2017

View reviewed changes

Francisco-Gamino added 2 commits May 24, 2017 12:57

Fixing ConvertFrom-Json on CoreCLR to be able to handle a collection …

8f42a20

…of strings which represent a JSON content.

Adding test case for ConvertFrom-Json to process an array of PSObject…

91a0877

…s as a single string.

Francisco-Gamino changed the title ~~Fixing #3284 ConvertFrom-JSON fails to parse syntactically-correct JSON array~~ ConvertFrom-Json deserializes an array of objects in multiple lines. May 24, 2017

Francisco-Gamino changed the title ~~ConvertFrom-Json deserializes an array of objects in multiple lines.~~ Make ConvertFrom-Json deserialize an array of objects with multiple lines. May 25, 2017

TravisEz13 merged commit 7aa7f38 into PowerShell:master May 25, 2017

daxian-dbw reviewed May 26, 2017

View reviewed changes

adamgauthier mentioned this pull request Nov 28, 2018

Remove workaround for fixed invalid json array deserializing bug #8346

Merged

7 tasks

Make ConvertFrom-Json deserialize an array of objects with multiple lines. #3823

Make ConvertFrom-Json deserialize an array of objects with multiple lines. #3823

Uh oh!

Conversation

Francisco-Gamino commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan left a comment

Choose a reason for hiding this comment

Uh oh!

JamesWTruher commented May 19, 2017

Uh oh!

SteveL-MSFT commented May 19, 2017

Uh oh!

Francisco-Gamino commented May 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented May 19, 2017

Uh oh!

TravisEz13 commented May 19, 2017 • edited by Francisco-Gamino Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented May 19, 2017

Uh oh!

Francisco-Gamino commented May 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Francisco-Gamino commented May 19, 2017 •

edited

Loading

TravisEz13 commented May 19, 2017 •

edited by Francisco-Gamino

Loading