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.
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
Add ResponseHeadersVariable Parameter to Invoke-RestMethod #4888
Add ResponseHeadersVariable Parameter to Invoke-RestMethod #4888
Conversation
@markekraus, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave a comment
/// Gets or sets the ResponseHeadersVariable property. | ||
/// </summary> | ||
[Parameter] | ||
[Alias("HV")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 'RHV'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer RHV too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
/// </summary> | ||
[Parameter] | ||
[Alias("HV")] | ||
public string ResponseHeadersVariable { get; set; } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
if (!String.IsNullOrEmpty(ResponseHeadersVariable)) | ||
{ | ||
PSVariableIntrinsics vi = SessionState.PSVariable; | ||
vi.Set(ResponseHeadersVariable, WebResponseHelper.GetHeadersDictionary(response)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
$headers.'Content-Type' | Should Be 'text/html; charset=utf-8' | ||
$headers.Server | Should Be 'Kestrel' | ||
} | ||
It "Verifies Invoke-RestMethod supports -HV alias" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never test parameter aliases. If we want test parameter aliases we must create one complex test. I think we should remove the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/// Gets or sets the ResponseHeadersVariable property. | ||
/// </summary> | ||
[Parameter] | ||
[Alias("HV")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer RHV too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! My comments are not blocking.
/// </summary> | ||
[Parameter] | ||
[Alias("RHV")] | ||
public string ResponseHeadersVariable { get; set; } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
if (!String.IsNullOrEmpty(ResponseHeadersVariable)) | ||
{ | ||
PSVariableIntrinsics vi = SessionState.PSVariable; | ||
vi.Set(ResponseHeadersVariable, WebResponseHelper.GetHeadersDictionary(response)); |
There was a problem hiding this comment.
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.
$response = Invoke-RestMethod -Uri $uri -ResponseHeadersVariable 'headers' | ||
|
||
$headers | Should Not Be 'prexisting' | ||
$headers | Should Not BeNullOrEmpty |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@markekraus Thanks for your contribution. |
@markekraus Resposne typo still exists in the body of OP.
|
Fixed. |
closes #4845
-ResponseHeadersVariable
parameter to Invoke-RestMethod-RHV
alias for the parameterWebResponseObject.Headers
is constructed forInvoke-WebRequest
Documentation Needed
The new parameter will need to be documented.