This project is archived and is in readonly mode.
find_or_create_by_foo when foo is protected attribute
Reported by Marc-André Lafortune | April 22nd, 2010 @ 05:19 PM | in 2.3.6
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
Comments and changes to this ticket
-
Ryan Bigg April 25th, 2010 @ 12:23 AM
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.
-
Ryan Bigg April 26th, 2010 @ 09:16 AM
- Assigned user set to Pratik
-
Marc-André Lafortune April 26th, 2010 @ 06:14 PM
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 nameI 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
-
Marc-André Lafortune April 26th, 2010 @ 06:22 PM
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
-
Tanel Suurhans April 30th, 2010 @ 10:04 PM
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"
-
Marc-André Lafortune May 4th, 2010 @ 07:03 PM
I'm fine with the arugment protection, as long as the other form find_or_create_by_name("Marc-André", :whatever => "value") is fixed.
-
Jeremy Kemper May 7th, 2010 @ 01:22 AM
- State changed from new to open
- Assigned user changed from Pratik to Santiago Pastorino
-
Santiago Pastorino May 7th, 2010 @ 04:40 AM
- Tag set to activerecord, find_or_create, patch
Patch for master tomorrow i will backport it for 2-3-stable
-
Repository May 7th, 2010 @ 05:52 PM
- State changed from open to committed
(from [9aaef5935660ba13531512fb7def4b8bdf14511d]) Make find_or_create and find_or_initialize work mixing explicit parameters and a hash [#4457 state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/9aaef5935660ba13531512fb7def4b... -
Santiago Pastorino May 7th, 2010 @ 06:22 PM
- Milestone set to 2.3.6
- State changed from committed to open
I'm going to bakport to 2-3-stable
-
Santiago Pastorino May 7th, 2010 @ 09:05 PM
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 -
Santiago Pastorino May 7th, 2010 @ 09:07 PM
WTF i send the same thing again, sorry.
This is the right one.
Thanks Marc-André. -
Repository May 7th, 2010 @ 09:32 PM
- 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 state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/f967b352d2abdb88b704266cdb06b5... -
Marc-André Lafortune May 8th, 2010 @ 05:20 AM
Great! Thanks Santiago.
I actually felt bad not taking the time to write a patch for this...
-
Santiago Pastorino May 8th, 2010 @ 09:11 PM
Thanks to you Marc-André, and give you some time to try the next issue ;).
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
Attachments
Referenced by
- 4457 find_or_create_by_foo when foo is protected attribute (from [9aaef5935660ba13531512fb7def4b8bdf14511d]) Make fi...
- 4457 find_or_create_by_foo when foo is protected attribute [#4457 state:committed]