Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ResponseHeadersVariable Parameter to Invoke-RestMethod #4888

Merged
merged 7 commits into from Sep 25, 2017
Merged

Add ResponseHeadersVariable Parameter to Invoke-RestMethod #4888

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6bc50d4
Add ResponseHeadersVariable to Invoke-RestMethod
markekraus Sep 21, 2017
bb81e3d
Add Tests for -ResponseHeadersVariable
markekraus Sep 21, 2017
a718058
[Feature] Run Feature Tests
markekraus Sep 21, 2017
2e5c81f
Remove test to address PR feedback
markekraus Sep 21, 2017
0073964
Alais HV -> RHV per request
markekraus Sep 22, 2017
d4b5305
[Feature] Rerun CI
markekraus Sep 22, 2017
8971595
[Feature] Remove Superfluous Assertion
markekraus Sep 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...hell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs
View file
Open in desktop
Expand Up @@ -60,6 +60,13 @@ public int MaximumFollowRelLink
set { base._maximumFollowRelLink = value; }
}

/// <summary>
/// Gets or sets the ResponseHeadersVariable property.
/// </summary>
[Parameter]
[Alias("RHV")]
public string ResponseHeadersVariable { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add any validation attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with it


#endregion Parameters

#region Helper Methods
Expand Down
6 changes: 6 additions & 0 deletions ...ll.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/InvokeRestMethodCommand.CoreClr.cs
View file
Open in desktop
Expand Up @@ -95,6 +95,12 @@ internal override void ProcessResponse(HttpResponseMessage response)
{
StreamHelper.SaveStreamToFile(responseStream, QualifiedOutFile, this);
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
}
}

Expand Down
22 changes: 22 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1
View file
Open in desktop
Expand Up @@ -2071,6 +2071,28 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {

#endregion charset encoding tests

Context 'Invoke-RestMethod ResponseHeadersVariable Tests' {
It "Verifies Invoke-RestMethod supports -ResponseHeadersVariable" {
$uri = Get-WebListenerUrl -Test '/'
$response = Invoke-RestMethod -Uri $uri -ResponseHeadersVariable 'headers'

$headers | Should Not BeNullOrEmpty
$headers.'Content-Type' | Should Be 'text/html; charset=utf-8'
$headers.Server | Should Be 'Kestrel'
}

It "Verifies Invoke-RestMethod supports -ResponseHeadersVariable overwriting existing variable" {
$uri = Get-WebListenerUrl -Test '/'
$headers = 'prexisting'
$response = Invoke-RestMethod -Uri $uri -ResponseHeadersVariable 'headers'

$headers | Should Not Be 'prexisting'
$headers | Should Not BeNullOrEmpty
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. and removed from the other test as well.

$headers.'Content-Type' | Should Be 'text/html; charset=utf-8'
$headers.Server | Should Be 'Kestrel'
}
}

BeforeEach {
if ($env:http_proxy) {
$savedHttpProxy = $env:http_proxy
Expand Down