Mime multipart request parsing. #146

Tratcher · 2014-11-05T01:13:56Z

This implementation includes several layers of support for reading multipart request bodies.

The lowest level is the MultipartReader that can parse any stream minimal buffering. It gives you the headers and body of each section one at a time and then you do what you want with the body of that section (buffer, discard, write to disk, etc.).
The next level up supports buffering the individual bodies to memory or disk and returning you an IEnumerable of all the sections.
The next level up exposes extension methods on HttpRequest to determine if this is a multipart request and to read it using the methods from level 2 above. This will also cache the results in a feature interface so they can be accessed again by later calls.
There is also extension method support for reading nested multipart entries.

Here is a functional sample:
aspnet/Entropy@4e083df

None of this work is prescriptive about how you want to interpret the content. If we want to interpret it as files then we would add one additional layer that would read the content-disposition header to get the file details. However, when we get strongly typed headers that information should be more readily available anyways.

Tratcher · 2014-11-05T19:08:46Z

@GrabYourPitchforks

kichalla · 2014-11-05T19:37:35Z

@Tratcher : Just FYI...in Web API, some users had concerns about upload files to a directory on the web server and wanted the ability to upload files directly to Amazon or Azure storage services...if its small files then one can read the multipart content in-memory and upload them to these external storage services, but obviously this is not a recommended approach for large files...so checking to see if we have/plan the extensibility points for achieving this scenario...

Tratcher · 2014-11-05T21:24:45Z

@kichalla That should be easy enough. See MultipartReader.ReadMultipartToFileAsync to see how to stream the data to the destination of your choice.

kichalla · 2014-11-05T23:41:54Z

👍 Thanks...

davidfowl · 2014-11-05T23:45:01Z

src/Microsoft.AspNet.WebUtilities/StreamHelperExtensions.cs

+            while (await stream.ReadAsync(buffer, 0, buffer.Length, cancellationToken) > 0)
+            {
+                // Not all streams support cancellation directly.
+                cancellationToken.ThrowIfCancellationRequested();


Why not check the passive way too?

if (cancellationToken.IsCancellationRequested)?
Then what? I can't return a cancelled task from an async method.

Ah yes true.

Eilon · 2014-11-06T00:31:57Z

@Tratcher this is one of the topics that we should discuss in next week's mtg to go over the HTTP abstractions. This is a pretty big feature area that we want to have a broad discussion on before committing any codez.

GrabYourPitchforks · 2014-11-12T19:10:43Z

src/Microsoft.AspNet.WebUtilities/ApmStreamWrapper.cs

+        {
+#if ASPNETCORE50
+            Task<int> t = (Task<int>)asyncResult;
+            t.Wait();


Lines 171 - 177 can be replaced by try { return t.GetAwaiter().GetResult(); } catch (Exception ex) { /* wrap and rethrow ex */ }. This has the added benefit of not exposing AggregateException up the stack.

That's even worse, actually 😄

Given that Wait always throws an AggregateException when the task doesn't run to completion, the if statement is either not reached - when an exception is thrown - or necessarily evaluates to true 💥

Tratcher · 2014-11-12T23:41:04Z

Updated.

Tratcher · 2014-12-04T18:44:48Z

Updated. Samples: aspnet/Entropy#32

Tratcher · 2014-12-04T18:45:10Z

@Eilon @muratg @davidfowl @lodejard

Tratcher · 2014-12-04T18:47:51Z

@ajaybhargavb

Tratcher · 2014-12-06T02:36:22Z

I need to move IFormCollection from Http to Extensions or WebUtilities.

Tratcher · 2014-12-06T02:38:42Z

src/Microsoft.AspNet.Http.Extensions/FileUploadReaderExtensions.cs

+        /// <param name="enableRewind">Indicates that the entire body should be buffered so it can be rewound afterwards.</param>
+        /// <param name="cancellationToken"></param>
+        /// <returns></returns>
+        public static async Task<IEnumerable<IFileUpload>> ReadFileUploadsAsync([NotNull] this HttpRequest request, bool enableRewind = true,


Should this be an IFileCollection : IDictionary<string, IFileUpload[]>, indexed by the CD name parameter?

Tratcher · 2014-12-08T17:50:27Z

Updated.

Eilon · 2014-12-08T18:10:58Z

src/Microsoft.AspNet.WebUtilities/BufferedReadStream.cs

+            var ignored = new ArraySegment<byte>(buffer, offset, count);
+            if (count == 0)
+            {
+                throw new ArgumentOutOfRangeException("count", "The value must be greater than zero.");


nameof (might want to check other instance in this PR)

Tratcher · 2014-12-08T19:02:30Z

Updated.

Eilon · 2014-12-08T19:43:18Z

The changes in response to my comments look good.

Tratcher · 2014-12-08T22:25:21Z

Also fixes: #84

davidfowl · 2014-12-08T22:27:43Z

src/Microsoft.AspNet.Http.Extensions/MultipartReaderExtensions.cs

+            var element = elements.Where(entry => entry.StartsWith("boundary=")).First();
+            var boundary = element.Substring("boundary=".Length);
+            // Remove quotes
+            if (boundary.Length >=2 && boundary[0] == '"' && boundary[boundary.Length - 1] == '"')


fomatting busted here

davidfowl · 2014-12-08T22:44:37Z

Tratcher · 2014-12-18T19:51:53Z

Redone, with updated samples: aspnet/Entropy#32

davidfowl · 2014-12-26T19:06:28Z

src/Microsoft.AspNet.WebUtilities/BufferedReadStream.cs

+
+namespace Microsoft.AspNet.WebUtilities
+{
+    public class BufferedReadStream : Stream


ajaybhargavb · 2015-01-05T23:26:12Z

The ReadFormAsync method doesn't support multiple files with same name. It always returns the last file.
Eg:

<input type="file" name="files" id="file1" />
<input type="file" name="files" id="file2" />

Here ReadFormAsync returns only the second file.

Tratcher · 2015-01-05T23:59:51Z

@ajaybhargavb Fixed.

rynowak · 2015-01-07T20:51:26Z

from me based on the public surface area

lodejard · 2015-01-07T21:23:56Z

src/Microsoft.AspNet.PipelineCore/FormFeature.cs


-            if (_bodyStream == null || _bodyStream != body)
+            // Read the form async and block, but don't block indefinitely, this could be a 2gb file upload.
+            using (var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)))


This would be a really difficult timeout to predict, reproduce, etc in production. How about adding an optional cancellation token argument to ReadForm() but have the default behavior assume a timeout for synchronous-request-body-reading is provided in a different way.

davidfowl reviewed Nov 5, 2014
View reviewed changes

GrabYourPitchforks reviewed Nov 12, 2014
View reviewed changes

Tratcher mentioned this pull request Dec 4, 2014

Multipart, form, and file upload samples aspnet/Entropy#32

Merged

Tratcher reviewed Dec 6, 2014
View reviewed changes

Eilon reviewed Dec 8, 2014
View reviewed changes

davidfowl reviewed Dec 8, 2014
View reviewed changes

davidfowl reviewed Dec 26, 2014
View reviewed changes

lodejard reviewed Jan 7, 2015
View reviewed changes

#139 - Mime multipart request parsing.

5872feb

Tratcher merged commit 5872feb into aspnet:dev Jan 7, 2015

Tratcher deleted the Multipart branch January 7, 2015 23:46

ajaybhargavb added a commit to aspnet/Mvc that referenced this pull request Jan 8, 2015

react to aspnet/HttpAbstractions#146 changes

52a984c

Mime multipart request parsing. #146

Mime multipart request parsing. #146

Uh oh!

Conversation

Tratcher commented Nov 5, 2014

Uh oh!

Tratcher commented Nov 5, 2014

Uh oh!

kichalla commented Nov 5, 2014

Uh oh!

Tratcher commented Nov 5, 2014

Uh oh!

kichalla commented Nov 5, 2014

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!

Eilon commented Nov 6, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Nov 12, 2014

Uh oh!

Tratcher commented Dec 4, 2014

Uh oh!

Tratcher commented Dec 4, 2014

Uh oh!

Tratcher commented Dec 4, 2014

Uh oh!

Tratcher commented Dec 6, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Dec 8, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Dec 8, 2014

Uh oh!

Eilon commented Dec 8, 2014

Uh oh!

Tratcher commented Dec 8, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Dec 8, 2014

Uh oh!

Tratcher commented Dec 18, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaybhargavb commented Jan 5, 2015

Uh oh!

Tratcher commented Jan 5, 2015

Uh oh!

rynowak commented Jan 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!