adds parameter sets to web cmdlets to allow for standard and non-stan… #3142
Conversation
…dard method verbs
|
I can see that the tests are failing, however I've confirmed this to be passed locally - please excuse if I'm missing something here |
|
I was working on a PR for this too. I'll drop a few comments on what I came across. |
| /// <summary> | ||
| /// gets or sets the parameter CustomMethod | ||
| /// </summary> | ||
| [Parameter(ParameterSetName = "CustomMethod")] |
There was a problem hiding this comment.
You can make this validate on input and removed the checks for IsNullOrEmpty. Also adding an alias now would be nice.
[ValidateNotNullOrEmpty]
[Alias("CM")]
| @@ -547,7 +558,8 @@ private Uri PrepareUri(Uri uri) | |||
| IDictionary bodyAsDictionary; | |||
| LanguagePrimitives.TryConvertTo<IDictionary>(Body, out bodyAsDictionary); | |||
| if ((null != bodyAsDictionary) | |||
| && (Method == WebRequestMethod.Default || Method == WebRequestMethod.Get)) | |||
| && ((IsStandardMethodSet() && (Method == WebRequestMethod.Default || Method == WebRequestMethod.Get)) | |||
| || (IsCustomMethodSet() && (string.IsNullOrEmpty(CustomMethod) || CustomMethod.ToUpper() == "GET")))) | |||
There was a problem hiding this comment.
Should be consistent and use ToUpperInvariant().
|
Core CLR needs the love too. WebRequestPSCmdlet.CoreClr.cs |
|
Completely overlooked core! Will sort, thank you :) |
| @@ -84,10 +84,22 @@ internal virtual WebRequest GetRequest(Uri uri) | |||
| request.Proxy = WebSession.Proxy; | |||
| } | |||
|
|
|||
| // set the method if the parameter was provided | |||
| if (WebRequestMethod.Default != Method) | |||
| switch (ParameterSetName) | |||
There was a problem hiding this comment.
I had this simplified to this. Since CustomMethod will always be null or a real string (param validation) checking for $null should be enough.
// set the custom method if the parameter was provided
if(CustomMethod != null)
{
request.Method = CustomMethod.ToUpperInvariant();
}
else
{
// set the method if the parameter was provided
if (WebRequestMethod.Default != Method)
{
request.Method = Method.ToString().ToUpperInvariant();
}
}|
Last commits add Core implementation and include suggestions :) |
|
Linux CI build errored, will need rerunning as it didn't actually fail |
|
@lee303 Please sign the CLA. I can't do anything with this PR until it is signed. |
|
All signed :) @mirichmo< 8000 /p> |
|
Closing and reopening PR to trigger CLA bot |
|
just noticed that the Apple build failed this time - failed to download dotnet from windows.net, hopefully someone with the powers can re-run |
|
@lee303 FYI, you can always close/reopen the PR to restart the CI |
|
@SteveL-MSFT Are you the appropriate person to review this? If not, can you please assign someone from the appropriate reviewer set? |
|
@mirichmo I'll review this |
| #Validate that parameter sets are functioning correctly | ||
| $command = "Invoke-RestMethod -Uri 'http://sandbox.lee.io/method.php' -Method GET -CustomMethod 'TEST'" | ||
| $result = ExecuteWebCommand -command $command | ||
| $result.Error | Should Not BeNullOrEmpty |
There was a problem hiding this comment.
You should use the ShouldBeErrorId pattern, see https://github.com/PowerShell/PowerShell/blob/master/test/powershell/engine/Module/TestModuleManifest.ps1#L50
There was a problem hiding this comment.
refactored test to utilise ShouldBeErrorId
| $result = ExecuteWebCommand -command $command | ||
| $result.Error | Should BeNullOrEmpty | ||
| ($result.Output | ConvertFrom-Json).method | Should Be "TEST" | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
you added code explicitly for when CustomMethod is get or post. You should add tests for that.
There was a problem hiding this comment.
tests now added for this
|
|
||
| It "Validate Invoke-WebRequest body is converted to query params for CustomMethod GET" { | ||
|
|
||
| $command = "Invoke-WebRequest -Uri 'http://httpbin.org/get' -Method GET -Body @{'testparam'='testvalue'}" |
There was a problem hiding this comment.
Shouldn't this be -custommethod?
There was a problem hiding this comment.
Completely missed that! Fixed
|
|
||
| $command = "Invoke-RestMethod -Uri 'http://httpbin.org/post' -CustomMethod POST -Body 'testparam=testvalue'" | ||
| $result = ExecuteWebCommand -command $command | ||
| $result.Output.headers.'Content-Type' | Should Be "application/x-www-form-urlencoded" |
There was a problem hiding this comment.
since you're sending a specific body, seems like you should just check it was returned correctly in the response:
$result.Output.form.testvalue | Should Be "testvalue"
There was a problem hiding this comment.
I've now refactored the tests for this
|
|
||
| $command = "Invoke-WebRequest -Uri 'http://httpbin.org/post' -CustomMethod POST -Body 'testparam=testvalue'" | ||
| $result = ExecuteWebCommand -command $command | ||
| ($result.Output.Content | ConvertFrom-Json).headers.'Content-Type' | Should Be "application/x-www-form-urlencoded" |
There was a problem hiding this comment.
since you're sending a specific body, seems like you should just check it was returned correctly in the response:
$result.Output.form.testvalue | Should Be "testvalue"
|
Thanks @lee303! |
…dard method verbs
Ref #3113
This PR adds the following to the Invoke-WebRequest and Invoke-RestMethod cmdlets:
StandardMethodParameterSet to the pre-existing Method parameterCustomMethodwith ParameterSetCustomMethodStandardMethodParameterSetThe underlying webrequest generation will use either
StandardMethodorCustomMethod, depending which set is used.CustomMethodsupports the pre-existing verbs defined in theWebRequestMethodenum