Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fix adding multiple instances of the same record to a has_many :through.

Fixes #3425.
  • Loading branch information...
commit 175c02d630682de02f2a5ca3790272c525f0999f 1 parent 01eae34
@jonleighton jonleighton authored
View
4 activerecord/CHANGELOG
@@ -1,5 +1,9 @@
*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]
View
64 activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -6,6 +6,11 @@ module Associations
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 @@ def insert_record(record, validate = true, raise = false)
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 @@ def build_record(attributes, options = {})
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 @@ def update_through_counter?(method)
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 @@ def delete_records(records, method)
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
View
10 activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -67,6 +67,16 @@ def test_associate_existing_record_twice_should_add_to_target_twice
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
Please sign in to comment.
Something went wrong with that request. Please try again.