Merge pull request #10604 from neerajdotname/delete_all_should_not_call_callbacks

Do not invoke callbacks when delete_all is called

Conflicts:
	activerecord/CHANGELOG.md

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*
+
 *   Calling default_scope without a proc will now raise `ArgumentError`.
 
     *Neeraj Singh*

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

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

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

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)

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'

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

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"
 

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