Skip to content

error message and status code in the Invoke-WebRequest cmdlet [Fixes#… #3089

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

Closed
2xmax wants to merge 5 commits into from
Closed

error message and status code in the Invoke-WebRequest cmdlet [Fixes#… #3089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
91db530
error message and status code in the Invoke-WebRequest cmdlet [Fixes#…
2xmax Feb 2, 2017
5eb8af4
docs
2xmax Feb 2, 2017
4e35191
fixed build
2xmax Feb 2, 2017
dabb348
moved error creation to a separate method
2xmax Feb 3, 2017
f37e8bb
exception handling as full clr does; simpler string formatting logic
2xmax Feb 7, 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
34 changes: 33 additions & 1 deletion ...erShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
using System.Security.Cryptography;
using System.Threading;
using System.Xml;
using Microsoft.PowerShell.CoreClr.Stubs;

namespace Microsoft.PowerShell.Commands
{
Expand Down Expand Up @@ -347,7 +348,10 @@ protected override void ProcessRecord()
WriteVerbose(reqVerboseMsg);

HttpResponseMessage response = GetResponse(client, request);
response.EnsureSuccessStatusCode();
if (!response.IsSuccessStatusCode)
{
ThrowNotSuccessStatusCodeError(request, response);
}

string contentType = ContentHelper.GetContentType(response);
string respVerboseMsg = string.Format(CultureInfo.CurrentCulture,
Expand Down Expand Up @@ -408,6 +412,34 @@ protected override void StopProcessing()

#region Helper Methods

private void ThrowNotSuccessStatusCodeError(HttpRequestMessage request, HttpResponseMessage response)
{
var detailMsg = "";
var reader = new StreamReader(StreamHelper.GetResponseStream(response));
Copy link
Member

@daxian-dbw daxian-dbw Feb 12, 2017

Choose a reason for hiding this comment

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

Could response.Content be null and raise NullReferenceException in GetResponseStream? Maybe we should check response.Content != null && response.Content.Headers.ContentLength > 0 before getting its stream.
And also, could response.Content.ReadAsStreamAsync raise any exception when the response is not in success status? Maybe the code dealing with stream should be put in a try/catch-all block, just like how it does in full PS.

try
{
detailMsg = reader.ReadToEnd();
Copy link
Member

@daxian-dbw daxian-dbw Feb 12, 2017

Choose a reason for hiding this comment

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

detailMsg = System.Text.RegularExpressions.Regex.Replace(detailMsg, "<[^>]*>", "");

In full PS, a simple regex replace is used to remove tags from the content. We probably should do the same.

}
catch (IOException)
{
}
catch (ArgumentException)
{
}
finally
{
reader.Dispose();
}

var msg = string.Format(CultureInfo.CurrentCulture, WebCmdletStrings.NotSuccessStatusCode, (int) response.StatusCode, response.ReasonPhrase);
ErrorRecord er = new ErrorRecord(new WebException(msg) { Response = response }, "WebCmdletWebResponseException", ErrorCategory.InvalidOperation, request);
Copy link
Member

@daxian-dbw daxian-dbw Feb 12, 2017

Choose a reason for hiding this comment

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

new WebException(msg) { Response = response }

It's great to get the detailed message for the error record to keep a consistent behavior. But I don't think we should use a stub type to mimic the exception in full PS.
Since the content of the response has been extracted and passed to the error record, is there a real scenario where the response object is necessary?

Copy link
Contributor Author

@2xmax 2xmax Feb 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@daxian-dbw Well, I agree that the Response property looks kind of weird here. However, it helps keep backward compatibility. For example, given an http request, I need to get the status code

Invoke-RestMethod http://httpbin.org/404 
$e = $Error[0]
$e | gm
$e.Exception | gm

In the case of non-200 response, is there another straight way to get the status code in the FullClr implementation except for $e.Exception.Response.StatusCode?

Copy link
Member

@daxian-dbw daxian-dbw Feb 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm thinking aloud here: how about we add the response object to "Exception.Data" for core PS? Users can get the status code by $e.Exception.Data.Response.
Yes, this is not backward compatible, but we are sort of not backward compatible anyways, for example, members/methods exposed by (invoke-webrequest www.bing.com).BaseResponse are different.

Copy link
Member

@daxian-dbw daxian-dbw Feb 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@TravisEz13 @lzybkr @vors can you please take a look and share your opinions?

Copy link
Contributor

@lzybkr lzybkr Feb 15, 2017

Choose a reason for hiding this comment

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

Exception.Data was a neat idea, but it's not discoverable and a poor api. Because you typically derive from System.Exception, it's easier to add your data as properties - and more reliable - your property can be readonly.

Copy link
Member

@TravisEz13 TravisEz13 Feb 15, 2017

Choose a reason for hiding this comment

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

There are real cases where the response data is needed. Some sites send details of the error in the body, etc. I agree that putting the response under data is less discoverable. The exception message from the base exception should not be altered, though.

Copy link
Contributor Author

@2xmax 2xmax Feb 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@daxian-dbw in both cases we considered we would have trade-offs. However, in the case of Exception.Data, we would have a breaking change, that could lead to code structures like this $error[0].Exception.Response, $error[0].Exception.Data.StatusCode | select -f 1 (another "great" example). Moreover, it would be hard to guess without taking a look at the source code or scrutinizing MSDN carefully that the status code was in the Data property, all the previous blog posts, kb articles would be outdated.

From another side, in the case of the Response field, oh yes, we would have that a little bit tricky exception logic in the CoreClr implementation, but all that logic would be encapsulated in that method and with tests we could get along with that. Plus, if we find a third solution later, we can refactor it whereas with that breaking change it could be far more complex.

Does anybody have the third solution for this problem:))?

Copy link
Member

@daxian-dbw daxian-dbw Feb 15, 2017

Choose a reason for hiding this comment

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

I agree Exception.Data is not discoverable, and also breaks script portability. How about we have an exception type (not defined in the stub namespace) derived from HttpRequestException that exposes a Response property and use that exception when status code is not success? I would like to get the real exception thrown from CoreCLR, and our exception type should have a constructor like public XXXException(Exception e) : base (e.Message, e), so that we can pass in the real HttpRequestException as an inner exception. try { } catch [HttpRequestException] { } would also work this way.

One problem here is that EnsureSuccessStatusCode would dispose HttpResponseMessage.Content, and that means user won't be able to read the body streams anymore. I guess that's not acceptable given @TravisEz13's comments. More thought is needed.

if (!String.IsNullOrEmpty(detailMsg))
{
er.ErrorDetails = new ErrorDetails(detailMsg);
}
ThrowTerminatingError(er);
}

/// <summary>
/// Sets the ContentLength property of the request and writes the specified content to the request's RequestStream.
/// </summary>
Expand Down
3 changes: 3 additions & 0 deletions src/Microsoft.PowerShell.Commands.Utility/resources/WebCmdletStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,4 +210,7 @@
<data name="JsonDeserializationFailed" xml:space="preserve">
<value>Conversion from JSON failed with error: {0}</value>
</data>
<data name="NotSuccessStatusCode" xml:space="preserve">
<value>The remote server returned an error: ({0}) {1}.</value>
</data>
</root>
18 changes: 18 additions & 0 deletions src/System.Management.Automation/CoreCLR/CorePsStub.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using System.Diagnostics.CodeAnalysis;
using Microsoft.Win32;
using System.Management.Automation.Remoting;
using System.Net.Http;

#pragma warning disable 1591, 1572, 1571, 1573, 1587, 1570, 0067

Expand Down Expand Up @@ -473,6 +474,23 @@ public sealed class AppDomainUnloadedException : Exception
{
}

/// <summary>
/// Stub for WebException
/// </summary>
public sealed class WebException : Exception
Copy link
Member

@daxian-dbw daxian-dbw Feb 11, 2017

Choose a reason for hiding this comment

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

I don't think creating a sub type named 'WebException' is the right solution. System.Net.WebException is exposed in .NET Core, in system.net.requests.dll. Adding a stub type with the same name just adds confusion.
All stubs in this file are for types that no longer exist in .NET Core, and eventually we want to clean up the code to remove most of those stubs.

{
/// <summary>
/// Creates a new instance of the WebException
/// </summary>
/// <param name="message"></param>
public WebException(string message) : base(message) { }

/// <summary>
/// Gets the error message returned from the remote host.
/// </summary>
public HttpResponseMessage Response { get; set; }
}

#endregion Exception_Related

#region SafeHandle_Related
Expand Down
14 changes: 14 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 @@ -399,6 +399,20 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {
$result = ExecuteWebCommand -command $command
$result.Error | Should BeNullOrEmpty
}

It "Invoke-WebRequest should read content if status is unsuccessful" {

$command = "Invoke-WebRequest http://httpbin.org/status/418"
$result = ExecuteWebCommand -command $command
$result.Error.ErrorDetails.Message | Should Match "\-=\[ teapot \]"
}

It "Invoke-WebRequest should return status code if status is unsuccessful" {

$command = "Invoke-WebRequest http://httpbin.org/status/500"
$result = ExecuteWebCommand -command $command
$result.Error.Exception.Response.StatusCode | Should be 500
}
}

Describe "Invoke-RestMethod tests" -Tags "Feature" {
Expand Down