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

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

Merged
merged 6 commits into from Mar 6, 2017
Merged

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

merged 6 commits into from Mar 6, 2017

Conversation

0x4c6565
Copy link
Contributor

@0x4c6565 0x4c6565 commented Feb 13, 2017 •
edited

…dard method verbs

Ref #3113

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

@0x4c6565
Copy link
Contributor Author

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
Copy link
Contributor

thezim commented Feb 13, 2017

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

thezim
thezim reviewed Feb 13, 2017
View reviewed changes
...erShell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs
/// <summary>
/// gets or sets the parameter CustomMethod
/// </summary>
[Parameter(ParameterSetName = "CustomMethod")]
Copy link
Contributor

@thezim thezim Feb 13, 2017 •
edited

Choose a reason for hiding this comment

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

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
thezim reviewed Feb 13, 2017
View reviewed changes
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Outdated
@@ -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"))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be consistent and use ToUpperInvariant().

@thezim
Copy link
Contributor

thezim commented Feb 13, 2017

Core CLR needs the love too.

WebRequestPSCmdlet.CoreClr.cs

@0x4c6565
Copy link
Contributor Author

Completely overlooked core! Will sort, thank you :)

thezim
thezim reviewed Feb 13, 2017
View reviewed changes
...PowerShell.Commands.Utility/commands/utility/WebCmdlet/FullClr/WebRequestPSCmdlet.FullClr.cs
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Feb 14, 2017
@0x4c6565
Copy link
Contributor Author

Last commits add Core implementation and include suggestions :)

@0x4c6565
Copy link
Contributor Author

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

@mirichmo
Copy link
Member

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

@0x4c6565
Copy link
Contributor Author

All signed :) @mirichmo

@joeyaiello
Copy link
Contributor

Closing and reopening PR to trigger CLA bot

@joeyaiello joeyaiello closed this Feb 17, 2017
@joeyaiello joeyaiello reopened this Feb 17, 2017
@0x4c6565
Copy link
Contributor Author

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
Copy link
Contributor

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

@0x4c6565 0x4c6565 closed this Mar 2, 2017
@0x4c6565 0x4c6565 reopened this Mar 2, 2017
@mirichmo
Copy link
Member

mirichmo commented Mar 2, 2017

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

@SteveL-MSFT SteveL-MSFT self-requested a review March 2, 2017 16:58
@SteveL-MSFT
Copy link
Member

@mirichmo I'll review this

SteveL-MSFT
SteveL-MSFT requested changes Mar 3, 2017
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
#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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored test to utilise ShouldBeErrorId

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
$result = ExecuteWebCommand -command $command
$result.Error | Should BeNullOrEmpty
($result.Output | ConvertFrom-Json).method | Should Be "TEST"
}
}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests now added for this

SteveL-MSFT
SteveL-MSFT requested changes Mar 3, 2017
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated

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'}"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be -custommethod?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely missed that! Fixed

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated

$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"
Copy link
Member

Choose a reason for hiding this comment

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

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"

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've now refactored the tests for this

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated

$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"
Copy link
Member

Choose a reason for hiding this comment

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

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
SteveL-MSFT approved these changes Mar 4, 2017
View reviewed changes
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@mirichmo mirichmo merged commit b043b44 into PowerShell:master Mar 6, 2017
@mirichmo
Copy link
Member

mirichmo commented Mar 6, 2017

Thanks @lee303!

@0x4c6565 0x4c6565 deleted the 3113-allow-nonstandard-verbs branch March 6, 2017 23:05
@iSazonov iSazonov removed the Review - Needed The PR is being reviewed label Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thezim thezim thezim left review comments

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

Assignees

@mirichmo mirichmo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@0x4c6565 @thezim @mirichmo @joeyaiello @SteveL-MSFT @PowerShellTeam @msftclas @iSazonov