Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Allow multiple keys in call to require #117

Merged
merged 1 commit into from

6 participants

@d-Pixie

The current version of require takes only one key and returns the value for that key (or raises if it's not present). I added code to support calls like this:

params = ActionController::Parameters.new(:username => 'user', :password => '<3<3<3<3', :version => 1)
params.require(:username, :password)
# => {"username"=>"user", "password"=>"<3<3<3<3", "version"=>1}

It still throws if the required parameters are missing, including all the missing keys:

params = ActionController::Parameters.new(:version => 1)
params.require(:username, :password)
# => "Required parameter(s) missing: username, password"

Since it's not reasonable to return the keys values in this case the method simply returns self instead. This allows for other methods to be chained as expected.

Basically it allows you to do:

params.require(:user, :post).permit(:user => [:name], :post => [:content])

instead of:

params.require(:user).permit(:name)
params.require(:post).permit(:content)

And for the cases where you don't have the normal, nested hashes (such as in my case of an API accepting a non nested list of parameters) you won't have to do one call to permit for every required parameter in the API call.

I don't know if this is interesting for inclusion, it's not really an edge case, but not the most common case either, nor if the approach presented here is the best/a good enough one ... So comments on the code are most welcome.

@jpb

I could really use this feature. The way require() works right now there is no way do catch ActionController::ParameterMissing and display an error message that lists all the required parameters.

lib/action_controller/parameters.rb
@@ -51,8 +51,13 @@ def permit!
self
end
- def require(key)
- self[key].presence || raise(ActionController::ParameterMissing.new(key))
+ def require(*keys)
+ if keys.length > 1
+ require_multi(keys)
@dhh Owner
dhh added a note

Use require_multiple as method name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/action_controller/parameters.rb
@@ -51,8 +51,13 @@ def permit!
self
end
- def require(key)
- self[key].presence || raise(ActionController::ParameterMissing.new(key))
+ def require(*keys)
+ if keys.length > 1
+ require_multi(keys)
+ else
+ key = keys.first
@dhh Owner
dhh added a note

Extract these two lines into require_single(key) such that require itself is a Composed Method at the same level of abstraction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/action_controller/parameters.rb
@@ -51,8 +51,13 @@ def permit!
self
end
- def require(key)
- self[key].presence || raise(ActionController::ParameterMissing.new(key))
+ def require(*keys)
+ if keys.length > 1
@dhh Owner
dhh added a note

use keys.many?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/action_controller/parameters.rb
@@ -98,6 +103,16 @@ def dup
end
protected
+
+ def require_multi(keys)
+ missing_keys = keys.select do |key|
+ not self[key].presence
@dhh Owner
dhh added a note

Use #blank?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dhh
Owner

Looks good to me. Added a few review notes. Also, needs a rebase.

@d-Pixie

Fixed all the issues mentioned by David (thank you for the pointers), squashed my commits into one and rebased of current master.Also added a short section in the README about the feature.

@d-Pixie

CI is failing, for some cases, but with an error: NameError: uninitialized constant Mocha::Integration. I haven't used Travis myself (guard for unit tests and a manual rspec before committing has sufficed so far), is this something I should fix?

lib/action_controller/parameters.rb
@@ -98,6 +102,21 @@ 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
+
+ missing_keys.compact.empty? || raise(ActionController::ParameterMissing.new(missing_keys.join(', ')))
@dhh Owner
dhh added a note

Change this to:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dhh dhh commented on the diff
lib/action_controller/parameters.rb
@@ -98,6 +102,21 @@ def dup
end
protected
+
@dhh Owner
dhh added a note

Nix blank line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dhh
Owner

Two more small issues, then good to go.

@d-Pixie

Rebased and updated. Sorry for the delay, I have been sick for two weeks :/

Jonas Schubert Erlandsson Implements require with multiple parameters
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.
7758a34
@d-Pixie

Rebased again ...

@dhh dhh merged commit 59f3365 into rails:master
@dhh
Owner

Can you submit a PR for rails/master as well with the same behavior?

@ajsharp

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.

Owner

@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 } }.

Owner

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

@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?

Owner

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.

Owner

@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.

@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.

@fxn

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

Owner

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.

@d-Pixie

@dhh, @ajsharp As @fxn pointed out require with multiple keys returns self if they are all present. It can't work, sanely, like to single key version since you would then have to glob the nested hashes together and that might clobber keys etc.
I tried to make this clear in the initial examples, but I must have gone awry somewhere.

My initial case for this was that I picked up the gem for an API implementation and looked at the docs quickly then went: params.require(:username, :password) and it blew up in my face ;) It seamed like it should work. After looking at the then implementation of require I realized that the semantics for the single and multi key versions could not be the same, but neither are the use cases.

So for normal rails use you would only use the single key version anyway, when was the last time you posted two models in a form and not in a nested relationship? This then while still providing a sane multi key implementation.

So, to further explain, the single key version works the same as ever. The multi key version will raise if any of the keys are missing and include all the missing keys in the error or return the hash, unmodified, if all the required keys are there.

If you have {username: "user", password: "<3<3<3", version: "1"} and require(:username, :password) you would get the original hash back that you can then run permit(:version) on.

I agree that the semantics here are not crystal. Maybe this would then be better as a new function to reduce the confusion? But on the other hand, and what made me go with this, it's a decent semantic for a multi key require, it's just not in line with the single key one ;)

@fxn
Owner

It is a little surprising to me that you have to permit what you just required. Do you believe the example in the CHANGELOG is plausible?

@d-Pixie
@dhh
Owner

This is way too unclear for me. I made a mistake merging. I'll revert. I don't think this confusing use case is something we need to support in the official gem. If you have this requirement, you can just implement it on your own in an app. It's not like strong params makes that any harder.

@fxn
Owner
@d-Pixie

Ok

@ajsharp

Ignore my last comment, just saw this, thx @dhh @fxn

@nixpulvis

:+1: Just what I need.

@taavo taavo referenced this pull request in josevalim/inherited_resources
Merged

Integrate with strong_parameters #260

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 17, 2013
  1. Implements require with multiple parameters

    Jonas Schubert Erlandsson authored
    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.
This page is out of date. Refresh to see the latest.
10 README.md
@@ -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)
+```
+
+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.
View
29 lib/action_controller/parameters.rb
@@ -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
@@ -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
@@ -98,6 +102,20 @@ def dup
end
protected
+ def require_single(keys)
@dhh Owner
dhh added a note

Nix blank line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ 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)
@@ -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
@@ -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)
@@ -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
View
2  test/action_controller_required_params_test.rb
@@ -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
View
4 test/log_on_unpermitted_params_test.rb
@@ -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
@@ -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
View
25 test/parameters_require_test.rb
@@ -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)
+ end
end
Something went wrong with that request. Please try again.