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

2xmax · 2017-02-03T02:02:37Z

before the change (in CoreCLR):

PS C:\>  Invoke-RestMethod http://httpbin.org/status/418
Invoke-RestMethod : Response status code does not indicate success: 418 (I'M A
TEAPOT).
PS C:\> $error[0].Exception.Response
(nothing)

after the change

PS C:\> Invoke-RestMethod http://httpbin.org/status/418
Invoke-RestMethod :
    -=[ teapot ]=-
       _...._
     .'  _ _ `.
    | ."` ^ `". _,
    \_;`"---"`|//
      |       ;/
      \_     _/
        `"""`
At line:1 char:1...
PS C:\> $error[0].Exception.Response
Version             : 1.1
Content             : System.Net.Http.StreamContent
StatusCode          : 418
ReasonPhrase        : I'M A TEAPOT
Headers             : {[Connection, System.String[]], [Date, System.String[]],
                      [Server, System.String[]], [Access-Control-Allow-Origin,
                      System.String[]]...}
RequestMessage      : Method: GET, RequestUri:
                      'http://httpbin.org/status/418', Version: 1.1, Content:
                      <null>, Headers:
                      {...
PS C:\> Invoke-RestMethod http://httpbin.org/status/500
Invoke-RestMethod : The remote server returned an error: (500) INTERNAL SERVER ERROR.

…2113]

PaulHigin · 2017-02-07T18:13:32Z

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs


+        private void ThrowNotSuccessStatusCodeError(HttpRequestMessage request, HttpResponseMessage response)
+        {
+            using(var reader = new StreamReader(StreamHelper.GetResponseStream(response)))


This should handle any errors that can occur while obtaining and reading the response stream, such as invalid argument and IOException exceptions.

wow, you've just found one more issue: IOException is not handled even if the response was ok (200), I thought it was handled somewhere in WebRequestPSCmdlet. I'm going to fix it as well.

..and it is also not handled in fullclr powershell if the response status code was 200 (non-200 handling is here). From another side, it seems legit since it reports the user that an I/O exception occurred and it is pretty clear what happened (e.g. "The connection with the server was terminated abnormally"). Do you consider to wrap some exceptions as inner one of WebException?

Thanks, I think it is Ok in this case. My original concern is hiding the WebRequest exception with an IOException caused while processing the WebRequest exception.

thx, now it handles this situation almost the same way as the full clr implementation does.

PaulHigin · 2017-02-07T18:32:07Z

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

    <value>Conversion from JSON failed with error: {0}</value>
  </data>
+  <data name="NotSuccessStatusCode" xml:space="preserve">
+    <value>The remote server returned an error: ({0:d}) {1}.</value>


Why is the DateTime format specifier {0:d} used here?

d is for decimal:) According to this doc, the format string can only contain the "G" or "g", "D" or "d", "X" or "x", and "F" or "f", i.e. it can't be unambiguous "N0" or "#". As an alternative, I can cast the enum to int explicitly without all these hacks)

I believe "d" in String.Format is reserved for DateTime formatting. I was using this as reference: http://www.csharp-examples.net/string-format-datetime/

PaulHigin · 2017-02-08T16:51:59Z

Thanks. LGTM.

daxian-dbw · 2017-02-11T22:05:41Z

src/System.Management.Automation/CoreCLR/CorePsStub.cs

+    /// <summary>
+    /// Stub for WebException
+    /// </summary>
+    public sealed class WebException : Exception


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.

daxian-dbw · 2017-02-12T06:44:48Z

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs

+            }
+
+            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);


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?

@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?

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.

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

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.

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.

@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:))?

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.

daxian-dbw · 2017-02-12T06:57:20Z

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs

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


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.

daxian-dbw · 2017-02-12T07:02:56Z

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs

+            var reader = new StreamReader(StreamHelper.GetResponseStream(response));
+            try
+            {
+                detailMsg = reader.ReadToEnd();


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.

daxian-dbw · 2017-02-24T23:37:09Z

@2xmax there is a new PR #3201 opened about the similar issue, would you mind take a look and leave your comments?

daxian-dbw · 2017-02-25T06:33:16Z

@2xmax #3201 has been merged, so I will close this PR. Feel free to re-open it if you have any concerns.

2xmax added 3 commits February 3, 2017 01:59

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

91db530

…2113]

docs

5eb8af4

fixed build

4e35191

msftclas added the cla-already-signed label Feb 3, 2017

2xmax closed this Feb 3, 2017

2xmax reopened this Feb 3, 2017

msftclas added the cla-already-signed label Feb 3, 2017

PowerShellTeam added the Review - Needed The PR is being reviewed label Feb 3, 2017

moved error creation to a separate method

dabb348

PaulHigin reviewed Feb 7, 2017

View reviewed changes

exception handling as full clr does; simpler string formatting logic

f37e8bb

daxian-dbw requested changes Feb 12, 2017

View reviewed changes

daxian-dbw self-assigned this Feb 12, 2017

daxian-dbw mentioned this pull request Feb 24, 2017

return HTTP response for error status as part of exception #3201

Merged

daxian-dbw closed this Feb 25, 2017

iSazonov removed the Review - Needed The PR is being reviewed label Mar 27, 2017

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

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

Uh oh!

Conversation

2xmax commented Feb 3, 2017 • 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!

2xmax Feb 7, 2017 • 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!

2xmax Feb 8, 2017 • 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!

2xmax Feb 8, 2017 • 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!

2xmax Feb 8, 2017 • 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!

PaulHigin commented Feb 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2xmax Feb 13, 2017 • 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!

daxian-dbw Feb 14, 2017 • 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!

daxian-dbw Feb 14, 2017 • 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!

2xmax Feb 15, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Feb 24, 2017

Uh oh!

daxian-dbw commented Feb 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

2xmax commented Feb 3, 2017 •

edited

Loading

2xmax Feb 7, 2017 •

edited

Loading

2xmax Feb 8, 2017 •

edited

Loading

2xmax Feb 8, 2017 •

edited

Loading

2xmax Feb 8, 2017 •

edited

Loading

2xmax Feb 13, 2017 •

edited

Loading

daxian-dbw Feb 14, 2017 •

edited

Loading

daxian-dbw Feb 14, 2017 •

edited

Loading

2xmax Feb 15, 2017 •

edited

Loading