HTTP Multipart/FormData support #3844

RX14 · 2017-01-05T21:49:04Z

Lets try again!

asterite · 2017-01-06T12:45:55Z

src/http/common.cr

@@ -223,6 +223,24 @@ module HTTP
    nil
  end

+  # Dequotes a RFC2616 quoted-string


Maybe show example code?

asterite · 2017-01-06T12:47:02Z

src/http/multipart/generator.cr

+
+    # Finite State Machine diagram: https://gist.github.com/RX14/221c1edfa98d1196711515d4b5c264eb
+
+    # Generates a content type header with multipart subtype *subtype*, and


Maybe "Returns"? I initially thought this was appended to the generated content (because it says "generates" and this is a Generator class)

asterite · 2017-01-06T12:47:52Z

src/http/multipart/generator.cr

+    end
+
+    # Yields an IO that can be used to write to a body part which is appended
+    # to the multipart message with the given *headers*. Throws is `#finish` or


"Throws if"

asterite · 2017-01-06T12:49:30Z

spec/std/http/multipart/parser_spec.cr

+require "spec"
+require "http"
+
+def mp_parse(delim, data)


private def

This was written before those existed!

asterite · 2017-01-06T12:55:03Z

src/http/multipart/parser.cr

+        begin
+          yield headers, body_io
+        ensure
+          # Read to end of body


I think here you can use IO#skip_to_end method that I added some days ago

(I'm happy to see more cases where this method is useful ^_^)

I think i'll send a PR which increases the buffer size of skip_to_end, gets_to_end so they trigger IO::Buffered to skip the buffer (>=4096).

Good idea. I can commit that directly. What buffer size would be good?

I added this as a commit to this PR, you can cherry-pick it to master if you want.

asterite · 2017-01-06T12:58:59Z

src/http/formdata.cr

+  # form_data = "--aA40\r\nContent-Disposition: form-data; name=\"field1\"\r\n\r\nfield data\r\n--aA40--"
+  # HTTP::FormData.parse(IO::Memory.new(form_data), "aA40") do |field, io|
+  #   field # => "field1"
+  #   data  # => "field data"


io.gets_to_end # => "field data"

asterite · 2017-01-06T12:59:16Z

src/http/formdata.cr

+  #
+  # HTTP::FormData.parse(request) do |field, io|
+  #   field # => "field1"
+  #   data  # => "field data"


io.gets_to_end # => "field data"

It's also missing how to access the meta and headers variables and what they are. Same above.

asterite · 2017-01-06T13:01:18Z

@RX14 Overall looks really good, I really like it! I don't know why I rejected it some time ago... sorry about that 😊

I made a few comments for fixes. After that I'll merge it, so we can have it for 0.20.4. Thank you for this! 🎉

ysbaddaden

Seems to look good overall. It's quite a large pull request, so I didn't delve too deep, yet noted a few things.

I would rename "generate" to "build", which would fit better in the stdlib, overall (e.g. String.build, XML.build, ...).

The public API looks good. We can quickly iterate the fields, values and have direct IO access. Though making the IO access required is maybe too much. Yielding many variables, too. Maybe a single Data struct, with #headers, #name, #value (aka io.gets_to_end or something more efficient), #io, and moving methods from Metadata directly to this struct would be nicer to work with.

Last but no least, everything is documented. Yet, I'm missing some high level examples for everyday use cases:

how do I build/send a multipart body with HTTP::Client (string, file, ...);
how do I parse the multipart body of a HTTP::Request (string, file, ...).

ysbaddaden · 2017-01-06T13:23:19Z

src/http/multipart.cr

+  end
+
+  # Parses the multipart boundary from the value of the Content-Type header,
+  # or nil if the boundary was not found.


What about?

Extracts the multipart boundary from the Content-Type header.
May return nil if the boundary was not found.

ysbaddaden · 2017-01-06T13:32:44Z

src/http/multipart/parser.cr

+  #
+  # Please note that the IO object yielded by `#next` is only valid until the
+  # block returns.
+  class Parser


What about a Parser#each iterator?

# Yields each part until the whole multipart message is consumed. See `#next` for details. def each while has_next? self.next do |headers, io, meta, headers| yield headers, io, meta, headers end end end

That usecase is meant to be HTTP::Multipart.parse.

But it's missing headers and meta?

ysbaddaden · 2017-01-06T13:38:52Z

src/http/multipart/generator.cr

+    end
+
+    # Finalizes the multipart message, this method must be called before the
+    # generated multipart message written to the IO is considered valid.


Maybe?

this method must be called to properly end the multipart message.

ysbaddaden · 2017-01-06T13:44:17Z

src/http/formdata.cr

+  #
+  # HTTP::FormData.parse(request) do |field, io|
+  #   field # => "field1"
+  #   data  # => "field data"


It's also missing how to access the meta and headers variables and what they are. Same above.

ysbaddaden · 2017-01-06T13:54:05Z

src/http/formdata.cr

+  # io.to_s                          # => "HTTP/1.1 200 OK\r\nContent-Type: multipart/form-data; boundary=\"boundary\"\r\n ...
+  # ```
+  #
+  # See: `FormData::Generator`


An example explaining how to create/send a multipart request with HTTP::Client would be more interesting (common) than a multipart response (quite uncommon).

The example could use generator.boundary (automatically generated) which is a better practice than the fixed "boundary" string. The example may then use whatever boundary that would have been generated.

ysbaddaden · 2017-01-06T14:11:34Z

src/http/multipart.cr

+  def self.parse(io, boundary)
+    parser = Parser.new(io, boundary)
+    while parser.has_next?
+      parser.next { |headers, io| yield headers, io }


Isn't it missing meta and headers? If not, how are we supposed to access them? Same below.

This is plain multipart, not multipart/form-data. Field names and file metadata are specific to the multipart/form-data mime-type.

ysbaddaden · 2017-01-06T14:18:18Z

src/http/multipart.cr

+
+  # Yields a `Multipart::Generator` to the given block, returning the generated
+  # message as a `String`.
+  def self.generate(boundary = "--------------------------#{SecureRandom.urlsafe_base64(18)}")


What about extracting the default boundary generation to a helper method, to avoid repeating it over and over?

asterite · 2017-01-06T14:49:25Z

I agree with all of @ysbaddaden comments. Let's postpone this PR for after 0.20.4 because I guess some changes are needed yet and I wanted to prepare a release today.

RX14 · 2017-01-06T15:25:14Z

I've found some more HTTP quoted-string craziness (ref, ref), so i'll take the time to sort that out properly.

RX14 · 2017-01-07T00:34:29Z

Updated the PR with my work today, github is fairly accurate on which points I have addressed and which I haven't. I'll split commits from the PR out into separate PRs on request.

RX14 · 2017-01-07T11:18:02Z

There seems to be some confusion between multipart/* and multipart/form-data. RFC1341 defines that every media type using the multipart type must have a specific format. This format consists of a sequence of body-parts, each of which consists of any number of headers (including 0) and a body. It does not define any required headers.

The multipart/form-data subtype is an extension of the multipart media type which defines that each body part must contain a Content-Disposition header, which contains the name of the form field and optional metadata which is used when the field value is a file.

For that reason, HTTP::Multipart only yields headers and an IO, whereas HTTP::FormData yields extra data parsed from the Content-Disposition header. I'm probably going to create a HTTP::FormData::Part struct which contains the data from each part.

ysbaddaden · 2017-01-07T14:49:25Z

Yes, I got confused. Thanks for the explanation.

Could you detail this in the docs for the different classes? Mostly insist on what they each apply to, and maybe note in HTTP::Multipart that we may be looking for HTTP::FormData instead?

In that light, I don't think the module helpers in HTTP::Multipart are needed. Having a few helpers in HTTP::FormData shall be enough. Other formats will use the HTTP::Multipart Parser and Builder classes directly.

Sija · 2017-01-07T23:02:27Z

src/http/formdata/parser.cr

+
+  struct Part
+    getter name : String
+    getter 


looks like there's some code missing in here, I guess it's WIP :)

RX14 · 2017-02-02T00:27:29Z

Sorry for the super-extended delay replying to these issue comments. I've done quite a bit of work on the PR, and it's ready to be reviewed again.

Sija · 2017-02-02T09:16:52Z

spec/std/http/http_spec.cr

@@ -7,6 +7,11 @@ describe HTTP do
    HTTP.parse_time("Sun, 06 Nov 1994 08:49:37 GMT").should eq(time)
  end

+  it "parses RFC1123 without day name" do


space between RFC and its number would do nice ;)

RX14 · 2017-02-06T16:53:44Z

I've rebased onto master so the CI should be fixed on OSX now. @ysbaddaden, another review would be appreciated :)

sdogruyol · 2017-02-13T10:55:38Z

It'd be great to have this at next Crystal release please 🙏 👍

ysbaddaden · 2017-02-13T13:50:42Z

@RX14 sorry, I've been quite busy lately, and haven't found the time to review this, yet.

asterite · 2017-02-13T20:14:11Z

src/http/common.cr

+  # ```
+  def self.quote_string(string, io)
+    string.each_byte do |byte|
+      case QUOTE_LOOKUP_TABLE[byte]


We think it's better here to just do case on the byte, without a lookup table. The code will be clearer and it won't be unsafe (because it uses Pointer).

Will do. Do we know if LLVM will transform case statements to lookup tables?

Am I going crazy or is this a compiler bug: https://gist.github.com/RX14/e2dd192c7cb0e7cfb2278c2c46f3bea3 ?

The when takes the value of the comparison, so its comparing against a bool

So it's like you are doing if byte == (byte <= 0x1F)

@asterite I would have sworn that i've seen case used like that before. I guess I shouldn't code at 1am.

You can definitely do that if you omit the case expression. Then it becomes like if elsif but with all the conditions aligned.

@asterite committed the new version using case, and it's even 4% faster!

asterite · 2017-02-13T20:14:59Z

src/http/formdata/generator.cr

+  # builder.finish
+  # io.to_s # => "--aA47\r\nContent-Disposition: form-data; name=\"name\"\r\n\r\njoe\r\n--aA47\r\nContent-Disposition: form-data; name=\"upload\"; filename=\"test.txt\"\r\n\r\nfile contents\r\n--aA47--"
+  # ```
+  class Builder


Could you rename the file (and the specs) to be named "builder" instead of "generator", now that the types are also named "Builder"?

Oh, sorry I thought I'd renamed them all! Thanks for the catch.

@asterite Renamed them

RX14 · 2017-02-14T11:52:20Z

@ysbaddaden next time please can you leave a PR review instead of a commit comment, the commit comments get a bit lost when I force-push.

RX14 · 2017-02-14T12:10:56Z

Interesting: this build failed due to lack of memory https://travis-ci.org/crystal-lang/crystal/jobs/201479637

RX14 · 2017-02-14T14:56:39Z

Finally! 🎉 🎉 🎉

asterite reviewed Jan 6, 2017

View reviewed changes

ysbaddaden reviewed Jan 6, 2017

View reviewed changes

RX14 force-pushed the feature/multipart2 branch 2 times, most recently from dde4072 to 6a3abb1 Compare January 7, 2017 00:32

Sija reviewed Jan 7, 2017

View reviewed changes

RX14 force-pushed the feature/multipart2 branch from 6a3abb1 to 1981831 Compare February 2, 2017 00:26

Sija reviewed Feb 2, 2017

View reviewed changes

mverzilli added kind:feature pr:needs-review topic:stdlib labels Feb 2, 2017

RX14 force-pushed the feature/multipart2 branch from 1981831 to e3ffafb Compare February 6, 2017 16:51

asterite reviewed Feb 13, 2017

View reviewed changes

RX14 added 2 commits February 14, 2017 11:38

Add helpers for HTTP quoted-string values

9faa142

Allow parsing of RFC1123 times without day names

7f6139b

RX14 force-pushed the feature/multipart2 branch from e3ffafb to faec93a Compare February 14, 2017 11:39

RX14 added 3 commits February 14, 2017 11:50

Add HTTP multipart support

fef5ad2

Add HTTP formdata support

a9bf7b7

Increase IO buffer sizes to reduce memory copies

4fa73fb

RX14 force-pushed the feature/multipart2 branch from faec93a to 4fa73fb Compare February 14, 2017 11:50

asterite added this to the 0.21.0 milestone Feb 14, 2017

asterite merged commit f0770cd into crystal-lang:master Feb 14, 2017

spalladino removed the pr:needs-review label Feb 17, 2017


		# Finite State Machine diagram: https://gist.github.com/RX14/221c1edfa98d1196711515d4b5c264eb

		# Generates a content type header with multipart subtype subtype, and

HTTP Multipart/FormData support #3844

HTTP Multipart/FormData support #3844

Conversation

RX14 commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Jan 6, 2017

ysbaddaden left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Jan 6, 2017

RX14 commented Jan 6, 2017

RX14 commented Jan 7, 2017

RX14 commented Jan 7, 2017

ysbaddaden commented Jan 7, 2017

Choose a reason for hiding this comment

RX14 commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Feb 6, 2017

sdogruyol commented Feb 13, 2017

ysbaddaden commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Feb 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Feb 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Feb 14, 2017

RX14 commented Feb 14, 2017

RX14 commented Feb 14, 2017

ysbaddaden left a comment •

edited

RX14 Feb 14, 2017 •

edited

RX14 Feb 14, 2017 •

edited