Add Multipart Support to Web Cmdlets #4782

markekraus · 2017-09-08T11:00:15Z

Summary

Partially implements #2112

Adds System.Net.Http.MultipartFormDataContent as a possible type for -Body
Adds /Multipart/ test to WebListener

This allows for the user to create their own Http.MultipartFormDataContent object and submit it. Since multipart/form-data submissions are highly flexible, adding direct support for it to the CmdLets may over-complicate the command parameters and a limited implementation would not address the broad scope of use cases. This at least allows the user to submit multipart forms using the Web Cmdlets and not have to manage their own HttpClient. Once this is introduced, limited multipart implementations can be expanded to use the code in this PR.

Documentation Needed

Note that documentation will probably need to make clear that -ContentType will be ignored when a MultipartFormDataContent object is supplied to -Body. Similarly, content headers specified by -Headers and -WebSession will be ignored too.

ContentType must be set by the MultipartFormDataContent object as it controls the boundary in the Content-Type header used to distinguish between the various fields supplied. It doesn't make sense to use any other Content-Type header when doing multipart/form-data anyway. If this isn't done here, the Foreach on line 421 throws. when Content is set with a MultipartFormDataContent it overwrites all the content headers, so trying to add content headers again causes an exception about ContentType already exists.

msftclas · 2017-09-08T11:00:20Z

@markekraus,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

CI Rerun count: I

iSazonov · 2017-09-08T13:51:23Z

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs

+                else if (content is MultipartFormDataContent)
+                {
+                    WebSession.ContentHeaders.Clear();
+                    MultipartFormDataContent multipartFormDataContent = content as MultipartFormDataContent;


We should exclude double casting. Here it is better use C# 7.0 pattern:

else if (content is MultipartFormDataContent multipartFormDataContent )

Awesome. I will do that.

iSazonov · 2017-09-08T13:53:47Z

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs

+            if (request == null)
+                throw new ArgumentNullException("request");
+            if (multipartContent == null)
+                throw new ArgumentNullException("multipartContent");


We should use curly braces:

if ( ... ) { ... }

or single line operator.

I can do this, but it will not match the surrounding code.

I believe we should use approved patterns for new code.
You can wait a view of other maintainers.

I agreed with @iSazonov. The new code is somewhat isolated as it's in its own method, so I think it's fine to use the more preferred style.

@daxian-dbw Awesome! format is already corrected in the most recent commit for this PR.

iSazonov · 2017-09-08T13:55:40Z

test/tools/WebListener/Controllers/MultipartController.cs

+        }
+        public ActionResult Index()
+        {
+


Please remove extra line.

iSazonov · 2017-09-08T16:42:13Z

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

    }

+    Context "Multipart/form-data Tests" {
+        It "Verifies Invoke-WebRequest Supports Multipart String Values" {


We use the same Context header 😕
I believe we should add cmdlet name Context "Invoke-WebRequest Multipart/form-data Tests"

Yes, because they are under different describe blocks. We did the same thing with HTTPS

I think it's somewhat confusing, but we can leave it for Context.

For clarity, would you like me to change it or leave it as is?

Leave it as is.

iSazonov · 2017-09-08T16:42:46Z

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

    }

+    Context "Multipart/form-data Tests" {
+        It "Verifies Invoke-RestMethod Supports Multipart String Values" {


Context "Invoke-RestMethod Multipart/form-data Tests"

daxian-dbw

@markekraus Thank you for the good work! Could you please take a look at the 2 comments I left?

daxian-dbw · 2017-09-11T22:10:37Z

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs

                }
+                else if (content is MultipartFormDataContent multipartFormDataContent)
+                {
+                    WebSession.ContentHeaders.Clear();


WebSession.ContentHeader is set at line 325 when ContentType is specified. Is it OK to ignore that as well?

Yes, ContentType must be set by the MultipartFormDataContent object as it controls the boundary in the Content-Type header used to distinguish between the various fields supplied. Documentation will probably need to make clear that -ContentType will be ignored when a MultipartFormDataContent object is supplied. It doesn't make sense to use any other Content-Type header when doing multipart/form-data anyway. If this isn't done here, the Foreach on line 421 throws. when Content is set with a MultipartFormDataContent it overwrites all the content headers, so trying to add content headers again causes an exception about ContentType already existing.

Thanks for the clarification. I updated the PR description to capture your words.

~~Then there is a potential problem at line 239~~
Nevermind, GetRequest happens before FillRequestStream, so we are good. Updated the PR description to mention -Header and -WebSession will be ignored as well.

daxian-dbw · 2017-09-11T22:13:07Z

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

+        $fileContent.Headers.ContentType = [System.Net.Http.Headers.MediaTypeHeaderValue]::Parse("text/plain")
+        $multipartContent.Add($fileContent)
+    }
+    # unary comma required


nit: can you please also mention why it's required -- $multipartContent will be unwrapped/enumerated otherwise.

daxian-dbw

LGTM

daxian-dbw · 2017-09-12T16:43:01Z

@markekraus Thanks again for the good work!

markekraus · 2017-09-12T16:55:47Z

@daxian-dbw For documentation, when is that handled? Should I submit a PR/Issue to PowerShell-Docs now or does it need to wait for the next beta release?

daxian-dbw · 2017-09-12T19:11:38Z

Yes, you are welcome to submit a PR to PowerShell-Docs now :)
Here is the guideline for maintainers about PRs that require doc changes (see CONTRIBUTING.md):

If your change warrants an update to user-facing documentation, a Maintainer will add the Documentation Needed label to your PR and add an issue to the PowerShell-Docs repository, so that we make sure to update the official documentation to reflect your contribution. As an example, this requirement includes any changes to cmdlets (including cmdlet parameters) and features which have associated about_* topics. While not required, we appreciate any contributors who add this label and create the issue themselves. Even better, all contributors are free to contribute the documentation themselves. (See Contributing to documentation related to PowerShell for more info.)

Please see https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#contributing-to-documentation for more details

Add Multipart Support to Web Cmdlets

403f1de

markekraus requested review from JamesWTruher, PaulHigin, TravisEz13, adityapatwardhan, dantraMSFT and daxian-dbw as code owners September 8, 2017 11:00

msftclas added the cla-already-signed label Sep 8, 2017

[Feature] Run Feature tests

0796005

CI Rerun count: I

markekraus force-pushed the MulitPart branch from 72a3636 to 0796005 Compare September 8, 2017 12:31

iSazonov reviewed Sep 8, 2017

View reviewed changes

[Feature] Address PR Feedback

895a01f

iSazonov reviewed Sep 8, 2017

View reviewed changes

daxian-dbw self-assigned this Sep 8, 2017

daxian-dbw reviewed Sep 11, 2017

View reviewed changes

More informative comment for GetMultipartBody

2cb449e

daxian-dbw added the Documentation Needed in this repo Documentation is needed in this repo label Sep 11, 2017

daxian-dbw approved these changes Sep 11, 2017

View reviewed changes

iSazonov approved these changes Sep 12, 2017

View reviewed changes

daxian-dbw merged commit fd3a003 into PowerShell:master Sep 12, 2017

daxian-dbw mentioned this pull request Sep 12, 2017

Multipart Support is added to Web Cmdlets and corresponding doc needs to be updated MicrosoftDocs/PowerShell-Docs#1650

Closed

7 tasks

This was referenced Sep 14, 2017

Document multipart/form-data support for 6.0 Web Cmdlets MicrosoftDocs/PowerShell-Docs#1656

Merged

Simplify usage of Multipart Uploads with WebRequestPSCmdlet #2112

Closed

markekraus deleted the MulitPart branch September 22, 2017 20:39

markekraus removed the Documentation Needed in this repo Documentation is needed in this repo label Oct 20, 2017

Add Multipart Support to Web Cmdlets #4782

Add Multipart Support to Web Cmdlets #4782

Uh oh!

Conversation

markekraus commented Sep 8, 2017 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Documentation Needed

Uh oh!

msftclas commented Sep 8, 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!

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!

daxian-dbw left a comment

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!

daxian-dbw Sep 11, 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!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Sep 12, 2017

Uh oh!

markekraus commented Sep 12, 2017

Uh oh!

daxian-dbw commented Sep 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

markekraus commented Sep 8, 2017 •

edited by daxian-dbw

Loading

daxian-dbw Sep 11, 2017 •

edited

Loading