Add explicit ContentType detection to Invoke-RestMethod #4692
Conversation
|
|
||
| [switch] $UseBasicParsing | ||
| ) | ||
| $result = [PSObject]@{Output = $null; Error = $null; Encoding = $null; Content = $null} |
There was a problem hiding this comment.
i think that '[PSObject]' should probably be '[PSCustomObject]' - the [PSObject] doesn't actually convert the type away from a hashtable. So, [PSObject] can be removed, or changed to [PSCustomObject]
| } | ||
| if ($result.Encoding -eq $null) | ||
| { | ||
| throw "Encoding not found in verbose output. Lines: $($result).Verbose.Count Content:$($result.Verbose)" |
There was a problem hiding this comment.
$($result).Verbose.Count should be $($result.Verbose.Count)
There was a problem hiding this comment.
Thanks, I'll fix it.
| { | ||
| $result.Error = $_ | select-object * | Out-String | ||
| } | ||
| finally |
There was a problem hiding this comment.
since the $TESTDRIVE is removed by pester, you only need set the verbosePreference back.
There was a problem hiding this comment.
The test drive isn't cleared until the end of the context block. line 322 uses a test for the file's existence. With the current configuration, subsequent calls to ExecuteRestMethod could result in false positives for the test on line 322 if the file is not removed at the end at the end of the function. This cleanup action would need to remain or the filename generated on each call to ExecuteRestMethod would need to be unique.
There was a problem hiding this comment.
As @markekraus indicates, this is by design and simpler than generating file names.
it also occurs to me that I shouldn't be relying on test/context lifetime for this function; I don't know if it will be called once or multiple times.
I'll be leaving the code as is.
| } | ||
|
|
||
| It "Verifies Invoke-WebRequest detects charset meta value when newlines are encountered in the element." { | ||
| $output = @' |
There was a problem hiding this comment.
just a question, would:
"<html>`n<head>`n<meta`n charset=""Unicode"" `n>`n</head>`n</html>`n"
be the same?
There was a problem hiding this comment.
Yes, but I find the HERE string much easier to read and it avoids me making silly escaping errors.
I'll be leaving it as is.
| $VerbosePreference = $verbosePreferenceSave | ||
| if (Test-Path -Path $verboseFile) | ||
| { | ||
| Remove-Item -Path $verboseFile -ErrorAction Ignore |
There was a problem hiding this comment.
Usualy we use SilentlyContinue not Ignore.
There was a problem hiding this comment.
Gotcha, I'll change it.
| @@ -296,8 +296,71 @@ function ExecuteWebRequest | |||
| return $result | |||
| } | |||
|
|
|||
| [string] $verboseEncodingPrefix = 'Content encoding: ' | |||
There was a problem hiding this comment.
Move $verboseEncodingPrefix into the function body so it does not become orphaned.
There was a problem hiding this comment.
I don't think I want a locally scoped variable that has to be initialized each call.
|
The |
| return $result | ||
| } | ||
|
|
||
| function GetSelfSignedCert { |
There was a problem hiding this comment.
This function is no longer needed it was removed in #4622
There was a problem hiding this comment.
it looks like Dongbo removed it when he merged.
There was a problem hiding this comment.
yes. the commit came in after I hit submit. both of these comments in this review are fixed now
| @@ -831,6 +983,31 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" { | |||
|
|
|||
| #endregion SkipHeaderVerification Tests | |||
|
|
|||
| #region Certificate Authentication Tests | |||
There was a problem hiding this comment.
Everything in this region was removed in #4622
|
@markekraus thanks! @dantraMSFT can you go through the file |
|
@daxian-dbw the merge looks fine and all tests pass on my systems. |
fix #3267
This change removes the call to ContentHelper.GetEncoding since it's logic for returning a default encoding when a ContentType header is not found disables subsequent checks for meta charset definitions.
All variations of the Invoke-WebRequest tests have been mirrored for Invoke-RestMethod. Verbose output is used to verify the encoding since Invoke-RestMethod returns the json content directly instead of a response object. (See ExecuteRestMethod)