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 — Add support for Content Headers to BasicHtmlWebResponseObject and HtmlWebResponseObject by markekraus · Pull Request #4494 · PowerShell/PowerShell

Conversation

@markekraus

@markekraus Mark Kraus (markekraus) commented Aug 4, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Fixes #4467

System.Net.Http.HttpResponseMessage has split the Content headers from the Response headers. This split appears to be intentional and permanent. This split results in the Content-Type and Content-Length headers to be missing from the WebResponseObject.Headers dictionary and from the RawContent property of BasicHtmlWebResponseObject and HtmlWebResponseObject.

This adds the Content headers back to those locations to make it similar to 5.1.

This also adds support to RawContent for multiple response headers with the same field name. System.Net.Http.Headers.HttpContentHeaders and System.Net.Http.Headers.HttpResponseHeaders implement values as arrays.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
chuanjiao10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@msftclas

Microsoft Contribution License Agreements (msftclas) commented Aug 4, 2017

Copy link
Copy Markdown

Mark Kraus (@markekraus),
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Aug 4, 2017
@TravisEz13
Travis Plunk (TravisEz13) requested a review from Steve Lee (SteveL-MSFT) August 5, 2017 17:00
@TravisEz13 Travis Plunk (TravisEz13) self-assigned this Aug 5, 2017
@iSazonov

Ilya (iSazonov) commented Aug 5, 2017

Copy link
Copy Markdown
Collaborator

Mark Kraus (@markekraus) Could you please add tests?
TravisEz13 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

Mark Kraus (markekraus) commented Aug 5, 2017

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) Tests Added. I wasn't exactly sure what tests to add. There is already significant validation being done on the Headers and RawContent properties. If I need more than these 2 tests please let me know.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) reviewed Aug 5, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated

@iSazonov Ilya (iSazonov) Aug 5, 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.

Typo Wen

Sorry, something went wrong.

Uh oh!

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

Mark Kraus (markekraus) commented Aug 5, 2017

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) doh... corrected.
All reactions

Sorry, something went wrong.

Uh oh!

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

Travis Plunk (TravisEz13) commented Aug 5, 2017

Copy link
Copy Markdown
Member

I agree with Ilya (@iSazonov) . The tests in the issue look like a good starting point.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13 Travis Plunk (TravisEz13) added the Area-Cmdlets-Utility label Aug 5, 2017
@markekraus

Mark Kraus (markekraus) commented Aug 5, 2017

Copy link
Copy Markdown
Contributor Author

Travis Plunk (@TravisEz13) Thanks. I added the 2 test that make sense. The other tests from the issue won't ever work unless CoreFX drastically changes their approach in System.Net.Http.HttpResponseMessage.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markekraus
Mark Kraus (markekraus) force-pushed the GetRawContentHeader branch from 73d19ed to 823dd03 Compare August 7, 2017 23:18
@markekraus

Mark Kraus (markekraus) commented Aug 8, 2017

Copy link
Copy Markdown
Contributor Author

Travis Plunk (@TravisEz13) Steve Lee (@SteveL-MSFT) Rebased PR on master to add test fixes from #4512. The new tests are passing. Let me know if I need to make any other changes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markekraus
[Feature] Add Support for Content Headers
75b6f54
@markekraus
Mark Kraus (markekraus) force-pushed the GetRawContentHeader branch from 823dd03 to 75b6f54 Compare August 17, 2017 09:18
@markekraus
Mark Kraus (markekraus) requested review from James Truher (JamesWTruher), Paul Higinbotham (PaulHigin) and Dan Travison (dantraMSFT) as code owners August 17, 2017 09:18
dantraMSFT
Dan Travison (dantraMSFT) reviewed Aug 17, 2017
View reviewed changes
Comment thread ...soft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/ContentHelper.CoreClr.cs Outdated
};

foreach (var entry in response.Headers)
foreach(var headerCollection in headerCollections)

@dantraMSFT Dan Travison (dantraMSFT) Aug 17, 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.

Minor point, insert a space after 'foreach' here and 'if' below to match the rest of the code.

Sorry, something went wrong.

Uh oh!

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

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

Dan Travison (@dantraMSFT) corrected.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
dantraMSFT
Dan Travison (dantraMSFT) reviewed Aug 17, 2017
View reviewed changes
Comment thread ....PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebResponseObject.CoreClr.cs Outdated
{
headers[entry.Key] = entry.Value;
}
if(BaseResponse.Content != null){

@dantraMSFT Dan Travison (dantraMSFT) Aug 17, 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.

Minor note: place '{' on a new line.

Sorry, something went wrong.

Uh oh!

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

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

Dan Travison (@dantraMSFT) corrected, along with space between if and (.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markekraus
Format changes
5e3854a
dantraMSFT
Dan Travison (dantraMSFT) suggested changes Aug 17, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
#endregion charset encoding tests

It "Verifies Invoke-WebRequest includes Content headers in Headers property" {
$command = "Invoke-WebRequest -Uri 'http://httpbin.org/get'"

@dantraMSFT Dan Travison (dantraMSFT) Aug 17, 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.

Use HTTPListener here instead of http://httpbin.org/get.
The tests under Context "BasicHtmlWebResponseObject Encoding tests" illustrate using 'test=response&output=$otuput' to pass HTML you want to get in the response. You should consider extending httplistener's 'response' option to allow additional options, such as specifying a content-type header value or other headers to support your testing. let me know if you need any help with it.

Sorry, something went wrong.

Uh oh!

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

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

Dan Travison (@dantraMSFT) Thanks. I will work on this and submit more and better tests with your suggestions.

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

#endregion charset encoding tests

It "Verifies Invoke-WebRequest includes Content headers in Headers property" {

@dantraMSFT Dan Travison (dantraMSFT) Aug 17, 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 think these two tests need to be either be extended or additional tests added.
1: I don't believe verifying the presence of the header is sufficient; I believe the test should verify the value(s) as well.
2: You added logic to support headers with multiple values so you need to test that case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markekraus
[feature] httplistener multiheader support & native contet header tests
dfef828
@markekraus
Mark Kraus (markekraus) requested review from Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan) and Dongbo Wang (daxian-dbw) as code owners August 18, 2017 12:18
@markekraus

Mark Kraus (markekraus) commented Aug 18, 2017

Copy link
Copy Markdown
Contributor Author

Dan Travison (@dantraMSFT) I have added the requested test changes and httplistener now supports a headers option with a JSON object containing response header name/value pairs on the response test.
dantraMSFT reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
dantraMSFT
Dan Travison (dantraMSFT) reviewed Aug 18, 2017
View reviewed changes
Comment thread test/tools/Modules/HttpListener/HttpListener.psm1
$statusCode = $queryItems["statuscode"]
$contentType = $queryItems["contenttype"]
$output = $queryItems["output"]
if ($queryItems['headers'])

@dantraMSFT Dan Travison (dantraMSFT) Aug 18, 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 would be helpful to add a comment here that illustrates how the 'headers' value should be formatted; especially since this is the only documentation we have for the listener. :)
At a minimum, show the json structure. Even better would be to show a code sample similar to what you used in your tests.

Sorry, something went wrong.

Uh oh!

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

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

Dan Travison (@dantraMSFT) added. comments with examples

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
dantraMSFT reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
dantraMSFT
Dan Travison (dantraMSFT) approved these changes Aug 18, 2017
View reviewed changes
TravisEz13
Travis Plunk (TravisEz13) approved these changes Aug 18, 2017
View reviewed changes
@TravisEz13

Travis Plunk (TravisEz13) commented Aug 18, 2017

Copy link
Copy Markdown
Member

LGTM
markekraus reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

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

Aditya Patwardhan (adityapatwardhan) commented Aug 18, 2017

Copy link
Copy Markdown
Member

Mark Kraus (@markekraus) Can you trigger of feature tests? They seems to have failed last time on Travis CI.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markekraus
[Feature] add comments
07d7b3d
@markekraus
Mark Kraus (markekraus) force-pushed the GetRawContentHeader branch from 42f5aca to 07d7b3d Compare August 18, 2017 20:58
@markekraus

Mark Kraus (markekraus) commented Aug 18, 2017

Copy link
Copy Markdown
Contributor Author

Aditya Patwardhan (@adityapatwardhan) feature tag added.
All reactions

Sorry, something went wrong.

Uh oh!

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

Mark Kraus (markekraus) commented Aug 19, 2017

Copy link
Copy Markdown
Contributor Author

Aditya Patwardhan (@adityapatwardhan) all tests passed this time. (last time an unrelated test hung accessing a 3rd party website).
All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Aug 19, 2017

Copy link
Copy Markdown
Collaborator

LGTM.

Thanks for great work!
markekraus reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13
Travis Plunk (TravisEz13) merged commit 297bba3 into PowerShell:master Aug 21, 2017
This was referenced Aug 22, 2017
Add support for multiple response headers with the same name from HttpListener #4639
Closed
[feature] Mark Multiple Response Header Test Pending #4640
Merged
Pull Request Tests with [feature] tag in commits should report failed in CI when "Feature" tests fail #4641
Closed
@markekraus
Mark Kraus (markekraus) deleted the GetRawContentHeader branch August 25, 2017 00:17
@markekraus Mark Kraus (markekraus) mentioned this pull request Sep 18, 2017
Initialize Headers Dictionary Only Once #4853
Merged
@markekraus Mark Kraus (markekraus) mentioned this pull request Oct 28, 2017
Link header pagination only parsing first rel #5257
Closed
@kilasuit Ryan Yates (kilasuit) added WG-Cmdlets general cmdlet issues and removed WG-Cmdlets-Utility labels Jul 17, 2026
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@markekraus @TravisEz13
Add support for Content Headers to BasicHtmlWebResponseObject and Htm…
9be6f73 
…lWebResponseObject (PowerShell#4494)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@TravisEz13 Travis Plunk (TravisEz13) TravisEz13 approved these changes
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Awaiting requested review from adityapatwardhan
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw
+1 more reviewer
@dantraMSFT Dan Travison (dantraMSFT) dantraMSFT approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@TravisEz13 Travis Plunk (TravisEz13)

Labels

WG-Cmdlets general cmdlet issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Invoke-WebRequest removes the Content-Type header from WebResponseObject's

Uh oh!

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

7 participants

@markekraus @msftclas @iSazonov @TravisEz13 @adityapatwardhan @dantraMSFT @kilasuit
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.