Skip to content

Add ResponseHeadersVariable Parameter to Invoke-RestMethod #4888

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

Merged
adityapatwardhan merged 7 commits into from
Sep 25, 2017
Merged

Add ResponseHeadersVariable Parameter to Invoke-RestMethod #4888

Show file tree
Hide file tree
Changes from all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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
Original file line number Diff line number Diff line change
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
Member

@SteveL-MSFT SteveL-MSFT Sep 23, 2017

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

@markekraus markekraus Sep 23, 2017 •
edited
Loading

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

@SteveL-MSFT SteveL-MSFT Sep 23, 2017

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
Original file line number Diff line number Diff line change
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

@iSazonov iSazonov Sep 21, 2017

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

@markekraus markekraus Sep 21, 2017

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

@SteveL-MSFT SteveL-MSFT Sep 23, 2017

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
20 changes: 20 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2071,6 +2071,26 @@ 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.'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.'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