Permalink
Browse files

Make find_or_create and find_or_initialize work mixing explicit param…

…eters and a hash. ht: Marc-André Lafortune

[#4457 state:committed]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
  • Loading branch information...
1 parent adcfb4e commit f967b352d2abdb88b704266cdb06b53f7d8ab401 @spastorino spastorino committed with jeremy May 7, 2010
Showing with 33 additions and 10 deletions.
  1. +2 −0 activerecord/CHANGELOG
  2. +14 −9 activerecord/lib/active_record/base.rb
  3. +17 −1 activerecord/test/cases/finder_test.rb
View
@@ -1,5 +1,7 @@
*2.3.6 (pending)*
+* find_or_create_by_attr(value, ...) works when attr is protected. #4457 [Santiago Pastorino, Marc-André Lafortune]
+
* JSON supports a custom root option: to_json(:root => 'custom') #4515 [Jatinder Singh]
* Destroy uses optimistic locking. If lock_version on the record you're destroying doesn't match lock_version in the database, a StaleObjectError is raised. #1966 [Curtis Hawthorne]
@@ -1942,23 +1942,28 @@ def self.#{method_id}(*args)
# end
self.class_eval <<-EOS, __FILE__, __LINE__ + 1
def self.#{method_id}(*args)
- guard_protected_attributes = false
-
- if args[0].is_a?(Hash)
- guard_protected_attributes = true
- attributes = args[0].with_indifferent_access
- find_attributes = attributes.slice(*[:#{attribute_names.join(',:')}])
- else
- find_attributes = attributes = construct_attributes_from_arguments([:#{attribute_names.join(',:')}], args)
+ attributes = [:#{attribute_names.join(',:')}]
+ protected_attributes_for_create = unprotected_attributes_for_create = {}
@radar

radar May 7, 2010

Contributor

Do you mean to do this? You're assigning the same object to two separate variables, and then merging them later on 1955 (when they're going to be the same?) Perhaps you mean to define these variables both as empty hashes? Is there something I'm missing?

@spastorino

spastorino May 8, 2010

Owner

Ryan, you're right in part. I didn't mean to do this but this works well because of the line 1949.
if line 1949 is executed the hashes are not the same anymore.
But if args hasn't a hash they will be the same as you said but the functionality don't fail. Why? because i merge the same hash so nothing happends and i execute attributes= twice for unprotected_attributes so it's ok.
Anyways is not the intended solution and of couse is best to do a separate assignment, i will fix this later.
Thanks.

+ args.each_with_index do |arg, i|
+ if arg.is_a?(Hash)
+ protected_attributes_for_create = args[i].with_indifferent_access
+ else
+ unprotected_attributes_for_create[attributes[i]] = args[i]
+ end
end
+ find_attributes = (protected_attributes_for_create.merge(unprotected_attributes_for_create)).slice(*attributes)
+
options = { :conditions => find_attributes }
set_readonly_option!(options)
record = find(:first, options)
if record.nil?
- record = self.new { |r| r.send(:attributes=, attributes, guard_protected_attributes) }
+ record = self.new do |r|
+ r.send(:attributes=, protected_attributes_for_create, true) unless protected_attributes_for_create.empty?
+ r.send(:attributes=, unprotected_attributes_for_create, false) unless unprotected_attributes_for_create.empty?
+ end
#{'yield(record) if block_given?'}
#{'record.save' if instantiator == :create}
record
@@ -838,14 +838,30 @@ def test_find_or_initialize_from_one_attribute_should_not_set_attribute_even_whe
assert c.new_record?
end
- def test_find_or_create_from_one_attribute_should_set_not_attribute_even_when_protected
+ def test_find_or_create_from_one_attribute_should_not_set_attribute_even_when_protected
c = Company.find_or_create_by_name({:name => "Fortune 1000", :rating => 1000})
assert_equal "Fortune 1000", c.name
assert_not_equal 1000, c.rating
assert c.valid?
assert !c.new_record?
end
+ def test_find_or_initialize_from_one_attribute_should_set_attribute_even_when_protected_and_also_set_the_hash
+ c = Company.find_or_initialize_by_rating(1000, {:name => "Fortune 1000"})
+ assert_equal "Fortune 1000", c.name
+ assert_equal 1000, c.rating
+ assert c.valid?
+ assert c.new_record?
+ end
+
+ def test_find_or_create_from_one_attribute_should_set_attribute_even_when_protected_and_also_set_the_hash
+ c = Company.find_or_create_by_rating(1000, {:name => "Fortune 1000"})
+ assert_equal "Fortune 1000", c.name
+ assert_equal 1000, c.rating
+ assert c.valid?
+ assert !c.new_record?
+ end
+
def test_find_or_initialize_from_one_attribute_should_set_attribute_even_when_protected
c = Company.find_or_initialize_by_name_and_rating("Fortune 1000", 1000)
assert_equal "Fortune 1000", c.name

0 comments on commit f967b35

Please sign in to comment.