Skip to content
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.

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

Normalize web exception handling between PS 5 and PS 7 #223

Open
HowardWolosky opened this issue Jun 6, 2020 · 3 comments
Open

Normalize web exception handling between PS 5 and PS 7 #223

HowardWolosky opened this issue Jun 6, 2020 · 3 comments
Labels
bug This relates to a bug in the existing module. help wanted Anyone in the community is welcome to do this work up for grabs Anyone in the community is welcome to do this work

Comments

@HowardWolosky
Copy link
Member

Issue Details

This was identified by @X-Guardian as part of his verification for #200.

Looking at Invoke-GHRestMethod in GithubCore, for this scenario using PowerShell 5, the [System.Net.WebException] code block at line 416 is being run, but with PowerShell 7, for the same error we are falling through to the final else block at line 459.

Adding some debug code, I can see that in PowerShell 5, $_.Exception.PSTypeNames at line 416 is:

System.Net.WebException
System.InvalidOperationException
System.SystemException
System.Exception
System.Object

and in PowerShell 7 $_.Exception.PSTypeNames at line 416 is:

Microsoft.PowerShell.Commands.HttpResponseException
System.Net.Http.HttpRequestException
System.Exception
System.Object

Adding -or $_.Exception -is [Microsoft.PowerShell.Commands.HttpResponseException]) to line 416 to make PowerShell 7 take the same code path as PowerShell 5, then causes PowerShell 7 to raise the following exception:

Get-HttpWebResponseContent: \\nas01\data\Users\Simon\Documents\GitHub\X-Guardian\PowerShellForGitHub\GitHubCore.ps1:429
Line |
 429 |  …    $rawContent = Get-HttpWebResponseContent -WebResponse $ex.Response
     |                                                             ~~~~~~~~~~~~
     | Cannot process argument transformation on parameter 'WebResponse'. Cannot convert the "StatusCode:
     | 404, ReasonPhrase: 'Not Found', Version: 1.1, Content: System.Net.Http.HttpConnectionResponseContent,
     | Headers: {   Date: Fri, 05 Jun 2020 22:17:24 GMT   Server: GitHub.com   Status: 404 Not Found
     | X-RateLimit-Limit: 5000   X-RateLimit-Remaining: 4981   X-RateLimit-Reset: 1591395451
     | X-OAuth-Scopes: admin:org, delete_repo, repo   X-Accepted-OAuth-Scopes: repo   X-GitHub-Media-Type:
     | github.nebula-preview; format=json, github.baptiste-preview; format=json, github.mercy-preview;
     | format=json   Access-Control-Expose-Headers: ETag, Link, Location, Retry-After, X-GitHub-OTP,
     | X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes,
     | X-Poll-Interval, X-GitHub-Media-Type, Deprecation, Sunset   Access-Control-Allow-Origin: *
     | Strict-Transport-Security: max-age=31536000; includeSubdomains; preload   X-Frame-Options: deny
     | X-Content-Type-Options: nosniff   X-XSS-Protection: 1; mode=block   Referrer-Policy:
     | origin-when-cross-origin, strict-origin-when-cross-origin   Content-Security-Policy: default-src
     | 'none'   Vary: Accept-Encoding   Vary: Accept   Vary: X-Requested-With   X-GitHub-Request-Id:
     | E2FD:32667:1C74CFD:222AAEC:5EDAC474   Content-Type: application/json; charset=utf-8   Content-Length:
     | 88 }" value of type "System.Net.Http.HttpResponseMessage" to type "System.Net.HttpWebResponse".

Steps to reproduce the issue

Observe the difference in behavior when calling this code between PS 5 and PS 7

   # Note: this repo doesn't exist -- intentionally trying to get the Exception.
   Get-GitHubRepository -OwnerName microsoft -RepositoryName PowerShellForGitHub2

Verbose logs showing the problem

n/a

Suggested solution to the issue

I did some initial investigation into this, and the additional work that will be needed is creating an equivalent version of Get-HttpWebResponseContent for a HttpResponseMessage. Initially looking, the methods to get the stream are only async, so either an additional wrapper needs to be written to convert the async method to be synchronous, or an alternate approach will need to be found.

Along the way, a little bit of refactoring may need to happen in the error handling of Invoke-GHRestMethod as well.

Requested Assignment

I'm just reporting this problem, but don't want to fix it.

Operating System

OsName               : Microsoft Windows 10 Pro
OsOperatingSystemSKU : 48
OsArchitecture       : 64-bit
WindowsVersion       : 1909
WindowsBuildLabEx    : 18362.1.amd64fre.19h1_release.190318-1202
OsLanguage           : en-US
OsMuiLanguages       : {en-US}

PowerShell Version

Name                           Value
----                           -----
PSVersion                      5.1.18362.752
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.18362.752
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

Module Version

Running: 0.14.0
Installed:
@HowardWolosky HowardWolosky added bug This relates to a bug in the existing module. up for grabs Anyone in the community is welcome to do this work help wanted Anyone in the community is welcome to do this work labels Jun 6, 2020
@X-Guardian
Copy link
Contributor

I would also suggest that rather than using throw, the module should use a helper function to build a System.Management.Automation.ErrorRecord object then call $PSCmdlet.ThrowTerminatingError. See Error Handling in PowerShell - Best Practices for more info.
This allows the creation of a richer error object, specifically with the ability to set the FullyQualifiedErrorId property to a different value to the Exception.Message property and doesn't expose unnecessary internal function code in the error output.

@HowardWolosky
Copy link
Member Author

That all sounds awesome to me. Any interest in tackling it yourself? You already sound well-versed in this area...

@X-Guardian
Copy link
Contributor

I'll take a look at it once PR #177 is merged, as that PR is making significant changes to the Invoke-GHRestMethod function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This relates to a bug in the existing module. help wanted Anyone in the community is welcome to do this work up for grabs Anyone in the community is welcome to do this work
Projects
None yet
Development

No branches or pull requests

2 participants