Add support for Content Headers to BasicHtmlWebResponseObject and HtmlWebResponseObject #4494

markekraus · 2017-08-04T10:54:40Z

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.

msftclas · 2017-08-04T10:54:44Z

@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

iSazonov · 2017-08-05T17:50:08Z

@markekraus Could you please add tests?

markekraus · 2017-08-05T19:33:28Z

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

iSazonov · 2017-08-05T20:12:46Z

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

markekraus · 2017-08-05T20:19:44Z

@iSazonov doh... corrected.

TravisEz13 · 2017-08-05T21:13:12Z

I agree with @iSazonov . The tests in the issue look like a good starting point.

markekraus · 2017-08-05T21:32:46Z

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

markekraus · 2017-08-08T09:53:31Z

@TravisEz13 @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.

dantraMSFT · 2017-08-17T18:25:22Z

...soft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/ContentHelper.CoreClr.cs

+            };

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


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

@dantraMSFT corrected.

dantraMSFT · 2017-08-17T18:25:46Z

....PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebResponseObject.CoreClr.cs

                {
                    headers[entry.Key] = entry.Value;
                }
+                if(BaseResponse.Content != null){


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

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

dantraMSFT · 2017-08-17T18:38:39Z

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

    #endregion charset encoding tests

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


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.

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

dantraMSFT · 2017-08-17T18:53:34Z

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


    #endregion charset encoding tests

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


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.

markekraus · 2017-08-18T12:23:32Z

@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 · 2017-08-18T17:53:46Z

test/tools/Modules/HttpListener/HttpListener.psm1

                            $statusCode = $queryItems["statuscode"]
                            $contentType = $queryItems["contenttype"]
                            $output = $queryItems["output"]
+                            if ($queryItems['headers'])


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.

@dantraMSFT added. comments with examples

TravisEz13 · 2017-08-18T19:57:06Z

LGTM

adityapatwardhan · 2017-08-18T20:49:09Z

@markekraus Can you trigger of feature tests? They seems to have failed last time on Travis CI.

markekraus · 2017-08-18T21:06:30Z

@adityapatwardhan feature tag added.

markekraus · 2017-08-19T16:48:01Z

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

iSazonov · 2017-08-19T19:17:24Z

LGTM.

Thanks for great work!

msftclas added the cla-already-signed label Aug 4, 2017

TravisEz13 requested a review from SteveL-MSFT August 5, 2017 17:00

TravisEz13 self-assigned this Aug 5, 2017

iSazonov reviewed Aug 5, 2017

View reviewed changes

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

Copy link

Collaborator

iSazonov Aug 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo Wen

TravisEz13 added the WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module label Aug 5, 2017

markekraus force-pushed the GetRawContentHeader branch from 73d19ed to 823dd03 Compare August 7, 2017 23:18

[Feature] Add Support for Content Headers

75b6f54

markekraus force-pushed the GetRawContentHeader branch from 823dd03 to 75b6f54 Compare August 17, 2017 09:18

markekraus requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners August 17, 2017 09:18

dantraMSFT reviewed Aug 17, 2017

View reviewed changes

Format changes

5e3854a

dantraMSFT suggested changes Aug 17, 2017

View reviewed changes

[feature] httplistener multiheader support & native contet header tests

dfef828

markekraus requested review from TravisEz13, adityapatwardhan and daxian-dbw as code owners August 18, 2017 12:18

dantraMSFT reviewed Aug 18, 2017

View reviewed changes

dantraMSFT approved these changes Aug 18, 2017

View reviewed changes

TravisEz13 approved these changes Aug 18, 2017

View reviewed changes

[Feature] add comments

07d7b3d

markekraus force-pushed the GetRawContentHeader branch from 42f5aca to 07d7b3d Compare August 18, 2017 20:58

TravisEz13 merged commit 297bba3 into PowerShell:master Aug 21, 2017

markekraus deleted the GetRawContentHeader branch August 25, 2017 00:17

markekraus mentioned this pull request Sep 18, 2017

Initialize Headers Dictionary Only Once #4853

Merged

markekraus mentioned this pull request Oct 28, 2017

Link header pagination only parsing first rel #5257

Closed


		#endregion charset encoding tests

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

Add support for Content Headers to BasicHtmlWebResponseObject and HtmlWebResponseObject #4494

Add support for Content Headers to BasicHtmlWebResponseObject and HtmlWebResponseObject #4494

Uh oh!

Conversation

markekraus commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Aug 4, 2017

Uh oh!

iSazonov commented Aug 5, 2017

Uh oh!

markekraus commented Aug 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markekraus commented Aug 5, 2017

Uh oh!

TravisEz13 commented Aug 5, 2017

Uh oh!

markekraus commented Aug 5, 2017

Uh oh!

markekraus commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markekraus Aug 17, 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!

markekraus commented Aug 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Aug 18, 2017

Uh oh!

adityapatwardhan commented Aug 18, 2017

Uh oh!

markekraus commented Aug 18, 2017

Uh oh!

markekraus commented Aug 19, 2017

Uh oh!

iSazonov commented Aug 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

markekraus commented Aug 4, 2017 •

edited

Loading

markekraus Aug 17, 2017 •

edited

Loading