New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] params.require accepts array of parameters that should be present or raise error #19565

Merged
merged 1 commit into from Aug 28, 2015

Conversation

Projects
None yet
@gaurish
Contributor

gaurish commented Mar 28, 2015

This PR adds a method require_all which allows you to require multiple values in one method.

for example, in our app. I have a person object which MUST have first_name & last_name. so to do this in current code, I have to write this:

params.require(:person).require(:first_name)
params.require(:person).require(:last_name)

Here it will be one line for each params, so say if I require 10params, it will be 10lines of repeated code which is not DRY. So I propose a method which does this in one line:

params.require(:person).require_all(:first_name, :last_name)

Comments welcome

@arunagw arunagw added the actionpack label Mar 28, 2015

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Mar 30, 2015

Member

We have similar use cases at Shopify.

thoughts on the API @dhh @rafaelfranca

Member

arthurnn commented Mar 30, 2015

We have similar use cases at Shopify.

thoughts on the API @dhh @rafaelfranca

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 30, 2015

Member

I like #require(*keys). If you ask for one key, you just get that value directly. If you ask for multiple keys, you get an array of values. This of course means that params.require(:two, :things).permit won't work, since permit only works on a hash. But imo that's fine.

Member

dhh commented Mar 30, 2015

I like #require(*keys). If you ask for one key, you just get that value directly. If you ask for multiple keys, you get an array of values. This of course means that params.require(:two, :things).permit won't work, since permit only works on a hash. But imo that's fine.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Mar 30, 2015

Contributor

What about require(one: { :thing1, :thing2 }) syntax? Then we can combine it with permit (by returning a hash-like object that responds to permit):

require(one: { :thing1, :thing2 }).permit(:thing3)

It's up to debate whether using this syntax should implicitly permit thing1 and thing2 to avoid having to repeat them in the permit part, but technical implementation could go either way.

Eugene

On Mar 30, 2015, at 8:39 AM, David Heinemeier Hansson notifications@github.com wrote:

I like #require(*keys). If you ask for one key, you just get that value directly. If you ask for multiple keys, you get an array of values. This of course means that params.require(:two, :things).permit won't work, since permit only works on a hash. But imo that's fine.


Reply to this email directly or view it on GitHub.

Contributor

egilburg commented Mar 30, 2015

What about require(one: { :thing1, :thing2 }) syntax? Then we can combine it with permit (by returning a hash-like object that responds to permit):

require(one: { :thing1, :thing2 }).permit(:thing3)

It's up to debate whether using this syntax should implicitly permit thing1 and thing2 to avoid having to repeat them in the permit part, but technical implementation could go either way.

Eugene

On Mar 30, 2015, at 8:39 AM, David Heinemeier Hansson notifications@github.com wrote:

I like #require(*keys). If you ask for one key, you just get that value directly. If you ask for multiple keys, you get an array of values. This of course means that params.require(:two, :things).permit won't work, since permit only works on a hash. But imo that's fine.


Reply to this email directly or view it on GitHub.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Mar 30, 2015

Contributor

Typo, meant require(one: [ :thing1, :thing2 ]).permit(:thing3)

For single value could also accept shortcut syntax of require(one: :thing1).permit(:thing2)

Eugene

On Mar 30, 2015, at 9:11 AM, Eugene Gilburg eugene.gilburg@gmail.com wrote:

What about require(one: { :thing1, :thing2 }) syntax? Then we can combine it with permit (by returning a hash-like object that responds to permit):

require(one: { :thing1, :thing2 }).permit(:thing3)

It's up to debate whether using this syntax should implicitly permit thing1 and thing2 to avoid having to repeat them in the permit part, but technical implementation could go either way.

Eugene

On Mar 30, 2015, at 8:39 AM, David Heinemeier Hansson notifications@github.com wrote:

I like #require(*keys). If you ask for one key, you just get that value directly. If you ask for multiple keys, you get an array of values. This of course means that params.require(:two, :things).permit won't work, since permit only works on a hash. But imo that's fine.


Reply to this email directly or view it on GitHub.

Contributor

egilburg commented Mar 30, 2015

Typo, meant require(one: [ :thing1, :thing2 ]).permit(:thing3)

For single value could also accept shortcut syntax of require(one: :thing1).permit(:thing2)

Eugene

On Mar 30, 2015, at 9:11 AM, Eugene Gilburg eugene.gilburg@gmail.com wrote:

What about require(one: { :thing1, :thing2 }) syntax? Then we can combine it with permit (by returning a hash-like object that responds to permit):

require(one: { :thing1, :thing2 }).permit(:thing3)

It's up to debate whether using this syntax should implicitly permit thing1 and thing2 to avoid having to repeat them in the permit part, but technical implementation could go either way.

Eugene

On Mar 30, 2015, at 8:39 AM, David Heinemeier Hansson notifications@github.com wrote:

I like #require(*keys). If you ask for one key, you just get that value directly. If you ask for multiple keys, you get an array of values. This of course means that params.require(:two, :things).permit won't work, since permit only works on a hash. But imo that's fine.


Reply to this email directly or view it on GitHub.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 30, 2015

Member

I don't want to mix require and permit semantics. Require means return the
value, raise if its not there. Permit means, return key in a hash, no
raising.

On Mon, Mar 30, 2015 at 6:16 PM, Eugene Gilburg notifications@github.com
wrote:

Typo, meant require(one: [ :thing1, :thing2 ]).permit(:thing3)

For single value could also accept shortcut syntax of require(one:
:thing1).permit(:thing2)

Eugene

On Mar 30, 2015, at 9:11 AM, Eugene Gilburg eugene.gilburg@gmail.com
wrote:

What about require(one: { :thing1, :thing2 }) syntax? Then we can
combine it with permit (by returning a hash-like object that responds to
permit):

require(one: { :thing1, :thing2 }).permit(:thing3)

It's up to debate whether using this syntax should implicitly permit
thing1 and thing2 to avoid having to repeat them in the permit part, but
technical implementation could go either way.

Eugene

On Mar 30, 2015, at 8:39 AM, David Heinemeier Hansson <
notifications@github.com> wrote:

I like #require(*keys). If you ask for one key, you just get that value
directly. If you ask for multiple keys, you get an array of values. This of
course means that params.require(:two, :things).permit won't work, since
permit only works on a hash. But imo that's fine.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#19565 (comment).

Member

dhh commented Mar 30, 2015

I don't want to mix require and permit semantics. Require means return the
value, raise if its not there. Permit means, return key in a hash, no
raising.

On Mon, Mar 30, 2015 at 6:16 PM, Eugene Gilburg notifications@github.com
wrote:

Typo, meant require(one: [ :thing1, :thing2 ]).permit(:thing3)

For single value could also accept shortcut syntax of require(one:
:thing1).permit(:thing2)

Eugene

On Mar 30, 2015, at 9:11 AM, Eugene Gilburg eugene.gilburg@gmail.com
wrote:

What about require(one: { :thing1, :thing2 }) syntax? Then we can
combine it with permit (by returning a hash-like object that responds to
permit):

require(one: { :thing1, :thing2 }).permit(:thing3)

It's up to debate whether using this syntax should implicitly permit
thing1 and thing2 to avoid having to repeat them in the permit part, but
technical implementation could go either way.

Eugene

On Mar 30, 2015, at 8:39 AM, David Heinemeier Hansson <
notifications@github.com> wrote:

I like #require(*keys). If you ask for one key, you just get that value
directly. If you ask for multiple keys, you get an array of values. This of
course means that params.require(:two, :things).permit won't work, since
permit only works on a hash. But imo that's fine.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#19565 (comment).

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Mar 30, 2015

Member

I like #require(*keys). If you ask for one key, you just get that value directly. If you ask for multiple keys, you get an array of values.

I don't know if I am reading it right, is it something along the lines of...

def require(*keys)
  if keys.length == 1
    # return value directly
  else
    # return array of values
  end
end

If that's the case, then I have a concern. The problem is you can't distinguish between "I'm asking for a single value" or "I'm passing an array that happens to contain only a single value". This makes it very annoying to use if you need to pass arrays with unknown number of elements:

params.require( *REQUIRED_KEYS ) # might return a single value, or perhaps an array

This is a problem when you extract these things into constants, or when you moving these code into marcos/helpers that you reuse across controllers, or when writing library code. I'd much prefer:

def require(keys)
  if Array === keys
    keys.map &method(:require)
  else
    # return single value
  end
end

So you'd have to call it by either params.require( :thing ) or params.require( [ :multiple, :things ] ) (or make those two separate methods as in the original proposal).

Member

chancancode commented Mar 30, 2015

I like #require(*keys). If you ask for one key, you just get that value directly. If you ask for multiple keys, you get an array of values.

I don't know if I am reading it right, is it something along the lines of...

def require(*keys)
  if keys.length == 1
    # return value directly
  else
    # return array of values
  end
end

If that's the case, then I have a concern. The problem is you can't distinguish between "I'm asking for a single value" or "I'm passing an array that happens to contain only a single value". This makes it very annoying to use if you need to pass arrays with unknown number of elements:

params.require( *REQUIRED_KEYS ) # might return a single value, or perhaps an array

This is a problem when you extract these things into constants, or when you moving these code into marcos/helpers that you reuse across controllers, or when writing library code. I'd much prefer:

def require(keys)
  if Array === keys
    keys.map &method(:require)
  else
    # return single value
  end
end

So you'd have to call it by either params.require( :thing ) or params.require( [ :multiple, :things ] ) (or make those two separate methods as in the original proposal).

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 30, 2015

Member

Happy with the latter options too. IMO, require(array) is the minority
concern.

On Mon, Mar 30, 2015 at 7:18 PM, Godfrey Chan notifications@github.com
wrote:

I like #require(*keys). If you ask for one key, you just get that value
directly. If you ask for multiple keys, you get an array of values.

I don't know if I am reading it right, is it something along the lines
of...

def require(*keys)
if keys.length == 1
# return value directly
else
# return array of values
endend

If that's the case, then I have a concern. The problem is you can't
distinguish between "I'm asking for a single value" or "I'm passing an
array that happens to contain only a single value". This makes it very
annoying to use if you need to pass arrays with unknown number of elements:

params.require( *REQUIRED_KEYS ) # might return a single value, or perhaps an array

This is a problem when you extract these things into constants, or when
you moving these code into marcos/helpers that you reuse across
controllers, or when writing library code. I'd much prefer:

def require(keys)
if Array === keys
keys.map &method(:require)
else
# return single value
endend

So you'd have to call it by either params.require( :thing ) or params.require(
[ :multiple, :things ] ) (or make those two separate methods as in the
original proposal).


Reply to this email directly or view it on GitHub
#19565 (comment).

Member

dhh commented Mar 30, 2015

Happy with the latter options too. IMO, require(array) is the minority
concern.

On Mon, Mar 30, 2015 at 7:18 PM, Godfrey Chan notifications@github.com
wrote:

I like #require(*keys). If you ask for one key, you just get that value
directly. If you ask for multiple keys, you get an array of values.

I don't know if I am reading it right, is it something along the lines
of...

def require(*keys)
if keys.length == 1
# return value directly
else
# return array of values
endend

If that's the case, then I have a concern. The problem is you can't
distinguish between "I'm asking for a single value" or "I'm passing an
array that happens to contain only a single value". This makes it very
annoying to use if you need to pass arrays with unknown number of elements:

params.require( *REQUIRED_KEYS ) # might return a single value, or perhaps an array

This is a problem when you extract these things into constants, or when
you moving these code into marcos/helpers that you reuse across
controllers, or when writing library code. I'd much prefer:

def require(keys)
if Array === keys
keys.map &method(:require)
else
# return single value
endend

So you'd have to call it by either params.require( :thing ) or params.require(
[ :multiple, :things ] ) (or make those two separate methods as in the
original proposal).


Reply to this email directly or view it on GitHub
#19565 (comment).

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Mar 30, 2015

Member

Agreed. params.require( :thing ) is probably still the most popular thing you need to do, so that should get the nice API regardless of what we pick for the array case.

In theory, the array thing allows you to write smart things like this:

first_name, last_name = params.require([ :first_name, :last_name ])

But I don't think you can find too many excuses to use that in practice :)

Member

chancancode commented Mar 30, 2015

Agreed. params.require( :thing ) is probably still the most popular thing you need to do, so that should get the nice API regardless of what we pick for the array case.

In theory, the array thing allows you to write smart things like this:

first_name, last_name = params.require([ :first_name, :last_name ])

But I don't think you can find too many excuses to use that in practice :)

@litch

This comment has been minimized.

Show comment
Hide comment
@litch

litch Mar 30, 2015

I actually just implemented something similar in a project of mine and have been debating where to generalize it to. The issue that I see is that this is really a mixing of concerns. If you're doing parameter validation (which is what require_all, and REALLY what require is as well, then you should be able to do type checks). The calling semantics I would reach for next after being able to do require_all would be:

first_name, last_name, addresses = params.require( first_name: String, last_name: String, addresses: Array)

But that obviously turns into a mess in a hurry, and doesn't seem to me like it should be in strong params.

permit and require just seem like such different concerns to me, and start down such different paths, that it seems like it would be a bad idea to add this.

litch commented Mar 30, 2015

I actually just implemented something similar in a project of mine and have been debating where to generalize it to. The issue that I see is that this is really a mixing of concerns. If you're doing parameter validation (which is what require_all, and REALLY what require is as well, then you should be able to do type checks). The calling semantics I would reach for next after being able to do require_all would be:

first_name, last_name, addresses = params.require( first_name: String, last_name: String, addresses: Array)

But that obviously turns into a mess in a hurry, and doesn't seem to me like it should be in strong params.

permit and require just seem like such different concerns to me, and start down such different paths, that it seems like it would be a bad idea to add this.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 30, 2015

Member

Yup, fine with that extension. Agree it's the minority case.

On Mon, Mar 30, 2015 at 7:26 PM, Godfrey Chan notifications@github.com
wrote:

Agreed. params.require( :thing ) is probably still the most popular thing
you need to do, so that should get the nice API regardless of what we pick
for the array case.

In theory, the array thing allows you to write smart things like this:

first_name, last_name = params.require([ :first_name, :last_name ])

But I don't think you can find too many excuses to use that in practice :)


Reply to this email directly or view it on GitHub
#19565 (comment).

Member

dhh commented Mar 30, 2015

Yup, fine with that extension. Agree it's the minority case.

On Mon, Mar 30, 2015 at 7:26 PM, Godfrey Chan notifications@github.com
wrote:

Agreed. params.require( :thing ) is probably still the most popular thing
you need to do, so that should get the nice API regardless of what we pick
for the array case.

In theory, the array thing allows you to write smart things like this:

first_name, last_name = params.require([ :first_name, :last_name ])

But I don't think you can find too many excuses to use that in practice :)


Reply to this email directly or view it on GitHub
#19565 (comment).

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Mar 30, 2015

Contributor
def require(thing, *things)
  things += thing if things.any?

  ...
end

A bit clunkier implementation but allows mainitaing the syntax.

Contributor

egilburg commented Mar 30, 2015

def require(thing, *things)
  things += thing if things.any?

  ...
end

A bit clunkier implementation but allows mainitaing the syntax.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 30, 2015

Member

I'd rather be explicit. If you want an array of values, supply an array of keys.

On Mar 30, 2015, at 19:36, Eugene Gilburg notifications@github.com wrote:

def require(thing, *things)
things += thing if things.any?

...
end
A bit clunkier implementation but allows mainitaing the syntax.


Reply to this email directly or view it on GitHub.

Member

dhh commented Mar 30, 2015

I'd rather be explicit. If you want an array of values, supply an array of keys.

On Mar 30, 2015, at 19:36, Eugene Gilburg notifications@github.com wrote:

def require(thing, *things)
things += thing if things.any?

...
end
A bit clunkier implementation but allows mainitaing the syntax.


Reply to this email directly or view it on GitHub.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Apr 1, 2015

Member

/cc @fxn

Member

senny commented Apr 1, 2015

/cc @fxn

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Apr 6, 2015

Member

I think there are two design drivers in conflict here.

In my mind, strong parameters is params whitelisting for security purposes. Applications declare what they accept, and strong parameters discards what doesn't match. Support for permitted scalars was motivated by past issues in which a hash could be injected where a string was assumed.

Then we have a second pulling force: request validation. This overlaps with model validation in most use cases of idiomatic Rails applications. For me, strong parameters doesn't address this problem, and in that sense require has always felt a little off to me.

Why add support to declare multiple required keys? Then, what about arbitrarily nested required keys? Presence is just one of many possible validations, what about limits on array size? Regexps for strings? Ranges for integers? Size for file uploads?

Personally, I wouldn't go in the direction of this PR because strong parameters is not about request validation.

Member

fxn commented Apr 6, 2015

I think there are two design drivers in conflict here.

In my mind, strong parameters is params whitelisting for security purposes. Applications declare what they accept, and strong parameters discards what doesn't match. Support for permitted scalars was motivated by past issues in which a hash could be injected where a string was assumed.

Then we have a second pulling force: request validation. This overlaps with model validation in most use cases of idiomatic Rails applications. For me, strong parameters doesn't address this problem, and in that sense require has always felt a little off to me.

Why add support to declare multiple required keys? Then, what about arbitrarily nested required keys? Presence is just one of many possible validations, what about limits on array size? Regexps for strings? Ranges for integers? Size for file uploads?

Personally, I wouldn't go in the direction of this PR because strong parameters is not about request validation.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Apr 6, 2015

Member

They are indeed separate concerns, but they're composing a whole that means
"get these params ready for model consumption". Whitelisting is the primary
element of that, but requiring feels like a reasonable wheel to go along
with particularly because it deals not with a business rule about what the
param contains, but merely "is it here or not".

The alternative to having require is to do params[:person].permit(:name,
:age) which will blow up when params[:person] isn't present. Nobody wants
to see their exception log filled with stuff like that.

So I like requiring is a reasonable 3rd wheel here as it pertains to
getting the params ready for #permit.

But I do think it's entirely fair to question whether #require makes sense
when it's NOT a pre-filter to a #permit call.

On Mon, Apr 6, 2015 at 3:15 PM, Xavier Noria notifications@github.com
wrote:

I think there are two design drivers in conflict here.

In my mind, strong parameters is params whitelisting for security
purposes
. Applications declare what they accept, and strong parameters
discards what doesn't match. Support for permitted scalars was motivated by
past issues in which a hash could be injected where a string was assumed.

Then we have a second pulling force: request validation. This overlaps
with model validation in most use cases of idiomatic Rails applications.
For me, strong parameters doesn't address this problem, and in that sense
require has always felt a little off to me.

Why add support to declare multiple required keys? Then, what about
arbitrarily nested required keys? Presence is just one of many possible
validations, what about limits on array size? Regexps for strings? Ranges
for integers? Size for file uploads?

Personally, I wouldn't go in the direction of this PR because strong
parameters is not about request validation.


Reply to this email directly or view it on GitHub
#19565 (comment).

Member

dhh commented Apr 6, 2015

They are indeed separate concerns, but they're composing a whole that means
"get these params ready for model consumption". Whitelisting is the primary
element of that, but requiring feels like a reasonable wheel to go along
with particularly because it deals not with a business rule about what the
param contains, but merely "is it here or not".

The alternative to having require is to do params[:person].permit(:name,
:age) which will blow up when params[:person] isn't present. Nobody wants
to see their exception log filled with stuff like that.

So I like requiring is a reasonable 3rd wheel here as it pertains to
getting the params ready for #permit.

But I do think it's entirely fair to question whether #require makes sense
when it's NOT a pre-filter to a #permit call.

On Mon, Apr 6, 2015 at 3:15 PM, Xavier Noria notifications@github.com
wrote:

I think there are two design drivers in conflict here.

In my mind, strong parameters is params whitelisting for security
purposes
. Applications declare what they accept, and strong parameters
discards what doesn't match. Support for permitted scalars was motivated by
past issues in which a hash could be injected where a string was assumed.

Then we have a second pulling force: request validation. This overlaps
with model validation in most use cases of idiomatic Rails applications.
For me, strong parameters doesn't address this problem, and in that sense
require has always felt a little off to me.

Why add support to declare multiple required keys? Then, what about
arbitrarily nested required keys? Presence is just one of many possible
validations, what about limits on array size? Regexps for strings? Ranges
for integers? Size for file uploads?

Personally, I wouldn't go in the direction of this PR because strong
parameters is not about request validation.


Reply to this email directly or view it on GitHub
#19565 (comment).

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Apr 6, 2015

Member

For me, an API that would keep things at the same level and protect permit calls is something along this line:

def create
  @person = Person.create(safe_params[:person])
end

def safe_params
  params.permit(person: [:name, :age])
end

Addressing buggy client code or hacked requests would need a new API I believe, one that validates that a params structure conforms to some specification.

Member

fxn commented Apr 6, 2015

For me, an API that would keep things at the same level and protect permit calls is something along this line:

def create
  @person = Person.create(safe_params[:person])
end

def safe_params
  params.permit(person: [:name, :age])
end

Addressing buggy client code or hacked requests would need a new API I believe, one that validates that a params structure conforms to some specification.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Apr 6, 2015

Member

permit doesn't mutate the hash, which is what we want. Things enter as { person: { name: 'david', age: '35' } but Person.create wants { name: 'david', age: '35' }. I don't think we should change #permit to change the
structure of the hash.

On Mon, Apr 6, 2015 at 3:53 PM, Xavier Noria notifications@github.com
wrote:

For me, an API that would keep things at the same level and protect permit
calls is something along this line:

def action
@person = Person.create(safe_params[:person])end
def safe_params
params.permit(person: [:name, :age])end

Addressing buggy client code or hacked requests would need a new API I
believe, one that validates that a params structure conforms to some
specification.


Reply to this email directly or view it on GitHub
#19565 (comment).

Member

dhh commented Apr 6, 2015

permit doesn't mutate the hash, which is what we want. Things enter as { person: { name: 'david', age: '35' } but Person.create wants { name: 'david', age: '35' }. I don't think we should change #permit to change the
structure of the hash.

On Mon, Apr 6, 2015 at 3:53 PM, Xavier Noria notifications@github.com
wrote:

For me, an API that would keep things at the same level and protect permit
calls is something along this line:

def action
@person = Person.create(safe_params[:person])end
def safe_params
params.permit(person: [:name, :age])end

Addressing buggy client code or hacked requests would need a new API I
believe, one that validates that a params structure conforms to some
specification.


Reply to this email directly or view it on GitHub
#19565 (comment).

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Apr 6, 2015

Member

In any case, I do think it fits under the charter of Strong Params to deal
with parameter validation to some extent. We don't want it to deal with
business rules, but type and presence constraints seem legit. Kinda like
how we let the database enforce unique constraints in some cases, but any
advanced logic lives in the model.

On Mon, Apr 6, 2015 at 3:56 PM, David Heinemeier Hannson <
david@loudthinking.com> wrote:

permit doesn't mutate the hash, which is what we want. Things enter as { person: { name: 'david', age: '35' } but Person.create wants { name: 'david', age: '35' }. I don't think we should change #permit to change the
structure of the hash.

On Mon, Apr 6, 2015 at 3:53 PM, Xavier Noria notifications@github.com
wrote:

For me, an API that would keep things at the same level and protect
permit calls is something along this line:

def action
@person = Person.create(safe_params[:person])end
def safe_params
params.permit(person: [:name, :age])end

Addressing buggy client code or hacked requests would need a new API I
believe, one that validates that a params structure conforms to some
specification.


Reply to this email directly or view it on GitHub
#19565 (comment).

Member

dhh commented Apr 6, 2015

In any case, I do think it fits under the charter of Strong Params to deal
with parameter validation to some extent. We don't want it to deal with
business rules, but type and presence constraints seem legit. Kinda like
how we let the database enforce unique constraints in some cases, but any
advanced logic lives in the model.

On Mon, Apr 6, 2015 at 3:56 PM, David Heinemeier Hannson <
david@loudthinking.com> wrote:

permit doesn't mutate the hash, which is what we want. Things enter as { person: { name: 'david', age: '35' } but Person.create wants { name: 'david', age: '35' }. I don't think we should change #permit to change the
structure of the hash.

On Mon, Apr 6, 2015 at 3:53 PM, Xavier Noria notifications@github.com
wrote:

For me, an API that would keep things at the same level and protect
permit calls is something along this line:

def action
@person = Person.create(safe_params[:person])end
def safe_params
params.permit(person: [:name, :age])end

Addressing buggy client code or hacked requests would need a new API I
believe, one that validates that a params structure conforms to some
specification.


Reply to this email directly or view it on GitHub
#19565 (comment).

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Apr 6, 2015

Member

Yeah, take into account in the previous example the action uses safe_params[:person].

Also, conceptually, for me request validation is not really about business rules (although it is of course related to them). In my mind the service publishes a spec, client code must conform to that spec, and the server validates that an API request conforms to said spec, otherwise it is not passed down. That's for me request validation, and it is something strong parameters does not do as of today.

Anyway, that's how I'd evolve (or not evolve) strong parameters and it's fine of course that we have different points of view. I understand the approach of doing just that little thing, and being able to declare nested required keys (for only one root key) it's a normal extension to the current require.

Member

fxn commented Apr 6, 2015

Yeah, take into account in the previous example the action uses safe_params[:person].

Also, conceptually, for me request validation is not really about business rules (although it is of course related to them). In my mind the service publishes a spec, client code must conform to that spec, and the server validates that an API request conforms to said spec, otherwise it is not passed down. That's for me request validation, and it is something strong parameters does not do as of today.

Anyway, that's how I'd evolve (or not evolve) strong parameters and it's fine of course that we have different points of view. I understand the approach of doing just that little thing, and being able to declare nested required keys (for only one root key) it's a normal extension to the current require.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Apr 6, 2015

Member

One more thing: being able to say that the first name of a person is required is unrelated to permit. If you do that in the controller it leans on the side of plain request validation, it is what typically has been validated at the model level, among the other many rules you can't validate in the controller anyway.

That's what makes me skeptical about this PR, we don't do request validation, but I have the intuition that a bit of that is being slipped here.

Member

fxn commented Apr 6, 2015

One more thing: being able to say that the first name of a person is required is unrelated to permit. If you do that in the controller it leans on the side of plain request validation, it is what typically has been validated at the model level, among the other many rules you can't validate in the controller anyway.

That's what makes me skeptical about this PR, we don't do request validation, but I have the intuition that a bit of that is being slipped here.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Apr 6, 2015

Member

Hey, since there has been a few messages today people in the thread may be wondering what's next :).

David's POV is clear, and strong parameters is his baby. I gave my feedback since I was cc'ed, but probably we won't agree, and that's fine. Let's move on with the PR!

Member

fxn commented Apr 6, 2015

Hey, since there has been a few messages today people in the thread may be wondering what's next :).

David's POV is clear, and strong parameters is his baby. I gave my feedback since I was cc'ed, but probably we won't agree, and that's fine. Let's move on with the PR!

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Apr 6, 2015

Member

Very much appreciate the input, Xavier. And I think we should consider how
we can also deal with the parameter validation aspect through a suitable
API. We used to have that on the controller. Forgot what it was called.

On Mon, Apr 6, 2015 at 6:08 PM, Xavier Noria notifications@github.com
wrote:

Hey, since there has been a few messages today people in the thread may be
wondering what's next :).

David's POV is clear, and strong parameters is his baby. I gave my
feedback since I was cc'ed, but probably we won't agree, and that's fine.
Let's move on with the PR!


Reply to this email directly or view it on GitHub
#19565 (comment).

Member

dhh commented Apr 6, 2015

Very much appreciate the input, Xavier. And I think we should consider how
we can also deal with the parameter validation aspect through a suitable
API. We used to have that on the controller. Forgot what it was called.

On Mon, Apr 6, 2015 at 6:08 PM, Xavier Noria notifications@github.com
wrote:

Hey, since there has been a few messages today people in the thread may be
wondering what's next :).

David's POV is clear, and strong parameters is his baby. I gave my
feedback since I was cc'ed, but probably we won't agree, and that's fine.
Let's move on with the PR!


Reply to this email directly or view it on GitHub
#19565 (comment).

@gaurish

This comment has been minimized.

Show comment
Hide comment
@gaurish

gaurish May 23, 2015

Contributor

So what needs to be done here, any pointers?

Contributor

gaurish commented May 23, 2015

So what needs to be done here, any pointers?

@gaurish

This comment has been minimized.

Show comment
Hide comment
@gaurish

gaurish Aug 27, 2015

Contributor

Updated PR based on the suggestions. To summarise the discussion.

  1. We need request validation(just type & presence) on controller level.
  2. I have updated the PR, removing require_all & added support for require to accept arrays.

so you can do

first_name, title = params.require([ :first_name, :title ])

if first_name or title params are empty, it will generate an error back to client with 400 bad request -- client, allowing client errors to be really be client errors & not server errors(5xx)

@dhh
Any thoughts, Feedback?

Contributor

gaurish commented Aug 27, 2015

Updated PR based on the suggestions. To summarise the discussion.

  1. We need request validation(just type & presence) on controller level.
  2. I have updated the PR, removing require_all & added support for require to accept arrays.

so you can do

first_name, title = params.require([ :first_name, :title ])

if first_name or title params are empty, it will generate an error back to client with 400 bad request -- client, allowing client errors to be really be client errors & not server errors(5xx)

@dhh
Any thoughts, Feedback?

@gaurish gaurish changed the title from [Feature] require_all parameters or raise error to [Feature] params.require accepts array of parameters that should be present or raise error Aug 27, 2015

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Aug 28, 2015

Member

API looks good to me. Someone else might review the implementation but also looks pretty straight forward to me.

Member

dhh commented Aug 28, 2015

API looks good to me. Someone else might review the implementation but also looks pretty straight forward to me.

@gaurish

This comment has been minimized.

Show comment
Hide comment
@gaurish

gaurish Aug 28, 2015

Contributor

Updated as per Feedback.

Contributor

gaurish commented Aug 28, 2015

Updated as per Feedback.

[Feature] params.require requires array of params
This PR adds ability to accept arrays which allows you to require multiple values in one method. so instead of this:

```ruby
params.require(:person).require(:first_name)
params.require(:person).require(:last_name)
```

Here it will be one line for each params, so say if I require 10params, it will be 10lines of repeated code which is not dry. So I have added new method which does this in one line:

```ruby
params.require(:person).require([:first_name, :last_name])
```

Comments welcome

kaspth added a commit that referenced this pull request Aug 28, 2015

Merge pull request #19565 from gaurish/multiple_require_params
[Feature] params.require accepts array of parameters that should be present or raise error

@kaspth kaspth merged commit ca24ab8 into rails:master Aug 28, 2015

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 28, 2015

Member

Thanks

Member

kaspth commented Aug 28, 2015

Thanks

@gaurish

This comment has been minimized.

Show comment
Hide comment
@gaurish

gaurish Aug 28, 2015

Contributor
Contributor

gaurish commented Aug 28, 2015

@sikachu

This comment has been minimized.

Show comment
Hide comment
@sikachu

sikachu Aug 28, 2015

Member

I know I'm late to the party, but I have a feeling that this promotes a feature to be used in the way that it's not intended to be.

What about actually adding assert_valid_keys to ActionController::Parameters? We already have that on Hash, and it's currently the way to make sure a Hash has all the given keys. Adding this recursive functionality to require makes this method doing too much IMHO.

Member

sikachu commented Aug 28, 2015

I know I'm late to the party, but I have a feeling that this promotes a feature to be used in the way that it's not intended to be.

What about actually adding assert_valid_keys to ActionController::Parameters? We already have that on Hash, and it's currently the way to make sure a Hash has all the given keys. Adding this recursive functionality to require makes this method doing too much IMHO.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Aug 28, 2015

Member

The apparently innocent patch had an important typo that wasn't catched by the tests, went through it in cbe7899.

If correcting a patch about an API you don't agree with isn't team playing I don't know what it is! 😄

Member

fxn commented Aug 28, 2015

The apparently innocent patch had an important typo that wasn't catched by the tests, went through it in cbe7899.

If correcting a patch about an API you don't agree with isn't team playing I don't know what it is! 😄

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Aug 28, 2015

Member

The examples are not good for certain, it is not obvious to the reader that first_name could be an injected hash because it has not been permitted.

Maybe if we had a non-artificial use case for an action that receives several root objects... anyone?

Member

fxn commented Aug 28, 2015

The examples are not good for certain, it is not obvious to the reader that first_name could be an injected hash because it has not been permitted.

Maybe if we had a non-artificial use case for an action that receives several root objects... anyone?

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Aug 28, 2015

Member

I can't think of a use case. Say

def user_params
  params.require(:user).permit(:name, :email, :bio).tap do |up|
    up.require([:name, :email])
  end
end

Besides being really ugly for my taste, are we implementing a value presence validator in the controller? Why? That's tipically a concern of the model (together with several other validators not available anyway). We have never promoted using require that way AFAIK.

On the other hand params.require(:user) is not a data validator, it is asserting the action expects a form for users, which is different (and as I argued above, still feels a bit off to me, but that has become idiomatic).

Regarding several root objects, I cannot find a nice example:

def safe_params
  user, product = params.require([:user, :product])
  return user.permit(...), product.permit(...) 
end

versus

def user_params
  # ...
end

def product_params
  # ...
end

Which usage patterns should Rails promote with this API?

Member

fxn commented Aug 28, 2015

I can't think of a use case. Say

def user_params
  params.require(:user).permit(:name, :email, :bio).tap do |up|
    up.require([:name, :email])
  end
end

Besides being really ugly for my taste, are we implementing a value presence validator in the controller? Why? That's tipically a concern of the model (together with several other validators not available anyway). We have never promoted using require that way AFAIK.

On the other hand params.require(:user) is not a data validator, it is asserting the action expects a form for users, which is different (and as I argued above, still feels a bit off to me, but that has become idiomatic).

Regarding several root objects, I cannot find a nice example:

def safe_params
  user, product = params.require([:user, :product])
  return user.permit(...), product.permit(...) 
end

versus

def user_params
  # ...
end

def product_params
  # ...
end

Which usage patterns should Rails promote with this API?

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Aug 29, 2015

Member

@sikachu @dhh let me make something explicit to make sure we are all talking about the same.

require is a value presence validator, not a key presence validator. If a form sends the user email as a blank field, and the controller checks require(:email), then we get a parameter missing exception albeit the parameter was sent.

I am pretty sure this was understood in the discussion above, I don't think going in that direction is good for strong params, but it was fine for David (and I agreed to disagree, so not discussing the acceptance of the PR, only following up because we need non-artificial examples for the docs, because the one in the patch can't end up there).

And if you think about it, this discussion is really unrelated to this PR, it is about promoting the use of require for scalars as a presence validator vs promoting only its usage for root objects. This same discussion could have happened without support for arrays.

Member

fxn commented Aug 29, 2015

@sikachu @dhh let me make something explicit to make sure we are all talking about the same.

require is a value presence validator, not a key presence validator. If a form sends the user email as a blank field, and the controller checks require(:email), then we get a parameter missing exception albeit the parameter was sent.

I am pretty sure this was understood in the discussion above, I don't think going in that direction is good for strong params, but it was fine for David (and I agreed to disagree, so not discussing the acceptance of the PR, only following up because we need non-artificial examples for the docs, because the one in the patch can't end up there).

And if you think about it, this discussion is really unrelated to this PR, it is about promoting the use of require for scalars as a presence validator vs promoting only its usage for root objects. This same discussion could have happened without support for arrays.

fxn added a commit that referenced this pull request Aug 29, 2015

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Sep 2, 2015

Member

Right. I had a couple of cases in my own code base with forms that didn't
have a shared root. Like user_name and password instead of
login[:user_name] and login[:password]. Using value presence checking on
that case made perfect sense to me.

On Sat, Aug 29, 2015 at 3:04 AM, Xavier Noria notifications@github.com
wrote:

@sikachu https://github.com/sikachu @dhh https://github.com/dhh let
me make something explicit to make sure we are all talking about the same.

require is a value presence validator, not a key presence validator.
If a form sends the user email as a blank field, and the controller checks
require(:email), then we get a parameter missing exception albeit the
parameter was sent.

I am pretty sure this was understood in the discussion above, I don't
think going in that direction is good for strong params, but it was fine
for David (and I agreed to disagree, so not discussing the acceptance of
the PR, only following up because we need non-artificial examples for the
docs, because the one in the patch can't end up there).

And if you think about it, this discussion is really unrelated to this PR,
it is about promoting the use of require for scalars as a presence
validator vs promoting only its usage for root objects. This same
discussion could have happened without support for arrays.


Reply to this email directly or view it on GitHub
#19565 (comment).

Member

dhh commented Sep 2, 2015

Right. I had a couple of cases in my own code base with forms that didn't
have a shared root. Like user_name and password instead of
login[:user_name] and login[:password]. Using value presence checking on
that case made perfect sense to me.

On Sat, Aug 29, 2015 at 3:04 AM, Xavier Noria notifications@github.com
wrote:

@sikachu https://github.com/sikachu @dhh https://github.com/dhh let
me make something explicit to make sure we are all talking about the same.

require is a value presence validator, not a key presence validator.
If a form sends the user email as a blank field, and the controller checks
require(:email), then we get a parameter missing exception albeit the
parameter was sent.

I am pretty sure this was understood in the discussion above, I don't
think going in that direction is good for strong params, but it was fine
for David (and I agreed to disagree, so not discussing the acceptance of
the PR, only following up because we need non-artificial examples for the
docs, because the one in the patch can't end up there).

And if you think about it, this discussion is really unrelated to this PR,
it is about promoting the use of require for scalars as a presence
validator vs promoting only its usage for root objects. This same
discussion could have happened without support for arrays.


Reply to this email directly or view it on GitHub
#19565 (comment).

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