diff options
3 files changed, 57 insertions, 21 deletions
diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 698a337dcc..fac9ad1188 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -39,6 +39,10 @@ *Rails 3.1.2 (unreleased)* +* Fix adding multiple instances of the same record to a has_many :through. [GH #3425] + + [Jon Leighton] + * Fix creating records in a through association with a polymorphic source type. [GH #3247] [Jon Leighton] 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 2e818dca5d..d82d041b69 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -6,6 +6,11 @@ module ActiveRecord class HasManyThroughAssociation < HasManyAssociation #:nodoc: include ThroughAssociation + def initialize(owner, reflection) + super + @through_records = {} + end + # Returns the size of the collection by executing a SELECT COUNT(*) query if the collection hasn't been # loaded and calling collection.size if it has. If it's more likely than not that the collection does # have a size larger than zero, and you need to fetch that collection afterwards, it'll take one fewer @@ -42,27 +47,42 @@ module ActiveRecord end end - through_record(record).save! + save_through_record(record) update_counter(1) record end private - def through_record(record) - through_association = owner.association(through_reflection.name) - attributes = construct_join_attributes(record) - - through_record = Array.wrap(through_association.target).find { |candidate| - candidate.attributes.slice(*attributes.keys) == attributes - } + def through_association + owner.association(through_reflection.name) + end - unless through_record - through_record = through_association.build(attributes) + # We temporarily cache through record that has been build, because if we build a + # through record in build_record and then subsequently call insert_record, then we + # want to use the exact same object. + # + # However, after insert_record has been called, we clear the cache entry because + # we want it to be possible to have multiple instances of the same record in an + # association + def build_through_record(record) + @through_records[record.object_id] ||= begin + through_record = through_association.build(construct_join_attributes(record)) through_record.send("#{source_reflection.name}=", record) + through_record end + end - through_record + def save_through_record(record) + build_through_record(record).save! + ensure + @through_records.delete(record.object_id) + end + + def through_record(record) + attributes = construct_join_attributes(record) + candidates = Array.wrap(through_association.target) + candidates.find { |c| c.attributes.slice(*attributes.keys) == attributes } end def build_record(attributes, options = {}) @@ -73,9 +93,9 @@ module ActiveRecord inverse = source_reflection.inverse_of if inverse if inverse.macro == :has_many - record.send(inverse.name) << through_record(record) + record.send(inverse.name) << build_through_record(record) elsif inverse.macro == :has_one - record.send("#{inverse.name}=", through_record(record)) + record.send("#{inverse.name}=", build_through_record(record)) end end @@ -104,7 +124,7 @@ module ActiveRecord def delete_records(records, method) ensure_not_nested - through = owner.association(through_reflection.name) + through = through_association scope = through.scoped.where(construct_join_attributes(*records)) case method @@ -126,14 +146,16 @@ module ActiveRecord end def delete_through_records(through, records) - if through_reflection.macro == :has_many - records.each do |record| - through.target.delete(through_record(record)) - end - else - records.each do |record| - through.target = nil if through.target == through_record(record) + records.each do |record| + through_record = through_record(record) + + if through_reflection.macro == :has_many + through.target.delete(through_record) + else + through.target = nil if through.target == through_record end + + @through_records.delete(record.object_id) end end 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 2f4dd9e55c..115e06144e 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -67,6 +67,16 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase end end + def test_associate_existing_record_twice_should_add_records_twice + post = posts(:thinking) + person = people(:david) + + assert_difference 'post.people.count', 2 do + post.people << person + post.people << person + end + end + def test_associating_new assert_queries(1) { posts(:thinking) } new_person = nil # so block binding catches it |