From 32d03af341eba4d0b263156f0241016b857c4d84 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Tue, 15 May 2007 21:36:21 +0000 Subject: Introduce the request.body stream. Lazy-read to parse parameters rather than always setting RAW_POST_DATA. Reduces the memory footprint of large binary PUT requests. git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@6740 5ecf4fe2-1ee6-0310-87b1-e25e094e27de --- actionpack/CHANGELOG | 2 + actionpack/lib/action_controller/cgi_ext.rb | 10 ++++ .../lib/action_controller/cgi_ext/parameters.rb | 20 ++------ .../action_controller/cgi_ext/query_extension.rb | 10 ++-- .../lib/action_controller/cgi_ext/stdinput.rb | 23 ++++++++++ actionpack/lib/action_controller/cgi_process.rb | 12 ++++- actionpack/lib/action_controller/request.rb | 34 ++++++++------ actionpack/lib/action_controller/test_process.rb | 30 +++++++----- actionpack/test/abstract_unit.rb | 2 + actionpack/test/controller/raw_post_test.rb | 53 ++++++++++++---------- actionpack/test/controller/test_test.rb | 15 +++++- actionpack/test/controller/webservice_test.rb | 10 ++-- 12 files changed, 142 insertions(+), 79 deletions(-) create mode 100644 actionpack/lib/action_controller/cgi_ext/stdinput.rb (limited to 'actionpack') diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index 16be11e8ee..90e5b6e975 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,7 @@ *SVN* +* Introduce the request.body stream. Lazy-read to parse parameters rather than always setting RAW_POST_DATA. Reduces the memory footprint of large binary PUT requests. [Jeremy Kemper] + * Add some performance enhancements to ActionView. * Cache base_paths in @@cached_base_paths diff --git a/actionpack/lib/action_controller/cgi_ext.rb b/actionpack/lib/action_controller/cgi_ext.rb index f4c8be2f76..1934ee704a 100644 --- a/actionpack/lib/action_controller/cgi_ext.rb +++ b/actionpack/lib/action_controller/cgi_ext.rb @@ -1,8 +1,18 @@ +require 'action_controller/cgi_ext/stdinput' require 'action_controller/cgi_ext/parameters' require 'action_controller/cgi_ext/query_extension' require 'action_controller/cgi_ext/cookie' require 'action_controller/cgi_ext/session' class CGI #:nodoc: + include ActionController::CgiExt::Stdinput include ActionController::CgiExt::Parameters + + class << self + alias :escapeHTML_fail_on_nil :escapeHTML + + def escapeHTML(string) + escapeHTML_fail_on_nil(string) unless string.nil? + end + end end diff --git a/actionpack/lib/action_controller/cgi_ext/parameters.rb b/actionpack/lib/action_controller/cgi_ext/parameters.rb index 639f131e00..a7f5393272 100644 --- a/actionpack/lib/action_controller/cgi_ext/parameters.rb +++ b/actionpack/lib/action_controller/cgi_ext/parameters.rb @@ -1,16 +1,6 @@ require 'cgi' require 'strscan' -class CGI #:nodoc: - class << self - alias :escapeHTML_fail_on_nil :escapeHTML - - def escapeHTML(string) - escapeHTML_fail_on_nil(string) unless string.nil? - end - end -end - module ActionController module CgiExt module Parameters @@ -72,18 +62,18 @@ module ActionController parser.result end - def parse_formatted_request_parameters(mime_type, raw_post_data) + def parse_formatted_request_parameters(mime_type, body) case strategy = ActionController::Base.param_parsers[mime_type] when Proc - strategy.call(raw_post_data) + strategy.call(body) when :xml_simple, :xml_node - raw_post_data.blank? ? {} : Hash.from_xml(raw_post_data).with_indifferent_access + body.blank? ? {} : Hash.from_xml(body).with_indifferent_access when :yaml - YAML.load(raw_post_data) + YAML.load(body) end rescue Exception => e # YAML, XML or Ruby code block errors { "exception" => "#{e.message} (#{e.class})", "backtrace" => e.backtrace, - "raw_post_data" => raw_post_data, "format" => mime_type } + "body" => body, "format" => mime_type } end private diff --git a/actionpack/lib/action_controller/cgi_ext/query_extension.rb b/actionpack/lib/action_controller/cgi_ext/query_extension.rb index 9e836eaaf8..147530b5ce 100644 --- a/actionpack/lib/action_controller/cgi_ext/query_extension.rb +++ b/actionpack/lib/action_controller/cgi_ext/query_extension.rb @@ -31,18 +31,14 @@ class CGI #:nodoc: # Set multipart to false by default. @multipart = false - # POST and PUT may have params in entity body. If content type is - # missing for POST, assume urlencoded. If content type is missing - # for PUT, don't assume anything and don't parse the parameters: - # it's likely binary data. - # - # The other HTTP methods have their params in the query string. + # POST and PUT may have params in entity body. If content type is missing + # or non-urlencoded, don't read the body or parse parameters: assume it's + # binary data. if method == :post || method == :put if boundary = extract_multipart_form_boundary(content_type) @multipart = true @params = read_multipart(boundary, content_length) elsif content_type.blank? || content_type !~ %r{application/x-www-form-urlencoded}i - read_params(method, content_length) @params = {} end end diff --git a/actionpack/lib/action_controller/cgi_ext/stdinput.rb b/actionpack/lib/action_controller/cgi_ext/stdinput.rb new file mode 100644 index 0000000000..b0ca63ef2f --- /dev/null +++ b/actionpack/lib/action_controller/cgi_ext/stdinput.rb @@ -0,0 +1,23 @@ +require 'cgi' + +module ActionController + module CgiExt + # Publicize the CGI's internal input stream so we can lazy-read + # request.body. Make it writable so we don't have to play $stdin games. + module Stdinput + def self.included(base) + base.class_eval do + remove_method :stdinput + attr_accessor :stdinput + end + + base.alias_method_chain :initialize, :stdinput + end + + def initialize_with_stdinput(type = nil, stdinput = $stdin) + @stdinput = stdinput + initialize_without_stdinput(type || 'query') + end + end + end +end diff --git a/actionpack/lib/action_controller/cgi_process.rb b/actionpack/lib/action_controller/cgi_process.rb index 59849c7364..ee6e5ca250 100644 --- a/actionpack/lib/action_controller/cgi_process.rb +++ b/actionpack/lib/action_controller/cgi_process.rb @@ -58,6 +58,16 @@ module ActionController #:nodoc: end end + # The request body is an IO input stream. If the RAW_POST_DATA environment + # variable is already set, wrap it in a StringIO. + def body + if raw_post = env['RAW_POST_DATA'] + StringIO.new(raw_post) + else + @cgi.stdinput + end + end + def query_parameters @query_parameters ||= (qs = self.query_string).empty? ? {} : CGI.parse_query_parameters(qs) @@ -66,7 +76,7 @@ module ActionController #:nodoc: def request_parameters @request_parameters ||= if ActionController::Base.param_parsers.has_key?(content_type) - CGI.parse_formatted_request_parameters(content_type, @env['RAW_POST_DATA']) + CGI.parse_formatted_request_parameters(content_type, body.read) else CGI.parse_request_parameters(@cgi.params) end diff --git a/actionpack/lib/action_controller/request.rb b/actionpack/lib/action_controller/request.rb index eed1563aba..2bc104b772 100755 --- a/actionpack/lib/action_controller/request.rb +++ b/actionpack/lib/action_controller/request.rb @@ -1,24 +1,27 @@ module ActionController - # Subclassing AbstractRequest makes these methods available to the request objects used in production and testing, - # CgiRequest and TestRequest + # CgiRequest and TestRequest provide concrete implementations. class AbstractRequest cattr_accessor :relative_url_root remove_method :relative_url_root - # Returns the hash of environment variables for this request, + # The hash of environment variables for this request, # such as { 'RAILS_ENV' => 'production' }. attr_reader :env - attr_accessor :format + # The requested content type, such as :html or :xml. + attr_writer :format - # Returns the HTTP request method as a lowercase symbol (:get, for example). Note, HEAD is returned as :get - # since the two are supposedly to be functionaly equivilent for all purposes except that HEAD won't return a response - # body (which Rails also takes care of elsewhere). + # The HTTP request method as a lowercase symbol, such as :get. + # Note, HEAD is returned as :get since the two are functionally + # equivalent from the application's perspective. def method - @request_method ||= (!parameters[:_method].blank? && @env['REQUEST_METHOD'] == 'POST') ? - parameters[:_method].to_s.downcase.to_sym : - @env['REQUEST_METHOD'].downcase.to_sym - + @request_method ||= + if @env['REQUEST_METHOD'] == 'POST' && !parameters[:_method].blank? + parameters[:_method].to_s.downcase.to_sym + else + @env['REQUEST_METHOD'].downcase.to_sym + end + @request_method == :head ? :get : @request_method end @@ -42,8 +45,8 @@ module ActionController method == :delete end - # Is this a HEAD request? HEAD is mapped as :get for request.method, so here we ask the - # REQUEST_METHOD header directly. Thus, for head, both get? and head? will return true. + # Is this a HEAD request? request.method sees HEAD as :get, so check the + # HTTP method directly. def head? @env['REQUEST_METHOD'].downcase.to_sym == :head end @@ -269,6 +272,11 @@ module ActionController #-- # Must be implemented in the concrete request #++ + + # The request body is an IO input stream. + def body + end + def query_parameters #:nodoc: end diff --git a/actionpack/lib/action_controller/test_process.rb b/actionpack/lib/action_controller/test_process.rb index b1eb9e38ef..4db8517b42 100644 --- a/actionpack/lib/action_controller/test_process.rb +++ b/actionpack/lib/action_controller/test_process.rb @@ -40,18 +40,15 @@ module ActionController #:nodoc: @session = TestSession.new end + # Wraps raw_post in a StringIO. + def body + StringIO.new(raw_post) + end + + # Either the RAW_POST_DATA environment variable or the URL-encoded request + # parameters. def raw_post - if raw_post = env['RAW_POST_DATA'] - raw_post - else - params = self.request_parameters.dup - %w(controller action only_path).each do |k| - params.delete(k) - params.delete(k.to_sym) - end - - params.map { |k,v| [ CGI.escape(k.to_s), CGI.escape(v.to_s) ].join('=') }.sort.join('&') - end + env['RAW_POST_DATA'] ||= url_encoded_request_parameters end def port=(number) @@ -140,6 +137,17 @@ module ActionController #:nodoc: @env["SERVER_PORT"] = 80 @env['REQUEST_METHOD'] = "GET" end + + def url_encoded_request_parameters + params = self.request_parameters.dup + + %w(controller action only_path).each do |k| + params.delete(k) + params.delete(k.to_sym) + end + + params.to_query + end end # A refactoring of TestResponse to allow the same behavior to be applied diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index e4b8b5b04a..3c576f3d20 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -3,8 +3,10 @@ $:.unshift(File.dirname(__FILE__) + '/../../activesupport/lib/active_support') $:.unshift(File.dirname(__FILE__) + '/fixtures/helpers') require 'yaml' +require 'stringio' require 'test/unit' require 'action_controller' +require 'action_controller/cgi_ext' require 'action_controller/test_process' # Show backtraces for deprecated behavior for quicker cleanup. diff --git a/actionpack/test/controller/raw_post_test.rb b/actionpack/test/controller/raw_post_test.rb index 1f8a93d149..5fd2c84ff0 100644 --- a/actionpack/test/controller/raw_post_test.rb +++ b/actionpack/test/controller/raw_post_test.rb @@ -1,6 +1,4 @@ require "#{File.dirname(__FILE__)}/../abstract_unit" -require 'stringio' -require 'action_controller/cgi_ext/query_extension' class RawPostDataTest < Test::Unit::TestCase def setup @@ -11,57 +9,66 @@ class RawPostDataTest < Test::Unit::TestCase def test_post_with_urlencoded_body ENV['REQUEST_METHOD'] = 'POST' ENV['CONTENT_TYPE'] = ' apPlication/x-Www-form-urlEncoded; charset=utf-8' - assert_equal ['1'], cgi_params['a'] - assert_has_raw_post_data + assert_equal ['1'], cgi.params['a'] + assert_raw_post_data end def test_post_with_empty_content_type_treated_as_urlencoded ENV['REQUEST_METHOD'] = 'POST' ENV['CONTENT_TYPE'] = '' - assert_equal ['1'], cgi_params['a'] - assert_has_raw_post_data + assert_equal ['1'], cgi.params['a'] + assert_raw_post_data end - def test_post_with_unrecognized_content_type_reads_body_but_doesnt_parse_params + def test_post_with_unrecognized_content_type_ignores_body ENV['REQUEST_METHOD'] = 'POST' ENV['CONTENT_TYPE'] = 'foo/bar' - assert cgi_params.empty? - assert_has_raw_post_data + assert cgi.params.empty? + assert_no_raw_post_data end def test_put_with_urlencoded_body ENV['REQUEST_METHOD'] = 'PUT' ENV['CONTENT_TYPE'] = 'application/x-www-form-urlencoded' - assert_equal ['1'], cgi_params['a'] - assert_has_raw_post_data + assert_equal ['1'], cgi.params['a'] + assert_raw_post_data end def test_put_with_empty_content_type_ignores_body ENV['REQUEST_METHOD'] = 'PUT' ENV['CONTENT_TYPE'] = '' - assert cgi_params.empty? - assert_has_raw_post_data + assert cgi.params.empty? + assert_no_raw_post_data end def test_put_with_unrecognized_content_type_ignores_body ENV['REQUEST_METHOD'] = 'PUT' ENV['CONTENT_TYPE'] = 'foo/bar' - assert cgi_params.empty? - assert_has_raw_post_data + assert cgi.params.empty? + assert_no_raw_post_data end private - def cgi_params - old_stdin, $stdin = $stdin, StringIO.new(@request_body.dup) - ENV['CONTENT_LENGTH'] = $stdin.size.to_s - CGI.new.params - ensure - $stdin = old_stdin + def cgi + unless defined? @cgi + ENV['CONTENT_LENGTH'] = @request_body.size.to_s + @cgi = CGI.new('query', StringIO.new(@request_body.dup)) + end + + @cgi end - def assert_has_raw_post_data(expected_body = @request_body) + def assert_raw_post_data assert_not_nil ENV['RAW_POST_DATA'] assert ENV['RAW_POST_DATA'].frozen? - assert_equal expected_body, ENV['RAW_POST_DATA'] + assert_equal @request_body, ENV['RAW_POST_DATA'] + + assert_equal '', cgi.stdinput.read + end + + def assert_no_raw_post_data + assert_nil ENV['RAW_POST_DATA'] + + assert_equal @request_body, cgi.stdinput.read end end diff --git a/actionpack/test/controller/test_test.rb b/actionpack/test/controller/test_test.rb index ec30f0ac8b..d786a05feb 100644 --- a/actionpack/test/controller/test_test.rb +++ b/actionpack/test/controller/test_test.rb @@ -13,6 +13,10 @@ class TestTest < Test::Unit::TestCase render :text => request.raw_post end + def render_body + render :text => request.body.read + end + def test_params render :text => params.inspect end @@ -93,8 +97,15 @@ HTML params = {:page => {:name => 'page name'}, 'some key' => 123} get :render_raw_post, params.dup - raw_post = params.map {|k,v| [CGI::escape(k.to_s), CGI::escape(v.to_s)].join('=')}.sort.join('&') - assert_equal raw_post, @response.body + assert_equal params.to_query, @response.body + end + + def test_body_stream + params = { :page => { :name => 'page name' }, 'some key' => 123 } + + get :render_body, params.dup + + assert_equal params.to_query, @response.body end def test_process_without_flash diff --git a/actionpack/test/controller/webservice_test.rb b/actionpack/test/controller/webservice_test.rb index 4558d401b0..bcdb3d5eb7 100644 --- a/actionpack/test/controller/webservice_test.rb +++ b/actionpack/test/controller/webservice_test.rb @@ -1,20 +1,16 @@ require File.dirname(__FILE__) + '/../abstract_unit' -require 'stringio' class WebServiceTest < Test::Unit::TestCase - class MockCGI < CGI #:nodoc: - attr_accessor :stdinput, :stdoutput, :env_table + attr_accessor :stdoutput, :env_table - def initialize(env, data = '') + def initialize(env, data = '') self.env_table = env - self.stdinput = StringIO.new(data) self.stdoutput = StringIO.new - super() + super(nil, StringIO.new(data)) end end - class TestController < ActionController::Base session :off -- cgit v1.2.3