GeezerVison

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

GeezerVison is showing github.com — Add explicit ContentType detection to Invoke-RestMethod by dantraMSFT · Pull Request #4692 · PowerShell/PowerShell

Conversation

Dan Travison (dantraMSFT) commented Aug 28, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

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)

Sorry, something went wrong.

Uh oh!

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

All reactions


          [Feature]

365d06d

Add explicit ContentType detection to Invoke-RestMethod

Dan Travison (dantraMSFT) requested review from James Truher (JamesWTruher) and Paul Higinbotham (PaulHigin) as code owners

August 28, 2017 22:36

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label

Aug 28, 2017

Dan Travison (dantraMSFT) mentioned this pull request

Aug 28, 2017

Use HTML meta charset attribute value, if present, when the Context-Type header does not specify it. #4338

Merged

James Truher (JamesWTruher) suggested changes

Aug 28, 2017

View reviewed changes

Comment thread

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


		[switch] $UseBasicParsing
		)
		$result = [PSObject]@{Output = $null; Error = $null; Encoding = $null; Content = $null}

James Truher (JamesWTruher) Aug 28, 2017

Copy link

Copy Markdown

Collaborator

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 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]

Sorry, something went wrong.

Uh oh!

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

All reactions

Dan Travison (dantraMSFT) Aug 29, 2017

Copy link

Copy Markdown

Contributor 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

Dan Travison (dantraMSFT) Aug 29, 2017

Copy link

Copy Markdown

Contributor 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

Comment thread

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

		}
		if ($result.Encoding -eq $null)
		{
		throw "Encoding not found in verbose output. Lines: $($result).Verbose.Count Content:$($result.Verbose)"

James Truher (JamesWTruher) Aug 28, 2017

Copy link

Copy Markdown

Collaborator

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.

$($result).Verbose.Count should be $($result.Verbose.Count)

Sorry, something went wrong.

Uh oh!

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

All reactions

Dan Travison (dantraMSFT) Aug 29, 2017

Copy link

Copy Markdown

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

Thanks, I'll fix it.

Sorry, something went wrong.

Uh oh!

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

All reactions

Dan Travison (dantraMSFT) Aug 29, 2017

Copy link

Copy Markdown

Contributor 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

Comment thread

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

		{
		$result.Error = $_ \| select-object * \| Out-String
		}
		finally

James Truher (JamesWTruher) Aug 28, 2017

Copy link

Copy Markdown

Collaborator

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.

since the $TESTDRIVE is removed by pester, you only need set the verbosePreference back.

Sorry, something went wrong.

Uh oh!

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

All reactions

Mark Kraus (markekraus) Aug 29, 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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

Dan Travison (dantraMSFT) Aug 29, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

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

As Mark Kraus (@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.

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

		}

		It "Verifies Invoke-WebRequest detects charset meta value when newlines are encountered in the element." {
		$output = @'

James Truher (JamesWTruher) Aug 28, 2017

Copy link

Copy Markdown

Collaborator

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.

just a question, would:

"<html>`n<head>`n<meta`n   charset=""Unicode""  `n>`n</head>`n</html>`n"

be the same?

Sorry, something went wrong.

Uh oh!

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

All reactions

Dan Travison (dantraMSFT) Aug 29, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

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

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

Ilya (iSazonov) reviewed

Aug 29, 2017

View reviewed changes

Comment thread

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

		$VerbosePreference = $verbosePreferenceSave
		if (Test-Path -Path $verboseFile)
		{
		Remove-Item -Path $verboseFile -ErrorAction Ignore

Ilya (iSazonov) Aug 29, 2017

Copy link

Copy Markdown

Collaborator

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.

Usualy we use SilentlyContinue not Ignore.

Sorry, something went wrong.

Uh oh!

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

All reactions

Dan Travison (dantraMSFT) Aug 29, 2017

Copy link

Copy Markdown

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

Gotcha, I'll change it.

Sorry, something went wrong.

Uh oh!

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

All reactions

Ilya (iSazonov) Aug 29, 2017

Copy link

Copy Markdown

Collaborator

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.

Closed.

Sorry, something went wrong.

Uh oh!

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

All reactions


          Address code review feedback.

2fb6c67

Mark Kraus (markekraus) suggested changes

Aug 29, 2017

View reviewed changes

Comment thread

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

		return $result
		}

		[string] $verboseEncodingPrefix = 'Content encoding: '

Mark Kraus (markekraus) Aug 29, 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.

Move $verboseEncodingPrefix into the function body so it does not become orphaned.

Sorry, something went wrong.

Uh oh!

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

All reactions

Dan Travison (dantraMSFT) Aug 29, 2017

Copy link

Copy Markdown

Contributor 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 don't think I want a locally scoped variable that has to be initialized each call.

Sorry, something went wrong.

Uh oh!

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

All reactions

Mark Kraus (markekraus) Aug 29, 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.

fair enough

Sorry, something went wrong.

Uh oh!

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

All reactions

James Truher (JamesWTruher) approved these changes

Aug 30, 2017

View reviewed changes

Dan Travison (dantraMSFT) assigned Dongbo Wang (daxian-dbw)

Aug 30, 2017

Dongbo Wang (daxian-dbw) commented Aug 31, 2017

Copy link

Copy Markdown

Member

The WebCmdlets.Tests.ps1 has conflicts due to the merge of #4622 🤕

All reactions

Sorry, something went wrong.

Uh oh!

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


          Merge branch 'master' into dantra/issue3267

5eb32d3

Mark Kraus (markekraus) suggested changes

Aug 31, 2017

View reviewed changes

Mark Kraus (markekraus) left a comment

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.

Artifacts from merge should be removed.

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

		return $result
		}

		function GetSelfSignedCert {

Mark Kraus (markekraus) Aug 31, 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.

This function is no longer needed it was removed in #4622

Sorry, something went wrong.

Uh oh!

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

All reactions

Dan Travison (dantraMSFT) Aug 31, 2017

Copy link

Copy Markdown

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

it looks like Dongbo removed it when he merged.

Sorry, something went wrong.

Uh oh!

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

All reactions

Mark Kraus (markekraus) Aug 31, 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.

yes. the commit came in after I hit submit. both of these comments in this review are fixed now

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


		#endregion SkipHeaderVerification Tests

		#region Certificate Authentication Tests

Mark Kraus (markekraus) Aug 31, 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.

Everything in this region was removed in #4622

Sorry, something went wrong.

Uh oh!

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

All reactions


          Resolve conflict issues

7be9fc5

Dongbo Wang (daxian-dbw) commented Aug 31, 2017

Copy link

Copy Markdown

Member

Mark Kraus (@markekraus) thanks! Dan Travison (@dantraMSFT) can you go through the file WebCmdlets.Tests.ps1 and see if things are good after resolving conflicts?

All reactions

Sorry, something went wrong.

Uh oh!

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

Dan Travison (dantraMSFT) commented Aug 31, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor Author

Dongbo Wang (@daxian-dbw) the merge looks fine and all tests pass on my systems.

All reactions

Sorry, something went wrong.

Uh oh!

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

Dongbo Wang (daxian-dbw) merged commit 96a90f8 into PowerShell:master

Aug 31, 2017

Dan Travison (dantraMSFT) deleted the dantra/issue3267 branch

August 31, 2017 19:41

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Add explicit ContentType detection to Invoke-RestMethod (PowerShell#4692

3db485c

)

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)

Reviewers

Ilya (iSazonov)

iSazonov left review comments

Paul Higinbotham (PaulHigin)

Awaiting requested review from PaulHigin

+2 more reviewers

Mark Kraus (markekraus)

markekraus requested changes

James Truher (JamesWTruher)

JamesWTruher approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Webcmdlets should parse the <html><head><meta charset="foo"> attribute for the correct encoding if not in http header

Uh oh!

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

6 participants

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 01402520 — 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.

Conversation

Dan Travison (dantraMSFT) commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Dan Travison (dantraMSFT) Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dan Travison (dantraMSFT) Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Dongbo Wang (daxian-dbw) commented Aug 31, 2017

Uh oh!

Mark Kraus (markekraus) left a comment

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!

Dongbo Wang (daxian-dbw) commented Aug 31, 2017

Uh oh!

Dan Travison (dantraMSFT) commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Dan Travison (dantraMSFT) commented Aug 28, 2017 •

edited

Loading

Uh oh!

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

Dan Travison (dantraMSFT) Aug 29, 2017 •

edited

Loading

Uh oh!

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

Dan Travison (dantraMSFT) Aug 29, 2017 •

edited

Loading

Uh oh!

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

Dan Travison (dantraMSFT) commented Aug 31, 2017 •

edited

Loading

Uh oh!

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