Skip to content

Commit

Permalink
Add RewindableInput wrapper to fix issues with middleware that impoli…
Browse files Browse the repository at this point in the history
…tely eat up non-rewindable input
  • Loading branch information
josh committed Jan 13, 2009
1 parent 5a43908 commit 1adc149
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 8 deletions.
13 changes: 7 additions & 6 deletions actionpack/lib/action_controller.rb
Expand Up @@ -59,23 +59,24 @@ def self.load_all!
autoload :MiddlewareStack, 'action_controller/middleware_stack'
autoload :MimeResponds, 'action_controller/mime_responds'
autoload :PolymorphicRoutes, 'action_controller/polymorphic_routes'
autoload :Request, 'action_controller/request'
autoload :RequestParser, 'action_controller/request_parser'
autoload :UrlEncodedPairParser, 'action_controller/url_encoded_pair_parser'
autoload :UploadedStringIO, 'action_controller/uploaded_file'
autoload :UploadedTempfile, 'action_controller/uploaded_file'
autoload :RecordIdentifier, 'action_controller/record_identifier'
autoload :Response, 'action_controller/response'
autoload :Request, 'action_controller/request'
autoload :RequestForgeryProtection, 'action_controller/request_forgery_protection'
autoload :RequestParser, 'action_controller/request_parser'
autoload :Rescue, 'action_controller/rescue'
autoload :Resources, 'action_controller/resources'
autoload :Response, 'action_controller/response'
autoload :RewindableInput, 'action_controller/rewindable_input'
autoload :Routing, 'action_controller/routing'
autoload :SessionManagement, 'action_controller/session_management'
autoload :StatusCodes, 'action_controller/status_codes'
autoload :Streaming, 'action_controller/streaming'
autoload :TestCase, 'action_controller/test_case'
autoload :TestProcess, 'action_controller/test_process'
autoload :Translation, 'action_controller/translation'
autoload :UploadedStringIO, 'action_controller/uploaded_file'
autoload :UploadedTempfile, 'action_controller/uploaded_file'
autoload :UrlEncodedPairParser, 'action_controller/url_encoded_pair_parser'
autoload :UrlRewriter, 'action_controller/url_rewriter'
autoload :UrlWriter, 'action_controller/url_rewriter'
autoload :VerbPiggybacking, 'action_controller/verb_piggybacking'
Expand Down
1 change: 1 addition & 0 deletions actionpack/lib/action_controller/middlewares.rb
Expand Up @@ -18,4 +18,5 @@
)
end

use ActionController::RewindableInput
use ActionController::VerbPiggybacking
40 changes: 40 additions & 0 deletions actionpack/lib/action_controller/rewindable_input.rb
@@ -0,0 +1,40 @@
module ActionController
class RewindableInput
class RewindableIO < ActiveSupport::BasicObject
def initialize(io)
@io = io
end

def read(*args)
read_original_io
@io.read(*args)
end

def rewind
read_original_io
@io.rewind
end

def method_missing(method, *args, &block)
@io.send(method, *args, &block)
end

private
def read_original_io
unless @str
@str = @io.read
@io = StringIO.new(@str)
end
end
end

def initialize(app)
@app = app
end

def call(env)
env['rack.input'] = RewindableIO.new(env['rack.input'])
@app.call(env)
end
end
end
Expand Up @@ -123,21 +123,41 @@ def teardown
InputWrapper = Rack::Lint::InputWrapper

test "parses unwindable stream" do
InputWrapper.any_instance.expects(:rewind).raises(Errno::ESPIPE)
InputWrapper.any_instance.stubs(:rewind).raises(Errno::ESPIPE)
params = parse_multipart('large_text_file')
assert_equal %w(file foo), params.keys.sort
assert_equal 'bar', params['foo']
end

test "uploads and reads file with unwindable input" do
InputWrapper.any_instance.expects(:rewind).raises(Errno::ESPIPE)
InputWrapper.any_instance.stubs(:rewind).raises(Errno::ESPIPE)

with_test_routing do
post '/read', :uploaded_data => fixture_file_upload(FIXTURE_PATH + "/hello.txt", "text/plain")
assert_equal "File: Hello", response.body
end
end

test "passes through rack middleware and uploads file" do
with_muck_middleware do
with_test_routing do
post '/read', :uploaded_data => fixture_file_upload(FIXTURE_PATH + "/hello.txt", "text/plain")
assert_equal "File: Hello", response.body
end
end
end

test "passes through rack middleware and uploads file with unwindable input" do
InputWrapper.any_instance.stubs(:rewind).raises(Errno::ESPIPE)

with_muck_middleware do
with_test_routing do
post '/read', :uploaded_data => fixture_file_upload(FIXTURE_PATH + "/hello.txt", "text/plain")
assert_equal "File: Hello", response.body
end
end
end

private
def fixture(name)
File.open(File.join(FIXTURE_PATH, name), 'rb') do |file|
Expand All @@ -164,4 +184,25 @@ def with_test_routing
yield
end
end

class MuckMiddleware
def initialize(app)
@app = app
end

def call(env)
req = Rack::Request.new(env)
req.params # Parse params
@app.call(env)
end
end

def with_muck_middleware
original_middleware = ActionController::Dispatcher.middleware
middleware = original_middleware.dup
middleware.use MuckMiddleware
ActionController::Dispatcher.middleware = middleware
yield
ActionController::Dispatcher.middleware = original_middleware
end
end
Expand Up @@ -150,8 +150,45 @@ def teardown
assert_parses expected, query
end

test "passes through rack middleware and parses params" do
with_muck_middleware do
assert_parses({ "a" => { "b" => "c" } }, "a[b]=c")
end
end

# The lint wrapper is used in integration tests
# instead of a normal StringIO class
InputWrapper = Rack::Lint::InputWrapper

test "passes through rack middleware and parses params with unwindable input" do
InputWrapper.any_instance.stubs(:rewind).raises(Errno::ESPIPE)
with_muck_middleware do
assert_parses({ "a" => { "b" => "c" } }, "a[b]=c")
end
end

private
class MuckMiddleware
def initialize(app)
@app = app
end

def call(env)
req = Rack::Request.new(env)
req.params # Parse params
@app.call(env)
end
end

def with_muck_middleware
original_middleware = ActionController::Dispatcher.middleware
middleware = original_middleware.dup
middleware.use MuckMiddleware
ActionController::Dispatcher.middleware = middleware
yield
ActionController::Dispatcher.middleware = original_middleware
end

def with_test_routing
with_routing do |set|
set.draw do |map|
Expand Down

4 comments on commit 1adc149

@spohlenz
Copy link
Contributor

Choose a reason for hiding this comment

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

This commit seems to cause problems on Passenger due to the fact that RewindableInput doesn’t define respond_to? (due to inheriting from BlankSlate).

respond_to? is called from parse_multipart_with_rewind in rack_ext.rb

@josh
Copy link
Contributor Author

@josh josh commented on 1adc149 Jan 15, 2009

Choose a reason for hiding this comment

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

I thought I tested with Passenger and it worked fine. Also respond_to? should be delegated to the IO object.

Can you please open a LH ticket and assign it to me.

(This whole patch was intended to fix uploads with Passenger ;)

@spohlenz
Copy link
Contributor

Choose a reason for hiding this comment

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

I’ll test on a clean rails and make sure its not something stupid I’m doing.

@spohlenz
Copy link
Contributor

Choose a reason for hiding this comment

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

Ticket is at http://rails.lighthouseapp.com/projects/8994/tickets/1767-post-requests-broken-on-passenger#ticket-1767-1

Please sign in to comment.