From d55406d2e991056b08f69eb68bcf9b17da807b6c Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 30 Jan 2011 19:07:08 +0000 Subject: Make record.association.destroy(*records) on habtm and hm:t only delete 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. --- .../has_and_belongs_to_many_associations_test.rb | 30 ++- .../has_many_through_associations_test.rb | 12 +- .../test/cases/autosave_association_test.rb | 241 +++++++++++++++------ 3 files changed, 199 insertions(+), 84 deletions(-) (limited to 'activerecord/test') diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb index 126b767d06..5dd2c9861e 100644 --- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb @@ -372,27 +372,34 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase 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 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase 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 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 96f4597726..6830478107 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -128,8 +128,10 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase 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? @@ -137,8 +139,10 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase 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? diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 11c0c5b0ef..c2f2609c9e 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -689,110 +689,207 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase 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_", + "after_adding_#{callback_type}_bird_" + ] - 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}_", - "after_adding_#{callback_type}_#{association_name.singularize}_" - ] + 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_", + "after_adding_#{callback_type}_parrot_" + ] + + 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 -- cgit v1.2.3