Skip to content

Add Simplified multipart/form-data Support to Web Cmdlets Through -Form Parameter #5972

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

Merged
iSazonov merged 11 commits into from
Jan 25, 2018
Merged

Add Simplified multipart/form-data Support to Web Cmdlets Through -Form Parameter #5972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d383458
[Feature] Add Simplified multipart/form-data Support to Web Cmdlets
markekraus Jan 20, 2018
2abd895
[feature] Add Tests and Address PR Feedback
markekraus Jan 22, 2018
cb249f0
[feature] Move private below internal
markekraus Jan 22, 2018
3939df9
[feature] Cleanup formatting
markekraus Jan 22, 2018
c45e693
[feature] Address PR Feedback
markekraus Jan 22, 2018
3bcf3af
[Feature] add missing -force
markekraus Jan 22, 2018
a64dfeb
[feature] Revert Content-Type to Match
markekraus Jan 22, 2018
aa187ff
[Feature] Rerun CI
markekraus Jan 23, 2018
b4ac5c4
[Feature] remove errant inc file
markekraus Jan 23, 2018
ca81f0a
Address PR Feedback
markekraus Jan 23, 2018
9e78323
[feature] Address PR Feedback
markekraus Jan 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 136 additions & 1 deletion ...owerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,14 @@ public virtual string CustomMethod
[Parameter(ValueFromPipeline = true)]
public virtual object Body { get; set; }

/// <summary>
/// Dictionary for use with RFC-7578 multipart/form-data submissions.
/// Keys are form fields and their respective values are form values.
/// A value may be a collection of form values or single form value.
/// </summary>
[Parameter]
public virtual IDictionary Form {get; set;}

/// <summary>
/// gets or sets the ContentType property
/// </summary>
Expand Down Expand Up @@ -430,6 +438,18 @@ internal virtual void ValidateParameters()
"WebCmdletBodyConflictException");
ThrowTerminatingError(error);
}
if ((null != Body) && (null != Form))
{
ErrorRecord error = GetValidationError(WebCmdletStrings.BodyFormConflict,
"WebCmdletBodyFormConflictException");
ThrowTerminatingError(error);
}
if ((null != InFile) && (null != Form))
{
ErrorRecord error = GetValidationError(WebCmdletStrings.FormInFileConflict,
"WebCmdletFormInFileConflictException");
ThrowTerminatingError(error);
}

// validate InFile path
if (InFile != null)
Expand Down Expand Up @@ -1075,8 +1095,21 @@ internal virtual void FillRequestStream(HttpRequestMessage request)
}
}

if (null != Form)
{
// Content headers will be set by MultipartFormDataContent which will throw unless we clear them first
WebSession.ContentHeaders.Clear();

var formData = new MultipartFormDataContent();
foreach (DictionaryEntry formEntry in Form)
{
// AddMultipartContent will handle PSObject unwrapping, Object type determination and enumerateing top level IEnumerables.
AddMultipartContent(fieldName: formEntry.Key, fieldValue: formEntry.Value, formData: formData, enumerate: true);
}
SetRequestContent(request, formData);
}
// coerce body into a usable form
if (Body != null)
else if (Body != null)
{
object content = Body;

Expand Down Expand Up @@ -1590,6 +1623,108 @@ internal void ParseLinkHeader(HttpResponseMessage response, System.Uri requestUr
}
}

/// <summary>
/// Adds content to a <see cref="MultipartFormDataContent" />. Object type detection is used to determine if the value is String, File, or Collection.
/// </summary>
/// <param name="fieldName">The Field Name to use.</param>
/// <param name="fieldValue">The Field Value to use.</param>
/// <param name="formData">The <see cref="MultipartFormDataContent" />> to update.</param>
/// <param name="enumerate">If true, collection types in <paramref name="fieldValue" /> will be enumerated. If false, collections will be treated as single value.</param>
private void AddMultipartContent(object fieldName, object fieldValue, MultipartFormDataContent formData, bool enumerate)
{
if (null == formData)
Copy link
Member

@SteveL-MSFT SteveL-MSFT Jan 24, 2018

Choose a reason for hiding this comment

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

Dbg.Assert() instead?

Copy link
Collaborator

@iSazonov iSazonov Jan 24, 2018

Choose a reason for hiding this comment

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

The pattern is common in the file.

Copy link
Collaborator

@rkeithhill rkeithhill Jan 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

FYI, this type of null check (null on the LHS) is a C++ idiom that is unnecessary in C# because if/while/do conditions in C# must evaluate to a Boolean. Therefore a mistaken assignment e.g. if (formData = null) would fail to compile because the type of formData can't be a Boolean (not if you're assigning null to it). In fact, the only time you need to put the literal on the LHS is when the variable type is a Boolean e.g. if (false == aBooleanVariable).

My team's internal coding guidelines suggest not using == or != in the Boolean variable case but use if (!aBooleanVariable) or if (aBooleanVariable) instead. Consequently, we never put the literal value on the LHS in C# code. And IMO this approach reads easier e.g. if (formData == null).

Copy link
Contributor Author

@markekraus markekraus Jan 24, 2018

Choose a reason for hiding this comment

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

@rkeithhill The problem is more that this file uses inconsistent forms and I forget which one is being used in which context. In some parts of the file null is left in others null is right. As long as there isn't some programmatic consideration (I suspected there wasn't), I don't have an opinion either way. null == foo and foo == null read exactly the same to me with out one being easier than the other. I have a habit of "literals go left" because it is a common pattern in many languages for various reasons.

Copy link
Contributor Author

@markekraus markekraus Jan 24, 2018

Choose a reason for hiding this comment

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

@SteveL-MSFT I'm not entire certain of the future of these new methods. I get the feeling they may end up moving to a helper class as static methods to be consumed in more places. throw seems more portable, IMO.

Copy link
Collaborator

@iSazonov iSazonov Jan 24, 2018

Choose a reason for hiding this comment

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

I like (formData == null) too.
But here we should use the file pattern.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Jan 24, 2018

Choose a reason for hiding this comment

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

@markekraus since the pattern already exists in this file, I'm fine keeping it for now. Consider this closed for me.

{
throw new ArgumentNullException("formDate");
}

// It is possible that the dictionary keys or values are PSObject wrapped depending on how the dictionary is defined and assigned.
// Before processing the field name and value we need to ensure we are working with the base objects and not the PSObject wrappers.

// Unwrap fieldName PSObjects
if (fieldName is PSObject namePSObject)
{
fieldName = namePSObject.BaseObject;
}

// Unwrap fieldValue PSObjects
if (fieldValue is PSObject valuePSObject)
{
fieldValue = valuePSObject.BaseObject;
}

// Treat a single FileInfo as a FileContent
if (fieldValue is FileInfo file)
{
formData.Add(GetMultipartFileContent(fieldName: fieldName, file: file));
return;
}

// Treat Strings and other single values as a StringContent.
// If enumeration is false, also treat IEnumerables as StringContents.
// String implements IEnumerable so the explicit check is required.
if (enumerate == false || fieldValue is String || !(fieldValue is IEnumerable))
{
formData.Add(GetMultipartStringContent(fieldName: fieldName, fieldValue: fieldValue));
return;
}

// Treat the value as a collection and enumerate it if enumeration is true
if (enumerate == true && fieldValue is IEnumerable items)
{
foreach (var item in items)
{
// Recruse, but do not enumerate the next level. IEnumerables will be treated as single values.
AddMultipartContent(fieldName: fieldName, fieldValue: item, formData: formData, enumerate: false);
}
}
}

/// <summary>
/// Gets a <see cref="StringContent" /> from the supplied field name and field value. Uses <see cref="ConvertTo<T>(Object)" /> to convert the objects to strings.
/// </summary>
/// <param name="fieldName">The Field Name to use for the <see cref="StringContent" />.</param>
/// <param name="fieldValue">The Field Value to use for the <see cref="StringContent" />.</param>
private StringContent GetMultipartStringContent(Object fieldName, Object fieldValue)
{
var contentDisposition = new ContentDispositionHeaderValue("form-data");
contentDisposition.Name = LanguagePrimitives.ConvertTo<String>(fieldName);

var result = new StringContent(LanguagePrimitives.ConvertTo<String>(fieldValue));
result.Headers.ContentDisposition = contentDisposition;

return result;
}

/// <summary>
/// Gets a <see cref="StreamContent" /> from the supplied field name and <see cref="Stream" />. Uses <see cref="ConvertTo<T>(Object)" /> to convert the fieldname to a string.
/// </summary>
/// <param name="fieldName">The Field Name to use for the <see cref="StreamContent" />.</param>
/// <param name="stream">The <see cref="Stream" /> to use for the <see cref="StreamContent" />.</param>
private StreamContent GetMultipartStreamContent(Object fieldName, Stream stream)
{
var contentDisposition = new ContentDispositionHeaderValue("form-data");
contentDisposition.Name = LanguagePrimitives.ConvertTo<String>(fieldName);

var result = new StreamContent(stream);
result.Headers.ContentDisposition = contentDisposition;
result.Headers.ContentType = new MediaTypeHeaderValue("application/octet-stream");

return result;
}

/// <summary>
/// Gets a <see cref="StreamContent" /> from the supplied field name and file. Calls <see cref="GetMultipartStreamContent(Object, Stream)" /> to create the <see cref="StreamContent" /> and then sets the file name.
/// </summary>
/// <param name="fieldName">The Field Name to use for the <see cref="StreamContent" />.</param>
/// <param name="file">The file to use for the <see cref="StreamContent" />.</param>
private StreamContent GetMultipartFileContent(Object fieldName, FileInfo file)
{
var result = GetMultipartStreamContent(fieldName: fieldName, stream: new FileStream(file.FullName, FileMode.Open));
result.Headers.ContentDisposition.FileName = file.Name;

return result;
}

#endregion Helper Methods
}
}
6 changes: 6 additions & 0 deletions src/Microsoft.PowerShell.Commands.Utility/resources/WebCmdletStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,12 @@
<data name="BodyConflict" xml:space="preserve">
<value>The cmdlet cannot run because the following conflicting parameters are specified: Body and InFile. Specify either Body or Infile, then retry. </value>
</data>
<data name="BodyFormConflict" xml:space="preserve">
<value>The cmdlet cannot run because the following conflicting parameters are specified: Body and Form. Specify either Body or Form, then retry. </value>
Copy link
Collaborator

@iSazonov iSazonov Jan 22, 2018

Choose a reason for hiding this comment

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

I see we use the file pattern but the messages can be shorter - "Conflicting parameters are specified: Body and Form."

Maybe fix this in the PR or new one?

Copy link
Contributor Author

@markekraus markekraus Jan 22, 2018

Choose a reason for hiding this comment

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

There is an existing issue #5140 can you comment there?

Copy link
Collaborator

@iSazonov iSazonov Jan 22, 2018

Choose a reason for hiding this comment

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

Done.

</data>
<data name="FormInFileConflict" xml:space="preserve">
<value>The cmdlet cannot run because the following conflicting parameters are specified: InFile and Form. Specify either InFile or Form, then retry. </value>
</data>
<data name="CredentialConflict" xml:space="preserve">
<value>The cmdlet cannot run because the following conflicting parameters are specified: Credential and UseDefaultCredentials. Specify either Credential or UseDefaultCredentials, then retry.</value>
</data>
Expand Down
Loading