From 8104589c0824c648a769be50e04fc8e7dbb26ba0 Mon Sep 17 00:00:00 2001 From: Florian Ebeling Date: Sun, 23 Sep 2018 15:18:57 +0200 Subject: 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. --- .../associations/collection_association.rb | 6 ++--- .../associations/has_many_association.rb | 8 ++++++ .../associations/has_many_through_association.rb | 30 ++++++++++++++++++++++ .../has_many_through_associations_test.rb | 10 ++++++++ 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)] -- cgit v1.2.3 From 11bad94237b17c078eb5cac8557e9ed9e37ec274 Mon Sep 17 00:00:00 2001 From: Florian Ebeling Date: Sun, 23 Sep 2018 17:56:05 +0200 Subject: Rename union to intersection --- activerecord/lib/active_record/associations/collection_association.rb | 2 +- activerecord/lib/active_record/associations/has_many_association.rb | 2 +- .../lib/active_record/associations/has_many_through_association.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 17f028c1a2..48bb9ab066 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -425,7 +425,7 @@ module ActiveRecord end def replace_common_records_in_memory(new_target, original_target) - common_records = union(new_target, original_target) + common_records = intersection(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 6beec4dea4..e224d3456a 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -135,7 +135,7 @@ module ActiveRecord a - b end - def union(a, b) + def intersection(a, b) a & b 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 b30d3ddac2..8a8149b777 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -170,7 +170,7 @@ module ActiveRecord from_set(set_a - set_b) end - def union(a, b) + def intersection(a, b) set_a = as_set(a) set_b = as_set(b) -- cgit v1.2.3 From f9157587c9d61a0e24a0dcba7b31aa698ddeb641 Mon Sep 17 00:00:00 2001 From: Florian Ebeling Date: Thu, 27 Sep 2018 10:19:08 +0200 Subject: Optimize difference and intersection --- .../associations/has_many_through_association.rb | 26 ++++++++-------------- 1 file changed, 9 insertions(+), 17 deletions(-) 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 8a8149b777..2322a49931 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -164,32 +164,24 @@ module ActiveRecord end def difference(a, b) - set_a = as_set(a) - set_b = as_set(b) + distribution = distribution(b) - from_set(set_a - set_b) + a.reject { |record| mark_occurrence(distribution, record) } end def intersection(a, b) - set_a = as_set(a) - set_b = as_set(b) + distribution = distribution(b) - from_set(set_a & set_b) + a.select { |record| mark_occurrence(distribution, record) } end - def as_set(records) - records.zip(occurences(records)) + def mark_occurrence(distribution, record) + distribution[record] > 0 && distribution[record] -= 1 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 + def distribution(array) + array.each_with_object(Hash.new(0)) do |record, distribution| + distribution[record] += 1 end end -- cgit v1.2.3