diff options
| -rw-r--r-- | activerecord/CHANGELOG | 8 | ||||
| -rw-r--r-- | activerecord/lib/active_record/associations.rb | 3 | ||||
| -rw-r--r-- | activerecord/lib/active_record/associations/has_many_through_association.rb | 17 | ||||
| -rw-r--r-- | activerecord/lib/active_record/associations/through_association.rb | 33 | ||||
| -rw-r--r-- | activerecord/test/cases/associations/has_many_through_associations_test.rb | 100 | ||||
| -rw-r--r-- | activerecord/test/models/person.rb | 27 | ||||
| -rw-r--r-- | activerecord/test/models/reference.rb | 12 | ||||
| -rw-r--r-- | activerecord/test/schema/schema.rb | 1 |
8 files changed, 182 insertions, 19 deletions
diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index aca81b0077..e49915a8cd 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,13 @@ *Rails 3.1.0 (unreleased)* +* Support the :dependent option on has_many :through associations. For historical and practical + reasons, :delete_all is the default deletion strategy employed by association.delete(*records), + despite the fact that the default strategy is :nullify for regular has_many. Also, this only + works at all if the source reflection is a belongs_to. For other situations, you should directly + modify the through association. + + [Jon Leighton] + * Changed the behaviour of association.destroy for has_and_belongs_to_many and has_many :through. From now on, 'destroy' or 'delete' on an association will be taken to mean 'get rid of the link', not (necessarily) 'get rid of the associated records'. diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 398936b3d8..b90838a52b 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1606,12 +1606,11 @@ module ActiveRecord send(reflection.name).each do |o| # No point in executing the counter update since we're going to destroy the parent anyway counter_method = ('belongs_to_counter_cache_before_destroy_for_' + self.class.name.downcase).to_sym - if(o.respond_to? counter_method) then + if o.respond_to?(counter_method) class << o self end.send(:define_method, counter_method, Proc.new {}) end - o.destroy 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 3174ea6373..c98ac79dc0 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -43,19 +43,18 @@ module ActiveRecord end end - # TODO - add dependent option support - def delete_records(records, method = @reflection.options[:dependent]) - through_association = @owner.send(@reflection.through_reflection.name) + def deletion_scope(records) + @owner.send(@reflection.through_reflection.name).where(construct_join_attributes(*records)) + end + def delete_records(records, method = @reflection.options[:dependent]) case method when :destroy - records.each do |record| - through_association.where(construct_join_attributes(record)).destroy_all - end + deletion_scope(records).destroy_all + when :nullify + deletion_scope(records).update_all(@reflection.source_reflection.foreign_key => nil) else - records.each do |record| - through_association.where(construct_join_attributes(record)).delete_all - end + deletion_scope(records).delete_all end end diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index c840a16160..4ae0669c96 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -74,21 +74,40 @@ module ActiveRecord right.create_on(right.create_and(conditions))) end - # Construct attributes for :through pointing to owner and associate. - def construct_join_attributes(associate) - # TODO: revisit this to allow it for deletion, supposing dependent option is supported - raise ActiveRecord::HasManyThroughCantAssociateThroughHasOneOrManyReflection.new(@owner, @reflection) if [:has_one, :has_many].include?(@reflection.source_reflection.macro) + # Construct attributes for :through pointing to owner and associate. This is used by the + # methods which create and delete records on the association. + # + # We only support indirectly modifying through associations which has a belongs_to source. + # This is the "has_many :tags, :through => :taggings" situation, where the join model + # typically has a belongs_to on both side. In other words, associations which could also + # be represented as has_and_belongs_to_many associations. + # + # We do not support creating/deleting records on the association where the source has + # some other type, because this opens up a whole can of worms, and in basically any + # situation it is more natural for the user to just create or modify their join records + # directly as required. + def construct_join_attributes(*records) + if @reflection.source_reflection.macro != :belongs_to + raise HasManyThroughCantAssociateThroughHasOneOrManyReflection.new(@owner, @reflection) + end join_attributes = { @reflection.source_reflection.foreign_key => - associate.send(@reflection.source_reflection.association_primary_key) + records.map { |record| + record.send(@reflection.source_reflection.association_primary_key) + } } if @reflection.options[:source_type] - join_attributes.merge!(@reflection.source_reflection.foreign_type => associate.class.base_class.name) + join_attributes[@reflection.source_reflection.foreign_type] = + records.map { |record| record.class.base_class.name } end - join_attributes + if records.count == 1 + Hash[join_attributes.map { |k, v| [k, v.first] }] + else + join_attributes + end end # The reason that we are operating directly on the scope here (rather than passing 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 6830478107..2aaf5750ba 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -155,6 +155,106 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase end end + def test_delete_through_belongs_to_with_dependent_nullify + Reference.make_comments = true + + person = people(:michael) + job = jobs(:magician) + reference = Reference.where(:job_id => job.id, :person_id => person.id).first + + assert_no_difference ['Job.count', 'Reference.count'] do + assert_difference 'person.jobs.count', -1 do + person.jobs_with_dependent_nullify.delete(job) + end + end + + assert_equal nil, reference.reload.job_id + ensure + Reference.make_comments = false + end + + def test_delete_through_belongs_to_with_dependent_delete_all + Reference.make_comments = true + + person = people(:michael) + job = jobs(:magician) + + # Make sure we're not deleting everything + assert person.jobs.count >= 2 + + assert_no_difference 'Job.count' do + assert_difference ['person.jobs.count', 'Reference.count'], -1 do + person.jobs_with_dependent_delete_all.delete(job) + end + end + + # Check that the destroy callback on Reference did not run + assert_equal nil, person.reload.comments + ensure + Reference.make_comments = false + end + + def test_delete_through_belongs_to_with_dependent_destroy + Reference.make_comments = true + + person = people(:michael) + job = jobs(:magician) + + # Make sure we're not deleting everything + assert person.jobs.count >= 2 + + assert_no_difference 'Job.count' do + assert_difference ['person.jobs.count', 'Reference.count'], -1 do + person.jobs_with_dependent_destroy.delete(job) + end + end + + # Check that the destroy callback on Reference ran + assert_equal "Reference destroyed", person.reload.comments + ensure + Reference.make_comments = false + end + + def test_belongs_to_with_dependent_destroy + person = PersonWithDependentDestroyJobs.find(1) + + # Create a reference which is not linked to a job. This should not be destroyed. + person.references.create! + + assert_no_difference 'Job.count' do + assert_difference 'Reference.count', -person.jobs.count do + person.destroy + end + end + end + + def test_belongs_to_with_dependent_delete_all + person = PersonWithDependentDeleteAllJobs.find(1) + + # Create a reference which is not linked to a job. This should not be destroyed. + person.references.create! + + assert_no_difference 'Job.count' do + assert_difference 'Reference.count', -person.jobs.count do + person.destroy + end + end + end + + def test_belongs_to_with_dependent_nullify + person = PersonWithDependentNullifyJobs.find(1) + + references = person.references.to_a + + assert_no_difference ['Reference.count', 'Job.count'] do + person.destroy + end + + references.each do |reference| + assert_equal nil, reference.reload.job_id + end + end + def test_replace_association assert_queries(4){posts(:welcome);people(:david);people(:michael); posts(:welcome).people(true)} diff --git a/activerecord/test/models/person.rb b/activerecord/test/models/person.rb index bee89de042..a18b9e44df 100644 --- a/activerecord/test/models/person.rb +++ b/activerecord/test/models/person.rb @@ -6,10 +6,14 @@ class Person < ActiveRecord::Base has_many :references has_many :bad_references has_many :fixed_bad_references, :conditions => { :favourite => true }, :class_name => 'BadReference' - has_many :jobs, :through => :references has_one :favourite_reference, :class_name => 'Reference', :conditions => ['favourite=?', true] has_many :posts_with_comments_sorted_by_comment_id, :through => :readers, :source => :post, :include => :comments, :order => 'comments.id' + has_many :jobs, :through => :references + has_many :jobs_with_dependent_destroy, :source => :job, :through => :references, :dependent => :destroy + has_many :jobs_with_dependent_delete_all, :source => :job, :through => :references, :dependent => :delete_all + has_many :jobs_with_dependent_nullify, :source => :job, :through => :references, :dependent => :nullify + belongs_to :primary_contact, :class_name => 'Person' has_many :agents, :class_name => 'Person', :foreign_key => 'primary_contact_id' has_many :agents_of_agents, :through => :agents, :source => :agents @@ -18,3 +22,24 @@ class Person < ActiveRecord::Base scope :males, :conditions => { :gender => 'M' } scope :females, :conditions => { :gender => 'F' } end + +class PersonWithDependentDestroyJobs < ActiveRecord::Base + self.table_name = 'people' + + has_many :references, :foreign_key => :person_id + has_many :jobs, :source => :job, :through => :references, :dependent => :destroy +end + +class PersonWithDependentDeleteAllJobs < ActiveRecord::Base + self.table_name = 'people' + + has_many :references, :foreign_key => :person_id + has_many :jobs, :source => :job, :through => :references, :dependent => :delete_all +end + +class PersonWithDependentNullifyJobs < ActiveRecord::Base + self.table_name = 'people' + + has_many :references, :foreign_key => :person_id + has_many :jobs, :source => :job, :through => :references, :dependent => :nullify +end diff --git a/activerecord/test/models/reference.rb b/activerecord/test/models/reference.rb index 4a17c936f5..06c4f79ef3 100644 --- a/activerecord/test/models/reference.rb +++ b/activerecord/test/models/reference.rb @@ -1,6 +1,18 @@ class Reference < ActiveRecord::Base belongs_to :person belongs_to :job + + class << self + attr_accessor :make_comments + end + + before_destroy :make_comments + + def make_comments + if self.class.make_comments + person.update_attributes :comments => "Reference destroyed" + end + end end class BadReference < ActiveRecord::Base diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 326c336317..09c7b7ba63 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -425,6 +425,7 @@ ActiveRecord::Schema.define do t.string :gender, :limit => 1 t.references :number1_fan t.integer :lock_version, :null => false, :default => 0 + t.string :comments end create_table :pets, :primary_key => :pet_id ,:force => true do |t| |