Skip to content

Fix Single Value JSON null in Invoke-RestMethod #5338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iSazonov merged 3 commits into from
Nov 14, 2017
Merged

Fix Single Value JSON null in Invoke-RestMethod #5338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11983cd
[Feature] Fix Single Value JSON null in Invoke-WebRequest
markekraus Nov 4, 2017
f6e173b
[Feature] Switch from Regex to JToken.Parse()
markekraus Nov 5, 2017
d968c99
[Feature] Address PR Feedback: use `n
markekraus Nov 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion ...hell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
using System.Management.Automation;
using System.IO;
using System.Xml;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

namespace Microsoft.PowerShell.Commands
{
Expand Down Expand Up @@ -168,16 +170,28 @@ private bool TryConvertToXml(string xml, out object doc, ref Exception exRef)

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

if (null == obj)
{
// This ensures that a null returned by ConvertFromJson() is the actual JSON null literal.
// if not, the ArgumentException will be caught.
JToken.Parse(json);
}

if (error != null)
{
exRef = error.Exception;
obj = null;
}
else
{
converted = true;
}
}
catch (ArgumentException ex)
{
Expand All @@ -189,7 +203,13 @@ private bool TryConvertToJson(string json, out object obj, ref Exception exRef)
exRef = ex;
obj = null;
}
return (null != obj);
catch (JsonException ex)
{
var msg = string.Format(System.Globalization.CultureInfo.CurrentCulture, WebCmdletStrings.JsonDeserializationFailed, ex.Message);
exRef = new ArgumentException(msg, ex);
obj = null;
}
return converted;
}

#endregion
Expand Down
16 changes: 16 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2613,6 +2613,22 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {

}

Context "Invoke-RestMethod Single Value JSON null support" {
BeforeAll {
$baseUrl = 'http://localhost:8081/PowerShell?test=response&contenttype=application/json&output='
}
It "Invoke-RestMethod Supports a Single Value JSON null" {
$url = '{0}{1}' -f $baseUrl, 'null'
Invoke-RestMethod -Uri $url | Should Be $null
}
It "Invoke-RestMethod Supports a Single Value JSON null and ignores whitespace" {
Copy link
Member

@TravisEz13 TravisEz13 Nov 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, add test(s) for the error case where the JSON cannot be parsed?

Copy link
Contributor Author

@markekraus markekraus Nov 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TravisEz13 if by "error" you mean empty strings or whitspace-only? I can add the tests, but they will just be ensuring a a String.Empty returns a String.Empty and that a whitespace string returns a whitespace string. When that error condition hits, the cmdlet just goes to the next serialization type (xml) and then when that fails it just returns the string.

It also looks like I would need to either add a WebListener controller to another switch option to HttpListener. Currently none of the available tests can return whitespace only responses. All of them truncate to 0 bytes when the body payload is all whitespace.

Copy link
Member

@TravisEz13 TravisEz13 Nov 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have the test to ensure good coverage, but I think this is adequate.

Copy link
Contributor Author

@markekraus markekraus Nov 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created #5414 to add coverage for that. I have a backlog of test and cleanup activities to do, I'll start on those after RC is underway.

$url = '{0}{1}' -f $baseUrl, " null "
Invoke-RestMethod -Uri $url | Should Be $null
$url = '{0}{1}' -f $baseUrl, " null `n"
Invoke-RestMethod -Uri $url | Should Be $null
}
}

BeforeEach {
if ($env:http_proxy) {
$savedHttpProxy = $env:http_proxy
Expand Down