This repository has been archived by the owner. It is now read-only.

Partially failing nested parameters - is this the intended behavior? #43

Open
svoop opened this Issue Oct 5, 2012 · 7 comments

Comments

Projects
None yet
3 participants
@svoop
Contributor

svoop commented Oct 5, 2012

The nested params tests fail if a non-numeric parameter similar to the following is added:

test "fields_for_style_nested_params" do
  params = ActionController::Parameters.new({
    book: {
      authors_attributes: {
        :'0' => { name: 'William Shakespeare', age_of_death: '52' },
        :'1' => { name: 'Unattributed Assistant' },
        :new => { name: 'Failing' }   # <== ADDED
      }
    }
  })
  permitted = params.permit book: { authors_attributes: [ :name ] }   # <== permitted is: {"book"=>{"authors_attributes"=>{}}}

  assert_not_nil permitted[:book][:authors_attributes]['0']
  assert_not_nil permitted[:book][:authors_attributes]['1']
  assert_nil permitted[:book][:authors_attributes]['0'][:age_of_death]
  assert_equal 'William Shakespeare', permitted[:book][:authors_attributes]['0'][:name]
  assert_equal 'Unattributed Assistant', permitted[:book][:authors_attributes]['1'][:name]
end

Turns out that one illegal key (:new) will do away with the entire authors_attributes hash in permitted. Is this behavior intentional?

Wouldn't it be better if only the illegal key were dropped and the other valid keys (:'0' and :'1') remained in the hash?

(I'm getting such a :new key in a project of mine because the same fields_for partial is used for both existing as well as newly added nested records. In order to do that, the template requires a child index and all numerical ones are already in use; 0 up for existing, -1 and below for new records.)

@fringd

This comment has been minimized.

Show comment Hide comment
@fringd

fringd Oct 16, 2012

totally jumping in out of the void, but i'd rather it completely explode if any parameters are illegal, forget dropping a single author, forget dropping all authors... just drop the whole thing, and throw Error::IllegalParameterOMFG

fringd commented Oct 16, 2012

totally jumping in out of the void, but i'd rather it completely explode if any parameters are illegal, forget dropping a single author, forget dropping all authors... just drop the whole thing, and throw Error::IllegalParameterOMFG

@svoop

This comment has been minimized.

Show comment Hide comment
@svoop

svoop Oct 16, 2012

Contributor

Or both behaviors (full kaboom or drop illegal keys) configurable via e.g. ActionController::StrongParameters.ignore_illegal_nested_parameter_keys

Contributor

svoop commented Oct 16, 2012

Or both behaviors (full kaboom or drop illegal keys) configurable via e.g. ActionController::StrongParameters.ignore_illegal_nested_parameter_keys

@fringd

This comment has been minimized.

Show comment Hide comment
@fringd

fringd Oct 16, 2012

so does strong parameters usually remove extraneous params or blow up if there are extraneous params?

fringd commented Oct 16, 2012

so does strong parameters usually remove extraneous params or blow up if there are extraneous params?

@svoop

This comment has been minimized.

Show comment Hide comment
@svoop

svoop Oct 16, 2012

Contributor

so does strong parameters usually remove extraneous params or blow up if there are extraneous params?

Say you have a nested params hash with one post -> many comments. If one of the comments has an illegal key, all comments are stripped from the params hash and only the blog params remain.

Contributor

svoop commented Oct 16, 2012

so does strong parameters usually remove extraneous params or blow up if there are extraneous params?

Say you have a nested params hash with one post -> many comments. If one of the comments has an illegal key, all comments are stripped from the params hash and only the blog params remain.

@dhh

This comment has been minimized.

Show comment Hide comment
@dhh

dhh Oct 16, 2012

Member

No kaboom. Just ignore illegal keys on a per-record basis (rather than all of them).

On Oct 16, 2012, at 13:16, Sven Schwyn notifications@github.com wrote:

so does strong parameters usually remove extraneous params or blow up if there are extraneous params?

Say you have a nested params hash with one post -> many comments. If one of the comments has an illegal key, all comments are stripped from the params hash and only the blog params remain.

Reply to this email directly or view it on GitHub.

Member

dhh commented Oct 16, 2012

No kaboom. Just ignore illegal keys on a per-record basis (rather than all of them).

On Oct 16, 2012, at 13:16, Sven Schwyn notifications@github.com wrote:

so does strong parameters usually remove extraneous params or blow up if there are extraneous params?

Say you have a nested params hash with one post -> many comments. If one of the comments has an illegal key, all comments are stripped from the params hash and only the blog params remain.

Reply to this email directly or view it on GitHub.

@fringd

This comment has been minimized.

Show comment Hide comment
@fringd

fringd Oct 16, 2012

@svoop so what if the params hash has params[:post][:not_allowed_attribute]

is the attribute removed, or is there an error, or is the whole [:post] killed?

fringd commented Oct 16, 2012

@svoop so what if the params hash has params[:post][:not_allowed_attribute]

is the attribute removed, or is there an error, or is the whole [:post] killed?

@dhh

This comment has been minimized.

Show comment Hide comment
@dhh

dhh Oct 16, 2012

Member

That attribute is ignored if not listed in the permit call. No blow up needed.

On Oct 16, 2012, at 16:12, fringd notifications@github.com wrote:

@svoop so what if the params hash has params[:post][:not_allowed_attribute]

is the attribute removed, or is there an error, or is the whole [:post] killed?


Reply to this email directly or view it on GitHub.

Member

dhh commented Oct 16, 2012

That attribute is ignored if not listed in the permit call. No blow up needed.

On Oct 16, 2012, at 16:12, fringd notifications@github.com wrote:

@svoop so what if the params hash has params[:post][:not_allowed_attribute]

is the attribute removed, or is there an error, or is the whole [:post] killed?


Reply to this email directly or view it on GitHub.

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