Merge pull request #359 from sikachu/params_wrapper.

Add `ActionController::ParamsWrapper` to wrap JSON parameters into a nested parameters.

7 files changed, 481 insertions, 0 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 07a23ddb63..184b2af018 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,9 @@
 *Rails 3.1.0 (unreleased)*
 
+* Add `ActionController::ParamsWrapper` to wrap parameters into a nested hash, and will be turned on for JSON request in new applications by default [Prem Sichanugrist]
+
+  This can be customizabled by setting `ActionController::Base.wrap_parameters` in `config/initializer/wrap_parameters.rb`
+
 * RJS has been extracted out to a gem. [fxn]
 
 * Implicit actions named not_implemented can be rendered [Santiago Pastorino]
diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb
index aab2b9dc25..eba5e9377b 100644
--- a/actionpack/lib/action_controller.rb
+++ b/actionpack/lib/action_controller.rb
@@ -23,6 +23,7 @@ module ActionController
     autoload :ImplicitRender
     autoload :Instrumentation
     autoload :MimeResponds
+    autoload :ParamsWrapper
     autoload :RackDelegation
     autoload :Redirecting
     autoload :Renderers
diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb
index ca0dccf575..373df7fb55 100644
--- a/actionpack/lib/action_controller/base.rb
+++ b/actionpack/lib/action_controller/base.rb
@@ -194,6 +194,7 @@ module ActionController
       Caching,
       MimeResponds,
       ImplicitRender,
+      ParamsWrapper,
 
       Cookies,
       Flash,
diff --git a/actionpack/lib/action_controller/metal/params_wrapper.rb b/actionpack/lib/action_controller/metal/params_wrapper.rb
new file mode 100644
index 0000000000..29ff546139
--- /dev/null
+++ b/actionpack/lib/action_controller/metal/params_wrapper.rb
@@ -0,0 +1,197 @@
+require 'active_support/core_ext/class/attribute'
+require 'action_dispatch/http/mime_types'
+
+module ActionController
+  # Wraps parameters hash into nested hash. This will allow client to submit
+  # POST request without having to specify a root element in it.
+  #
+  # By default, this functionality won't be enabled by default. You can enable
+  # it globally by setting +ActionController::Base.wrap_parameters+:
+  #
+  #     ActionController::Base.wrap_parameters = [:json]
+  #
+  # You could also turn it on per controller by setting the format array to
+  # non-empty array:
+  #
+  #     class UsersController < ApplicationController
+  #       wrap_parameters :format => [:json, :xml]
+  #     end
+  #
+  # If you enable +ParamsWrapper+ for +:json+ format. Instead of having to
+  # send JSON parameters like this:
+  #
+  #     {"user": {"name": "Konata"}}
+  #
+  # You can now just send a parameters like this:
+  #
+  #     {"name": "Konata"}
+  #
+  # And it will be wrapped into a nested hash with the key name matching
+  # controller's name. For example, if you're posting to +UsersController+,
+  # your new +params+ hash will look like this:
+  #
+  #     {"name" => "Konata", "user" => {"name" => "Konata"}}
+  #
+  # You can also specify the key in which the parameters should be wrapped to,
+  # and also the list of attributes it should wrap by using either +:only+ or
+  # +:except+ options like this:
+  #
+  #     class UsersController < ApplicationController
+  #       wrap_parameters :person, :only => [:username, :password]
+  #     end
+  #
+  # If you're going to pass the parameters to an +ActiveModel+ object (such as
+  # +User.new(params[:user])+), you might consider passing the model class to
+  # the method instead. The +ParamsWrapper+ will actually try to determine the
+  # list of attribute names from the model and only wrap those attributes:
+  #
+  #     class UsersController < ApplicationController
+  #       wrap_parameters Person
+  #     end
+  #
+  # You still could pass +:only+ and +:except+ to set the list of attributes
+  # you want to wrap.
+  #
+  # By default, if you don't specify the key in which the parameters would be
+  # wrapped to, +ParamsWrapper+ will actually try to determine if there's
+  # a model related to it or not. This controller, for example:
+  #
+  #     class Admin::UsersController < ApplicationController
+  #     end
+  #
+  # will try to check if +Admin::User+ or +User+ model exists, and use it to
+  # determine the wrapper key respectively. If both of the model doesn't exists,
+  # it will then fallback to use +user+ as the key.
+  module ParamsWrapper
+    extend ActiveSupport::Concern
+
+    EXCLUDE_PARAMETERS = %w(authenticity_token _method utf8)
+
+    included do
+      class_attribute :_wrapper_options
+      self._wrapper_options = {:format => []}
+    end
+
+    module ClassMethods
+      # Sets the name of the wrapper key, or the model which +ParamsWrapper+
+      # would use to determine the attribute names from.
+      #
+      # ==== Examples
+      #   wrap_parameters :format => :xml
+      #     # enables the parmeter wrappes for XML format
+      #
+      #   wrap_parameters :person
+      #     # wraps parameters into +params[:person]+ hash
+      #
+      #   wrap_parameters Person
+      #     # wraps parameters by determine the wrapper key from Person class
+      #     (+person+, in this case) and the list of attribute names
+      #
+      #   wrap_parameters :only => [:username, :title]
+      #     # wraps only +:username+ and +:title+ attributes from parameters.
+      #
+      #   wrap_parameters false
+      #     # disable parameters wrapping for this controller altogether.
+      #
+      # ==== Options
+      # * <tt>:format</tt> - The list of formats in which the parameters wrapper
+      #   will be enabled.
+      # * <tt>:only</tt> - The list of attribute names which parmeters wrapper
+      #   will wrap into a nested hash.
+      # * <tt>:only</tt> - The list of attribute names which parmeters wrapper
+      #   will exclude from a nested hash.
+      def wrap_parameters(name_or_model_or_options, options = {})
+        if !name_or_model_or_options.is_a? Hash
+          if name_or_model_or_options != false
+            options = options.merge(:name_or_model => name_or_model_or_options)
+          else
+            options = opions.merge(:format => [])
+          end
+        else
+          options = name_or_model_or_options
+        end
+
+        options[:name_or_model] ||= _default_wrap_model
+        self._wrapper_options = self._wrapper_options.merge(options)
+      end
+
+      # Sets the default wrapper key or model which will be used to determine
+      # wrapper key and attribute names. Will be called automatically when the
+      # module is inherited.
+      def inherited(klass)
+        if klass._wrapper_options[:format].present?
+          klass._wrapper_options = klass._wrapper_options.merge(:name_or_model => klass._default_wrap_model)
+        end
+        super
+      end
+
+      # Determine the wrapper model from the controller's name. By convention,
+      # this could be done by trying to find the defined model that has the
+      # same singularize name as the controller. For example, +UsersController+
+      # will try to find if the +User+ model exists.
+      def _default_wrap_model
+        model_name = self.name.sub(/Controller$/, '').singularize
+
+        begin
+          model_klass = model_name.constantize
+        rescue NameError => e
+          unscoped_model_name = model_name.split("::", 2).last
+          break if unscoped_model_name == model_name
+          model_name = unscoped_model_name
+        end until model_klass
+
+        model_klass
+      end
+    end
+
+    # Performs parameters wrapping upon the request. Will be called automatically
+    # by the metal call stack.
+    def process_action(*args)
+      if _wrapper_enabled?
+        wrapped_hash = { _wrapper_key => request.request_parameters.slice(*_wrapped_keys) }
+        wrapped_filtered_hash = { _wrapper_key => request.filtered_parameters.slice(*_wrapped_keys) }
+
+        # This will make the wrapped hash accessible from controller and view
+        request.parameters.merge! wrapped_hash
+        request.request_parameters.merge! wrapped_hash
+
+        # This will make the wrapped hash displayed in the log file
+        request.filtered_parameters.merge! wrapped_filtered_hash
+      end
+      super
+    end
+
+    private
+      # Returns the wrapper key which will use to stored wrapped parameters.
+      def _wrapper_key
+        @_wrapper_key ||= if _wrapper_options[:name_or_model]
+            _wrapper_options[:name_or_model].to_s.demodulize.underscore
+          else
+            self.class.controller_name.singularize
+          end
+      end
+
+      # Returns the list of parameters which will be selected for wrapped.
+      def _wrapped_keys
+        @_wrapped_keys ||= if _wrapper_options[:only]
+            Array(_wrapper_options[:only]).collect(&:to_s)
+          elsif _wrapper_options[:except]
+            request.request_parameters.keys - Array(_wrapper_options[:except]).collect(&:to_s) - EXCLUDE_PARAMETERS
+          elsif _wrapper_options[:name_or_model].respond_to?(:column_names)
+            _wrapper_options[:name_or_model].column_names
+          else
+            request.request_parameters.keys - EXCLUDE_PARAMETERS
+          end
+      end
+
+      # Returns the list of enabled formats.
+      def _wrapper_formats
+        Array(_wrapper_options[:format])
+      end
+
+      # Checks if we should perform parameters wrapping.
+      def _wrapper_enabled?
+        _wrapper_formats.any?{ |format| format == request.content_mime_type.try(:ref) } && request.request_parameters[_wrapper_key].nil?
+      end
+  end
+end
diff --git a/actionpack/test/controller/params_wrapper_test.rb b/actionpack/test/controller/params_wrapper_test.rb
new file mode 100644
index 0000000000..2e5d096fcd
--- /dev/null
+++ b/actionpack/test/controller/params_wrapper_test.rb
@@ -0,0 +1,187 @@
+require 'abstract_unit'
+
+module Admin; class User; end; end
+
+class ParamsWrapperTest < ActionController::TestCase
+  class UsersController < ActionController::Base
+    def test
+      render :json => params.except(:controller, :action)
+    end
+  end
+
+  class User; end
+  class Person; end
+
+  tests UsersController
+
+  def test_derivered_name_from_controller
+    with_default_wrapper_options do
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'username' => 'sikachu' }
+      assert_equal '{"username":"sikachu","user":{"username":"sikachu"}}', @response.body
+    end
+  end
+
+  def test_specify_wrapper_name
+    with_default_wrapper_options do
+      UsersController.wrap_parameters :person
+
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'username' => 'sikachu' }
+      assert_equal '{"username":"sikachu","person":{"username":"sikachu"}}', @response.body
+    end
+  end
+
+  def test_specify_wrapper_model
+    with_default_wrapper_options do
+      UsersController.wrap_parameters Person
+
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'username' => 'sikachu' }
+      assert_equal '{"username":"sikachu","person":{"username":"sikachu"}}', @response.body
+    end
+  end
+
+  def test_specify_only_option
+    with_default_wrapper_options do
+      UsersController.wrap_parameters :only => :username
+
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'username' => 'sikachu', 'title' => 'Developer' }
+      assert_equal '{"username":"sikachu","title":"Developer","user":{"username":"sikachu"}}', @response.body
+    end
+  end
+
+  def test_specify_except_option
+    with_default_wrapper_options do
+      UsersController.wrap_parameters :except => :title
+
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'username' => 'sikachu', 'title' => 'Developer' }
+      assert_equal '{"username":"sikachu","title":"Developer","user":{"username":"sikachu"}}', @response.body
+    end
+  end
+
+  def test_specify_both_wrapper_name_and_only_option
+    with_default_wrapper_options do
+      UsersController.wrap_parameters :person, :only => :username
+
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'username' => 'sikachu', 'title' => 'Developer' }
+      assert_equal '{"username":"sikachu","title":"Developer","person":{"username":"sikachu"}}', @response.body
+    end
+  end
+
+  def test_not_enabled_format
+    with_default_wrapper_options do
+      @request.env['CONTENT_TYPE'] = 'application/xml'
+      post :test, { 'username' => 'sikachu', 'title' => 'Developer' }
+      assert_equal '{"username":"sikachu","title":"Developer"}', @response.body
+    end
+  end
+
+  def test_specify_format
+    with_default_wrapper_options do
+      UsersController.wrap_parameters :format => :xml
+
+      @request.env['CONTENT_TYPE'] = 'application/xml'
+      post :test, { 'username' => 'sikachu', 'title' => 'Developer' }
+      assert_equal '{"username":"sikachu","title":"Developer","user":{"username":"sikachu","title":"Developer"}}', @response.body
+    end
+  end
+
+  def test_not_wrap_reserved_parameters
+    with_default_wrapper_options do
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'authenticity_token' => 'pwned', '_method' => 'put', 'utf8' => '&#9731;', 'username' => 'sikachu' }
+      assert_equal '{"authenticity_token":"pwned","_method":"put","utf8":"&#9731;","username":"sikachu","user":{"username":"sikachu"}}', @response.body
+    end
+  end
+
+  def test_no_double_wrap_if_key_exists
+    with_default_wrapper_options do
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'user' => { 'username' => 'sikachu' }}
+      assert_equal '{"user":{"username":"sikachu"}}', @response.body
+    end
+  end
+
+  def test_nested_params
+    with_default_wrapper_options do
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'person' => { 'username' => 'sikachu' }}
+      assert_equal '{"person":{"username":"sikachu"},"user":{"person":{"username":"sikachu"}}}', @response.body
+    end
+  end
+
+  def test_derived_wrapped_keys_from_matching_model
+    with_default_wrapper_options do
+      User.expects(:respond_to?).with(:column_names).returns(true)
+      User.expects(:column_names).returns(["username"])
+
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'username' => 'sikachu', 'title' => 'Developer' }
+      assert_equal '{"username":"sikachu","title":"Developer","user":{"username":"sikachu"}}', @response.body
+    end
+  end
+
+  def test_derived_wrapped_keys_from_specified_model
+    with_default_wrapper_options do
+      Person.expects(:respond_to?).with(:column_names).returns(true)
+      Person.expects(:column_names).returns(["username"])
+
+      UsersController.wrap_parameters Person
+
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'username' => 'sikachu', 'title' => 'Developer' }
+      assert_equal '{"username":"sikachu","title":"Developer","person":{"username":"sikachu"}}', @response.body
+    end
+  end
+
+  private
+    def with_default_wrapper_options(&block)
+      @controller.class._wrapper_options = {:format => [:json]}
+      @controller.class.inherited(@controller.class)
+      yield
+    end
+end
+
+class NamespacedParamsWrapperTest < ActionController::TestCase
+  module Admin
+    class UsersController < ActionController::Base
+      def test
+        render :json => params.except(:controller, :action)
+      end
+    end
+
+    class User; end
+  end
+  class User; end
+  class Person; end
+
+  tests Admin::UsersController
+
+  def test_derivered_name_from_controller
+    with_default_wrapper_options do
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'username' => 'sikachu' }
+      assert_equal '{"username":"sikachu","user":{"username":"sikachu"}}', @response.body
+    end
+  end
+
+  def test_namespace_lookup_when_namespaced_model_available
+    with_default_wrapper_options do
+      Admin::User.expects(:respond_to?).with(:column_names).returns(false)
+
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :test, { 'username' => 'sikachu' }
+    end
+  end
+
+  private
+    def with_default_wrapper_options(&block)
+      @controller.class._wrapper_options = {:format => [:json]}
+      @controller.class.inherited(@controller.class)
+      yield
+    end
+end
diff --git a/actionpack/test/dispatch/request/json_params_parsing_test.rb b/actionpack/test/dispatch/request/json_params_parsing_test.rb
index 34db7a4c66..d854d55173 100644
--- a/actionpack/test/dispatch/request/json_params_parsing_test.rb
+++ b/actionpack/test/dispatch/request/json_params_parsing_test.rb
@@ -63,3 +63,56 @@ class JsonParamsParsingTest < ActionDispatch::IntegrationTest
       end
     end
 end
+
+class RootLessJSONParamsParsingTest < ActionDispatch::IntegrationTest
+  class UsersController < ActionController::Base
+    wrap_parameters :format => :json
+
+    class << self
+      attr_accessor :last_request_parameters, :last_parameters
+    end
+
+    def parse
+      self.class.last_request_parameters = request.request_parameters
+      self.class.last_parameters = params
+      head :ok
+    end
+  end
+
+  def teardown
+    UsersController.last_request_parameters = nil
+  end
+
+  test "parses json params for application json" do
+    assert_parses(
+      {"user" => {"username" => "sikachu"}, "username" => "sikachu"},
+      "{\"username\": \"sikachu\"}", { 'CONTENT_TYPE' => 'application/json' }
+    )
+  end
+
+  test "parses json params for application jsonrequest" do
+    assert_parses(
+      {"user" => {"username" => "sikachu"}, "username" => "sikachu"},
+      "{\"username\": \"sikachu\"}", { 'CONTENT_TYPE' => 'application/jsonrequest' }
+    )
+  end
+
+  private
+    def assert_parses(expected, actual, headers = {})
+      with_test_routing(UsersController) do
+        post "/parse", actual, headers
+        assert_response :ok
+        assert_equal(expected, UsersController.last_request_parameters)
+        assert_equal(expected.merge({"action" => "parse"}), UsersController.last_parameters)
+      end
+    end
+
+    def with_test_routing(controller)
+      with_routing do |set|
+        set.draw do
+          match ':action', :to => controller
+        end
+        yield
+      end
+    end
+end
diff --git a/actionpack/test/dispatch/request/xml_params_parsing_test.rb b/actionpack/test/dispatch/request/xml_params_parsing_test.rb
index ad9de02eb4..38453dfe48 100644
--- a/actionpack/test/dispatch/request/xml_params_parsing_test.rb
+++ b/actionpack/test/dispatch/request/xml_params_parsing_test.rb
@@ -115,3 +115,41 @@ class LegacyXmlParamsParsingTest < XmlParamsParsingTest
       {'HTTP_X_POST_DATA_FORMAT' => 'xml'}
     end
 end
+
+class RootLessXmlParamsParsingTest < ActionDispatch::IntegrationTest
+  class TestController < ActionController::Base
+    wrap_parameters :person, :format => :xml
+
+    class << self
+      attr_accessor :last_request_parameters
+    end
+
+    def parse
+      self.class.last_request_parameters = request.request_parameters
+      head :ok
+    end
+  end
+
+  def teardown
+    TestController.last_request_parameters = nil
+  end
+
+  test "parses hash params" do
+    with_test_routing do
+      xml = "<name>David</name>"
+      post "/parse", xml, {'CONTENT_TYPE' => 'application/xml'}
+      assert_response :ok
+      assert_equal({"name" => "David", "person" => {"name" => "David"}}, TestController.last_request_parameters)
+    end
+  end
+
+  private
+    def with_test_routing
+      with_routing do |set|
+        set.draw do
+          match ':action', :to => ::RootLessXmlParamsParsingTest::TestController
+        end
+        yield
+      end
+    end
+end