Invoke-RestMethod cmdlet error if the response is neither JSON nor XML by 2xmax · Pull Request #2862 · PowerShell/PowerShell

2xmax · Dec 9, 2016

This pull request fixes the Invoke-RestMethod cmdlet behavior if the response is neither XML nor JSON, e.g. just a plain text from a webservice.

Before the change:

PS> Invoke-RestMethod http://httpbin.org/encoding/utf8
Invoke-RestMethod : Unexpected character encountered while parsing value: <. Path '', line 0, position 0.
At line:1 char:1
+ Invoke-RestMethod http://httpbin.org/encoding/utf8
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  + CategoryInfo : NotSpecified: (:) [Invoke-RestMethod], JsonReaderException
  + FullyQualifiedErrorId : Newtonsoft.Json.JsonReaderException,Microsoft.PowerShell.Commands.InvokeRestMethodComman
  d

After the change:

PS> Invoke-RestMethod http://httpbin.org/encoding/utf8
<h1>Unicode Demo</h1>
...

…is neither xml nor json.

msftclas · Dec 9, 2016

Hi @2xmax, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

msftclas · Dec 9, 2016

Hi @2xmax, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

2xmax · Dec 11, 2016

Eliminated the duplication caused by platform-specific libs exceptions. Also, locally I wrote some C# unit tests for the JsonObject class related to the problem I was trying to solve but found only a few C# unit tests in the project for some reason. If it makes sense, I can bring in the C# unit tests as well.

iSazonov · Dec 11, 2016

-            // JArray is a collection
-            else if (obj is JArray)
+                // JObject is a IDictionary
+                if (obj is JObject)


Please avoid double casting.
Use this pattern:

var dictionary = obj as JObject; if (dictionary != null) {

iSazonov · Dec 11, 2016

+                }
+
+                // JArray is a collection
+                else if (obj is JArray)


Please avoid double casting.

Ok, I'm going to refactor this expression too. However, it will increase the nesting.

2xmax · Dec 12, 2016

@daxian-dbw , @iSazonov, it is ready for your review. Would you mind looking this over?

iSazonov · Dec 13, 2016

                        // Hence consider the the entire input as a single Json content.
                    }
-#if CORECLR
-                    catch (Newtonsoft.Json.JsonSerializationException)


Why we don't want to mask this exception?

Since it is already handled here. I decided to fix the behavior of JsonObject making them invariant of a platform rather than handling serialization exceptions for every platform. You can also take a look at the C# unit tests I mentioned above.

I like this change, so when using JsonObject.ConvertFromJson in C#, you don't need to catch different exceptions between FullCLR and CoreCLR.

Thanks. Clear.

iSazonov · Dec 13, 2016

@daxian-dbw I see in JsonObject.cs multiple double castings. Can we ask @2xmax to fix this or better make new PR?
Also in JsonObject.cs a null checks is absent
for theValue in line 142:

JValue theValue = entry.Value as JValue; 
result.Properties.Add(new PSNoteProperty(entry.Key, theValue.Value));

in line 184

result.Add(((JValue)element).Value);

@2xmax Before contribution you must sign CLA (a contribution license agreement) https://cla.microsoft.com

daxian-dbw · Dec 13, 2016

Thanks @2xmax for the contribution and @iSazonov for the code review!
I was distracted by other things. I will go through the changes this morning 😄

daxian-dbw · Dec 13, 2016

-                var list = obj as JArray;
-                obj = PopulateFromJArray(list, out error);
+                // the same as JavaScriptSerializer does
+                throw new ArgumentException("input", je);


new ArgumentException("input", je);

The first parameter is supposed to be the exception message. You need to add a resource string in WebCmdletStrings.resx for the message. The string could be something like this: Conversion from JSON failed with error: {0}. So the message generated would be var message = string.Format(CurrentCulture, WebCmdletStrings.ResourceStringName, je.Message).

daxian-dbw · Dec 13, 2016

+                // JObject is a IDictionary
                var dictionary = obj as JObject;
-                obj = PopulateFromJDictionary(dictionary, out error);
+                if (dictionary != null)


I hope Declaration Expressions can be brought back in C# 7. It will make code like this much more concise.

daxian-dbw · Dec 13, 2016

@2xmax we don't support NUnit, but the support to xUnit is on the way, see #2574.
The JsonObject unit tests you wrote are great. Since we don't have good support to xUnit yet, is it possible that you can convert them into Pester tests?

daxian-dbw · Dec 13, 2016

I see in JsonObject.cs multiple double castings. Can we ask @2xmax to fix this or better make new PR?

@iSazonov I think general code refactoring can go to a separate PR. Yes, those double casting should be avoid, but like @2xmax mentioned it will increase nesting, which is also annoying. Declaration Expressions would really help in such cases, if it can make into C# 7.0.

Also in JsonObject.cs a null checks is absent. full comment

Good catch. I'm not sure if it's really a potential bug, or it's OK because the value can only be one of JArray, JObject and JValue. @2xmax can you comment? If it's a potential bug, then we can open an issue to track it.

…message

2xmax · Dec 14, 2016

Thanks for the comments!

I have ported the unit tests to Pester, changed the error message, signed the CLA

Good catch. I'm not sure if it's really a potential bug, or it's OK because the value can only be one of JArray, JObject and JValue. @2xmax can you comment? If it's a potential bug, then we can open an issue to track it.

According to this doc, there be no other value except JArray, JObject, and JValue but maybe it is still worth to examine the Json.NET unit test suite in details; I haven't found any contradiction, yet I feel like maybe we could beware of a breaking change (e.g. caused by the nuget package update) by writing some tests here.

For these untyped properties, the Json.NET serializer will read the JSON into LINQ to JSON objects and set them to the property. JObject will be created for JSON objects; JArray will be created for JSON arrays, and JValue will be created for primitive JSON values

iSazonov · Dec 14, 2016

@@ -0,0 +1,53 @@
+# Unit tests for JsonObject
+Describe 'JsonObject' -tags "CI" {


Please move the comment to Describe

Describe 'Unit tests for JsonObject' -tags "CI" {

iSazonov · Dec 14, 2016

+  <data name="JsonDeserializationFailed" xml:space="preserve">
+    <value>Conversion from JSON failed with error: {0}</value>
+  </data>
 </root>


Please add newline.

this file is autogenerated

iSazonov · Dec 14, 2016

+# Unit tests for JsonObject
+Describe 'JsonObject' -tags "CI" {
+
+    function ShouldThrow


Please use the template with FullyQualifiedErrorId to check for proper errors

it "Get-Item on a nonexisting file should have error PathNotFound" { try { get-item "ThisFileCannotPossiblyExist" -ErrorAction Stop throw "No Exception!" } catch { $_.FullyQualifiedErrorId | should be "PathNotFound,Microsoft.PowerShell.Commands.GetItemCommand" } }

And see my comment about testcase.

This man tells that I've got to verify FullyQualifiedErrorId, going to fix this, thx. However, it does not explicitly state that all the expected exception checks should
follow this template. Nevertheless, If I had adhered to the whole template, imho, the test would have been less clear and concise, so I'd prefer to keep the ShouldThrow checker. Moreover, the copypaste is prone to mistakes, just take a look at my quick findings:

grep -Hn -B 4 -ir "FullyQualifiedErrorId.*should" --include *Test*.ps1 --group-separator=$'\n\n---------------------------------\n\n'

Here developers simply forgot to throw before catch:

engine/ParameterBinding/ParameterBinding.Tests.ps1-16- test-PositionalBinding1 1 engine/ParameterBinding/ParameterBinding.Tests.ps1-17- } engine/ParameterBinding/ParameterBinding.Tests.ps1-18- catch engine/ParameterBinding/ParameterBinding.Tests.ps1-19- { engine/ParameterBinding/ParameterBinding.Tests.ps1:20: $_.FullyQualifiedErrorId | should be "AmbiguousPositionalParameterNoName,test-PositionalBinding1" --- Modules/Microsoft.PowerShell.Security/ExecutionPolicy.Tests.ps1-920- try { Modules/Microsoft.PowerShell.Security/ExecutionPolicy.Tests.ps1-921- Set-ExecutionPolicy -Scope $policyScope -ExecutionPolicy Restricted Modules/Microsoft.PowerShell.Security/ExecutionPolicy.Tests.ps1-922- } Modules/Microsoft.PowerShell.Security/ExecutionPolicy.Tests.ps1-923- catch { Modules/Microsoft.PowerShell.Security/ExecutionPolicy.Tests.ps1:924: $_.FullyQualifiedErrorId | Should Be "CantSetGroupPolicy,Microsoft.PowerShell.Commands.SetExecutionPolicyCommand" -- Modules/Microsoft.PowerShell.Security/SecureString.Tests.ps1-15- try { Modules/Microsoft.PowerShell.Security/SecureString.Tests.ps1-16- ConvertFrom-SecureString -secureString $null -key $null Modules/Microsoft.PowerShell.Security/SecureString.Tests.ps1-17- } Modules/Microsoft.PowerShell.Security/SecureString.Tests.ps1-18- catch { Modules/Microsoft.PowerShell.Security/SecureString.Tests.ps1:19: $_.FullyQualifiedErrorId | should be "ParameterArgumentValidationErrorNullNotAllowed,Microsoft.PowerShell.Commands.ConvertFromSecureStringCommand" -- Modules/Microsoft.PowerShell.Utility/Export-Alias.Tests.ps1-104- try{ Modules/Microsoft.PowerShell.Utility/Export-Alias.Tests.ps1-105- Export-Alias $fulltestpath abcd02 Modules/Microsoft.PowerShell.Utility/Export-Alias.Tests.ps1-106- } Modules/Microsoft.PowerShell.Utility/Export-Alias.Tests.ps1-107- catch{ Modules/Microsoft.PowerShell.Utility/Export-Alias.Tests.ps1:108: $_.FullyQualifiedErrorId | Should be "FileOpenFailure,Microsoft.PowerShell.Commands.ExportAliasCommand"

Here instead of err checking we will get a nullref:

Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-76- It "Call Get-TimeZone using ID param and multiple items, where first and third are invalid ids - expect error" { Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-77- $null = Get-TimeZone -Id @("Cape Verde Standard","Morocco Standard Time","Azores Standard") ` Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-78- -ErrorVariable errVar -ErrorAction SilentlyContinue Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-79- $errVar.Count -eq 2 | Should Be $true Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1:80: $errVar[0].FullyQualifiedErrorID | Should Be "TimeZoneNotFound,Microsoft.PowerShell.Commands.GetTimeZoneCommand"

Perhaps this is the reason why in places like here or here were desided to implement the custom assertions. Of course, all the mistakes I encountered are related to lack of calibration, but I merely revealed you a couple of drawbacks.

All in all, you can still insist on it, persuade me by proposing other arguments and other ideas to refactor it.

the comment about testcase is valid, thx

It's totally fine to have custom assertions in your test. I think @iSazonov meant that the try/catch section in ShouldThrow should be better use the template, so it would be like

function ShouldThrow { .... try { & $InputObject throw "Should throw ArgumentException" } catch { $_.FullyQualifiedErrorId | should be "ArgumentException" } }

For those tests you found out, they were written before we came up with the guidelines, but we do want future test to follow the guidelines as much as possible 😄

iSazonov · Dec 14, 2016

+        $errRecord = $null
+        { [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson("{a:1", [ref]$errRecord) } | ShouldThrow 'System.ArgumentException'
+    }
+}


Please add newline.

iSazonov · Dec 14, 2016

+
+    It 'empty string' {
+        $errRecord = $null
+        [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson("", [ref]$errRecord)


All test use the same code with different data so it is recommended to use testcase
Init code must be moved to BeforeAll block. (Use the same link to docs)

An example of -testcases can be found here. It's helpful to avoid duplication when there are a lot It blocks with same logic but different test data.

Since the number of test cases with the same logic is not many (3 for $errRecord | Should BeNullOrEmpty and 2 for ShouldThrow), I don't insist on changing the existing test cases.

thank you guys for clarification, the test has been refactored

iSazonov · Dec 14, 2016

According to this doc, there be no other value except JArray, JObject, and JValue

Yes, my question just is can we hope that this will be not changed in the future (newtonsoft is not under our control)? May be protect it and forget? :-) Or at least add a relevant comment.

2xmax · Dec 14, 2016

Yes, my question just is can we hope that this will be not changed in the future (newtonsoft is not under our control)? May be protect it and forget? :-) Or at least add a relevant comment.

I already responded you in the same sentence you quoted "...we could beware of a breaking change (e.g. caused by the nuget package update) by writing some tests here," perhaps it was too long and convoluted. To put it simply, we can write some tests (a safety net) to prevent this.

Yet I still consider this discussion is out of the scope of this PR and it is better to converse about this in a separate issue or a Slack channel.

daxian-dbw · Dec 14, 2016

+        @{ str = "{a:1}" }
+    )
+
+    It 'no error for valid string' -TestCase $validStrings {


It's better to make the test case name different for different test cases, so that when one of the test cases failed, you will know which test data hashtable caused the failure. The recommended way is:

$validStrings = @( @{ name = "empty"; str = "" } @{ name = "spaces"; str = " " } @{ name = "{a:1}"; str = "{a:1}" } ) It "no error for valid string - <name>" -TestCase $validStrings { .... }

When running, you will see

[+] no error for valid string - empty 407ms [+] no error for valid string - spaces 75ms [+] no error for valid string - {a:1} 9ms

daxian-dbw · Dec 14, 2016

+        @{ str = "{a:1" }
+    )
+
+    It 'throw ArgumentException for invalid string' -TestCase $invalidStrings  {


iSazonov · Dec 15, 2016

@2xmax Thanks for your contribution.
There is some problem with CLA. Maybe you used different email addresses here and when signing CLA.

@daxian-dbw Thanks for your great comments!

2xmax · Dec 15, 2016

Thank you both for the comments too!

tried to sign the agreement one more time with no luck. Seems I'm not the only person who is having problems with the cla bot microsoft/MixedRealityToolkit-Unity#370

daxian-dbw · Dec 15, 2016

@2xmax I changed the label to cla-signed based on your comment. Thanks again for your contribution!

2xmax added 3 commits December 9, 2016 01:17

This change fixes the Invoke-RestMethod cmdlet behavior if the input …

dd26d01

…is neither xml nor json.

test description

c91f083

fixed the failed tests

5ae33c9

msftclas added the cla-required label Dec 9, 2016

2xmax changed the title ~~Invoke-RestMethod cmdlet error if the input is neither json nor xml~~ Invoke-RestMethod cmdlet error if the response is neither json nor xml Dec 9, 2016

2xmax changed the title ~~Invoke-RestMethod cmdlet error if the response is neither json nor xml~~ Invoke-RestMethod cmdlet error if the response is neither JSON nor XML Dec 9, 2016

2xmax closed this Dec 9, 2016

2xmax reopened this Dec 9, 2016

msftclas added the cla-required label Dec 9, 2016

PowerShellTeam added the Review - Needed The PR is being reviewed label Dec 9, 2016

daxian-dbw self-assigned this Dec 10, 2016

removed some duplication

ad9d14b

iSazonov reviewed Dec 11, 2016

View reviewed changes

refactoring: eliminated some double castings

a250f8e

iSazonov reviewed Dec 13, 2016

View reviewed changes

daxian-dbw requested changes Dec 13, 2016

View reviewed changes

daxian-dbw reviewed Dec 13, 2016

View reviewed changes

added JsonObject unit tests; enhanced JSON deserialization exception …

52ad2d7

…message

iSazonov reviewed Dec 14, 2016

View reviewed changes

refactored JsonObject.Tests.ps1

ea22114

2xmax added 2 commits December 14, 2016 22:25

entitled test cases

524d2af

formatting

62670ec

daxian-dbw reviewed Dec 14, 2016

View reviewed changes

daxian-dbw approved these changes Dec 14, 2016

View reviewed changes

daxian-dbw added cla-signed and removed cla-required labels Dec 15, 2016

daxian-dbw merged commit 8be3109 into PowerShell:master Dec 15, 2016

2xmax mentioned this pull request Dec 20, 2016

Wrong expected exception assertion in tests #2908

Merged

2xmax mentioned this pull request Feb 7, 2017

Invoke-RestMethod does not count on Content-Type field. Broken compared to PS 5.0. #2245

Closed

Search code, repositories, users, issues, pull requests...

Conversation

2xmax commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Dec 9, 2016

Uh oh!

msftclas commented Dec 9, 2016

Uh oh!

2xmax commented Dec 11, 2016 • 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!

2xmax commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2xmax Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

iSazonov commented Dec 13, 2016

Uh oh!

daxian-dbw commented Dec 13, 2016

Uh oh!

daxian-dbw Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Dec 13, 2016

Uh oh!

daxian-dbw commented Dec 13, 2016

Uh oh!

2xmax commented Dec 14, 2016 • 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!

iSazonov commented Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2xmax commented Dec 14, 2016

Uh oh!

daxian-dbw Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

2xmax commented Dec 9, 2016 •

edited

Loading

2xmax commented Dec 11, 2016 •

edited

Loading

2xmax Dec 13, 2016 •

edited

Loading

daxian-dbw Dec 13, 2016 •

edited

Loading

2xmax commented Dec 14, 2016 •

edited

Loading

iSazonov commented Dec 14, 2016 •

edited

Loading

daxian-dbw Dec 14, 2016 •

edited

Loading

2xmax commented Dec 15, 2016 •

edited

Loading