Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

bug 1108: fix a bug with find_or_create_by and additional values

There was a bug with find_or_create_by_x introduced in 2.3.9 - if you
included extra parameters for the create() then those parameters would
confuse the find() so you'd never get to the create().  This patch
filters the parameters so we only pass to find() the subset that it's
interested in.  The code for the filtering was modelled on the code in
base.rb's method_missing().
  • Loading branch information...
commit fdfc8e3b9c4905057677fd009f463a377be60b93 1 parent f5ed5c3
Toby Cabot ccabot authored tenderlove committed
22 activerecord/lib/active_record/associations/association_collection.rb
View
@@ -381,7 +381,8 @@ def method_missing(method, *args)
return find(:first, :conditions => args.first) || create(args.first)
when /^find_or_create_by_(.*)$/
rest = $1
- return send("find_by_#{rest}", *args) ||
+ find_args = pull_finder_args_from(DynamicFinderMatch.match(method).attribute_names, *args)
+ return send("find_by_#{rest}", find_args) ||
method_missing("create_by_#{rest}", *args)
when /^create_by_(.*)$/
return create($1.split('_and_').zip(args).inject({}) { |h,kv| k,v=kv ; h[k] = v ; h })
@@ -448,6 +449,25 @@ def add_record_to_target_with_callbacks(record)
end
private
+ # Separate the "finder" args from the "create" args given to a
+ # find_or_create_by_ call. Returns an array with the
+ # parameter values in the same order as the keys in the
+ # "names" array. This code was based on code in base.rb's
+ # method_missing method.
+ def pull_finder_args_from(names, *args)
+ attributes = names.collect { |name| name.intern }
+ attribute_hash = {}
+ args.each_with_index do |arg, i|
+ if arg.is_a?(Hash)
+ attribute_hash.merge! arg
+ else
+ attribute_hash[attributes[i]] = arg
+ end
+ end
+ attribute_hash = attribute_hash.with_indifferent_access
+ attributes.collect { |attr| attribute_hash[attr] }
+ end
+
def create_record(attrs)
attrs.update(@reflection.options[:conditions]) if @reflection.options[:conditions].is_a?(Hash)
ensure_owner_is_not_new
17 activerecord/test/cases/associations/has_many_associations_test.rb
View
@@ -65,6 +65,23 @@ def test_find_or_create_by
assert_equal person, person.readers.first.person
end
+ def test_find_or_create_by_with_additional_parameters
+ post = Post.create! :title => 'test_find_or_create_by_with_additional_parameters', :body => 'this is the body'
+ comment = post.comments.create! :body => 'test comment body', :type => 'test'
+
+ assert_equal comment, post.comments.find_or_create_by_body('test comment body')
+
+ post.comments.find_or_create_by_body(:body => 'other test comment body', :type => 'test')
+ assert_equal 2, post.comments.count
+ assert_equal 2, post.comments.length
+ post.comments.find_or_create_by_body('other other test comment body', :type => 'test')
+ assert_equal 3, post.comments.count
+ assert_equal 3, post.comments.length
+ post.comments.find_or_create_by_body_and_type('3rd test comment body', 'test')
+ assert_equal 4, post.comments.count
+ assert_equal 4, post.comments.length
+ end

This test is not sufficient, and allowed a bug to pop in.

Run this twice and you get a duplicate :
post.comments.find_or_create_by_body_and_type('3rd test comment body', 'test')
post.comments.find_or_create_by_body_and_type('3rd test comment body', 'test')
assert_equal 4, post.comments.count
assert_equal 4, post.comments.length

Actually your change broke the "find" part of the "find_or_create", as a new entry is created each time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
def test_find_or_create
person = Person.create! :first_name => 'tenderlove'
post = Post.find :first
Jérémy FRERE

This test is not sufficient, and allowed a bug to pop in.

Run this twice and you get a duplicate :
post.comments.find_or_create_by_body_and_type('3rd test comment body', 'test')
post.comments.find_or_create_by_body_and_type('3rd test comment body', 'test')
assert_equal 4, post.comments.count
assert_equal 4, post.comments.length

Actually your change broke the "find" part of the "find_or_create", as a new entry is created each time.

Please sign in to comment.
Something went wrong with that request. Please try again.