From b3f894c5282244b41221f98dfac5296cea5a4485 Mon Sep 17 00:00:00 2001 From: Sebastian Sogamoso Date: Mon, 11 Mar 2013 08:00:19 -0500 Subject: Change ActionController::Parameters#require behavior when value is empty When the value for the required key is empty an ActionController::ParameterMissing is raised which gets caught by ActionController::Base and turned into a 400 Bad Request reply with a message in the body saying the key is missing, which is misleading. With these changes, ActionController::EmptyParameter will be raised which ActionController::Base will catch and turn into a 400 Bad Request reply with a message in the body saying the key value is empty. --- .../action_controller/metal/strong_parameters.rb | 32 ++++++++++++++++------ .../middleware/exception_wrapper.rb | 3 +- .../parameters/parameters_require_test.rb | 8 +++++- actionpack/test/controller/required_params_test.rb | 19 +++++++++++++ actionpack/test/dispatch/debug_exceptions_test.rb | 6 ++++ 5 files changed, 57 insertions(+), 11 deletions(-) diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index acad8a0799..0748d75a69 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -9,8 +9,6 @@ module ActionController # params = ActionController::Parameters.new(a: {}) # params.fetch(:b) # # => ActionController::ParameterMissing: param not found: b - # params.require(:a) - # # => ActionController::ParameterMissing: param not found: a class ParameterMissing < KeyError attr_reader :param # :nodoc: @@ -20,6 +18,20 @@ module ActionController end end + # Raised when a required parameter value is empty. + # + # params = ActionController::Parameters.new(a: {}) + # params.require(:a) + # # => ActionController::EmptyParameter: value is empty for required key: a + class EmptyParameter < IndexError + attr_reader :param + + def initialize(param) + @param = param + super("value is empty for required key: #{param}") + end + end + # Raised when a supplied parameter is not expected. # # params = ActionController::Parameters.new(a: "123", b: "456") @@ -154,20 +166,22 @@ module ActionController self end - # Ensures that a parameter is present. If it's present, returns - # the parameter at the given +key+, otherwise raises an - # ActionController::ParameterMissing error. + # Ensures that a parameter is present. If it's present and not empty, + # returns the parameter at the given +key+, if it's empty raises + # an ActionController::EmptyParameter error, otherwise + # raises an ActionController::ParameterMissing error. # # ActionController::Parameters.new(person: { name: 'Francesco' }).require(:person) # # => {"name"=>"Francesco"} # - # ActionController::Parameters.new(person: nil).require(:person) - # # => ActionController::ParameterMissing: param not found: person - # # ActionController::Parameters.new(person: {}).require(:person) + # # => ActionController::EmptyParameter: value is empty for required key: person + # + # ActionController::Parameters.new(name: {}).require(:person) # # => ActionController::ParameterMissing: param not found: person def require(key) - self[key].presence || raise(ParameterMissing.new(key)) + raise(ActionController::ParameterMissing.new(key)) unless self.key?(key) + self[key].presence || raise(ActionController::EmptyParameter.new(key)) end # Alias of #require. diff --git a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb index 7489ce8028..af32a1f9d7 100644 --- a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb +++ b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb @@ -13,7 +13,8 @@ module ActionDispatch 'ActionController::UnknownFormat' => :not_acceptable, 'ActionController::InvalidAuthenticityToken' => :unprocessable_entity, 'ActionController::BadRequest' => :bad_request, - 'ActionController::ParameterMissing' => :bad_request + 'ActionController::ParameterMissing' => :bad_request, + 'ActionController::EmptyParameter' => :bad_request ) cattr_accessor :rescue_templates diff --git a/actionpack/test/controller/parameters/parameters_require_test.rb b/actionpack/test/controller/parameters/parameters_require_test.rb index bdaba8d2d8..21b3eaa6b5 100644 --- a/actionpack/test/controller/parameters/parameters_require_test.rb +++ b/actionpack/test/controller/parameters/parameters_require_test.rb @@ -2,8 +2,14 @@ require 'abstract_unit' require 'action_controller/metal/strong_parameters' class ParametersRequireTest < ActiveSupport::TestCase - test "required parameters must be present not merely not nil" do + test "required parameters must be present" do assert_raises(ActionController::ParameterMissing) do + ActionController::Parameters.new(name: {}).require(:person) + end + end + + test "required parameters can't be blank" do + assert_raises(ActionController::EmptyParameter) do ActionController::Parameters.new(person: {}).require(:person) end end diff --git a/actionpack/test/controller/required_params_test.rb b/actionpack/test/controller/required_params_test.rb index 343d57c300..b27069140b 100644 --- a/actionpack/test/controller/required_params_test.rb +++ b/actionpack/test/controller/required_params_test.rb @@ -5,6 +5,11 @@ class BooksController < ActionController::Base params.require(:book).require(:name) head :ok end + + def update + params.require(:book) + head :ok + end end class ActionControllerRequiredParamsTest < ActionController::TestCase @@ -20,6 +25,20 @@ class ActionControllerRequiredParamsTest < ActionController::TestCase end end + test "empty required parameters will raise an exception" do + assert_raise ActionController::EmptyParameter do + put :update, {book: {}} + end + + assert_raise ActionController::EmptyParameter do + put :update, {book: ''} + end + + assert_raise ActionController::EmptyParameter do + put :update, {book: nil} + end + end + test "required parameters that are present will not raise" do post :create, { book: { name: "Mjallo!" } } assert_response :ok diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb index 6035f0361e..9fafc3a0e9 100644 --- a/actionpack/test/dispatch/debug_exceptions_test.rb +++ b/actionpack/test/dispatch/debug_exceptions_test.rb @@ -41,6 +41,8 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest raise ActionController::UrlGenerationError, "No route matches" when "/parameter_missing" raise ActionController::ParameterMissing, :missing_param_key + when "/required_key_empty_value" + raise ActionController::EmptyParameter, :empty_param_key else raise "puke!" end @@ -120,6 +122,10 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest get "/parameter_missing", {}, {'action_dispatch.show_exceptions' => true} assert_response 400 assert_match(/ActionController::ParameterMissing/, body) + + get "/required_key_empty_value", {}, {'action_dispatch.show_exceptions' => true} + assert_response 400 + assert_match(/ActionController::EmptyParameter/, body) end test "does not show filtered parameters" do -- cgit v1.2.3