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

@@ -734,6 +734,24 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {

    #endregion SkipHeaderVerification Tests

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


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

@@ -36,6 +36,12 @@ public partial class WebResponseObject
                {
                    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

@@ -1070,6 +1070,24 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {

    #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

@@ -1070,6 +1070,24 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {

    #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

@@ -133,6 +133,14 @@ Function Start-HTTPListener {
                            $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

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 dantraMSFT, JamesWTruher and PaulHigin 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 adityapatwardhan, daxian-dbw and TravisEz13 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Content Headers to BasicHtmlWebResponseObject and HtmlWebResponseObject #4494

Add support for Content Headers to BasicHtmlWebResponseObject and HtmlWebResponseObject #4494

markekraus commented Aug 4, 2017 •

edited

msftclas commented Aug 4, 2017

iSazonov commented Aug 5, 2017

markekraus commented Aug 5, 2017

iSazonov Aug 5, 2017

markekraus commented Aug 5, 2017

TravisEz13 commented Aug 5, 2017

markekraus commented Aug 5, 2017

markekraus commented Aug 8, 2017

dantraMSFT Aug 17, 2017

markekraus Aug 17, 2017 •

edited

dantraMSFT Aug 17, 2017

markekraus Aug 17, 2017

dantraMSFT Aug 17, 2017

markekraus Aug 17, 2017

dantraMSFT Aug 17, 2017

markekraus commented Aug 18, 2017

dantraMSFT Aug 18, 2017

markekraus Aug 18, 2017

TravisEz13 commented Aug 18, 2017

adityapatwardhan commented Aug 18, 2017

markekraus commented Aug 18, 2017

markekraus commented Aug 19, 2017

iSazonov commented Aug 19, 2017

		@@ -734,6 +734,24 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {

		#endregion SkipHeaderVerification Tests

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


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

		@@ -1070,6 +1070,24 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {

		#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

Conversation

markekraus commented Aug 4, 2017 • edited

msftclas commented Aug 4, 2017

iSazonov commented Aug 5, 2017

markekraus commented Aug 5, 2017

Choose a reason for hiding this comment

markekraus commented Aug 5, 2017

TravisEz13 commented Aug 5, 2017

markekraus commented Aug 5, 2017

markekraus commented Aug 8, 2017

Choose a reason for hiding this comment

markekraus Aug 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markekraus commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TravisEz13 commented Aug 18, 2017

adityapatwardhan commented Aug 18, 2017

markekraus commented Aug 18, 2017

markekraus commented Aug 19, 2017

iSazonov commented Aug 19, 2017

markekraus commented Aug 4, 2017 •

edited

markekraus Aug 17, 2017 •

edited