return HTTP response for error status as part of exception by SteveL-MSFT · Pull Request #3201 · PowerShell/PowerShell

SteveL-MSFT · 2017-02-24T19:02:23Z

Addresses #2193

When Invoke-WebRequest or Invoke-RestMethod receives a HTTS status error code, an exception is returned and the user can't get the HTTP response without additional work. This change makes it more consistent with Windows PowerShell in that a Response property which contains the HttpResponse is a member of the resulting Exception.

…xperience to Windows PowerShell added tests for invoke-webrequest and invoke-restmethod for http error cases

…xperience to Windows PowerShell added tests for invoke-webrequest and invoke-restmethod for http error cases fixed bad merge

PaulHigin · 2017-02-24T22:14:54Z

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

 {
+    /// <summary>
+    /// Exception class for webcmdlets to enable returning HTTP error response
+    /// </summary>    


Should this class be sealed? Or is there a reason to derive from it?

Agree, no reason to derive from it

PaulHigin · 2017-02-24T22:22:38Z

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

+
+        /// <summary>
+        /// HTTP error status code
+        /// </summary>


It looks like this property is never set in the error handling code below. Does this just return the response.StatusCode?

Was originally adding the individual properties before I saw the example of Windows PowerShell just having a Response property. Didn't remove them. Will remove.

PaulHigin · 2017-02-24T22:23:24Z

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

+
+        /// <summary>
+        /// HTTP error headers
+        /// </summary>


Same for this property. Is this _response.Headers?

Will remove.

PaulHigin · 2017-02-24T22:39:58Z

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

+
+        /// <summary>
+        /// HTTP error response
+        /// </summary>


I believe we normally use automatic implemented getter/setter property syntax:

public HttpResponseMessage Response { get; set; }

PaulHigin · 2017-02-24T22:47:27Z

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

+                        {
+                            string message = String.Format(CultureInfo.CurrentCulture, WebCmdletStrings.ResponseStatusCodeFailure,
+                                response.StatusCode.ToString(), response.ReasonPhrase);
+                            HttpResponseException httpEx = new HttpResponseException(message);


You could use this syntax here for creating the HttpResponseException:

var httpEx = new HttpResponseException(message) { Response = response, Status = ... Headers = ... };

Or you could make the constructor take all property values and make the properties read only.

Will change constructor

PaulHigin · 2017-02-24T22:48:08Z

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

+        $result = ExecuteWebCommand -command $command
+
+        $result.Error.Exception | Should BeOfType Microsoft.PowerShell.Commands.HttpResponseException
+        $result.Error.Exception.Response.StatusCode | Should Be 418


Should we test for Exception.Headers and Exception.Status too?

Those are getting removed

…ed Headers and Status members, added response to constuctor based on feedback

SteveL-MSFT · 2017-02-24T23:16:13Z

@PaulHigin thanks for the feedback, addressed each of them

PaulHigin · 2017-02-24T23:24:02Z

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

+
+        /// <summary>
+        /// HTTP error response
+        /// </summary>


We should make the setter private { get; private set; }. Otherwise LGTM.

I agree. WebException.Response only has a getter.

Makes sense

daxian-dbw · 2017-02-24T23:35:56Z

@SteveL-MSFT We have another similar PR here #3089, please take a look at the comments in that PR to see any of them apply in this fix as well.
I suggest we have our HttpResponseException derive from System.Net.Http.HttpRequestException, so that try { Invoke-WebRequest <url> } catch [HttpRequestException] { } would continue to work after this change.

daxian-dbw · 2017-02-24T23:41:51Z

@2xmax (the author of #3089). would you mind take a look?

2xmax · 2017-02-24T23:56:36Z

will the new version of the cmdlet output the content if the code was unsuccessful? I could say in the context of my scope, that was a real issue and we even already rewrote our code to curl instead of powershell whereas the status code was not a case, afaik we were able to parse it using regex:)).

UPD: sorry, I found it the response property. But now the problem is, the response now is not disposed properly as the FullClr impl does

daxian-dbw · 2017-02-25T00:03:23Z

@2xmax you are right, this PR only covers partial of #3089. It doesn't capture the content from the response and put it in ErrorDetail as full powershell does (see code here).

@SteveL-MSFT, do you want to address it in this same PR or a different one?

daxian-dbw · 2017-02-25T00:05:16Z

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

 {
+    /// <summary>
+    /// Exception class for webcmdlets to enable returning HTTP error response
+    /// </summary>    


I suggest we make HttpResponseException derive from HttpRequestException, so that { Invoke-WebRequest <url> } catch [HttpRequestException] { } will continue to work after this change.

daxian-dbw · 2017-02-25T00:11:13Z

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

+                            HttpResponseException httpEx = new HttpResponseException(message, response);
+                            ErrorRecord er = new ErrorRecord(httpEx, "WebCmdletWebResponseException", ErrorCategory.InvalidOperation, request);
+                            er.ErrorDetails = new ErrorDetails(message);
+                            ThrowTerminatingError(er);


The message for ErrorDetails should be the content of the response (after using regex replace to remove tags). This was what #3089 tried to fix, and the corresponding code in full powershell is here

Will make the change

2xmax · 2017-02-25T00:16:09Z

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

        $result = ExecuteWebCommand -command $command
        $result.Error | Should BeNullOrEmpty
    }
+


pls add a test with a response content

…hes continue to work added response stripping html tags to ErrorDetails added test to verify response is in ErrorDetails

SteveL-MSFT · 2017-02-25T00:34:11Z

Addressed feedback. This should also address #2113

new commits pushed

daxian-dbw · 2017-02-25T01:09:23Z

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

+                            HttpResponseException httpEx = new HttpResponseException(message, response);
+                            ErrorRecord er = new ErrorRecord(httpEx, "WebCmdletWebResponseException", ErrorCategory.InvalidOperation, request);
+                            string detailMsg = "";
+                            using (StreamReader reader = new StreamReader(StreamHelper.GetResponseStream(response)))


Exceptions may be thrown out in StreamHelper.GetResponseStream(response) or reader.ReadToEnd(). We should put the using block in a try/catch-all block, just like what we did in full ps.

daxian-dbw · 2017-02-25T01:13:22Z

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

+                        if (!response.IsSuccessStatusCode)
+                        {
+                            string message = String.Format(CultureInfo.CurrentCulture, WebCmdletStrings.ResponseStatusCodeFailure,
+                                Convert.ToInt32(response.StatusCode).ToString(), response.ReasonPhrase);


Convert.ToInt32(response.StatusCode).ToString()

A minor comment: maybe just (int)response.StatusCode as in HttpResponseMessage.cs?
If we want to avoid boxing the int argument, then it should be ((int)response.StatusCode).ToString(), but maybe it's an overkill 😄

I'm ok with just the cast

daxian-dbw · 2017-02-25T01:14:49Z

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

+                                // remove HTML tags making it easier to read
+                                detailMsg = System.Text.RegularExpressions.Regex.Replace(reader.ReadToEnd(), "<[^>]*>","");
+                            }
+                            er.ErrorDetails = new ErrorDetails(detailMsg);


er.ErrorDetails should be set only if detailMsg is not an empty string..

daxian-dbw · 2017-02-25T01:17:46Z

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

+        $result.Error.Exception | Should BeOfType Microsoft.PowerShell.Commands.HttpResponseException
+        $result.Error.Exception.Response.StatusCode | Should Be 418
+        $result.Error.Exception.Response.ReasonPhrase | Should Be "I'm a teapot"
+        $result.Error.Exception.Message | Should Be "Response status code does not indicate success: 418 (I'm a teapot)."


Minor comment: this might fail if we are going to run this test on a localized machine.

@daxian-dbw Is a match good enough for the part returned from the server?

changed to Match

Yes, I think so, $result.Error.Exception.Message | Should BeLike * 418 (I'm a teapot) should be good, since they are returned from the Http service, not affected by PS localization.

daxian-dbw · 2017-02-25T01:18:07Z

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

+        $result.Error.Exception | Should BeOfType Microsoft.PowerShell.Commands.HttpResponseException
+        $result.Error.Exception.Response.StatusCode | Should Be 418
+        $result.Error.Exception.Response.ReasonPhrase | Should Be "I'm a teapot"
+        $result.Error.Exception.Message | Should Be "Response status code does not indicate success: 418 (I'm a teapot)."


Same minor comment here.

changed to Match

…lMsg is empty changed error message test to be localization friendly by just matching the part returned from server

2xmax · 2017-02-25T01:40:32Z

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

 {
+    /// <summary>
+    /// Exception class for webcmdlets to enable returning HTTP error response
+    /// </summary>    


I suggest moving the class into a separate file

I did a search of the existing source and it appears that in some cases Exceptions are declared in a separate file (like SessionStateExceptions.cs), but in many other cases, the Exception is declared where it's used (like parserutils.cs).

2xmax · 2017-02-25T01:44:44Z

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

+                            HttpResponseException httpEx = new HttpResponseException(message, response);
+                            ErrorRecord er = new ErrorRecord(httpEx, "WebCmdletWebResponseException", ErrorCategory.InvalidOperation, request);
+                            string detailMsg = "";
+                            try


I suggest using try-catch-finally to avoid nesting

@2xmax do you mean like this?

StreamReader reader = null; try { reader = new StreamReader(StreamHelper.GetResponseStream(response)) .... } catch { } finally { if (reader != null) { reader.Dispose(); } }

… if detailMsg is empty changed error message test to be localization friendly by just matching the part returned from server

daxian-dbw · 2017-02-25T01:56:11Z

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

+                            HttpResponseException httpEx = new HttpResponseException(message, response);
+                            ErrorRecord er = new ErrorRecord(httpEx, "WebCmdletWebResponseException", ErrorCategory.InvalidOperation, request);
+                            string detailMsg = "";
+                            StreamReader reader = new StreamReader(StreamHelper.GetResponseStream(response));


I'm afraid you need to put new StreamReader(StreamHelper.GetResponseStream(response)); in the try block. Maybe like this:

string detailMsg = ""; StreamReader reader = null; try { reader = new StreamReader(StreamHelper.GetResponseStream(response)) detailMsg = System.Text.RegularExpressions.Regex.Replace(reader.ReadToEnd(), "<[^>]*>",""); } catch { } finally { if (reader != null) { reader.Dispose(); } }

daxian-dbw · 2017-02-25T01:57:06Z

...Shell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs.orig

@@ -0,0 +1,610 @@
+#if CORECLR
+
+/********************************************************************++


And this file got committed by accident 😄

new commits were pushed.

moved allocation of StreamReader into try block

SteveL-MSFT added 3 commits February 24, 2017 09:23

added http response to new HttpResponseException to provide similar e…

0d8be7b

…xperience to Windows PowerShell added tests for invoke-webrequest and invoke-restmethod for http error cases

fixed bad merge

6fe216a

added http response to new HttpResponseException to provide similar e…

1920187

…xperience to Windows PowerShell added tests for invoke-webrequest and invoke-restmethod for http error cases fixed bad merge

msftclas added the cla-not-required label Feb 24, 2017

PaulHigin reviewed Feb 24, 2017

View reviewed changes

SteveL-MSFT added 2 commits February 24, 2017 15:10

sealed HttpResponseException class, used auto-accessors, removed unus…

8483c7c

…ed Headers and Status members, added response to constuctor based on feedback

Merge branch 'weberror'

5dd0b5f

PaulHigin reviewed Feb 24, 2017

View reviewed changes

daxian-dbw self-assigned this Feb 24, 2017

daxian-dbw mentioned this pull request Feb 24, 2017

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

Closed

daxian-dbw previously requested changes Feb 25, 2017

View reviewed changes

2xmax reviewed Feb 25, 2017

View reviewed changes

derived HttpResponseException from HttpRequestException so other catc…

eaaaa7f

…hes continue to work added response stripping html tags to ErrorDetails added test to verify response is in ErrorDetails

daxian-dbw requested changes Feb 25, 2017

View reviewed changes

added try..catch around reading response stream and checking if detai…

4a8e693

…lMsg is empty changed error message test to be localization friendly by just matching the part returned from server

2xmax reviewed Feb 25, 2017

View reviewed changes

daxian-dbw previously approved these changes Feb 25, 2017

View reviewed changes

SteveL-MSFT added 2 commits February 24, 2017 17:50

added try..catch..finally around reading response stream and checking…

4a420db

… if detailMsg is empty changed error message test to be localization friendly by just matching the part returned from server

Merge branch 'master' of github.com:SteveL-MSFT/PowerShell

c3c939f

daxian-dbw reviewed Feb 25, 2017

View reviewed changes

SteveL-MSFT and others added 3 commits February 24, 2017 21:16

remove accidentally included file

4b1e21c

moved allocation of StreamReader into try block

move allocation of StreamReader into try block

8fbe863

Add null check before disposing reader

650839b

daxian-dbw approved these changes Feb 25, 2017

View reviewed changes

daxian-dbw merged commit c74b2a7 into PowerShell:master Feb 25, 2017

daxian-dbw mentioned this pull request Feb 25, 2017

WebRequestPSCmdlets do not contain Response object in Exception on Mac OS X #2113

Closed

		@@ -0,0 +1,610 @@
		#if CORECLR

		/********************************************************************++

Conversation

SteveL-MSFT commented Feb 24, 2017

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!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Feb 24, 2017

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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented Feb 24, 2017

Uh oh!

2xmax commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented Feb 25, 2017

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!

SteveL-MSFT commented Feb 25, 2017

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

daxian-dbw commented Feb 24, 2017 •

edited

Loading

2xmax commented Feb 24, 2017 •

edited

Loading

daxian-dbw Feb 25, 2017 •

edited

Loading