From f319e4a9421d8815717fd6ca6191268fed9e536d Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Mon, 13 May 2013 17:10:23 -0400 Subject: Do not invoke callbacks when delete_all is called Method `delete_all` should not be invoking callbacks and this feature was deprecated in Rails 4.0. This is being removed. `delete_all` will continue to honor the `:dependent` option. However if `:dependent` value is `:destroy` then the default deletion strategy for that collection will be applied. User can also force a deletion strategy by passing parameter to `delete_all`. For example you can do `@post.comments.delete_all(:nullify)` --- activerecord/CHANGELOG.md | 13 +++++++ .../associations/collection_association.rb | 42 +++++++++++++++------- .../active_record/associations/collection_proxy.rb | 4 +-- .../associations/has_many_associations_test.rb | 27 ++++++++++++-- .../has_many_through_associations_test.rb | 34 ++++++++++++++++++ activerecord/test/cases/locking_test.rb | 1 + activerecord/test/models/bulb.rb | 6 ++++ activerecord/test/models/car.rb | 1 + activerecord/test/models/parrot.rb | 6 ++++ 9 files changed, 117 insertions(+), 17 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index d6354a75f4..9370ec6456 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,16 @@ +* Do not invoke callbacks when `delete_all` is called on collection. + + Method `delete_all` should not be invoking callbacks and this + feature was deprecated in Rails 4.0. This is being removed. + `delete_all` will continue to honor the `:dependent` option. However + if `:dependent` value is `:destroy` then the default deletion + strategy for that collection will be applied. + + User can also force a deletion strategy by passing parameter to + `delete_all`. For example you can do `@post.comments.delete_all(:nullify)` . + + *Neeraj Singh* + * Remove implicit join references that were deprecated in 4.0. Example: diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 9833822f8f..dbe97cabaf 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -153,11 +153,35 @@ module ActiveRecord end end - # Remove all records from this association. + # Removes all records from the association without calling callbacks + # on the associated records. It honors the `:dependent` option. However + # if the `:dependent` value is `:destroy` then in that case the default + # deletion strategy for the association is applied. + # + # You can force a particular deletion strategy by passing a parameter. + # + # Example: + # + # @author.books.delete_all(:nullify) + # @author.books.delete_all(:delete_all) # # See delete for more info. - def delete_all - delete(:all).tap do + def delete_all(dependent = nil) + if dependent.present? && ![:nullify, :delete_all].include?(dependent) + raise ArgumentError, "Valid values are :nullify or :delete_all" + end + + dependent = if dependent.present? + dependent + elsif options[:dependent] == :destroy + # since delete_all should not invoke callbacks so use the default deletion strategy + # for :destroy + reflection.is_a?(ActiveRecord::Reflection::ThroughReflection) ? :delete_all : :nullify + else + options[:dependent] + end + + delete(:all, dependent: dependent).tap do reset loaded! end @@ -214,18 +238,10 @@ module ActiveRecord # are actually removed from the database, that depends precisely on # +delete_records+. They are in any case removed from the collection. def delete(*records) - dependent = options[:dependent] + _options = records.extract_options! + dependent = _options[:dependent] || options[:dependent] if records.first == :all - - if dependent && dependent == :destroy - message = 'In Rails 4.1 delete_all on associations would not fire callbacks. ' \ - 'It means if the :dependent option is :destroy then the associated ' \ - 'records would be deleted without loading and invoking callbacks.' - - ActiveRecord::Base.logger ? ActiveRecord::Base.logger.warn(message) : $stderr.puts(message) - end - if loaded? || dependent == :destroy delete_or_destroy(load_target, dependent) else diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb index f836d45d0e..9381bd4456 100644 --- a/activerecord/lib/active_record/associations/collection_proxy.rb +++ b/activerecord/lib/active_record/associations/collection_proxy.rb @@ -417,8 +417,8 @@ module ActiveRecord # # Pet.find(1, 2, 3) # # => ActiveRecord::RecordNotFound - def delete_all - @association.delete_all + def delete_all(dependent = nil) + @association.delete_all(dependent) end # Deletes the records of the collection directly from the database diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 8f5e18b863..fc40f95e26 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -148,6 +148,14 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_equal 'defaulty', bulb.name end + def test_do_not_call_callbacks_for_delete_all + bulb_count = Bulb.count + car = Car.create(:name => 'honda') + car.funky_bulbs.create! + assert_nothing_raised { car.reload.funky_bulbs.delete_all } + assert_equal bulb_count + 1, Bulb.count, "bulbs should have been deleted using :nullify strategey" + end + def test_building_the_associated_object_with_implicit_sti_base_class firm = DependentFirm.new company = firm.companies.build @@ -930,18 +938,33 @@ class HasManyAssociationsTest < ActiveRecord::TestCase firm = companies(:first_firm) client_id = firm.dependent_clients_of_firm.first.id assert_equal 1, firm.dependent_clients_of_firm.size + assert_equal 1, Client.find_by_id(client_id).client_of - # :dependent means destroy is called on each client + # :nullify is called on each client firm.dependent_clients_of_firm.clear assert_equal 0, firm.dependent_clients_of_firm.size assert_equal 0, firm.dependent_clients_of_firm(true).size - assert_equal [client_id], Client.destroyed_client_ids[firm.id] + assert_equal [], Client.destroyed_client_ids[firm.id] # Should be destroyed since the association is dependent. + assert_nil Client.find_by_id(client_id).client_of + end + + def test_delete_all_with_option_delete_all + firm = companies(:first_firm) + client_id = firm.dependent_clients_of_firm.first.id + firm.dependent_clients_of_firm.delete_all(:delete_all) assert_nil Client.find_by_id(client_id) end + def test_delete_all_accepts_limited_parameters + firm = companies(:first_firm) + assert_raise(ArgumentError) do + firm.dependent_clients_of_firm.delete_all(:destroy) + end + end + def test_clearing_an_exclusively_dependent_association_collection firm = companies(:first_firm) client_id = firm.exclusively_dependent_clients_of_firm.first.id 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 70c6b489aa..ee88fd490a 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -57,6 +57,40 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert post.reload.people(true).include?(person) end + def test_delete_all_for_with_dependent_option_destroy + person = people(:david) + assert_equal 1, person.jobs_with_dependent_destroy.count + + assert_no_difference 'Job.count' do + assert_difference 'Reference.count', -1 do + person.reload.jobs_with_dependent_destroy.delete_all + end + end + end + + def test_delete_all_for_with_dependent_option_nullify + person = people(:david) + assert_equal 1, person.jobs_with_dependent_nullify.count + + assert_no_difference 'Job.count' do + assert_no_difference 'Reference.count' do + person.reload.jobs_with_dependent_nullify.delete_all + end + end + end + + def test_delete_all_for_with_dependent_option_delete_all + person = people(:david) + assert_equal 1, person.jobs_with_dependent_delete_all.count + + assert_no_difference 'Job.count' do + assert_difference 'Reference.count', -1 do + person.reload.jobs_with_dependent_delete_all.delete_all + end + end + end + + def test_associate_existing_record_twice_should_add_to_target_twice post = posts(:thinking) person = people(:david) diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb index 0030f1b464..827fcf3d50 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -8,6 +8,7 @@ require 'models/legacy_thing' require 'models/reference' require 'models/string_key_object' require 'models/car' +require 'models/bulb' require 'models/engine' require 'models/wheel' require 'models/treasure' diff --git a/activerecord/test/models/bulb.rb b/activerecord/test/models/bulb.rb index 0109ef4f83..4361188e21 100644 --- a/activerecord/test/models/bulb.rb +++ b/activerecord/test/models/bulb.rb @@ -37,3 +37,9 @@ class CustomBulb < Bulb self.frickinawesome = true if name == 'Dude' end end + +class FunkyBulb < Bulb + before_destroy do + raise "before_destroy was called" + end +end diff --git a/activerecord/test/models/car.rb b/activerecord/test/models/car.rb index ac42f444e1..a14a9febba 100644 --- a/activerecord/test/models/car.rb +++ b/activerecord/test/models/car.rb @@ -1,6 +1,7 @@ class Car < ActiveRecord::Base has_many :bulbs + has_many :funky_bulbs, class_name: 'FunkyBulb', dependent: :destroy has_many :foo_bulbs, -> { where(:name => 'foo') }, :class_name => "Bulb" has_many :frickinawesome_bulbs, -> { where :frickinawesome => true }, :class_name => "Bulb" diff --git a/activerecord/test/models/parrot.rb b/activerecord/test/models/parrot.rb index c4ee2bd19d..e76e83f314 100644 --- a/activerecord/test/models/parrot.rb +++ b/activerecord/test/models/parrot.rb @@ -21,3 +21,9 @@ end class DeadParrot < Parrot belongs_to :killer, :class_name => 'Pirate' end + +class FunkyParrot < Parrot + before_destroy do + raise "before_destroy was called" + end +end -- cgit v1.2.3