Fix + Failing test for find_or_initialize_ updating has_many collection size.

[mlangenberg]

When a record is initialized through a has_many association, the record is added to the target association.
On a record.save, this record is not updated or replaced in the in-memory association.

e.g.

@tree.apples.size #=> 1
apple = @tree.apples.find_or_initialize_by_color('Red')
@tree.apples.size #=> 2, so it added a record in-memory.
apple.color = 'Green'
apple.save
@tree.apples.size #=> 3 !!! It did reload records from database. It did not clear in-memory records.
@tree.apples.last.color #=> 'Red'
@tree.apples.last.new_record? #=> true
@tree.apples.second.color #=> 'Green'
@tree.apples.second.new_record? #=> false

[mlangenberg]

What I don't get is that it only happens when using find_or_initalize_by. When using a = @tree.apples.new, then a.save, it does 'replace' the in-memory record.

[mlangenberg]

The problem is not in the save call, but in the find_or_initialize. The codepath is different from a build. Looks like the record added to association is different from the one returned from the call, based on object_id.

Or it doing record.dup or it is executing the find_or_initialize twice.

[mlangenberg]

Maybe the implications aren't totally clear.
Currently all method_missing calls on CollectionProxy that are matched by the DynamicFinderMatch are executed twice. This is not only an unnecessary performance penalty, it can also lead to non-deterministic behavior.

For example, if you use post.comments.find_by_foo somewhere, it will run it twice. Same for post.comments.find_or_create. A post.comments.find_or_initialize will initialize a new object twice, returning the latest.

[kenn]

bump.

We hit this problem when we migrated from Rails 3.0 to Rails 3.1.

http://stackoverflow.com/questions/8798555/find-or-initialize-by-on-has-many-association-causes-duplicate-error

[willbryant]

Are you sure this affects find_or_create?

If so, any idea why this bug is not showing up in the existing activerecord test suite? I wouldn't expect that there would necessarily test for the number of queries run or objects initialized, but there are has_many tests that check that find_or_create only creates once, for example:

  def test_dynamic_find_or_create_from_two_attributes_using_an_association
    author = authors(:david)
    number_of_posts = Post.count
    another = author.posts.find_or_create_by_title_and_body("Another Post", "This is the Body")
    assert_equal number_of_posts + 1, Post.count
    assert_equal another, author.posts.find_or_create_by_title_and_body("Another Post", "This is the Body")
    assert another.persisted?
  end

Which is passing on the 3-1-stable branch. For what it's worth, there is also a check on #size after using find_or_initialize, though it could well be doing the find twice:

  def test_find_or_initialize_updates_collection_size
    number_of_clients = companies(:first_firm).clients_of_firm.size
    companies(:first_firm).clients_of_firm.find_or_initialize_by_name("name" => "Another Client")
    assert_equal number_of_clients + 1, companies(:first_firm).clients_of_firm.size
  end

If you could create a failing test case for the problem I think that would help get this patch merged a lot.

[carlosantoniodasilva]

@mlangenberg I think this has already changed on 3-2/master, if you confirm that please close this issue. Thanks.

[kenn]

Confirmed it is fixed as of Rails 3.2.2. Thanks!

[mlangenberg]

It's fixed in 3.2 indeed. Thanks all.