Skip to content
This repository

Allow multiple keys in call to require #117

Merged
merged 1 commit into from about 1 year ago

6 participants

Jonas Schubert Erlandsson James Brennan David Heinemeier Hansson Xavier Noria Alex Sharp Nathan Lilienthal
Jonas Schubert Erlandsson

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.

James Brennan
jpb commented March 22, 2013

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!
51 51
       self
52 52
     end
53 53
 
54  
-    def require(key)
55  
-      self[key].presence || raise(ActionController::ParameterMissing.new(key))
  54
+    def require(*keys)
  55
+      if keys.length > 1
  56
+        require_multi(keys)
1
David Heinemeier Hansson Owner
dhh added a note March 24, 2013

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!
51 51
       self
52 52
     end
53 53
 
54  
-    def require(key)
55  
-      self[key].presence || raise(ActionController::ParameterMissing.new(key))
  54
+    def require(*keys)
  55
+      if keys.length > 1
  56
+        require_multi(keys)
  57
+      else
  58
+        key = keys.first 
1
David Heinemeier Hansson Owner
dhh added a note March 24, 2013

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!
51 51
       self
52 52
     end
53 53
 
54  
-    def require(key)
55  
-      self[key].presence || raise(ActionController::ParameterMissing.new(key))
  54
+    def require(*keys)
  55
+      if keys.length > 1
1
David Heinemeier Hansson Owner
dhh added a note March 24, 2013

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
98 103
     end
99 104
 
100 105
     protected
  106
+
  107
+      def require_multi(keys)
  108
+        missing_keys = keys.select do |key|
  109
+          not self[key].presence
1
David Heinemeier Hansson Owner
dhh added a note March 24, 2013

Use #blank?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
David Heinemeier Hansson
Owner
dhh commented March 24, 2013

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

Jonas Schubert Erlandsson

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.

Jonas Schubert Erlandsson

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
98 102
     end
99 103
 
100 104
     protected
  105
+
  106
+      def require_single(keys)
  107
+        key = keys.first 
  108
+        self[key].presence || raise(ActionController::ParameterMissing.new(key))
  109
+      end
  110
+
  111
+      def require_multiple(keys)
  112
+        missing_keys = keys.select do |key|
  113
+          self[key].blank?
  114
+        end
  115
+
  116
+        missing_keys.compact.empty? || raise(ActionController::ParameterMissing.new(missing_keys.join(', ')))
1
David Heinemeier Hansson Owner
dhh added a note March 27, 2013

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
David Heinemeier Hansson dhh commented on the diff March 27, 2013
lib/action_controller/parameters.rb
... ...
@@ -98,6 +102,21 @@ def dup
98 102
     end
99 103
 
100 104
     protected
  105
+
1
David Heinemeier Hansson Owner
dhh added a note March 27, 2013

Nix blank line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
David Heinemeier Hansson
Owner
dhh commented March 27, 2013

Two more small issues, then good to go.

Jonas Schubert Erlandsson

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
Jonas Schubert Erlandsson

Rebased again ...

David Heinemeier Hansson dhh merged commit 59f3365 into from April 17, 2013
David Heinemeier Hansson dhh closed this April 17, 2013
David Heinemeier Hansson
Owner
dhh commented April 17, 2013

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

Alex Sharp

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.

Xavier Noria

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.

Jonas Schubert Erlandsson

@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 ;)

Xavier Noria
Owner
fxn commented April 18, 2013

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?

Jonas Schubert Erlandsson
David Heinemeier Hansson
Owner
dhh commented April 18, 2013

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.

Xavier Noria
Owner
fxn commented April 18, 2013
Jonas Schubert Erlandsson

Ok

Alex Sharp

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

Nathan Lilienthal

:+1: Just what I need.

Taavo Smith taavo referenced this pull request in josevalim/inherited_resources May 19, 2013
Merged

Integrate with strong_parameters #260

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

Showing 1 unique commit by 1 author.

Apr 17, 2013
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
This page is out of date. Refresh to see the latest.
10  README.md
Source Rendered
@@ -70,6 +70,16 @@ This declaration whitelists the `name`, `emails` and `friends` attributes. It is
70 70
 
71 71
 Thanks to Nick Kallen for the permit idea!
72 72
 
  73
+## Requiring multiple values
  74
+
  75
+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:
  76
+
  77
+``` ruby
  78
+params.require(:username, :password).permit(:version)
  79
+```
  80
+
  81
+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.
  82
+
73 83
 ## Handling of Unpermitted Keys
74 84
 
75 85
 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.
29  lib/action_controller/parameters.rb
@@ -22,7 +22,7 @@ class UnpermittedParameters < IndexError
22 22
 
23 23
     def initialize(params)
24 24
       @params = params
25  
-      super("found unpermitted parameters: #{params.join(", ")}")
  25
+      super("found unpermitted parameter(s): #{params.join(", ")}")
26 26
     end
27 27
   end
28 28
 
@@ -51,8 +51,12 @@ def permit!
51 51
       self
52 52
     end
53 53
 
54  
-    def require(key)
55  
-      self[key].presence || raise(ActionController::ParameterMissing.new(key))
  54
+    def require(*keys)
  55
+      if keys.many?
  56
+        require_multiple(keys)
  57
+      else
  58
+        require_single(keys)
  59
+      end
56 60
     end
57 61
 
58 62
     alias :required :require
@@ -98,6 +102,20 @@ def dup
98 102
     end
99 103
 
100 104
     protected
  105
+      def require_single(keys)
  106
+        key = keys.first 
  107
+        self[key].presence || raise(ActionController::ParameterMissing.new(key))
  108
+      end
  109
+
  110
+      def require_multiple(keys)
  111
+        missing_keys = keys.select do |key|
  112
+          self[key].blank?
  113
+        end
  114
+
  115
+        raise(ActionController::ParameterMissing.new(missing_keys.join(', '))) if missing_keys.compact.any?
  116
+        self
  117
+      end
  118
+
101 119
       def convert_value(value)
102 120
         if value.class == Hash
103 121
           self.class.new_from_hash_copying_default(value)
@@ -109,7 +127,6 @@ def convert_value(value)
109 127
       end
110 128
 
111 129
     private
112  
-
113 130
       def convert_hashes_to_parameters(key, value)
114 131
         if value.is_a?(Parameters) || !value.is_a?(Hash)
115 132
           value
@@ -217,7 +234,7 @@ def unpermitted_parameters!(params)
217 234
         unpermitted_keys = unpermitted_keys(params)
218 235
 
219 236
         if unpermitted_keys.any?  
220  
-          case self.class.action_on_unpermitted_parameters  
  237
+          case self.class.action_on_unpermitted_parameters
221 238
           when :log
222 239
             name = "unpermitted_parameters.action_controller"
223 240
             ActiveSupport::Notifications.instrument(name, :keys => unpermitted_keys)
@@ -237,7 +254,7 @@ module StrongParameters
237 254
 
238 255
     included do
239 256
       rescue_from(ActionController::ParameterMissing) do |parameter_missing_exception|
240  
-        render :text => "Required parameter missing: #{parameter_missing_exception.param}", :status => :bad_request
  257
+        render :text => "Required parameter(s) missing: #{parameter_missing_exception.param}", :status => :bad_request
241 258
       end
242 259
     end
243 260
 
2  test/action_controller_required_params_test.rb
@@ -25,6 +25,6 @@ class ActionControllerRequiredParamsTest < ActionController::TestCase
25 25
 
26 26
   test "missing parameters will be mentioned in the return" do
27 27
     post :create, { :magazine => { :name => "Mjallo!" } }
28  
-    assert_equal "Required parameter missing: book", response.body
  28
+    assert_match "book", response.body
29 29
   end
30 30
 end
4  test/log_on_unpermitted_params_test.rb
@@ -16,7 +16,7 @@ def teardown
16 16
       :fishing => "Turnips"
17 17
     })
18 18
 
19  
-    assert_logged("Unpermitted parameters: fishing") do
  19
+    assert_logged("Unpermitted parameter(s): fishing") do
20 20
       params.permit(:book => [:pages])
21 21
     end
22 22
   end
@@ -26,7 +26,7 @@ def teardown
26 26
       :book => { :pages => 65, :title => "Green Cats and where to find then." }
27 27
     })
28 28
 
29  
-    assert_logged("Unpermitted parameters: title") do
  29
+    assert_logged("Unpermitted parameter(s): title") do
30 30
       params.permit(:book => [:pages])
31 31
     end
32 32
   end
25  test/parameters_require_test.rb
@@ -7,4 +7,29 @@ class ParametersRequireTest < ActiveSupport::TestCase
7 7
       ActionController::Parameters.new(:person => {}).require(:person)
8 8
     end
9 9
   end
  10
+
  11
+  test "permit multiple required parameters" do
  12
+    params = ActionController::Parameters.new(:username => 'user', :password => '<3<3<3<3')
  13
+    assert_nothing_raised(ActionController::ParameterMissing) do
  14
+      params.require(:username, :password)
  15
+    end
  16
+
  17
+    assert params.has_key?(:username)
  18
+    assert params.has_key?(:password)
  19
+  end
  20
+
  21
+  test "multiple required parameters must be present not merely not nil" do
  22
+    params = ActionController::Parameters.new(:username => '', :password => nil)
  23
+    assert_raises(ActionController::ParameterMissing) do
  24
+      params.require(:username, :password)
  25
+    end
  26
+  end
  27
+
  28
+  test "all parameters are returned after required with multiple parameters" do
  29
+    params = ActionController::Parameters.new(:username => 'user', :password => '<3<3<3<3', :version => 1)
  30
+
  31
+    params.require(:username, :password)
  32
+
  33
+    assert params.has_key?(:version)
  34
+  end
10 35
 end
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.