Restore the use of `#add_to_target` for nested attribute updates on existing records, and don't bother updating the association if the update is going to be rejected anyway.

This requires adding a `skip_callbacks` argument to `#add_to_target`
so that we don't call the callbacks multiple times in this case,
which is functionally an application of existing association data,
rather than an addition of a new record to the association.

2 files changed, 14 insertions, 15 deletions

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 8ce02afef8..228c500f0a 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -290,7 +290,7 @@ module ActiveRecord
 
       # Returns true if the collection is empty.
       #
-      # If the collection has been loaded 
+      # If the collection has been loaded
       # it is equivalent to <tt>collection.size.zero?</tt>. If the
       # collection has not been loaded, it is equivalent to
       # <tt>collection.exists?</tt>. If the collection has not already been
@@ -366,8 +366,8 @@ module ActiveRecord
         target
       end
 
-      def add_to_target(record)
-        callback(:before_add, record)
+      def add_to_target(record, skip_callbacks = false)
+        callback(:before_add, record) unless skip_callbacks
         yield(record) if block_given?
 
         if association_scope.distinct_value && index = @target.index(record)
@@ -376,7 +376,7 @@ module ActiveRecord
           @target << record
         end
 
-        callback(:after_add, record)
+        callback(:after_add, record) unless skip_callbacks
         set_inverse_instance(record)
 
         record
diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb
index 403ad98c3b..df28451bb7 100644
--- a/activerecord/lib/active_record/nested_attributes.rb
+++ b/activerecord/lib/active_record/nested_attributes.rb
@@ -465,18 +465,17 @@ module ActiveRecord
             association.build(attributes.except(*UNASSIGNABLE_KEYS))
           end
         elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s }
-          # 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
+          unless 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)
+            # 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
+              association.add_to_target(existing_record, :skip_callbacks)
+            end
 
-          if !call_reject_if(association_name, attributes)
             assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
           end
         else