Skip to content
This repository has been archived by the owner on Aug 17, 2017. It is now read-only.

Commit

Permalink
Implements require with multiple parameters
Browse files Browse the repository at this point in the history
Normal use is unchanged.

When used with multiple parameters the error raised in case a
parameter is missing lists all the missing parameters.

Returns the params hash if no required parameters are missing.
  • Loading branch information
Jonas Schubert Erlandsson committed Apr 17, 2013
1 parent c789f57 commit 7758a34
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 9 deletions.
10 changes: 10 additions & 0 deletions README.md
Expand Up @@ -70,6 +70,16 @@ This declaration whitelists the `name`, `emails` and `friends` attributes. It is

Thanks to Nick Kallen for the permit idea!

## Requiring multiple values

In some cases you might want to require several parameters to be present before proceeding, such as for an API where the params might be a flat hash. You can use `require` to do this:

``` ruby
params.require(:username, :password).permit(:version)

This comment has been minimized.

Copy link
@ajsharp

ajsharp Apr 17, 2013

My understanding of the require / permit syntax was that, if you do something like params.require(:post).permit(:title, :body), that the :title and :body params were permitted only within the :post key, e.g. {post: {title: 'Title', body: 'Body'}. It seems like your example here assumes that the version param will be passed along as a top-level param, along with username and password. Am I misunderstanding the API? Thanks.

This comment has been minimized.

Copy link
@dhh

dhh Apr 17, 2013

Member

@ajsharp, this is for when you don't have a root level like post. So imagine params: { username: x, password: y }. Instead of { post: { title: x, body: y } }.

This comment has been minimized.

Copy link
@fxn

fxn Apr 17, 2013

Member

Is the example plausible? In which situation do you get username and password parameters and want them to be filtered out?

This comment has been minimized.

Copy link
@ajsharp

ajsharp Apr 17, 2013

@ajsharp, this is for when you don't have a root level like post. So imagine params: { username: x, password: y }. Instead of { post: { title: x, body: y } }.

@dhh Right. My question was about how the permit call works in a case where you're requiring multiple root-level params. In the post example, permit seems to whitelist the title and body attributes nested within the post key. So first question: Is that indeed how the library behaves, where, if you do require(:key).permit(*keys), it will only whitelist those params on the required key, and not at the top-level?

And my second question is, in the example here -- params.require(:username, :password).permit(:version) -- the resulting behavior is that version is whitelisted at the top-level, along with username and password, right?

This comment has been minimized.

Copy link
@dhh

dhh Apr 17, 2013

Member

You're right, that actually is confusing. It doesn't know which element to go off.

@d-Pixie, any thoughts here? Otherwise this doesn't add up for me either.

This comment has been minimized.

Copy link
@fxn

fxn Apr 17, 2013

Member

@ajsharp The singular require returns the hash expected to be associated with the required key. The plural version implemented in this patch just returns self.

The method permit whitelists its receiver. So, the nested hash in the singular case, and params itself in the multiple case. Thus, in the example above :username and :password are filtered out. You'll get a hash whose only key is :version.

This comment has been minimized.

Copy link
@ajsharp

ajsharp Apr 18, 2013

@fxn Thanks much for the explanation. IMO that API behavior is a bit tricky, so it might be worth it to include a note in the readme about this.

Also, not to get too off topic, but IMO this particular quirk of the API fails the test of "least surprise". When I look at the public API, the first thought I have is "wait, how does that work"? Unfortunately, I don't have any ideas for how to improve it ;) Hopefully this feedback is helpful.

```

When used with multiple inputs `require` raises a `ActionController::MissingParameter` error with *all* the missing attributes specified in the message. If no values are missing the params hash is returned unmodified.

## Handling of Unpermitted Keys

By default parameter keys that are not explicitly permitted will be logged in the development and test environment. In other environments these parameters will simply be filtered out and ignored.
Expand Down
29 changes: 23 additions & 6 deletions lib/action_controller/parameters.rb
Expand Up @@ -22,7 +22,7 @@ class UnpermittedParameters < IndexError

def initialize(params)
@params = params
super("found unpermitted parameters: #{params.join(", ")}")
super("found unpermitted parameter(s): #{params.join(", ")}")
end
end

Expand Down Expand Up @@ -51,8 +51,12 @@ def permit!
self
end

def require(key)
self[key].presence || raise(ActionController::ParameterMissing.new(key))
def require(*keys)
if keys.many?
require_multiple(keys)
else
require_single(keys)
end
end

alias :required :require
Expand Down Expand Up @@ -98,6 +102,20 @@ def dup
end

protected
def require_single(keys)
key = keys.first
self[key].presence || raise(ActionController::ParameterMissing.new(key))
end

def require_multiple(keys)
missing_keys = keys.select do |key|
self[key].blank?
end

raise(ActionController::ParameterMissing.new(missing_keys.join(', '))) if missing_keys.compact.any?
self
end

def convert_value(value)
if value.class == Hash
self.class.new_from_hash_copying_default(value)
Expand All @@ -109,7 +127,6 @@ def convert_value(value)
end

private

def convert_hashes_to_parameters(key, value)
if value.is_a?(Parameters) || !value.is_a?(Hash)
value
Expand Down Expand Up @@ -217,7 +234,7 @@ def unpermitted_parameters!(params)
unpermitted_keys = unpermitted_keys(params)

if unpermitted_keys.any?
case self.class.action_on_unpermitted_parameters
case self.class.action_on_unpermitted_parameters
when :log
name = "unpermitted_parameters.action_controller"
ActiveSupport::Notifications.instrument(name, :keys => unpermitted_keys)
Expand All @@ -237,7 +254,7 @@ module StrongParameters

included do
rescue_from(ActionController::ParameterMissing) do |parameter_missing_exception|
render :text => "Required parameter missing: #{parameter_missing_exception.param}", :status => :bad_request
render :text => "Required parameter(s) missing: #{parameter_missing_exception.param}", :status => :bad_request
end
end

Expand Down
2 changes: 1 addition & 1 deletion test/action_controller_required_params_test.rb
Expand Up @@ -25,6 +25,6 @@ class ActionControllerRequiredParamsTest < ActionController::TestCase

test "missing parameters will be mentioned in the return" do
post :create, { :magazine => { :name => "Mjallo!" } }
assert_equal "Required parameter missing: book", response.body
assert_match "book", response.body
end
end
4 changes: 2 additions & 2 deletions test/log_on_unpermitted_params_test.rb
Expand Up @@ -16,7 +16,7 @@ def teardown
:fishing => "Turnips"
})

assert_logged("Unpermitted parameters: fishing") do
assert_logged("Unpermitted parameter(s): fishing") do
params.permit(:book => [:pages])
end
end
Expand All @@ -26,7 +26,7 @@ def teardown
:book => { :pages => 65, :title => "Green Cats and where to find then." }
})

assert_logged("Unpermitted parameters: title") do
assert_logged("Unpermitted parameter(s): title") do
params.permit(:book => [:pages])
end
end
Expand Down
25 changes: 25 additions & 0 deletions test/parameters_require_test.rb
Expand Up @@ -7,4 +7,29 @@ class ParametersRequireTest < ActiveSupport::TestCase
ActionController::Parameters.new(:person => {}).require(:person)
end
end

test "permit multiple required parameters" do
params = ActionController::Parameters.new(:username => 'user', :password => '<3<3<3<3')
assert_nothing_raised(ActionController::ParameterMissing) do
params.require(:username, :password)
end

assert params.has_key?(:username)
assert params.has_key?(:password)
end

test "multiple required parameters must be present not merely not nil" do
params = ActionController::Parameters.new(:username => '', :password => nil)
assert_raises(ActionController::ParameterMissing) do
params.require(:username, :password)
end
end

test "all parameters are returned after required with multiple parameters" do
params = ActionController::Parameters.new(:username => 'user', :password => '<3<3<3<3', :version => 1)

params.require(:username, :password)

assert params.has_key?(:version)

This comment has been minimized.

Copy link
@fxn

fxn Apr 17, 2013

Member

I think this test should check the value returned by the call to require.

This comment has been minimized.

Copy link
@d-Pixie

d-Pixie via email Apr 18, 2013

Contributor

This comment has been minimized.

Copy link
@fxn

fxn Apr 18, 2013

Member

I mean, the interface of require is its return value:

params.require(...).permit(...)

The suite has to check that the hash returned by require satisfies such and such. Granted, the implementation uses self, but if tomorrow you return self.dup the suite should still pass because the contract with the user has not changed.

This comment has been minimized.

Copy link
@d-Pixie

d-Pixie via email Apr 18, 2013

Contributor
end
end

0 comments on commit 7758a34

Please sign in to comment.