Merge branch 'pp'

* pp:
  remove outdated comment
  all parameter parsing is done through the request object now.
  let the request object handle parsing XML posts
  remove setting request parameters for JSON requests
  remove the request parameter from `parse_formatted_parameters`
  do not instantiate a param parser middleware
  push the parameter parsers on to the class
  stop eagerly parsing parameters
  only wrap the strategy with exception handling
  pull `normalize_encode_params` up
  remove the `default` parameter from the parser method
  move parameter parsing to the request object

5 files changed, 67 insertions, 56 deletions

diff --git a/actionpack/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb
index fbbaa1a887..f16c851456 100644
--- a/actionpack/lib/action_controller/test_case.rb
+++ b/actionpack/lib/action_controller/test_case.rb
@@ -33,6 +33,9 @@ module ActionController
 
       self.session = session
       self.session_options = TestSession::DEFAULT_OPTIONS
+      @custom_param_parsers = {
+        Mime::XML => lambda { |raw_post| Hash.from_xml(raw_post)['hash'] }
+      }
     end
 
     def query_string=(string)
@@ -74,26 +77,18 @@ module ActionController
             set_header k, 'application/x-www-form-urlencoded'
           end
 
-          # FIXME: setting `request_parametes` is normally handled by the
-          # params parser middleware, and we should remove this roundtripping
-          # when we switch to caling `call` on the controller
-
           case content_mime_type.to_sym
           when nil
             raise "Unknown Content-Type: #{content_type}"
           when :json
             data = ActiveSupport::JSON.encode(non_path_parameters)
-            params = ActiveSupport::JSON.decode(data).with_indifferent_access
-            self.request_parameters = params
           when :xml
             data = non_path_parameters.to_xml
-            params = Hash.from_xml(data)['hash']
-            self.request_parameters = params
           when :url_encoded_form
             data = non_path_parameters.to_query
           else
+            @custom_param_parsers[content_mime_type] = ->(_) { non_path_parameters }
             data = non_path_parameters.to_query
-            self.request_parameters = non_path_parameters
           end
         end
 
@@ -136,6 +131,12 @@ module ActionController
         "multipart/form-data; boundary=#{Rack::Test::MULTIPART_BOUNDARY}"
       end
     end.new
+
+    private
+
+    def params_parsers
+      super.merge @custom_param_parsers
+    end
   end
 
   class LiveTestResponse < Live::Response
diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb
index 3c9f8cd9e4..27fadb708e 100644
--- a/actionpack/lib/action_dispatch/http/parameters.rb
+++ b/actionpack/lib/action_dispatch/http/parameters.rb
@@ -3,6 +3,20 @@ module ActionDispatch
     module Parameters
       PARAMETERS_KEY = 'action_dispatch.request.path_parameters'
 
+      DEFAULT_PARSERS = {
+        Mime::JSON => lambda { |raw_post|
+          data = ActiveSupport::JSON.decode(raw_post)
+          data.is_a?(Hash) ? data : {:_json => data}
+        }
+      }
+
+      def self.included(klass)
+        class << klass
+          attr_accessor :parameter_parsers
+        end
+
+        klass.parameter_parsers = DEFAULT_PARSERS
+      end
       # Returns both GET and POST \parameters in a single hash.
       def parameters
         params = get_header("action_dispatch.request.parameters")
@@ -31,6 +45,27 @@ module ActionDispatch
       def path_parameters
         get_header(PARAMETERS_KEY) || {}
       end
+
+      private
+
+      def parse_formatted_parameters(parsers)
+        return yield if content_length.zero?
+
+        strategy = parsers.fetch(content_mime_type) { return yield }
+
+        begin
+          strategy.call(raw_post)
+        rescue => e # JSON or Ruby code block errors
+          my_logger = logger || ActiveSupport::Logger.new($stderr)
+          my_logger.debug "Error occurred while parsing request parameters.\nContents:\n\n#{raw_post}"
+
+          raise ParamsParser::ParseError.new(e.message, e)
+        end
+      end
+
+      def params_parsers
+        ActionDispatch::Request.parameter_parsers
+      end
     end
   end
 end
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
index b2566c4820..eaa7e88b34 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -348,8 +348,14 @@ module ActionDispatch
     # Override Rack's POST method to support indifferent access
     def POST
       fetch_header("action_dispatch.request.request_parameters") do
-        self.request_parameters = Request::Utils.normalize_encode_params(super || {})
+        pr = parse_formatted_parameters(params_parsers) do |params|
+          super || {}
+        end
+        self.request_parameters = Request::Utils.normalize_encode_params(pr)
       end
+    rescue ParamsParser::ParseError # one of the parse strategies blew up
+      self.request_parameters = Request::Utils.normalize_encode_params(super || {})
+      raise
     rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e
       raise ActionController::BadRequest.new(:request, e)
     end
diff --git a/actionpack/lib/action_dispatch/middleware/params_parser.rb b/actionpack/lib/action_dispatch/middleware/params_parser.rb
index 9cde9c9b98..18af0a583a 100644
--- a/actionpack/lib/action_dispatch/middleware/params_parser.rb
+++ b/actionpack/lib/action_dispatch/middleware/params_parser.rb
@@ -18,48 +18,13 @@ module ActionDispatch
       end
     end
 
-    DEFAULT_PARSERS = {
-      Mime::JSON => lambda { |raw_post|
-        data = ActiveSupport::JSON.decode(raw_post)
-        data = {:_json => data} unless data.is_a?(Hash)
-        Request::Utils.normalize_encode_params(data)
-      }
-    }
-
     # Create a new +ParamsParser+ middleware instance.
     #
     # The +parsers+ argument can take Hash of parsers where key is identifying
     # content mime type, and value is a lambda that is going to process data.
-    def initialize(app, parsers = {})
-      @app, @parsers = app, DEFAULT_PARSERS.merge(parsers)
+    def self.new(app, parsers = {})
+      ActionDispatch::Request.parameter_parsers = ActionDispatch::Request::DEFAULT_PARSERS.merge(parsers)
+      app
     end
-
-    def call(env)
-      request = Request.new(env)
-
-      parse_formatted_parameters(request, @parsers) do |params|
-        request.request_parameters = params
-      end
-
-      @app.call(env)
-    end
-
-    private
-      def parse_formatted_parameters(request, parsers)
-        return if request.content_length.zero?
-
-        strategy = parsers.fetch(request.content_mime_type) { return nil }
-
-        yield strategy.call(request.raw_post)
-
-      rescue => e # JSON or Ruby code block errors
-        logger(request).debug "Error occurred while parsing request parameters.\nContents:\n\n#{request.raw_post}"
-
-        raise ParseError.new(e.message, e)
-      end
-
-      def logger(request)
-        request.logger || ActiveSupport::Logger.new($stderr)
-      end
   end
 end
diff --git a/actionpack/test/controller/webservice_test.rb b/actionpack/test/controller/webservice_test.rb
index b26f037c36..8b37c9599e 100644
--- a/actionpack/test/controller/webservice_test.rb
+++ b/actionpack/test/controller/webservice_test.rb
@@ -97,24 +97,28 @@ class WebServiceTest < ActionDispatch::IntegrationTest
   end
 
   def test_parsing_json_doesnot_rescue_exception
-    with_test_route_set do
-      with_params_parsers Mime::JSON => Proc.new { |data| raise Interrupt } do
-        assert_raises(Interrupt) do
-          post "/",
-            params: '{"title":"JSON"}}',
-            headers: { 'CONTENT_TYPE' => 'application/json' }
-        end
+    req = Class.new(ActionDispatch::Request) do
+      def params_parsers
+        { Mime::JSON => Proc.new { |data| raise Interrupt } }
       end
+
+      def content_length; get_header('rack.input').length; end
+    end.new({ 'rack.input' => StringIO.new('{"title":"JSON"}}'), 'CONTENT_TYPE' => 'application/json' })
+
+    assert_raises(Interrupt) do
+      req.request_parameters
     end
   end
 
   private
     def with_params_parsers(parsers = {})
       old_session = @integration_session
-      @app = ActionDispatch::ParamsParser.new(app.routes, parsers)
+      original_parsers = ActionDispatch::Request.parameter_parsers
+      ActionDispatch::Request.parameter_parsers = original_parsers.merge parsers
       reset!
       yield
     ensure
+      ActionDispatch::Request.parameter_parsers = original_parsers
       @integration_session = old_session
     end