kenichi / angelo

Sinatra-like DSL for Reel that supports WebSockets and SSE
Other
303 stars 23 forks source link

multipart/form-data #47

Open Adam-Stomski opened 9 years ago

Adam-Stomski commented 9 years ago

Does angelo support file uploads? Im trying to send file from Rails apllication to angelo server with form like

<form method="POST" enctype="multipart/form-data" action="http://127.0.0.1:4567/images">
  <div>
    <input name="image" type="file" />
  </div>
  <div>
    <input type="submit" value="Submit" />
  </div>
</form>

server code:

class SampleServer < Angelo::Base
  post '/images' do
    puts request.inspect
    puts params
  end
end

and log:

I, [2015-02-13T11:18:10.885547 #4130]  INFO -- : 127.0.0.1 - - "POST /images HTTP/1.1" 200 5
#<Reel::Request POST /images HTTP/1.1 @headers={"Host"=>"127.0.0.1:4567", "Connection"=>"keep-alive", "Content-Length"=>"3437206", "Cache-Control"=>"max-age=0", "Accept"=>"text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8", "Origin"=>"http://localhost:3000", "User-Agent"=>"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.111 Safari/537.36", "Content-Type"=>"multipart/form-data; boundary=----WebKitFormBoundaryMMXfV2Ron4mihT2p", "Referer"=>"http://localhost:3000/", "Accept-Encoding"=>"gzip, deflate", "Accept-Language"=>"en-US,en;q=0.8"}>
{}
kenichi commented 9 years ago

@Adam-Stomski I looked through reel, sinatra, and rack code... Looks like rack handles this, merging a tempfile into the params in this case. I'm now confused as to whether this type of thing should be in reel or angelo (though I think it should definitely be in one). I'm guessing that it "works" if using reel-rack. @tarcieri @digitalextremist thoughts?

tommay commented 9 years ago

Putting it in reel would mean people wouldn't have to re-invent it. But I haven't actually looked into the reel API so I don't know how much sense that makes.

On Thu, Feb 19, 2015 at 11:14 AM, kenichi nakamura <notifications@github.com

wrote:

@Adam-Stomski https://github.com/Adam-Stomski I looked through reel, sinatra, and rack code... Looks like rack handles this, merging a tempfile into the params in this case. I'm now confused as to whether this type of thing should be in reel or angelo (though I think it should definitely be in one). I'm guessing that it "works" if using reel-rack. @tarcieri https://github.com/tarcieri @_de thoughts?

— Reply to this email directly or view it on GitHub https://github.com/kenichi/angelo/issues/47#issuecomment-75116130.

kenichi commented 9 years ago

@tommay I thought I had looked at it quite a bit, but realized that I completely missed the Reel::StreamResponse and Reel::EventStream classes when added SSE and chunked support to Angelo. Also, reel doesn't do params, so I'm wondering what would even be the approach there re: rack-style params[:file] => Tempfile

tarcieri commented 9 years ago

I could go either way on this one. Reel is trying to provide a lightweight, below-Rack level abstraction, but if you really think it makes sense to add it to Reel I'd be ok with that.

digitalextremist commented 9 years ago

This is something I have code for, per celluloid/reel#149 ... @Adam-Stomski I believe mulitipart form-data ought to be in Reel ... because no, it does not work properly with Rack either. That is what originally got me started modifying Reel a very long time ago - no multipart form-data support. I will work on the referenced ticket and have it reviewed to make sure it behaves the way we're discussing.

One thing I did notice though, is that form-data is always parsed in Rack which means non-multipart requests get an ever-so-slight performance dip, whereas in how I've used multipart with Reel so far, I believe I treat it more like a WebSocket is, which is to grab the data if it is expected to be multipart at an endpoint level, not in the request handler itself.

tarcieri commented 9 years ago

FWIW, @ixti is working on this sort of thing in https://github.com/httprb/form_data.rb

digitalextremist commented 9 years ago

@tarcieri I remember you mentioning that, and I checked it out. The server-side handling of multipart requests seems undocumented. The gem seems geared toward acting as a client posting requests right now, but I'm sure when I did further I'll find a parser. This is the one I've been using myself:

https://github.com/danabr/multipart-parser

If @ixti's gem is going to be more active, I'll switch over to that myself also, and in fact remove my multipart form-data bolt on, and pull this into Reel in a way @kenichi can see works with angelo.

kenichi commented 9 years ago

yeah, i don't see any server-side parsing in that http-form_data gem... what am i missing?

tarcieri commented 9 years ago

Nonexistent at the moment, but potentially something @ixti might be interested in adding

digitalextremist commented 9 years ago

Maybe I'll work with @ixti to add support for parsing on the server-side.

@kenichi, do you have specific hopes for multipart support for your DSL? I also have a Reel-based DSL, and I'd rather not custom tailor the implementation of multipart to mine... or Sinatra/Rack either.

Question for @kenichi and @tarcieri: is it acceptable to use a "hijack" style handling of multipart form-data, in this sense -- when we hijack a websocket in the Rack approach to websockets, we're at a certain endpoint and anticipate a certain behavior/state of the browser. We're never touching every single HTTP request to make it compatible with websocket requests, and the same ought to be true of multipart requests with form-data ... agreed? To me, this question alone answers enough to build requirements for Reel handling its own multipart form-data, regardless of what parser we use.

kenichi commented 9 years ago

@digitalextremist i have no specific hopes for multipart support in angelo; however, i do agree now that it should be handled in reel, so our respective DSLs can build off that. As for reel-rack support, if it doesn't work now, I see no problem in it continuing to not work :) but i think maybe an off-switch that reel-rack can set so rack can attempt to do it's thing may be a good idea.

my thoughts, as not fully formed as they are, lean towards some methods added to Reel::Request like in your issue. request.multipart? seems a great start to me. i'd like to do more research into the way rack does this with the tempfile and everything - see if there are issues with that approach that we can avoid. i'd also be super interested in @tenderlove's thoughts on how the_metal / rack-2 is going to approach this.

pachacamac commented 7 years ago

So there is still no solution? Any idea/best practice on how to handle file uploads with angelo?

kenichi commented 7 years ago

@pachacamac hello! last summer, i was able to help a google summer of code student through adding rudimentary support for both sessions and multipart to reel. it's not merged yet, and still a bit rough, but you can try it:

Gemfile:

gem 'reel', github: 'pulkit4tech/reel', branch: 'gsoc16'

Angelo app class file:

require 'reel/request/multipart'

# ...

  post '/' do
    halt 400 unless request.multipart?
    request.multipart.each do |filename, mp|
      puts "received file: '#{filename}'"
      puts "#{mp[:data].size} bytes, #{mp[:data].path}"
    end
    redirect '/'
  end