Setting an association replaces records with the same id in memory

The records weren't being replaced since equality in Active Record is
defined in terms of `id` only. It is reasonable to expect that the
references would be replaced in memory, even if no queries are actually
executed. This change did not appear to affect any other parts of the
code base. I chose not to execute callbacks since we're not actually
modifying the association in a way that will be persisted.

Fixes #17730

2 files changed, 82 insertions, 2 deletions

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index d59cebb08b..2b7d39893d 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -358,6 +358,7 @@ module ActiveRecord
         if owner.new_record?
           replace_records(other_array, original_target)
         else
+          replace_common_records_in_memory(other_array, original_target)
           if other_array != original_target
             transaction { replace_records(other_array, original_target) }
           end
@@ -385,11 +386,18 @@ module ActiveRecord
         target
       end
 
-      def add_to_target(record, skip_callbacks = false)
+      def add_to_target(record, skip_callbacks = false, &block)
+        if association_scope.distinct_value
+          index = @target.index(record)
+        end
+        replace_on_target(record, index, skip_callbacks, &block)
+      end
+
+      def replace_on_target(record, index, skip_callbacks)
         callback(:before_add, record) unless skip_callbacks
         yield(record) if block_given?
 
-        if association_scope.distinct_value && index = @target.index(record)
+        if index
           @target[index] = record
         else
           @target << record
@@ -534,6 +542,14 @@ module ActiveRecord
           target
         end
 
+        def replace_common_records_in_memory(new_target, original_target)
+          common_records = new_target & original_target
+          common_records.each do |record|
+            skip_callbacks = true
+            replace_on_target(record, @target.index(record), skip_callbacks)
+          end
+        end
+
         def concat_records(records, should_raise = false)
           result = true
 
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index 5a963b6458..8b7ab11570 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -1997,4 +1997,68 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal 1, car.bulbs.count
     assert_equal 1, car.tyres.count
   end
+
+  test 'associations replace in memory when records have the same id' do
+    bulb = Bulb.create!
+    car = Car.create!(bulbs: [bulb])
+
+    new_bulb = Bulb.find(bulb.id)
+    new_bulb.name = "foo"
+    car.bulbs = [new_bulb]
+
+    assert_equal "foo", car.bulbs.first.name
+  end
+
+  test 'in memory replacement executes no queries' do
+    bulb = Bulb.create!
+    car = Car.create!(bulbs: [bulb])
+
+    new_bulb = Bulb.find(bulb.id)
+
+    assert_no_queries do
+      car.bulbs = [new_bulb]
+    end
+  end
+
+  test 'in memory replacements do not execute callbacks' do
+    raise_after_add = false
+    klass = Class.new(ActiveRecord::Base) do
+      self.table_name = :cars
+      has_many :bulbs, after_add: proc { raise if raise_after_add }
+
+      def self.name
+        "Car"
+      end
+    end
+    bulb = Bulb.create!
+    car = klass.create!(bulbs: [bulb])
+
+    new_bulb = Bulb.find(bulb.id)
+    raise_after_add = true
+
+    assert_nothing_raised do
+      car.bulbs = [new_bulb]
+    end
+  end
+
+  test 'in memory replacements sets inverse instance' do
+    bulb = Bulb.create!
+    car = Car.create!(bulbs: [bulb])
+
+    new_bulb = Bulb.find(bulb.id)
+    car.bulbs = [new_bulb]
+
+    assert_same car, new_bulb.car
+  end
+
+  test 'in memory replacement maintains order' do
+    first_bulb = Bulb.create!
+    second_bulb = Bulb.create!
+    car = Car.create!(bulbs: [first_bulb, second_bulb])
+
+    same_bulb = Bulb.find(first_bulb.id)
+    car.bulbs = [second_bulb, same_bulb]
+
+    assert_equal [first_bulb, second_bulb], car.bulbs
+  end
 end