rails/rails — pull request #5476 by tandem-softworks (2012-03-16 21:44:54+00:00)
Title: Nested attributes with callbacks bug

Hi everybody, this is my first public interaction with the rails
project. So, feel free to correct me if I'm not following the proper
procedures.

I discovered a bug in the nested attributes assignment: It occurs when
an association callback changes the "loaded-ness" of the association
during the assignment. After association.loaded? changes from false to
true, the destroy flag fails to function. The first commit illustrates
the issue with a unit test. The test fails in the master branch -
cherry-picked to 3-2-stable, 3-1-stable, 3-0-stable fails the same way.

The second commit is the suggested fix. It replaces a section that
previously addressed the same issue of recognizing if a record was
already in the proxy_target array. When it wasn't, it called
add_to_target, which in turn calls the before_add callback. IMO this is
not correct, because the record concerned already exists in the database
as part of the association. The documentation of the callback states
that an exception may be raised to prevent the record from being added
to the association which contradicts the fact. Therefore I just plainly
appended the record to the proxy_target array. (The unit test in the
first commit pins down this behaviour). The fix is tested on sqlite on
master, 3-2-stable, 3-1-stable. The fix to 3-0-stable is slightly
different, so there is another pull request off of branch
nested_attributes_with_callbacks_bug_3-0 (if I manage to create one...).

Considering the whole method
assign_nested_attributes_for_collection_association - it almost
exclusively calls methods on the association and its logic is deeply
entangled with the implmentation details of the CollectionAssociation
class. I've seen some effort to augment the responsibilites of the
association classes, which IMO points in the right direction. A next
step to clear up this spot would be to move the main part of the method
to something like "CollectionAssociation.attributes=" - any comments!?