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

            error = null;
 #if CORECLR
            object obj = null;
            try
            {
+                // JsonConvert.DeserializeObject does not throw an exception when an invalid array is passed,
+                // so we need to do extra logic to identify this scenario using JArray.Parse(input).
+                if ((input.IndexOf("[", StringComparison.OrdinalIgnoreCase) >= 0) ||


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

+                    (input.IndexOf("]", StringComparison.OrdinalIgnoreCase) >= 0))
+                {
+                    // Remove carriage return, new-line and empty space from the string.
+                    input = input.Trim(System.Environment.NewLine.ToCharArray());


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

+                    // Remove carriage return, new-line and empty space from the string.
+                    input = input.Trim(System.Environment.NewLine.ToCharArray());
+                    input = input.Replace(" ", "");
+


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

+                    // The input from the pipeline is can be represented in the following two ways:
+                    // 1. Each input to the buffer is a complete Json content. There can be multiple inputs of this format.
+                    // 2. The complete buffer input collectively represent a single JSon format. This is typically the majority of the case.
+                    if (input.StartsWith("[", StringComparison.OrdinalIgnoreCase) || input.EndsWith("]", StringComparison.OrdinalIgnoreCase))


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

+        3]
+        #>
+        $filePath = Join-Path $TESTDRIVE test2.json
+        "[1,`r`n2,`r`n3]" | Out-File $filePath


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

@@ -1455,4 +1454,43 @@ Describe "Json Bug fixes"  -Tags "Feature" {
    {
        RunJsonTest $testCase
    }
+
+    It "ConvertFrom-Json can parse an array of PSObjects as a single string." {
+


Can the variations be converted to us 10000 e -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

+        Add-Member -InputObject $obj2 -NotePropertyName "objectName" -NotePropertyValue "object2Name"
+        Add-Member -InputObject $obj2 -NotePropertyName "objectValue" -NotePropertyValue "object2Value"
+
+        # Add them to an array


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

+        2,
+        3]
+        #>
+        $filePath = Join-Path $TESTDRIVE test2.json


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

+                // JsonConvert.DeserializeObject does not throw an exception when an invalid array is passed,
+                // so we need to do extra logic to identify this scenario using JArray.Parse(input).
+
+                // If input contains '[' or ']', we would like to analyze it further as these markers for a Json data array.


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

+                // JsonConvert.DeserializeObject does not throw an exception when an invalid array is passed,
+                // so we need to do extra logic to identify this scenario using JArray.Parse(input).
+
+                // If input contains '[' or ']', we would like to analyze it further as these markers for a Json data array.


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

+                    (input.IndexOf("]", StringComparison.OrdinalIgnoreCase) >= 0))
+                {
+                    // Remove carriage return, new-line and empty space from the string.
+                    string stringToValidate = input.Replace(System.Environment.NewLine, string.Empty);


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

+                {
+                    // Remove carriage return, new-line and empty space from the string.
+                    string stringToValidate = input.Replace(System.Environment.NewLine, string.Empty);
+                    stringToValidate = stringToValidate.Replace(" ", string.Empty);


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

+                    string stringToValidate = input.Replace(System.Environment.NewLine, string.Empty);
+                    stringToValidate = stringToValidate.Replace(" ", string.Empty);
+
+                    // After removing all white space and new-lines, we are only interested in


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

+                        // Use JArray.Parse to see if this is a valid array. When an exception is thrown, this means that
+                        // we are potentially processing partial content -- this would be the case for number 2 below.
+
+                        // A Json input from the pipeline is can be represented in the following two ways:


is can be

Typo

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

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

+
+                        // A Json input from the pipeline is can be represented in the following two ways:
+                        // 1. Each input to the buffer is a complete Json content. There can be multiple inputs of this format.
+                        // 2. The complete buffer input collectively represent a single JSon format. This is typically the majority of the case.


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, dantraMSFT and adityapatwardhan 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

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

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment