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.

activerecord/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   `add_to_target` now accepts a second optional `skip_callbacks` argument
+
+    If truthy, it will skip the :before_add and :after_add callbacks.
+
+    *Ben Woosley*
+
 *   Fix interactions between `:before_add` callbacks and nested attributes
     assignment of `has_many` associations, when the association was not
     yet loaded:

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)
+      def add_to_target(record, skip_callbacks = false)
-        callback(:before_add, record)
+        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

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
+          unless call_reject_if(association_name, attributes)
-          # proxy_target array (either by finding it, or adding it if not found)
+            # Make sure we are operating on the actual object which is in the association's
-          # Take into account that the proxy_target may have changed due to callbacks
+            # proxy_target array (either by finding it, or adding it if not found)
-          target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s }
+            # Take into account that the proxy_target may have changed due to callbacks
-          if target_record
+            target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s }
-            existing_record = target_record
+            if target_record
-          else
+              existing_record = target_record
-            #FIXME: there is no good way of adding the record without callback
+            else
-            association.target << existing_record
+              association.add_to_target(existing_record, :skip_callbacks)
-          end
+            end
 
-          if !call_reject_if(association_name, attributes)
             assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
           end
         else