microsoft / PowerShellForGitHub

Microsoft PowerShell wrapper for GitHub API
Other
600 stars 189 forks source link

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

Open HowardWolosky opened 4 years ago

HowardWolosky commented 4 years ago

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:
X-Guardian commented 4 years ago

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 commented 4 years ago

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

X-Guardian commented 4 years ago

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