Fix interactions between :before_add callbacks and nested attributes assignment

Issue #1: :before_add callback is called when nested attributes assignment assigns to existing record if the association is not yet loaded
Issue #2: Nested Attributes assignment does not affect the record in the association target when callback triggers loading of the association

3 files changed, 168 insertions, 11 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 3010f42cd6..6b3e5df1a7 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,5 +1,18 @@
+*   Fix interactions between `:before_add` callbacks and nested attributes
+    assignment of `has_many` associations, when the association was not
+    yet loaded:
+
+    - A `:before_add` callback was being called when a nested attributes
+      assignment assigned to an existing record.
+
+    - Nested Attributes assignment did not affect the record in the
+      association target when a `:before_add` callback triggered the
+      loading of the association
+
+    *Jörg Schray*
+
 *   Allow enable_extension migration method to be revertible.
- 
+
     *Eric Tipton*
 
 *   Type cast hstore values on write, so that the value is consistent
diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb
index e53e8553ad..403ad98c3b 100644
--- a/activerecord/lib/active_record/nested_attributes.rb
+++ b/activerecord/lib/active_record/nested_attributes.rb
@@ -465,16 +465,15 @@ module ActiveRecord
             association.build(attributes.except(*UNASSIGNABLE_KEYS))
           end
         elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s }
-          unless association.loaded? || call_reject_if(association_name, attributes)
-            # Make sure we are operating on the actual object which is in the association's
-            # proxy_target array (either by finding it, or adding it if not found)
-            target_record = association.target.detect { |record| record == existing_record }
-
-            if target_record
-              existing_record = target_record
-            else
-              association.add_to_target(existing_record)
-            end
+          # Make sure we are operating on the actual object which is in the association's
+          # proxy_target array (either by finding it, or adding it if not found)
+          # Take into account that the proxy_target may have changed due to callbacks
+          target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s }
+          if target_record
+            existing_record = target_record
+          else
+            #FIXME: there is no good way of adding the record without callback
+            association.target << existing_record
           end
 
           if !call_reject_if(association_name, attributes)
diff --git a/activerecord/test/cases/nested_attributes_with_callbacks_test.rb b/activerecord/test/cases/nested_attributes_with_callbacks_test.rb
new file mode 100644
index 0000000000..95989ffed4
--- /dev/null
+++ b/activerecord/test/cases/nested_attributes_with_callbacks_test.rb
@@ -0,0 +1,145 @@
+require "cases/helper"
+require "models/pirate"
+require "models/bird"
+
+class TestNestedAttributesWithCallbacksInterferingWithAssignment < ActiveRecord::TestCase
+  Pirate.has_many(:birds_with_interfering_callback,
+                  :class_name => "Bird",
+                  :before_add => proc { |p,b|
+                    @@add_callback_called << b
+                    p.birds_with_interfering_callback.to_a
+                  })
+  Pirate.has_many(:birds_with_callback,
+                  :class_name => "Bird",
+                  :before_add => proc { |p,b| @@add_callback_called << b })
+
+  Pirate.accepts_nested_attributes_for(:birds_with_interfering_callback,
+                                       :birds_with_callback,
+                                       :allow_destroy => true)
+
+  def setup
+    @@add_callback_called = []
+    @expect_callbacks_for = []
+    @pirate_with_two_birds = Pirate.new.tap do |pirate|
+      pirate.catchphrase = "Don't call me!"
+      pirate.birds_attributes = [{:name => 'Bird1'},{:name => 'Bird2'}]
+      pirate.save!
+    end
+    @birds = @pirate_with_two_birds.birds.to_a
+    @birds_attributes = @birds.map do |bird|
+      bird.attributes.slice("id","name")
+    end
+  end
+
+  def new_birds
+    @pirate_with_two_birds.birds_with_callback.to_a - @birds
+  end
+
+  def new_bird_attributes
+     [{'name' => "New Bird"}]
+  end
+
+  def bird2_deletion_attributes
+    [{'id' => @birds[1].id.to_s, "_destroy" => true}]
+  end
+
+  def update_new_and_destroy_bird_attributes
+    [{'id' => @birds[0].id.to_s, 'name' => 'New Name'},
+     {'name' => "New Bird"},
+     {'id' => @birds[1].id.to_s, "_destroy" => true}]
+  end
+
+  # Characterizing when :before_add callback is called
+  test ":before_add called for new bird when not loaded" do
+    assert_birds_with_callback_not_loaded
+    assert_callback_called_for_new_bird_assignment
+  end
+
+  test ":before_add called for new bird when loaded" do
+    @pirate_with_two_birds.birds_with_callback.load_target
+    assert_callback_called_for_new_bird_assignment
+  end
+
+  def assert_callback_called_for_new_bird_assignment
+    @pirate_with_two_birds.birds_with_callback_attributes = new_bird_attributes
+    assert_equal(1,new_birds.size)
+    assert_callback_called_for_new_birds
+  end
+
+  test ":before_add not called for identical assignment when not loaded" do
+    assert_birds_with_callback_not_loaded
+    assert_callback_not_called_for_identical_assignment
+  end
+
+  test ":before_add not called for identical assignment when loaded" do
+    @pirate_with_two_birds.birds_with_callback.load_target
+    assert_callback_not_called_for_identical_assignment
+  end
+
+  def assert_callback_not_called_for_identical_assignment
+    @pirate_with_two_birds.birds_with_callback_attributes = @birds_attributes
+    assert_equal([],new_birds)
+    assert_callback_called_for_new_birds
+  end
+
+  test ":before_add not called for destroy assignment when not loaded" do
+    assert_birds_with_callback_not_loaded
+    assert_callback_not_called_for_destroy_assignment
+  end
+
+  test ":before_add not called for destroy assignment when loaded" do
+    @pirate_with_two_birds.birds_with_callback.load_target
+    assert_callback_not_called_for_destroy_assignment
+  end
+
+  def assert_callback_not_called_for_destroy_assignment
+    @pirate_with_two_birds.birds_with_callback_attributes =
+      bird2_deletion_attributes
+    assert_callback_called_for_new_birds
+  end
+
+  def assert_birds_with_callback_not_loaded
+    assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?)
+  end
+
+  def assert_callback_called_for_new_birds
+    assert_equal(new_birds,@@add_callback_called)
+  end
+
+  # Ensuring that the records in the association target are updated,
+  # whether the association is loaded before or not
+  test "Assignment updates records in target when not loaded" do
+    assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?)
+    assert_assignment_affects_records_in_target(:birds_with_callback)
+  end
+
+  test "Assignment updates records in target when loaded" do
+    @pirate_with_two_birds.birds_with_callback.load_target
+    assert_assignment_affects_records_in_target(:birds_with_callback)
+  end
+
+  test("Assignment updates records in target when not loaded" +
+       " and callback loads target") do
+    assert_equal(false,
+                 @pirate_with_two_birds.birds_with_interfering_callback.loaded?)
+    assert_assignment_affects_records_in_target(
+     :birds_with_interfering_callback)
+  end
+
+  test("Assignment updates records in target when loaded" +
+       " and callback loads target") do
+    @pirate_with_two_birds.birds_with_interfering_callback.load_target
+    assert_assignment_affects_records_in_target(
+     :birds_with_interfering_callback)
+  end
+
+  def assert_assignment_affects_records_in_target(association_name)
+    @pirate_with_two_birds.send("#{association_name}_attributes=",
+                                update_new_and_destroy_bird_attributes)
+    association = @pirate_with_two_birds.send(association_name)
+    birds_in_target = @birds.map { |b| association.detect { |b_in_t| b_in_t == b }}
+    assert_equal(true,birds_in_target[0].name_changed?,'First record not changed')
+    assert_equal(true,birds_in_target[1].marked_for_destruction?,
+                 'Second record not marked for destruction')
+  end
+end