Add ResponseHeadersVariable Parameter to Invoke-RestMethod by markekraus · Pull Request #4888 · PowerShell/PowerShell

markekraus · Sep 21, 2017

Adds -ResponseHeadersVariable parameter to Invoke-RestMethod
Adds -RHV alias for the parameter
A variable with the provided name is created in the calling scope which contains the HTTP Response Headers as a dictionary constructed in the same manner WebResponseObject.Headers is constructed for Invoke-WebRequest
Add tests for the parameter

Documentation Needed

The new parameter will need to be documented.

msftclas · Sep 21, 2017

@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

Leave a comment

iSazonov · Sep 21, 2017

+        /// Gets or sets the ResponseHeadersVariable property.
+        /// </summary>
+        [Parameter]
+        [Alias("HV")]


Maybe 'RHV'?

I'm up for suggestions on this one. I was following suit with SessionVariable's SV. I would have used -HeadersVariable for the name if that wouldn't have been mistaken for -Headers

I prefer RHV too.

iSazonov · Sep 21, 2017

+        /// </summary>
+        [Parameter]
+        [Alias("HV")]
+        public string ResponseHeadersVariable { get; set; }


Should we add any validation attributes?

SessionVariable does something similar and it doesn't have any validation. I'm not sure what value it would add. Aside from being null or empty (which is tested to set it on line 99) a variable name can be any valid string.

iSazonov · Sep 21, 2017

+                if (!String.IsNullOrEmpty(ResponseHeadersVariable))
+                {
+                    PSVariableIntrinsics vi = SessionState.PSVariable;
+                    vi.Set(ResponseHeadersVariable, WebResponseHelper.GetHeadersDictionary(response));


Should we return ReadOnlyDictionary?
Does it make sense to return an entire response?

I have seen cases where users are taking the returned dictionary, updating it, and then supplying back to further calls. I don't see much benefit. Also, System.Net.Http.Headers.HttpResponseHeaders is not read-only.

$Res = invoke-webrequest https://google.com $Res.BaseResponse.Headers.TryAddWithoutValidation('testy','lala') $Res.BaseResponse.Headers.GetValues('testy')

results with lala

As for the returning entire response, that would be somewhat pointless with Invoke-RestMethod, IMO. If the user needs the full response object, they can use Invoke-WebRequest. This is just adding the ability to get the response headers which many API's use to provide information along with the JSON/XML response. But, a separate PR could be made to return the response object if that is something people really want.

Agree that we should keep complex self-parsing usage to Invoke-WebRequest and target most common easy to use scenarios for Invoke-RestMethod. This seems fine to me.

iSazonov · Sep 21, 2017

+            $headers.'Content-Type' | Should Be 'text/html; charset=utf-8'
+            $headers.Server | Should Be 'Kestrel'
+        }
+        It "Verifies Invoke-RestMethod supports -HV alias" {


We never test parameter aliases. If we want test parameter aliases we must create one complex test. I think we should remove the test.

adityapatwardhan

LGTM

adityapatwardhan · Sep 22, 2017

+        /// Gets or sets the ResponseHeadersVariable property.
+        /// </summary>
+        [Parameter]
+        [Alias("HV")]


I prefer RHV too.

SteveL-MSFT

Thanks for the contribution! My comments are not blocking.

SteveL-MSFT · Sep 23, 2017

+        /// </summary>
+        [Parameter]
+        [Alias("RHV")]
+        public string ResponseHeadersVariable { get; set; }


I think the guideline is to still use singular ResponseHeaderVariable unless the output is always not singular. It also seems that HTTP 1.1 calls the whole thing a Response Header and the individual headers as fields.

I chose Headers because we are using the -Headers parameter as well as WebResponseObject.Headers (which this provides parity for) and HttpResponseMessage.Headers & HttpResponseMessage.Content.Headers (which the dictionary is generated from). I can change it, but it would be the odd-man-out.

If the resulting variable provided a string with the raw Response Header, I would agree. But I think ResponseHeadersVariable makes clear that it is collection of some kind.

I'm fine with it

SteveL-MSFT · Sep 23, 2017

+                if (!String.IsNullOrEmpty(ResponseHeadersVariable))
+                {
+                    PSVariableIntrinsics vi = SessionState.PSVariable;
+                    vi.Set(ResponseHeadersVariable, WebResponseHelper.GetHeadersDictionary(response));


Agree that we should keep complex self-parsing usage to Invoke-WebRequest and target most common easy to use scenarios for Invoke-RestMethod. This seems fine to me.

SteveL-MSFT · Sep 23, 2017

+            $response = Invoke-RestMethod -Uri $uri -ResponseHeadersVariable 'headers'
+
+            $headers | Should Not Be 'prexisting'
+            $headers | Should Not BeNullOrEmpty


Seems like this test isn't needed as the next line would fail

Fixed. and removed from the other test as well.

adityapatwardhan · Sep 25, 2017

@markekraus Thanks for your contribution.

pr3sidentspence · Dec 31, 2018

@markekraus Resposne typo still exists in the body of OP.

Adds -ResposneHeadersVariable parameter ...

iSazonov · Jan 1, 2019

Fixed.

markekraus added 2 commits September 21, 2017 04:12

Add ResponseHeadersVariable to Invoke-RestMethod

6bc50d4

Add Tests for -ResponseHeadersVariable

bb81e3d

markekraus requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners September 21, 2017 09:53

msftclas added the cla-already-signed label Sep 21, 2017

[Feature] Run Feature Tests

a718058

iSazonov changed the title ~~Add ResposneHeadersVariable Parameter to Invoke-RestMethod~~ Add ResponseHeadersVariable Parameter to Invoke-RestMethod Sep 21, 2017

iSazonov reviewed Sep 21, 2017

View reviewed changes

Remove test to address PR feedback

2e5c81f

markekraus mentioned this pull request Sep 21, 2017

Improve UTF8 Tests for Invoke-WebRequest and Invoke-RestMethod #4890

Closed

daxian-dbw assigned adityapatwardhan Sep 21, 2017

adityapatwardhan added the Documentation Needed in this repo Documentation is needed in this repo label Sep 21, 2017

adityapatwardhan approved these changes Sep 22, 2017

View reviewed changes

markekraus added 2 commits September 22, 2017 13:39

Alais HV -> RHV per request

0073964

[Feature] Rerun CI

d4b5305

SteveL-MSFT reviewed Sep 23, 2017

View reviewed changes

[Feature] Remove Superfluous Assertion

8971595

SteveL-MSFT approved these changes Sep 23, 2017

View reviewed changes

iSazonov approved these changes Sep 23, 2017

View reviewed changes

adityapatwardhan merged commit 3237d43 into PowerShell:master Sep 25, 2017

This was referenced Sep 26, 2017

'/' WebListener test broken #4919

Closed

Add ResponseHeadersVariable Parameter Documentation MicrosoftDocs/PowerShell-Docs#1696

Merged

markekraus mentioned this pull request Oct 5, 2017

beta.8 changelog draft #5006

Merged

markekraus removed the Documentation Needed in this repo Documentation is needed in this repo label Oct 20, 2017

markekraus deleted the InvokeRestMethodResponseHeadersVar branch January 19, 2018 18:58

Search code, repositories, users, issues, pull requests...

Conversation

markekraus commented Sep 21, 2017 • edited by iSazonov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation Needed

Uh oh!

msftclas commented Sep 21, 2017

Uh oh!

iSazonov 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!

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!

adityapatwardhan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markekraus Sep 23, 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!

adityapatwardhan commented Sep 25, 2017

Uh oh!

pr3sidentspence commented Dec 31, 2018

Uh oh!

iSazonov commented Jan 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

markekraus commented Sep 21, 2017 •

edited by iSazonov

Loading

markekraus Sep 23, 2017 •

edited

Loading