Fix handling of duplicates for `replace` on has_many-through

There was a bug in the handling of duplicates when
assigning (replacing) associated records, which made the result
dependent on whether a given record was associated already before
being assigned anew. E.g.

    post.people = [person, person]
    post.people.count
    # => 2

while

    post.people = [person]
    post.people = [person, person]
    post.people.count
    # => 1

This change adds a test to provoke the former incorrect behavior, and
fixes it.

Cause of the bug was the handling of record collections as sets, and
using `-` (difference) and `&` (union) operations on them
indiscriminately. This temporary conversion to sets would eliminate
duplicates.

The fix is to decorate record collections for these operations, and
only for the `has_many :through` case. It is done by counting
occurrences, and use the record together with the occurrence number as
element, in order to make them work well in sets. Given

    a, b = *Person.all

then the collection used for finding the difference or union of
records would be internally changed from

     [a, b, a]

to

     [[a, 1], [b, 1], [a, 2]]

for these operations. So a first occurrence and a second occurrence
would be distinguishable, which is all that is necessary for this
task.

Fixes #33942.

4 files changed, 51 insertions, 3 deletions

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 840d900bbc..17f028c1a2 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -413,9 +413,9 @@ module ActiveRecord
         end
 
         def replace_records(new_target, original_target)
-          delete(target - new_target)
+          delete(difference(target, new_target))
 
-          unless concat(new_target - target)
+          unless concat(difference(new_target, target))
             @target = original_target
             raise RecordNotSaved, "Failed to replace #{reflection.name} because one or more of the " \
                                   "new records could not be saved."
@@ -425,7 +425,7 @@ module ActiveRecord
         end
 
         def replace_common_records_in_memory(new_target, original_target)
-          common_records = new_target & original_target
+          common_records = union(new_target, original_target)
           common_records.each do |record|
             skip_callbacks = true
             replace_on_target(record, @target.index(record), skip_callbacks)
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index cf85a87fa7..6beec4dea4 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -130,6 +130,14 @@ module ActiveRecord
           end
           saved_successfully
         end
+
+        def difference(a, b)
+          a - b
+        end
+
+        def union(a, b)
+          a & b
+        end
     end
   end
 end
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index f84ac65fa2..b30d3ddac2 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -163,6 +163,36 @@ module ActiveRecord
           end
         end
 
+        def difference(a, b)
+          set_a = as_set(a)
+          set_b = as_set(b)
+
+          from_set(set_a - set_b)
+        end
+
+        def union(a, b)
+          set_a = as_set(a)
+          set_b = as_set(b)
+
+          from_set(set_a & set_b)
+        end
+
+        def as_set(records)
+          records.zip(occurences(records))
+        end
+
+        def from_set(record_set)
+          record_set.map(&:first)
+        end
+
+        def occurences(array)
+          counts = Hash.new(0)
+
+          array.map do |object|
+            counts[object] += 1
+          end
+        end
+
         def through_records_for(record)
           attributes = construct_join_attributes(record)
           candidates = Array.wrap(through_association.target)
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index 7b405c74c4..cf514957ca 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -586,6 +586,16 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     assert_not_includes posts(:welcome).reload.people.reload, people(:michael)
   end
 
+  def test_replace_association_with_duplicates
+    post   = posts(:thinking)
+    person = people(:david)
+
+    assert_difference "post.people.count", 2 do
+      post.people = [person]
+      post.people = [person, person]
+    end
+  end
+
   def test_replace_order_is_preserved
     posts(:welcome).people.clear
     posts(:welcome).people = [people(:david), people(:michael)]