adds parameter sets to web cmdlets to allow for standard and non-stan… by 0x4c6565 · Pull Request #3142 · PowerShell/PowerShell

0x4c6565 · 2017-02-13T22:57:32Z

…dard method verbs

This PR adds the following to the Invoke-WebRequest and Invoke-RestMethod cmdlets:

Adds new StandardMethod ParameterSet to the pre-existing Method parameter
Adds new parameter CustomMethod with ParameterSet CustomMethod
Defaults to StandardMethod ParameterSet

The underlying webrequest generation will use either StandardMethod or CustomMethod, depending which set is used. CustomMethod supports the pre-existing verbs defined in the WebRequestMethod enum

…dard method verbs

0x4c6565 · 2017-02-13T23:17:27Z

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

PS C:\Users\Lee\Documents\PowerShell> iwr -uri sandbox.lee.io/method.php


StatusCode        : 200
StatusDescription : OK
Content           : {"method":"GET","body":""}
RawContent        : HTTP/1.1 200 OK
                    Connection: close
                    Content-Length: 26
                    Content-Type: text/html; charset=UTF-8
                    Date: Mon, 13 Feb 2017 23:14:47 GMT
                    Server: Apache/2.2.15 (CentOS)
                    X-Powered-By: PHP/5.6.30

                    {"metho...
Forms             : {}
Headers           : {[Connection, close], [Content-Length, 26], [Content-Type, text/html; charset=UTF-8], [Date, Mon,
                    13 Feb 2017 23:14:47 GMT]...}
Images            : {}
InputFields       : {}
Links             : {}
ParsedHtml        : mshtml.HTMLDocumentClass
RawContentLength  : 26



PS C:\Users\Lee\Documents\PowerShell> irm -uri sandbox.lee.io/method.php

method body
------ ----
GET

thezim · 2017-02-13T23:22:08Z

I was working on a PR for this too. I'll drop a few comments on what I came across.

thezim · 2017-02-13T23:23:57Z

+        /// <summary>
+        /// gets or sets the parameter CustomMethod
+        /// </summary>
+        [Parameter(ParameterSetName = "CustomMethod")]


You can make this validate on input and removed the checks for IsNullOrEmpty. Also adding an alias now would be nice.

[ValidateNotNullOrEmpty] [Alias("CM")]

thezim · 2017-02-13T23:26:00Z

            if ((null != bodyAsDictionary)
-                && (Method == WebRequestMethod.Default || Method == WebRequestMethod.Get))
+                && ((IsStandardMethodSet() && (Method == WebRequestMethod.Default || Method == WebRequestMethod.Get))
+                     || (IsCustomMethodSet() && (string.IsNullOrEmpty(CustomMethod) || CustomMethod.ToUpper() == "GET"))))


Should be consistent and use ToUpperInvariant().

thezim · 2017-02-13T23:30:52Z

Core CLR needs the love too.

WebRequestPSCmdlet.CoreClr.cs

0x4c6565 · 2017-02-13T23:41:32Z

Completely overlooked core! Will sort, thank you :)

thezim · 2017-02-13T23:45:12Z


-            // set the method if the parameter was provided
-            if (WebRequestMethod.Default != Method)
+            switch (ParameterSetName)


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(); } }

0x4c6565 · 2017-02-16T23:50:09Z

Last commits add Core implementation and include suggestions :)

0x4c6565 · 2017-02-17T00:03:16Z

Linux CI build errored, will need rerunning as it didn't actually fail

mirichmo · 2017-02-17T00:14:14Z

@lee303 Please sign the CLA. I can't do anything with this PR until it is signed.

0x4c6565 · 2017-02-17T00:44:47Z

All signed :) @mirichmo

joeyaiello · 2017-02-17T01:34:54Z

Closing and reopening PR to trigger CLA bot

0x4c6565 · 2017-02-22T19:23:15Z

just noticed that the Apple build failed this time - failed to download dotnet from windows.net, hopefully someone with the powers can re-run

joeyaiello · 2017-03-01T19:53:04Z

@lee303 FYI, you can always close/reopen the PR to restart the CI

mirichmo · 2017-03-02T01:16:50Z

@SteveL-MSFT Are you the appropriate person to review this? If not, can you please assign someone from the appropriate reviewer set?

SteveL-MSFT · 2017-03-02T16:58:50Z

@mirichmo I'll review this

SteveL-MSFT · 2017-03-03T18:49:28Z

+        #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


You should use the ShouldBeErrorId pattern, see https://github.com/PowerShell/PowerShell/blob/master/test/powershell/engine/Module/TestModuleManifest.ps1#L50

refactored test to utilise ShouldBeErrorId

SteveL-MSFT · 2017-03-03T18:49:57Z

+        ($result.Output | ConvertFrom-Json).method | Should Be "TEST"
+    }
 }



you added code explicitly for when CustomMethod is get or post. You should add tests for that.

tests now added for this

SteveL-MSFT · 2017-03-03T23:53:22Z

+
+    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'}"


Shouldn't this be -custommethod?

Completely missed that! Fixed

SteveL-MSFT · 2017-03-03T23:56:34Z

+
+        $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"


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"

I've now refactored the tests for this

SteveL-MSFT · 2017-03-03T23:57:24Z

+
+        $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"


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"

SteveL-MSFT

LGTM. Thanks for the contribution!

mirichmo · 2017-03-06T19:42:27Z

Thanks @lee303!

adds parameter sets to web cmdlets to allow for standard and non-stan…

7c11001

…dard method verbs

msftclas added the cla-required label Feb 13, 2017

thezim reviewed Feb 13, 2017

View reviewed changes

PowerShellTeam added the Review - Needed The PR is being reviewed label Feb 14, 2017

lzybkr assigned mirichmo Feb 16, 2017

0x4c6565 added 2 commits February 16, 2017 23:06

add CoreCLI implementation

8f25fd9

Adds CM alias and notnullempty for CustomMethod parameter

ecf7624

joeyaiello closed this Feb 17, 2017

joeyaiello reopened this Feb 17, 2017

msftclas added the cla-signed label Feb 17, 2017

mirichmo removed the cla-required label Feb 18, 2017

0x4c6565 closed this Mar 2, 2017

0x4c6565 reopened this Mar 2, 2017

msftclas added the cla-already-signed label Mar 2, 2017

SteveL-MSFT self-requested a review March 2, 2017 16:58

Merge branch 'master' into 3113-allow-nonstandard-verbs

51fb2fb

SteveL-MSFT requested changes Mar 3, 2017

View reviewed changes

Add tests for Invoke-[WebRequest|RestMethod] CustomMethod parameter

5d6763b

SteveL-MSFT requested changes Mar 3, 2017

View reviewed changes

Fix webcmdlet tests - incorrect parameter name

f5db053

SteveL-MSFT approved these changes Mar 4, 2017

View reviewed changes

mirichmo merged commit b043b44 into PowerShell:master Mar 6, 2017

0x4c6565 deleted the 3113-allow-nonstandard-verbs branch March 6, 2017 23:05

iSazonov removed the Review - Needed The PR is being reviewed label Mar 27, 2017


		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'}"

Conversation

0x4c6565 commented Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x4c6565 commented Feb 13, 2017

Uh oh!

thezim commented Feb 13, 2017

Uh oh!

thezim Feb 13, 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!

thezim commented Feb 13, 2017

Uh oh!

0x4c6565 commented Feb 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x4c6565 commented Feb 16, 2017

Uh oh!

0x4c6565 commented Feb 17, 2017

Uh oh!

mirichmo commented Feb 17, 2017

Uh oh!

0x4c6565 commented Feb 17, 2017

Uh oh!

joeyaiello commented Feb 17, 2017

Uh oh!

0x4c6565 commented Feb 22, 2017

Uh oh!

joeyaiello commented Mar 1, 2017

Uh oh!

mirichmo commented Mar 2, 2017

Uh oh!

SteveL-MSFT commented Mar 2, 2017

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!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

mirichmo commented Mar 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

0x4c6565 commented Feb 13, 2017 •

edited

Loading

thezim Feb 13, 2017 •

edited

Loading