GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — return HTTP response for error status as part of exception by SteveL-MSFT · Pull Request #3201 · PowerShell/PowerShell

Conversation

@SteveL-MSFT

@SteveL-MSFT Steve Lee (SteveL-MSFT) commented Feb 24, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Steve Lee (SteveL-MSFT) added 3 commits February 24, 2017 09:23
@SteveL-MSFT
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
@SteveL-MSFT
fixed bad merge
6fe216a
@SteveL-MSFT
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 Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Feb 24, 2017
PaulHigin
Paul Higinbotham (PaulHigin) reviewed Feb 24, 2017
View reviewed changes
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs
{
/// <summary>
/// Exception class for webcmdlets to enable returning HTTP error response
/// </summary>

@PaulHigin Paul Higinbotham (PaulHigin) Feb 24, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 24, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Agree, no reason to derive from it

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

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

@PaulHigin Paul Higinbotham (PaulHigin) Feb 24, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 24, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs

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

@PaulHigin Paul Higinbotham (PaulHigin) Feb 24, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Same for this property. Is this _response.Headers?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 24, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will remove.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

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

@PaulHigin Paul Higinbotham (PaulHigin) Feb 24, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

public HttpResponseMessage Response { get; set; }

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 24, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will fix

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated
{
string message = String.Format(CultureInfo.CurrentCulture, WebCmdletStrings.ResponseStatusCodeFailure,
response.StatusCode.ToString(), response.ReasonPhrase);
HttpResponseException httpEx = new HttpResponseException(message);

@PaulHigin Paul Higinbotham (PaulHigin) Feb 24, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 24, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will change constructor

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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

@PaulHigin Paul Higinbotham (PaulHigin) Feb 24, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 24, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Those are getting removed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Steve Lee (SteveL-MSFT) added 2 commits February 24, 2017 15:10
@SteveL-MSFT
sealed HttpResponseException class, used auto-accessors, removed unus…
8483c7c 
…ed Headers and Status members, added response to constuctor based on feedback
@SteveL-MSFT
Merge branch 'weberror'
5dd0b5f
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Feb 24, 2017

Copy link
Copy Markdown
Member Author

Paul Higinbotham (@PaulHigin) thanks for the feedback, addressed each of them
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
PaulHigin
Paul Higinbotham (PaulHigin) reviewed Feb 24, 2017
View reviewed changes
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs

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

@PaulHigin Paul Higinbotham (PaulHigin) Feb 24, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 24, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I agree. WebException.Response only has a getter.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Makes sense

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw Dongbo Wang (daxian-dbw) self-assigned this Feb 24, 2017
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Feb 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Steve Lee (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw Dongbo Wang (daxian-dbw) mentioned this pull request Feb 24, 2017
error message and status code in the Invoke-WebRequest cmdlet [Fixes#… #3089
Closed
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Feb 24, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@2xmax

Max Maximov (2xmax) commented Feb 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Feb 25, 2017

Copy link
Copy Markdown
Member

Max Maximov (@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).

Steve Lee (@SteveL-MSFT), do you want to address it in this same PR or a different one?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw
Dongbo Wang (daxian-dbw) previously requested changes Feb 25, 2017
View reviewed changes
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs
{
/// <summary>
/// Exception class for webcmdlets to enable returning HTTP error response
/// </summary>

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Agree

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...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);

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will make the change

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
2xmax
Max Maximov (2xmax) reviewed Feb 25, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1
$result = ExecuteWebCommand -command $command
$result.Error | Should BeNullOrEmpty
}

@2xmax Max Maximov (2xmax) Feb 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

pls add a test with a response content

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

will do

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT
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
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Feb 25, 2017

Copy link
Copy Markdown
Member Author

Addressed feedback. This should also address #2113
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw
Dongbo Wang (daxian-dbw) dismissed their stale review February 25, 2017 00:58

new commits pushed

daxian-dbw
Dongbo Wang (daxian-dbw) requested changes Feb 25, 2017
View reviewed changes
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated
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)))

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

ok

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated
if (!response.IsSuccessStatusCode)
{
string message = String.Format(CultureInfo.CurrentCulture, WebCmdletStrings.ResponseStatusCodeFailure,
Convert.ToInt32(response.StatusCode).ToString(), response.ReasonPhrase);

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm ok with just the cast

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated
// remove HTML tags making it easier to read
detailMsg = System.Text.RegularExpressions.Regex.Replace(reader.ReadToEnd(), "<[^>]*>","");
}
er.ErrorDetails = new ErrorDetails(detailMsg);

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

ok

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
$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)."

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

changed to Match

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
$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)."

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Same minor comment here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

changed to Match

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT
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
Max Maximov (2xmax) reviewed Feb 25, 2017
View reviewed changes
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs
{
/// <summary>
/// Exception class for webcmdlets to enable returning HTTP error response
/// </summary>

@2xmax Max Maximov (2xmax) Feb 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I suggest moving the class into a separate file

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
2xmax
Max Maximov (2xmax) reviewed Feb 25, 2017
View reviewed changes
Comment thread ...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

@2xmax Max Maximov (2xmax) Feb 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I suggest using try-catch-finally to avoid nesting

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

ok

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 25, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Max Maximov (@2xmax) do you mean like this?

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
daxian-dbw
Dongbo Wang (daxian-dbw) previously approved these changes Feb 25, 2017
View reviewed changes
Steve Lee (SteveL-MSFT) added 2 commits February 24, 2017 17:50
@SteveL-MSFT
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
@SteveL-MSFT
Merge branch 'master' of github.com:SteveL-MSFT/PowerShell
c3c939f
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Feb 25, 2017
View reviewed changes
Comment thread ...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated
HttpResponseException httpEx = new HttpResponseException(message, response);
ErrorRecord er = new ErrorRecord(httpEx, "WebCmdletWebResponseException", ErrorCategory.InvalidOperation, request);
string detailMsg = "";
StreamReader reader = new StreamReader(StreamHelper.GetResponseStream(response));

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Feb 25, 2017
View reviewed changes
Comment thread ...Shell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs.orig Outdated
@@ -0,0 +1,610 @@
#if CORECLR

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

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

And this file got committed by accident 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Feb 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw
Dongbo Wang (daxian-dbw) dismissed their stale review February 25, 2017 02:00

new commits were pushed.

Steve Lee (SteveL-MSFT) and others added 3 commits February 24, 2017 21:16
@SteveL-MSFT
remove accidentally included file
4b1e21c 
moved allocation of StreamReader into try block
@SteveL-MSFT
move allocation of StreamReader into try block
8fbe863
@daxian-dbw
Add null check before disposing reader
650839b
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Feb 25, 2017
View reviewed changes
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit c74b2a7 into PowerShell:master Feb 25, 2017
@daxian-dbw Dongbo Wang (daxian-dbw) mentioned this pull request Feb 25, 2017
WebRequestPSCmdlets do not contain Response object in Exception on Mac OS X #2113
Closed
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@SteveL-MSFT @daxian-dbw
return HTTP response for error status as part of exception (PowerShel…
2298453 
…l#3201)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
+2 more reviewers
@2xmax Max Maximov (2xmax) 2xmax left review comments
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@SteveL-MSFT @daxian-dbw @2xmax @PaulHigin @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.