GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add Multipart Support to Web Cmdlets by markekraus · Pull Request #4782 · PowerShell/PowerShell

Conversation

Mark Kraus (markekraus) commented Sep 8, 2017 •

edited by daxian-dbw

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Add Multipart Support to Web Cmdlets

403f1de

Mark Kraus (markekraus) requested review from James Truher (JamesWTruher), Paul Higinbotham (PaulHigin), Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan), Dan Travison (dantraMSFT) and Dongbo Wang (daxian-dbw) as code owners

September 8, 2017 11:00

Microsoft Contribution License Agreements (msftclas) commented Sep 8, 2017

Copy link

Copy Markdown

Mark Kraus (@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label

Sep 8, 2017


          [Feature] Run Feature tests

CI Rerun count: I

Mark Kraus (markekraus) force-pushed the MulitPart branch from 72a3636 to 0796005 Compare

September 8, 2017 12:31

Ilya (iSazonov) reviewed

Sep 8, 2017

View reviewed changes

Comment thread

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

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

Ilya (iSazonov) Sep 8, 2017

Copy link

Copy Markdown

Collaborator

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 should exclude double casting. Here it is better use C# 7.0 pattern:

else if (content is MultipartFormDataContent multipartFormDataContent )

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mark Kraus (markekraus) Sep 8, 2017

Copy link

Copy Markdown

Contributor Author

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.

Awesome. I will do that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

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

Ilya (iSazonov) Sep 8, 2017

Copy link

Copy Markdown

Collaborator

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 should use curly braces:

if ( ... )
{
    ...
}

or single line operator.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mark Kraus (markekraus) Sep 8, 2017

Copy link

Copy Markdown

Contributor Author

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 can do this, but it will not match the surrounding code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Sep 8, 2017

Copy link

Copy Markdown

Collaborator

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 believe we should use approved patterns for new code.
You can wait a view of other maintainers.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Sep 11, 2017

Copy link

Copy Markdown

Member

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 agreed with Ilya (@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mark Kraus (markekraus) Sep 11, 2017

Copy link

Copy Markdown

Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/tools/WebListener/Controllers/MultipartController.cs Outdated

		}
		public ActionResult Index()
		{

Ilya (iSazonov) Sep 8, 2017

Copy link

Copy Markdown

Collaborator

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.

Please remove extra line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mark Kraus (markekraus) Sep 8, 2017

Copy link

Copy Markdown

Contributor Author

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.

will do

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          [Feature] Address PR Feedback

895a01f

Ilya (iSazonov) reviewed

Sep 8, 2017

View reviewed changes

Comment thread

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

		}

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

Comment thread

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

		}

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

Ilya (iSazonov) Sep 8, 2017

Copy link

Copy Markdown

Collaborator

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) self-assigned this

Sep 8, 2017

Dongbo Wang (daxian-dbw) reviewed

Sep 11, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) left a comment

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

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

Dongbo Wang (daxian-dbw) Sep 11, 2017

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mark Kraus (markekraus) Sep 11, 2017

Copy link

Copy Markdown

Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Sep 11, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

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

Dongbo Wang (daxian-dbw) Sep 11, 2017

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mark Kraus (markekraus) Sep 11, 2017

Copy link

Copy Markdown

Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          More informative comment for GetMultipartBody

2cb449e

Dongbo Wang (daxian-dbw) added the Documentation Needed in this repo Documentation is needed in this repo label

Sep 11, 2017

Dongbo Wang (daxian-dbw) approved these changes

Sep 11, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) approved these changes

Sep 12, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) merged commit fd3a003 into PowerShell:master

Sep 12, 2017

Dongbo Wang (daxian-dbw) commented Sep 12, 2017

Copy link

Copy Markdown

Member

Mark Kraus (@markekraus) Thanks again for the good work!

markekraus and ankar84 reacted with hooray emoji

All reactions

🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mark Kraus (markekraus) commented Sep 12, 2017

Copy link

Copy Markdown

Contributor Author

Dongbo Wang (@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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (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

2 of 7 tasks

Dongbo Wang (daxian-dbw) commented Sep 12, 2017

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Mark Kraus (markekraus) deleted the MulitPart branch

September 22, 2017 20:39

Mark Kraus (markekraus) removed the Documentation Needed in this repo Documentation is needed in this repo label

Oct 20, 2017

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Add Multipart Support to Web Cmdlets (PowerShell#4782)

3be5b76

Partially implements  PowerShell#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.

Reviewers

Dongbo Wang (daxian-dbw)

daxian-dbw approved these changes

Ilya (iSazonov)

iSazonov approved these changes

Aditya Patwardhan (adityapatwardhan)

Awaiting requested review from adityapatwardhan

Dan Travison (dantraMSFT)

Awaiting requested review from dantraMSFT

James Truher (JamesWTruher)

Awaiting requested review from JamesWTruher

Paul Higinbotham (PaulHigin)

Awaiting requested review from PaulHigin

Travis Plunk (TravisEz13)

Awaiting requested review from TravisEz13

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Mark Kraus (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!

Microsoft Contribution License Agreements (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!

Dongbo Wang (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!

Dongbo Wang (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!

Dongbo Wang (daxian-dbw) left a comment

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) commented Sep 12, 2017

Uh oh!

Mark Kraus (markekraus) commented Sep 12, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Sep 12, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mark Kraus (markekraus) commented Sep 8, 2017 •

edited by daxian-dbw

Loading

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) Sep 11, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.