Make ConvertFrom-Json deserialize an array of objects with multiple lines. by Francisco-Gamino · Pull Request #3823 · PowerShell/PowerShell · GitHub

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)

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"]'

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

Fixed.

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

Fixed.

Maybe a && here as well?

Same as the comment above.

Use [Environment]::NewLine instead

Test code updated as per Jim's recommendation below.

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.

this should just be:

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

Great suggestion. I will update the test code.

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.

LGTM

tests are much cleaner now - LGTM

James Truher (@JamesWTruher) can you mark your review as Approved?

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

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.

as these markers for

Do you mean these markers are for?

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?

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

white spaces include new lines.

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

is can be

Typo

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

Francisco Gamino (@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?

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.

Chatted with Francisco Gamino (@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.

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

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

…ines. (PowerShell#3823) * Fixing ConvertFrom-Json on CoreCLR to be able to handle a collection of strings which represent a JSON content. * Adding test case for ConvertFrom-Json to process an array of PSObjects as a single string.

Francisco Gamino (Francisco-Gamino) requested review from James Truher (JamesWTruher), Aditya Patwardhan (adityapatwardhan) and Dan Travison (dantraMSFT) May 19, 2017 16:24

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label May 19, 2017

Aditya Patwardhan (adityapatwardhan) requested changes May 19, 2017

View reviewed changes

James Truher (JamesWTruher) reviewed May 19, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) approved these changes May 19, 2017

View reviewed changes

Travis Plunk (TravisEz13) self-assigned this May 19, 2017

Jason Shirk (lzybkr) reviewed May 19, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) reviewed May 19, 2017

View reviewed changes

Francisco Gamino (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 (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 (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

Travis Plunk (TravisEz13) merged commit 7aa7f38 into PowerShell:master May 25, 2017

Dongbo Wang (daxian-dbw) reviewed May 26, 2017

View reviewed changes

Adam Gauthier (adamgauthier) mentioned this pull request Nov 28, 2018

Remove workaround for fixed invalid json array deserializing bug #8346

Merged

7 tasks

+                              // 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)

Make ConvertFrom-Json deserialize an array of objects with multiple lines. by Francisco-Gamino · Pull Request #3823 · PowerShell/PowerShell · GitHub

Uh oh!

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

Make ConvertFrom-Json deserialize an array of objects with multiple lines.#3823Travis Plunk (TravisEz13) merged 2 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom Francisco-Gamino:convertfrom-json-fixCopy head branch name to clipboard

Conversation

Francisco Gamino (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!

Aditya Patwardhan (adityapatwardhan) left a comment

Choose a reason for hiding this comment

Uh oh!

James Truher (JamesWTruher) commented May 19, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented May 19, 2017

Uh oh!

Francisco Gamino (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!

Dongbo Wang (daxian-dbw) commented May 19, 2017

Uh oh!

Travis Plunk (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!

Dongbo Wang (daxian-dbw) commented May 19, 2017

Uh oh!

Francisco Gamino (Francisco-Gamino) commented May 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Make ConvertFrom-Json deserialize an array of objects with multiple lines.#3823
Travis Plunk (TravisEz13) merged 2 commits into
PowerShell:masterfrom
Francisco-Gamino:convertfrom-json-fix

Francisco Gamino (Francisco-Gamino) commented May 19, 2017 •
edited
Loading

Travis Plunk (TravisEz13) commented May 19, 2017 •
edited by Francisco-Gamino
Loading