Browse files

Make record.association.destroy(*records) on habtm and hm:t only dele…

…te records in the join table. This is to make the destroy method more consistent across the different types of associations. For more details see the CHANGELOG entry.
  • Loading branch information...
1 parent 5f1ea2a commit d55406d2e991056b08f69eb68bcf9b17da807b6c @jonleighton jonleighton committed Jan 30, 2011
View
19 activerecord/CHANGELOG
@@ -1,5 +1,24 @@
*Rails 3.1.0 (unreleased)*
+* 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'.
+
+ Previously, has_and_belongs_to_many.destroy(*records) would destroy the records themselves. It
+ would not delete any records in the join table. Now, it deletes the records in the join table.
+
+ Previously, has_many_through.destroy(*records) would destroy the records themselves, and the
+ records in the join table. [Note: This has not always been the case; previous version of Rails
+ only deleted the records themselves.] Now, it destroys only the records in the join table.
+
+ Note that this change is backwards-incompatible to an extent, but there is unfortunately no
+ way to 'deprecate' it before changing it. The change is being made in order to have
+ consistency as to the meaning of 'destroy' or 'delete' across the different types of associations.
+
+ If you wish to destroy the records themselves, you can do records.association.each(&:destroy)
+
+ [Jon Leighton]
+
* Add :bulk => true option to change_table to make all the schema changes defined in change_table block using a single ALTER statement. [Pratik Naik]
Example:
View
8 activerecord/lib/active_record/associations/association_collection.rb
@@ -192,7 +192,7 @@ def delete(*records)
def destroy(*records)
records = find(records) if records.any? {|record| record.kind_of?(Fixnum) || record.kind_of?(String)}
remove_records(records) do |_records, old_records|
- old_records.each { |record| record.destroy }
+ delete_records(old_records, :destroy) if old_records.any?
end
load_target
@@ -462,6 +462,12 @@ def remove_records(*records)
end
end
+ # Delete the given records from the association, using one of the methods :destroy,
+ # :delete_all or :nullify. The default method used is given by the :dependent option.
+ def delete_records(records, method = @reflection.options[:dependent])
+ raise NotImplementedError
+ end
+
def callback(method, record)
callbacks_for(method).each do |callback|
case callback
View
2 activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -40,7 +40,7 @@ def count_records
load_target.size
end
- def delete_records(records)
+ def delete_records(records, method = nil)
if sql = @reflection.options[:delete_sql]
records.each { |record| @owner.connection.delete(interpolate_sql(sql, record)) }
else
View
22 activerecord/lib/active_record/associations/has_many_association.rb
@@ -54,20 +54,20 @@ def cached_counter_attribute_name
end
# Deletes the records according to the <tt>:dependent</tt> option.
- def delete_records(records)
- case @reflection.options[:dependent]
- when :destroy
- records.each { |r| r.destroy }
- when :delete_all
- @reflection.klass.delete(records.map { |r| r.id })
- else
- updates = { @reflection.foreign_key => nil }
- conditions = { @reflection.association_primary_key => records.map { |r| r.id } }
+ def delete_records(records, method = @reflection.options[:dependent])
+ case method
+ when :destroy
+ records.each { |r| r.destroy }
+ when :delete_all
+ @reflection.klass.delete(records.map { |r| r.id })
+ else
+ updates = { @reflection.foreign_key => nil }
+ conditions = { @reflection.association_primary_key => records.map { |r| r.id } }
- scoped.where(conditions).update_all(updates)
+ scoped.where(conditions).update_all(updates)
end
- if has_cached_counter? && @reflection.options[:dependent] != :destroy
+ if has_cached_counter? && method != :destroy
@owner.class.update_counters(@owner.id, cached_counter_attribute_name => -records.size)
end
end
View
21 activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -8,13 +8,6 @@ class HasManyThroughAssociation < HasManyAssociation #:nodoc:
alias_method :new, :build
- def destroy(*records)
- transaction do
- delete_records(records.flatten)
- super
- end
- 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
@@ -51,10 +44,18 @@ def target_reflection_has_associated_record?
end
# TODO - add dependent option support
- def delete_records(records)
+ def delete_records(records, method = @reflection.options[:dependent])
through_association = @owner.send(@reflection.through_reflection.name)
- records.each do |associate|
- through_association.where(construct_join_attributes(associate)).delete_all
+
+ case method
+ when :destroy
+ records.each do |record|
+ through_association.where(construct_join_attributes(record)).destroy_all
+ end
+ else
+ records.each do |record|
+ through_association.where(construct_join_attributes(record)).delete_all
+ end
end
end
View
30 activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
@@ -372,27 +372,34 @@ def test_removing_associations_on_destroy
def test_destroying
david = Developer.find(1)
- active_record = Project.find(1)
+ project = Project.find(1)
david.projects.reload
assert_equal 2, david.projects.size
- assert_equal 3, active_record.developers.size
+ assert_equal 3, project.developers.size
- assert_difference "Project.count", -1 do
- david.projects.destroy(active_record)
+ assert_no_difference "Project.count" do
+ david.projects.destroy(project)
end
+ join_records = Developer.connection.select_all("SELECT * FROM developers_projects WHERE developer_id = #{david.id} AND project_id = #{project.id}")
+ assert join_records.empty?
+
assert_equal 1, david.reload.projects.size
assert_equal 1, david.projects(true).size
end
- def test_destroying_array
+ def test_destroying_many
david = Developer.find(1)
david.projects.reload
+ projects = Project.all
- assert_difference "Project.count", -Project.count do
- david.projects.destroy(Project.find(:all))
+ assert_no_difference "Project.count" do
+ david.projects.destroy(*projects)
end
+ join_records = Developer.connection.select_all("SELECT * FROM developers_projects WHERE developer_id = #{david.id}")
+ assert join_records.empty?
+
assert_equal 0, david.reload.projects.size
assert_equal 0, david.projects(true).size
end
@@ -401,7 +408,14 @@ def test_destroy_all
david = Developer.find(1)
david.projects.reload
assert !david.projects.empty?
- david.projects.destroy_all
+
+ assert_no_difference "Project.count" do
+ david.projects.destroy_all
+ end
+
+ join_records = Developer.connection.select_all("SELECT * FROM developers_projects WHERE developer_id = #{david.id}")
+ assert join_records.empty?
+
assert david.projects.empty?
assert david.projects(true).empty?
end
View
12 activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -128,17 +128,21 @@ def test_delete_association
end
def test_destroy_association
- assert_difference ["Person.count", "Reader.count"], -1 do
- posts(:welcome).people.destroy(people(:michael))
+ assert_no_difference "Person.count" do
+ assert_difference "Reader.count", -1 do
+ posts(:welcome).people.destroy(people(:michael))
+ end
end
assert posts(:welcome).reload.people.empty?
assert posts(:welcome).people(true).empty?
end
def test_destroy_all
- assert_difference ["Person.count", "Reader.count"], -1 do
- posts(:welcome).people.destroy_all
+ assert_no_difference "Person.count" do
+ assert_difference "Reader.count", -1 do
+ posts(:welcome).people.destroy_all
+ end
end
assert posts(:welcome).reload.people.empty?
View
241 activerecord/test/cases/autosave_association_test.rb
@@ -689,110 +689,207 @@ def test_should_save_changed_child_objects_if_parent_is_saved
assert_equal 'NewName', @parrot.reload.name
end
- # has_many & has_and_belongs_to
- %w{ parrots birds }.each do |association_name|
- define_method("test_should_destroy_#{association_name}_as_part_of_the_save_transaction_if_they_were_marked_for_destroyal") do
- 2.times { |i| @pirate.send(association_name).create!(:name => "#{association_name}_#{i}") }
+ def test_should_destroy_has_many_as_part_of_the_save_transaction_if_they_were_marked_for_destruction
+ 2.times { |i| @pirate.birds.create!(:name => "birds_#{i}") }
- assert !@pirate.send(association_name).any? { |child| child.marked_for_destruction? }
+ assert !@pirate.birds.any? { |child| child.marked_for_destruction? }
- @pirate.send(association_name).each { |child| child.mark_for_destruction }
- klass = @pirate.send(association_name).first.class
- ids = @pirate.send(association_name).map(&:id)
+ @pirate.birds.each { |child| child.mark_for_destruction }
+ klass = @pirate.birds.first.class
+ ids = @pirate.birds.map(&:id)
- assert @pirate.send(association_name).all? { |child| child.marked_for_destruction? }
- ids.each { |id| assert klass.find_by_id(id) }
+ assert @pirate.birds.all? { |child| child.marked_for_destruction? }
+ ids.each { |id| assert klass.find_by_id(id) }
- @pirate.save
- assert @pirate.reload.send(association_name).empty?
- ids.each { |id| assert_nil klass.find_by_id(id) }
+ @pirate.save
+ assert @pirate.reload.birds.empty?
+ ids.each { |id| assert_nil klass.find_by_id(id) }
+ end
+
+ def test_should_skip_validation_on_has_many_if_marked_for_destruction
+ 2.times { |i| @pirate.birds.create!(:name => "birds_#{i}") }
+
+ @pirate.birds.each { |bird| bird.name = '' }
+ assert !@pirate.valid?
+
+ @pirate.birds.each do |bird|
+ bird.mark_for_destruction
+ bird.expects(:valid?).never
end
+ assert_difference("Bird.count", -2) { @pirate.save! }
+ end
+
+ def test_should_skip_validation_on_has_many_if_destroyed
+ @pirate.birds.create!(:name => "birds_1")
- define_method("test_should_skip_validation_on_the_#{association_name}_association_if_marked_for_destruction") do
- 2.times { |i| @pirate.send(association_name).create!(:name => "#{association_name}_#{i}") }
- children = @pirate.send(association_name)
+ @pirate.birds.each { |bird| bird.name = '' }
+ assert !@pirate.valid?
+
+ @pirate.birds.each { |bird| bird.destroy }
+ assert @pirate.valid?
+ end
- children.each { |child| child.name = '' }
- assert !@pirate.valid?
+ def test_a_child_marked_for_destruction_should_not_be_destroyed_twice_while_saving_has_many
+ @pirate.birds.create!(:name => "birds_1")
+
+ @pirate.birds.each { |bird| bird.mark_for_destruction }
+ assert @pirate.save
+
+ @pirate.birds.each { |bird| bird.expects(:destroy).never }
+ assert @pirate.save
+ end
- children.each do |child|
- child.mark_for_destruction
- child.expects(:valid?).never
+ def test_should_rollback_destructions_if_an_exception_occurred_while_saving_has_many
+ 2.times { |i| @pirate.birds.create!(:name => "birds_#{i}") }
+ before = @pirate.birds.map { |c| c.mark_for_destruction ; c }
+
+ # Stub the destroy method of the the second child to raise an exception
+ class << before.last
+ def destroy(*args)
+ super
+ raise 'Oh noes!'
end
- assert_difference("#{association_name.classify}.count", -2) { @pirate.save! }
end
- define_method("test_should_skip_validation_on_the_#{association_name}_association_if_destroyed") do
- @pirate.send(association_name).create!(:name => "#{association_name}_1")
- children = @pirate.send(association_name)
+ assert_raise(RuntimeError) { assert !@pirate.save }
+ assert_equal before, @pirate.reload.birds
+ end
+
+ # Add and remove callbacks tests for association collections.
+ %w{ method proc }.each do |callback_type|
+ define_method("test_should_run_add_callback_#{callback_type}s_for_has_many") do
+ association_name_with_callbacks = "birds_with_#{callback_type}_callbacks"
+
+ pirate = Pirate.new(:catchphrase => "Arr")
+ pirate.send(association_name_with_callbacks).build(:name => "Crowe the One-Eyed")
- children.each { |child| child.name = '' }
- assert !@pirate.valid?
+ expected = [
+ "before_adding_#{callback_type}_bird_<new>",
+ "after_adding_#{callback_type}_bird_<new>"
+ ]
- children.each { |child| child.destroy }
- assert @pirate.valid?
+ assert_equal expected, pirate.ship_log
end
- define_method("test_a_child_marked_for_destruction_should_not_be_destroyed_twice_while_saving_#{association_name}") do
- @pirate.send(association_name).create!(:name => "#{association_name}_1")
- children = @pirate.send(association_name)
+ define_method("test_should_run_remove_callback_#{callback_type}s_for_has_many") do
+ association_name_with_callbacks = "birds_with_#{callback_type}_callbacks"
- children.each { |child| child.mark_for_destruction }
- assert @pirate.save
- children.each { |child| child.expects(:destroy).never }
- assert @pirate.save
+ @pirate.send(association_name_with_callbacks).create!(:name => "Crowe the One-Eyed")
+ @pirate.send(association_name_with_callbacks).each { |c| c.mark_for_destruction }
+ child_id = @pirate.send(association_name_with_callbacks).first.id
+
+ @pirate.ship_log.clear
+ @pirate.save
+
+ expected = [
+ "before_removing_#{callback_type}_bird_#{child_id}",
+ "after_removing_#{callback_type}_bird_#{child_id}"
+ ]
+
+ assert_equal expected, @pirate.ship_log
end
+ end
- define_method("test_should_rollback_destructions_if_an_exception_occurred_while_saving_#{association_name}") do
- 2.times { |i| @pirate.send(association_name).create!(:name => "#{association_name}_#{i}") }
- before = @pirate.send(association_name).map { |c| c.mark_for_destruction ; c }
+ def test_should_destroy_habtm_as_part_of_the_save_transaction_if_they_were_marked_for_destruction
+ 2.times { |i| @pirate.parrots.create!(:name => "parrots_#{i}") }
- # Stub the destroy method of the the second child to raise an exception
- class << before.last
- def destroy(*args)
- super
- raise 'Oh noes!'
- end
- end
+ assert !@pirate.parrots.any? { |parrot| parrot.marked_for_destruction? }
+ @pirate.parrots.each { |parrot| parrot.mark_for_destruction }
- assert_raise(RuntimeError) { assert !@pirate.save }
- assert_equal before, @pirate.reload.send(association_name)
+ assert_no_difference "Parrot.count" do
+ @pirate.save
end
- # Add and remove callbacks tests for association collections.
- %w{ method proc }.each do |callback_type|
- define_method("test_should_run_add_callback_#{callback_type}s_for_#{association_name}") do
- association_name_with_callbacks = "#{association_name}_with_#{callback_type}_callbacks"
+ assert @pirate.reload.parrots.empty?
- pirate = Pirate.new(:catchphrase => "Arr")
- pirate.send(association_name_with_callbacks).build(:name => "Crowe the One-Eyed")
+ join_records = Pirate.connection.select_all("SELECT * FROM parrots_pirates WHERE pirate_id = #{@pirate.id}")
+ assert join_records.empty?
+ end
- expected = [
- "before_adding_#{callback_type}_#{association_name.singularize}_<new>",
- "after_adding_#{callback_type}_#{association_name.singularize}_<new>"
- ]
+ def test_should_skip_validation_on_habtm_if_marked_for_destruction
+ 2.times { |i| @pirate.parrots.create!(:name => "parrots_#{i}") }
- assert_equal expected, pirate.ship_log
- end
+ @pirate.parrots.each { |parrot| parrot.name = '' }
+ assert !@pirate.valid?
+
+ @pirate.parrots.each do |parrot|
+ parrot.mark_for_destruction
+ parrot.expects(:valid?).never
+ end
- define_method("test_should_run_remove_callback_#{callback_type}s_for_#{association_name}") do
- association_name_with_callbacks = "#{association_name}_with_#{callback_type}_callbacks"
+ @pirate.save!
+ assert @pirate.reload.parrots.empty?
+ end
+
+ def test_should_skip_validation_on_habtm_if_destroyed
+ @pirate.parrots.create!(:name => "parrots_1")
- @pirate.send(association_name_with_callbacks).create!(:name => "Crowe the One-Eyed")
- @pirate.send(association_name_with_callbacks).each { |c| c.mark_for_destruction }
- child_id = @pirate.send(association_name_with_callbacks).first.id
+ @pirate.parrots.each { |parrot| parrot.name = '' }
+ assert !@pirate.valid?
- @pirate.ship_log.clear
- @pirate.save
+ @pirate.parrots.each { |parrot| parrot.destroy }
+ assert @pirate.valid?
+ end
+
+ def test_a_child_marked_for_destruction_should_not_be_destroyed_twice_while_saving_habtm
+ @pirate.parrots.create!(:name => "parrots_1")
+
+ @pirate.parrots.each { |parrot| parrot.mark_for_destruction }
+ assert @pirate.save
+
+ assert_queries(1) do
+ assert @pirate.save
+ end
+ end
- expected = [
- "before_removing_#{callback_type}_#{association_name.singularize}_#{child_id}",
- "after_removing_#{callback_type}_#{association_name.singularize}_#{child_id}"
- ]
+ def test_should_rollback_destructions_if_an_exception_occurred_while_saving_habtm
+ 2.times { |i| @pirate.parrots.create!(:name => "parrots_#{i}") }
+ before = @pirate.parrots.map { |c| c.mark_for_destruction ; c }
- assert_equal expected, @pirate.ship_log
+ class << @pirate.parrots
+ def destroy(*args)
+ super
+ raise 'Oh noes!'
end
end
+
+ assert_raise(RuntimeError) { assert !@pirate.save }
+ assert_equal before, @pirate.reload.parrots
+ end
+
+ # Add and remove callbacks tests for association collections.
+ %w{ method proc }.each do |callback_type|
+ define_method("test_should_run_add_callback_#{callback_type}s_for_habtm") do
+ association_name_with_callbacks = "parrots_with_#{callback_type}_callbacks"
+
+ pirate = Pirate.new(:catchphrase => "Arr")
+ pirate.send(association_name_with_callbacks).build(:name => "Crowe the One-Eyed")
+
+ expected = [
+ "before_adding_#{callback_type}_parrot_<new>",
+ "after_adding_#{callback_type}_parrot_<new>"
+ ]
+
+ assert_equal expected, pirate.ship_log
+ end
+
+ define_method("test_should_run_remove_callback_#{callback_type}s_for_habtm") do
+ association_name_with_callbacks = "parrots_with_#{callback_type}_callbacks"
+
+ @pirate.send(association_name_with_callbacks).create!(:name => "Crowe the One-Eyed")
+ @pirate.send(association_name_with_callbacks).each { |c| c.mark_for_destruction }
+ child_id = @pirate.send(association_name_with_callbacks).first.id
+
+ @pirate.ship_log.clear
+ @pirate.save
+
+ expected = [
+ "before_removing_#{callback_type}_parrot_#{child_id}",
+ "after_removing_#{callback_type}_parrot_#{child_id}"
+ ]
+
+ assert_equal expected, @pirate.ship_log
+ end
end
end

0 comments on commit d55406d

Please sign in to comment.