Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

find_or_create_by_foo when foo is protected attribute

#4457

I understand that for security reasons it is a good idea that methods like find_or_create_by_foo(params) do not write on protected attributes.

I question the rationale for not writing on foo, though, even if it is protected.

1) find_or_create_by_foo is used to get a record that matches the given value for foo. If it won't write foo because it is protected, it doesn't fulfill its purpose.

2) foo is specified explicitely by the developper. I would take that as an 'ok' to write on that field.

3) With current behavior, one must specify foo twice to get the desired result:
find_or_create_by_foo(:foo => "bar", :baz => "baz") {|r| r.foo = "bar" } Not very DRY

Typical case is where foo is actually a foo_id... Typically you want to protect that (usually doesn't change after creation, especially not from a params web request).

Thanks

Reported by Marc-André Lafortune · April 22nd, 2010 @ 05:19 PM

State: committed
Milestone: 2.3.6
Assigned to: Santiago Pastorino Santiago Pastorino
Importance: Low

Activity

  1. Ryan Bigg
    Ryan Bigg

    Rather than using "foo", "bar" and "baz", could you use real terms so we can think how you're thinking? if foo is indeed a foreign_key then you should be using the association builders to assign it, not find_or_create.

    April 25th, 2010 @ 12:23 AM

  2. Ryan Bigg
    Ryan Bigg
    • Assigned user set to Pratik

    April 26th, 2010 @ 09:16 AM

  3. Marc-André Lafortune
    Marc-André Lafortune

    I am not sure what you mean by "should be using the association builders", so maybe I am missing something.

    To be a bit more convincing that this is a bug, let me add:

    0)

    If Tag#name is protected:

    Tag.find_or_create_by_name("Summer") # assigns name
    Tag.find_or_create_by_name(:name =>"Summer") # doesn't assign name

    I believe both forms should be equivalent, especially that there is no way to:
    Tag.find_or_create_by_name("Summer", :extra_info => "blah")

    That later form would be nice, BTW, but I guess I'll concentrate on arguing about this current bug.

    An examples with an association I can think of:

    Summary.belongs_to :original_content

    summary = Summary.find_or_create_by_original_content_id(:original_content_id => 42, &:summarize)

    Moreover, I can't think of an example where it would not be desirable or prudent to assign 'foo'.

    Thanks

    April 26th, 2010 @ 06:14 PM

  4. Marc-André Lafortune
    Marc-André Lafortune

    Actually, is it only me, or the example from the doc will not work (protected fields or not)?

    User.find_or_create_by_name('Bob', :age => 40) # this won't set the age in Rails 2.3.5

    April 26th, 2010 @ 06:22 PM

  5. Tanel Suurhans
    Tanel Suurhans

    Tested this on 2-3-stable.

    author = Author.find_or_create_by_name(:name => "bm", :author_address_id => 1)
    author.inspect # => #<Author id: 10, :name => "bm", :author_address_id => 1, :author_address_extra_id => nil>
    

    Also tested on 2.3.5 on one of my applications and indeed it does not seem to work.

    page = Page.find_or_create_by_name("my page", :content => "ponies here", :state => Page::STATE_HIDDEN)
    page.inspect # => #<Page id: 1, :state => "PUBLIC", :name => "my page", :content => nil>
    

    I see test cases covering additional values, as well as hash values for find_or_create_by in 2-3-stable, so i would assume its all working there.

    As for your second set of examples - in my opinion this behavior is expected. The attr_protected protects attributes against mass(hash) assignment.
    In your first example, its an explicit assignment, in your second one its basically a mass-assignment. So it's consistent with the rest of the API.

    Piece of the documentation from ActiveRecord::Base

    class Customer < ActiveRecord::Base
      attr_protected :credit_rating
    end
    customer = Customer.new("name" => David, "credit_rating" => "Excellent")
    customer.credit_rating # => nil
    customer.attributes = { "description" => "Jolly fellow", "credit_rating" => "Superb" }
    customer.credit_rating # => nil
    customer.credit_rating = "Average"
    customer.credit_rating # => "Average"
    

    April 30th, 2010 @ 10:04 PM

  6. Marc-André Lafortune
    Marc-André Lafortune

    I'm fine with the arugment protection, as long as the other form find_or_create_by_name("Marc-André", :whatever => "value") is fixed.

    May 4th, 2010 @ 07:03 PM

  7. Jeremy Kemper
    Jeremy Kemper
    • State changed from new to open
    • Assigned user changed from Pratik to Santiago Pastorino

    May 7th, 2010 @ 01:22 AM

  8. Santiago Pastorino
    Santiago Pastorino
    • Tag set to activerecord, find_or_create, patch

    Patch for master tomorrow i will backport it for 2-3-stable

    May 7th, 2010 @ 04:40 AM

  9. Repository
    Repository
    • State changed from open to committed

    (from [9aaef5935660ba13531512fb7def4b8bdf14511d]) Make find_or_create and find_or_initialize work mixing explicit parameters and a hash [#4457 find_or_create_by_foo when foo is protected attribute state:committed]

    Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
    http://github.com/rails/rails/commit/9aaef5935660ba13531512fb7def4b...

    May 7th, 2010 @ 05:52 PM

  10. Santiago Pastorino
    Santiago Pastorino
    • Milestone set to 2.3.6
    • State changed from committed to open

    I'm going to bakport to 2-3-stable

    May 7th, 2010 @ 06:22 PM

  11. Santiago Pastorino
  12. Santiago Pastorino
    Santiago Pastorino

    Sorry i think Marc-André Lafortune deserves a mention on the patch, i forgot it twice.
    Here is the 2-3-stable patch with the mention

    May 7th, 2010 @ 09:05 PM

  13. Santiago Pastorino
    Santiago Pastorino

    WTF i send the same thing again, sorry.
    This is the right one.
    Thanks Marc-André.

    May 7th, 2010 @ 09:07 PM

  14. Repository
    Repository
    • State changed from open to committed

    (from [f967b352d2abdb88b704266cdb06b53f7d8ab401]) Make find_or_create and find_or_initialize work mixing explicit parameters and a hash. ht: Marc-André Lafortune

    [#4457 find_or_create_by_foo when foo is protected attribute state:committed]

    Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
    http://github.com/rails/rails/commit/f967b352d2abdb88b704266cdb06b5...

    May 7th, 2010 @ 09:32 PM

  15. Marc-André Lafortune
    Marc-André Lafortune

    Great! Thanks Santiago.

    I actually felt bad not taking the time to write a patch for this...

    May 8th, 2010 @ 05:20 AM

  16. Santiago Pastorino
    Santiago Pastorino

    Thanks to you Marc-André, and give you some time to try the next issue ;).

    May 8th, 2010 @ 09:11 PM