Add -AsHashtable switch to ConvertFrom-Json for issue #3623 #5043

bergmeister · 2017-10-06T22:49:46Z

Address #3623.

Implementation symmetric to traditional code path that returns a PSCustomObject. Code is shared on the top level but 2 low level methods were difficult to share, therefore 2 separate but similar methods were created for HashTables.
All existing tests related to this change were adapted to also test against this new switch and were slightly improved.

Update (@TravisEz13):
-AsHashtable is an existing pattern used in Group-Object

iSazonov

@bergmeister Thanks for great contribution!

iSazonov · 2017-10-09T09:23:42Z

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

        [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")]
-        public static object ConvertFromJson(string input, out ErrorRecord error)
+        public static object ConvertFromJson(string input, bool returnHashTable, out ErrorRecord error)


We shouldn't change a public API. Please add new overload.

Thanks. I did not know PowerShell also exposes some of its classes. I'll re-add the old signature API that will then internally call the new overload.

iSazonov · 2017-10-09T09:29:00Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

+        @{ name = "object"; str = "{a:1}"; ReturnHashTable = $true  }
+        @{ name = "empty";  str = "";      ReturnHashTable = $false }
+        @{ name = "spaces"; str = "  ";    ReturnHashTable = $false }
+        @{ name = "object"; str = "{a:1}"; ReturnHashTable = $false }
    )


Please move to BeforeAll block.

I am not sure what you exactly mean by that. I removed the redundant line about `$TestCasesForReturnHashTableParameter that was a leftover.

Please see sample
We should place $TestCasesForReturnHashTableParameter and $validStrings in the BeforeAll block.

iSazonov · 2017-10-09T09:31:38Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

    )

-    It 'no error for valid string - <name>' -TestCase $validStrings {
-        param ($str)
+    It 'no error for valid string - <name> when ReturnHashTable is <ReturnHashTable>' -TestCase $validStrings {


Maybe 'no error for valid string - with -ReturnHashTable:`$<ReturnHashTable>'

I changed to match your suggestion whilst still displaying the name of the string.

iSazonov · 2017-10-09T09:36:24Z

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

+        /// <summary>
+        /// Returned data structure is a Hashtable instead a CustomPSObject.
+        /// </summary>
+        [Parameter(Mandatory = false)]


It is default. Please remove.

What do you mean by that? Can you elaborate please (what is 'it')?

[Parameter(Mandatory = false)] is default

Ok. I see, thanks.

iSazonov · 2017-10-09T09:42:00Z

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

+                        else
+                        {
+                            obj = PopulateFromJArray(list, out error);
+                        }
                    }


Please add 'else' with Diagnostics.Assert (if the object is not JObject or JArray).

Because of the cast to JArray a few lines above, obj cannot be something else. I initially put your suggestion in but even the compiler tells me that:
error CS0184: The given expression is never of the provided ('JObject') type

I meant:

Diagnostics.Assert( false, "Only JObject or JArray is supported.");

…minor suggestions in test naming

…tch since the parameter is not mandatory by default

…ld have been only the Mandatory=false part that should be removed.

iSazonov · 2017-10-10T05:39:58Z

...erShell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs

@@ -171,7 +171,7 @@ private bool TryConvertToJson(string json, out object obj, ref Exception exRef)
            try
            {
                ErrorRecord error;
-                obj = JsonObject.ConvertFromJson(json, out error);
+                obj = JsonObject.ConvertFromJson(json, false, out error);


Please revert the change - we have the overload.

Yes, you're right.

iSazonov · 2017-10-10T05:48:08Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

+        @{ name = "plain text"; str = "plaintext"; ReturnHashTable = $true  }
+        @{ name = "part";       str = '{"a" :';    ReturnHashTable = $true  }
+        @{ name = "plain text"; str = "plaintext"; ReturnHashTable = $false }
+        @{ name = "part";       str = '{"a" :';    ReturnHashTable = $false }


I see we have a check for input == null but haven't the test - please add new test with str = $null.

…that is not necessary any more since the original method signature has been restored. -Improve test to add case for $null

TravisEz13 · 2017-10-10T20:36:19Z

-AsHashtable is an existing pattern used in Group-Object. I don't think this needs committee review.

iSazonov · 2017-10-11T14:22:04Z

We have discussion for this #3623

TravisEz13 · 2017-10-16T18:43:29Z

@dantraMSFT Can you review this?

SteveL-MSFT · 2017-10-17T04:33:01Z

As part of this PR, if we can detect that the json has an property named empty string for the pscustomobject path and provide an error message like:

The provided json includes a property who name is an empty string, this is only supported using the `-AsHashTable` switch.

Then we can resolve #1755 as fixed.

… -AsHashTable via error message. Fixed a typo that I found in the message strings.

SteveL-MSFT · 2017-10-17T22:53:55Z

src/Microsoft.PowerShell.Commands.Utility/resources/WebCmdletStrings.resx

    <value>Path '{0}' resolves to a directory. Specify a path including a file name, and then retry the command.</value>
  </data>
+  <data name="EmptyKeyInJsonString" xml:space="preserve">
+    <value>The provided json includes a property whose name is an empty string, this is only supported using the -AsHashTable switch.</value>


Should probably capitalize json as JSON

OK. Fixed it.

iSazonov

Do we address the comment about case sensitivity?

iSazonov · 2017-10-18T03:43:10Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

    )

-    It 'no error for valid string - <name>' -TestCase $validStrings {
-        param ($str)
+    It 'no error for valid string ''<name>'' with -ReturnHashTable:$<ReturnHashTable>' -TestCase $validStrings {


Please start test descriptions with capital.

We use $validStrings one time. Please use our common template:

It 'no error for valid string ''<name>'' with -ReturnHashTable:$<ReturnHashTable>' -TestCase @( @{ name = "null"; str = $null; ReturnHashTable = $true } ... ) { ... }

iSazonov · 2017-10-18T03:46:40Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

    )

-    It 'throw ArgumentException for invalid string - <name>' -TestCase $invalidStrings  {
-        param ($str)
+    It 'throw ArgumentException for invalid string ''<name>'' with -ReturnHashTable:$<ReturnHashTable>' -TestCase $invalidStrings  {


iSazonov · 2017-10-18T03:49:51Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

+        { [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($str, $ReturnHashTable, [ref]$errRecord) } | ShouldBeErrorId "ArgumentException"
+    }
+
+    $jsonWithEmptyKey = '{"": "Value"}'


We should put variables in BeforeAll block or in It block.

iSazonov · 2017-10-18T03:54:37Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

+    It 'throw InvalidOperationException when json contains empty key name' {
+        $errorRecord = $null
+        [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($jsonWithEmptyKey, [ref]$errorRecord)
+        $errorRecord.Exception.GetType() | Should Be System.InvalidOperationException


Usually we don't check an exception types. Please remove.
(For type check we use Should BeOfType)

iSazonov · 2017-10-18T03:55:24Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

+        $errorRecord = $null
+        [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($jsonWithEmptyKey, [ref]$errorRecord)
+        $errorRecord.Exception.GetType() | Should Be System.InvalidOperationException
+        $errorRecord.FullyQualifiedErrorId | Should Be EmptyKeyInJsonString


Please put EmptyKeyInJsonString in quotas. It is more readable.

iSazonov · 2017-10-18T03:57:20Z

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertFrom-Json.Tests.ps1

-    It 'can convert a single-line object' {
-        ('{"a" : "1"}' | ConvertFrom-Json).a | Should Be 1
+
+    $testCasesWithAndWithoutAsHashtableSwitch = @(


Please put this in BeforeAll block.

iSazonov · 2017-10-18T03:57:51Z

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertFrom-Json.Tests.ps1

+        @{ AsHashtable = $false }
+    )
+
+    It 'can convert a single-line object with AsHashtable switch set to <AsHashtable>' -TestCase $testCasesWithAndWithoutAsHashtableSwitch {


Please start test descriptions with capital.

iSazonov · 2017-10-19T06:25:49Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

+        $jsonWithEmptyKey = '{"": "Value"}'
+    }
+
+


Please remove extra line.

iSazonov · 2017-10-19T06:31:16Z

@SteveL-MSFT @TravisEz13 @dantraMSFT Could you please review?

Also should we address the comment from @powercode

Apart from object size and performance, it may matter with case sensitivity. I have run into issues where I couldn't use the PowerShell cmdlets because the json contained 'Property' and 'property'.

@iSazonov

Removed extra newline in pester test as requested by @iSazonov

iSazonov · 2017-10-19T06:50:01Z

@bergmeister Thanks for your contribution! LGTM with above question about case sensitivity.

SteveL-MSFT · 2017-10-21T00:45:00Z

@iSazonov @powercode regarding case sensitivity, I think we should treat it the same as the empty key. [Hashtable] allows two keys differing in case, so if we detect that, we should just tell the user to use -AsHashTable.

…found out that newtonsoft.json would squash keys that have the same casing into one and just use the last value. I still handle this case should the behaviour change.

bergmeister · 2017-10-22T15:35:00Z

@SteveL-MSFT @iSazonov @powercode I added a specific error message to point the user to the new -AsHashTable option.
As part of this I also found out that actually no error is thrown in the case of duplicate keys with the same casing. This is due to the behaviour of Newtonsoft.Json that just uses the last item if there are multiple items with the same key of the same casing. The PowerShell code currently caters for it should this behaviour change in a newer version of Newtonsoft.Json. This should definitely be discussed in another issue if you want to make a breaking change to throw when encountering case matching duplicate keys.

iSazonov

@bergmeister Thanks for your contribution! I believe we are near to merge.
Feel free to open new Issue to discuss the discovered problem.

iSazonov · 2017-10-22T18:07:44Z

src/Microsoft.PowerShell.Commands.Utility/resources/WebCmdletStrings.resx

+    <value>Cannot convert the JSON string because a dictionary that was converted from the string contains the duplicated key '{0}'.</value>
+  </data>
+  <data name="KeysWithDifferentCasingInJsonString" xml:space="preserve">
+    <value>Cannot convert the JSON string because a PSObject does not support keys with different casing. Please use the -AsHashTable switch instead. The key that was attempted to be added to the exisiting key '{0}' was '{1}'.</value>


Oh, very long message!

I know. I shortened it a bit but I think the last sentence still adds value.

iSazonov · 2017-10-22T18:13:00Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

+        It 'Not throw when json contains key (same casing) when ReturnHashTable is true' {
+            $errorRecord = $null
+            $result = [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($jsonContainingKeysWithDifferentCasing, $true, [ref]$errorRecord)
+            $result | Should Not Be $null


I think it makes sense to check the hashtable values.

SteveL-MSFT · 2017-10-22T18:13:58Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

        $errRecord = $null
-        [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($str, [ref]$errRecord)
+        [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($str, $ReturnHashTable, [ref]$errRecord)


Please validate the returned object

You should capture the output as $result and validate it is what you expect

SteveL-MSFT · 2017-10-22T18:14:36Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

+        It 'Not throw when json contains empty key name when ReturnHashTable is true' {
+            $errorRecord = $null
+            $result = [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($jsonWithEmptyKey, $true, [ref]$errorRecord)
+            $result | Should Not Be $null


Please validate the returned object

SteveL-MSFT · 2017-10-22T18:14:42Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

+        It 'Not throw when json contains key (same casing) when ReturnHashTable is true' {
+            $errorRecord = $null
+            $result = [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($jsonContainingKeysWithDifferentCasing, $true, [ref]$errorRecord)
+            $result | Should Not Be $null


Please validate the returned object

This one is resolved. Thanks.

…quested in PR. Unfortunately, Pester (3.4) does not seem to support equality assertion for hashtables...

bergmeister · 2017-10-22T20:25:38Z

@iSazonov @SteveL-MSFT I added more validation to the positive test cases and shortened the error message a bit. I opened issue #5199 for the problem with Newtonsoft.Json for keys with matching cases, which is unrelated to this PR.

SteveL-MSFT · 2017-10-22T22:01:38Z

test/powershell/Modules/Microsoft.PowerShell.Utility/JsonObject.Tests.ps1

+            $result = [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($jsonWithEmptyKey, $true, [ref]$errorRecord)
+            $result | Should Not Be $null
+            $result.Count | Should Be 1
+            $result.$('') | Should Be 'Value'


You should simplify this to just: $result."" | Should Be 'Value'

bergmeister · 2017-11-14T12:26:17Z

@SteveL-MSFT I see that the documentation here has not been updated yet for the new -AsHashTable switch (or is it already done in another branch?). Am I supposed to do that or shall I simply open an issue in the docs repo?

iSazonov · 2017-11-14T13:38:17Z

@bergmeister Feel free to open an Issue in docs repo or push PR with the fix.

SteveL-MSFT · 2017-11-14T17:47:38Z

@bergmeister preferably submit a PR to powershell-docs repo. The Documentation Needed label here means you don't need to open an issue in the powershell-docs repo. But my team won't be reviewing and adding docs until sometime after RC is out. If you submit a PR for the doc update, please reference this PR so we can remove the Documentation Needed label. Thanks!

Add -AsHashtable switch to ConvertFrom-Json for issue 3623

d06de27

bergmeister requested review from dantraMSFT, JamesWTruher and PaulHigin as code owners October 6, 2017 22:49

iSazonov assigned TravisEz13 Oct 9, 2017

iSazonov reviewed Oct 9, 2017

View reviewed changes

bergmeister added 3 commits October 9, 2017 22:13

Keep old ConvertFromJson API for backwards compatibility and address …

b8dd5b6

…minor suggestions in test naming

Remove redundant '[Parameter(Mandatory = false)]' on -AsHashtable swi…

a709c29

…tch since the parameter is not mandatory by default

Last commit accidentally removed entirely [Parameter()] line, it shou…

e1fe619

…ld have been only the Mandatory=false part that should be removed.

iSazonov reviewed Oct 10, 2017

View reviewed changes

-Revert change in InvokeRestMethodCommand originally made in this PR …

c334128

…that is not necessary any more since the original method signature has been restored. -Improve test to add case for $null

TravisEz13 added WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module Review - Committee The PR/Issue needs a review from the PowerShell Committee and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 10, 2017

Detect if key is empty string to resolve 1755 issue by redirecting to…

0d5a6fb

… -AsHashTable via error message. Fixed a typo that I found in the message strings.

SteveL-MSFT requested changes Oct 17, 2017

View reviewed changes

bergmeister added 2 commits October 17, 2017 23:56

Fix test syntax and simplify test name

af44072

make 'json' uppercase 'JSON' in error message as suggested in PR review.

123128c

iSazonov reviewed Oct 18, 2017

View reviewed changes

Address PR comments about tests (mostly style) by @iSazonov

69df716

iSazonov reviewed Oct 19, 2017

View reviewed changes

Update JsonObject.Tests.ps1

7d77ae2

Removed extra newline in pester test as requested by @iSazonov

Report error when keys have different casing. As part of this I also …

db0d6ce

…found out that newtonsoft.json would squash keys that have the same casing into one and just use the last value. I still handle this case should the behaviour change.

iSazonov reviewed Oct 22, 2017

View reviewed changes

SteveL-MSFT requested changes Oct 22, 2017

View reviewed changes

Add more validation to positive tests and shorten error message as re…

dfa9126

…quested in PR. Unfortunately, Pester (3.4) does not seem to support equality assertion for hashtables...

SteveL-MSFT reviewed Oct 22, 2017

View reviewed changes

Minor syntax simplification in jsonobject.tests.ps1

935054b

iSazonov approved these changes Oct 23, 2017

View reviewed changes

SteveL-MSFT approved these changes Oct 24, 2017

View reviewed changes

TravisEz13 merged commit f5f0d3a into PowerShell:master Oct 25, 2017

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Nov 14, 2017

This was referenced Nov 14, 2017

ConvertFrom-Json fails to parse project.lock.json #1755

Closed

Add documentation of new -AsHashtable switch for ConvertFrom-Json introduced by PR #5043 MicrosoftDocs/PowerShell-Docs#1858

Merged

markekraus mentioned this pull request Feb 20, 2018

ConvertFrom-Json should have option to produce hashtables instead of objects #3159

Closed

strawgate mentioned this pull request May 25, 2018

Hashtables resulting from ConvertFrom-JSON -AsHashtable have case sensitive keys #6928

Closed

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Add -AsHashtable switch to ConvertFrom-Json for issue #3623' MicrosoftDocs/PowerShell-Docs#3079

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

Add -AsHashtable switch to ConvertFrom-Json for issue #3623 #5043

Add -AsHashtable switch to ConvertFrom-Json for issue #3623 #5043

Conversation

bergmeister commented Oct 6, 2017 • edited by iSazonov

iSazonov left a 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

iSazonov Oct 10, 2017 • edited

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

TravisEz13 commented Oct 10, 2017 • edited

iSazonov commented Oct 11, 2017

TravisEz13 commented Oct 16, 2017

SteveL-MSFT commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov left a comment • edited

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

iSazonov commented Oct 19, 2017

iSazonov commented Oct 19, 2017

SteveL-MSFT commented Oct 21, 2017

bergmeister commented Oct 22, 2017

iSazonov left a comment • edited

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

bergmeister commented Oct 22, 2017

Choose a reason for hiding this comment

bergmeister commented Nov 14, 2017

iSazonov commented Nov 14, 2017

SteveL-MSFT commented Nov 14, 2017

bergmeister commented Oct 6, 2017 •

edited by iSazonov

iSazonov Oct 10, 2017 •

edited

TravisEz13 commented Oct 10, 2017 •

edited

iSazonov left a comment •

edited

iSazonov left a comment •

edited