diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index d6354a75f41ff7fc32534959f03e7aa3d3bdaa5f..9370ec6456978d69ba3b1791af87dc9bcf6fdee5 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 9833822f8f2183f9730a39e1895e4dad88ca78f6..dbe97cabaf4acbe8ce211c6bd75058d420114b46 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -153,11 +153,35 @@ def transaction(*args) 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 @@ def count(column_name = nil, count_options = {}) # 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 f836d45d0e88285d6968321c42cd70847e6f555c..9381bd4456b347b27fef5e761392a870c40f653f 100644 --- a/activerecord/lib/active_record/associations/collection_proxy.rb +++ b/activerecord/lib/active_record/associations/collection_proxy.rb @@ -417,8 +417,8 @@ def replace(other_array) # # 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 8f5e18b863adb443fdeff039cffba697b4bd1aa8..fc40f95e26a1662d2c33d0c63d635aea113327a4 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 @@ def test_create_from_association_with_nil_values_should_work 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 @@ def test_clearing_a_dependent_association_collection 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 70c6b489aa1ffefe1fe368ee085b0cde3be3fe50..ee88fd490afaa9074f01fb3fca8479aeba31a840 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 @@ def test_associate_existing 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 0030f1b464c0b9136938e5c42620f248e4f3818f..827fcf3d50262a93afddb077d70ee834071b6011 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -8,6 +8,7 @@ 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 0109ef4f8355d107ed9eda47c725a263352e9c65..4361188e218a6acade34abff2c3e010856d6a6e6 100644 --- a/activerecord/test/models/bulb.rb +++ b/activerecord/test/models/bulb.rb @@ -37,3 +37,9 @@ def set_awesomeness 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 ac42f444e193675745dff828956c3215fbc15781..a14a9febba524001503336c1052ba4447a555974 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 c4ee2bd19d16ed79a24d187e3b2d36fa967d3ce8..e76e83f314e51e679231686fd4f762b86cbd5223 100644 --- a/activerecord/test/models/parrot.rb +++ b/activerecord/test/models/parrot.rb @@ -21,3 +21,9 @@ class LiveParrot < Parrot class DeadParrot < Parrot belongs_to :killer, :class_name => 'Pirate' end + +class FunkyParrot < Parrot + before_destroy do + raise "before_destroy was called" + end +end