Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Change ActionController::Parameters#require behavior when value is empty #9660

Merged
merged 1 commit into from

7 participants

@sebasoga

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.

@sebasoga sebasoga 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.
b3f894c
@tomav

Same problem here

@rafaelfranca
Owner

OMG! 8 months without review :cry:

I'll try to get this into 4.0.2

@guilleiguaran guilleiguaran merged commit c2b5a8e into rails:master
@robin850
Collaborator

@guilleiguaran : this should be backported to 4-0-stable no ?

@carlosantoniodasilva

Wouldn't this need a changelog entry?

@dhh
Owner
dhh commented

I don't like this. We don't need two different exceptions, with two different things to catch if you're doing your own exception handling, to signify such a small change. Just expand the default message to be "Either key is missing or the value is empty".

@guilleiguaran

@dhh agree, I'll work now changing this.

@guilleiguaran guilleiguaran referenced this pull request from a commit
@guilleiguaran guilleiguaran Revert "Merge pull request #9660 from sebasoga/change_strong_paramete…
…rs_require_behaviour"

This reverts commit c2b5a8e, reversing
changes made to 1918b12.

See: #9660 (comment)
f886fe2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 11, 2013
  1. @sebasoga

    Change ActionController::Parameters#require behavior when value is empty

    sebasoga authored
    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.
This page is out of date. Refresh to see the latest.
View
32 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 @@ def initialize(param) # :nodoc:
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 @@ def permit!
self
end
- # Ensures that a parameter is present. If it's present, returns
- # the parameter at the given +key+, otherwise raises an
- # <tt>ActionController::ParameterMissing</tt> 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 <tt>ActionController::EmptyParameter</tt> error, otherwise
+ # raises an <tt>ActionController::ParameterMissing</tt> 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.
View
3  actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
@@ -13,7 +13,8 @@ class ExceptionWrapper
'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
View
8 actionpack/test/controller/parameters/parameters_require_test.rb
@@ -2,8 +2,14 @@
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
View
19 actionpack/test/controller/required_params_test.rb
@@ -5,6 +5,11 @@ def create
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
View
6 actionpack/test/dispatch/debug_exceptions_test.rb
@@ -41,6 +41,8 @@ def call(env)
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 @@ def setup
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
Something went wrong with that request. Please try again.