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.

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

Add Multipart Support to Web Cmdlets #4782

Merged
merged 4 commits into from Sep 12, 2017

Conversation

@markekraus
Copy link
Collaborator

commented Sep 8, 2017

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

This comment has been minimized.

Copy link

commented Sep 8, 2017

@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

[Feature] Run Feature tests
CI Rerun count: I

@markekraus markekraus force-pushed the markekraus:MulitPart branch to 0796005 Sep 8, 2017

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 8, 2017

Collaborator

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

else if (content is MultipartFormDataContent multipartFormDataContent ) 

This comment has been minimized.

Copy link
@markekraus

markekraus Sep 8, 2017

Author Collaborator

Awesome. I will do that.

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 8, 2017

Collaborator

We should use curly braces:

if ( ... )
{
    ...
}

or single line operator.

This comment has been minimized.

Copy link
@markekraus

markekraus Sep 8, 2017

Author Collaborator

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 8, 2017

Collaborator

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

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Sep 11, 2017

Member

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.

This comment has been minimized.

Copy link
@markekraus

markekraus Sep 11, 2017

Author Collaborator

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

}
public ActionResult Index()
{

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 8, 2017

Collaborator

Please remove extra line.

This comment has been minimized.

Copy link
@markekraus

markekraus Sep 8, 2017

Author Collaborator

will do

@@ -1200,6 +1233,41 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {
}
}

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 8, 2017

Collaborator

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

This comment has been minimized.

Copy link
@markekraus

markekraus Sep 8, 2017

Author Collaborator

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 8, 2017

Collaborator

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

This comment has been minimized.

Copy link
@markekraus

markekraus Sep 8, 2017

Author Collaborator

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 8, 2017

Collaborator

Leave it as is.

@@ -1737,6 +1805,38 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {
}
}

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 8, 2017

Collaborator

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

@daxian-dbw daxian-dbw self-assigned this Sep 8, 2017

@daxian-dbw
Copy link
Member

left a comment

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

@@ -389,6 +389,11 @@ internal virtual void FillRequestStream(HttpRequestMessage request)
byte[] bytes = content as byte[];
SetRequestContent(request, bytes);
}
else if (content is MultipartFormDataContent multipartFormDataContent)
{
WebSession.ContentHeaders.Clear();

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Sep 11, 2017

Member

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

This comment has been minimized.

Copy link
@markekraus

markekraus Sep 11, 2017

Author Collaborator

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.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Sep 11, 2017

Member

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.

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

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Sep 11, 2017

Member

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

This comment has been minimized.

Copy link
@markekraus

markekraus Sep 11, 2017

Author Collaborator

fixed

@daxian-dbw
Copy link
Member

left a comment

LGTM

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@daxian-dbw

This comment has been minimized.

Copy link
Member

commented Sep 12, 2017

@markekraus Thanks again for the good work!

@markekraus

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 12, 2017

@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

This comment has been minimized.

Copy link
Member

commented Sep 12, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.