Ensure that delete_all on collection proxy returns affected count

kamipo · kamipo · commit 4d7354aa352b · 2018-12-04T14:33:31.000+09:00
Unlike the `Relation#delete_all`, `delete_all` on collection proxy
doesn't return affected count. Since the `CollectionProxy` is a subclass
of the `Relation`, this inconsistency is probably not intended, so it
should return the count consistently.
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   Ensure that `delete_all` on collection proxy returns affected count.
+
+    *Ryuta Kamizono*
+
 *   Add the ability to prevent writes to a database for the duration of a block.
 
     Allows the application to prevent writes to a database. This can be useful when
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -99,6 +99,7 @@ def delete_count(method, scope)
         def delete_or_nullify_all_records(method)
           count = delete_count(method, scope)
           update_counter(-count)
+          count
         end
 
         # Deletes the records according to the <tt>:dependent</tt> option.
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -161,6 +161,8 @@ def delete_records(records, method)
           else
             update_counter(-count)
           end
+
+          count
         end
 
         def difference(a, b)
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -264,7 +264,7 @@ def test_do_not_call_callbacks_for_delete_all
     car = Car.create(name: "honda")
     car.funky_bulbs.create!
     assert_equal 1, car.funky_bulbs.count
-    assert_nothing_raised { car.reload.funky_bulbs.delete_all }
+    assert_equal 1, car.reload.funky_bulbs.delete_all
     assert_equal 0, car.funky_bulbs.count, "bulbs should have been deleted using :delete_all strategy"
   end
 
@@ -1405,7 +1405,7 @@ def test_delete_all
     assert_equal 3, clients.count
 
     assert_difference "Client.count", -(clients.count) do
-      companies(:first_firm).dependent_clients_of_firm.delete_all
+      assert_equal clients.count, companies(:first_firm).dependent_clients_of_firm.delete_all
     end
   end
 
@@ -1502,10 +1502,20 @@ def test_clearing_a_dependent_association_collection
   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)
+    count = firm.dependent_clients_of_firm.count
+    assert_equal count, firm.dependent_clients_of_firm.delete_all(:delete_all)
     assert_nil Client.find_by_id(client_id)
   end
 
+  def test_delete_all_with_option_nullify
+    firm = companies(:first_firm)
+    client_id = firm.dependent_clients_of_firm.first.id
+    count = firm.dependent_clients_of_firm.count
+    assert_equal firm, Client.find(client_id).firm
+    assert_equal count, firm.dependent_clients_of_firm.delete_all(:nullify)
+    assert_nil Client.find(client_id).firm
+  end
+
   def test_delete_all_accepts_limited_parameters
     firm = companies(:first_firm)
     assert_raise(ArgumentError) do
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -200,7 +200,7 @@ def test_delete_all_for_with_dependent_option_destroy
 
     assert_no_difference "Job.count" do
       assert_difference "Reference.count", -1 do
-        person.reload.jobs_with_dependent_destroy.delete_all
+        assert_equal 1, person.reload.jobs_with_dependent_destroy.delete_all
       end
     end
   end
@@ -211,7 +211,7 @@ def test_delete_all_for_with_dependent_option_nullify
 
     assert_no_difference "Job.count" do
       assert_no_difference "Reference.count" do
-        person.reload.jobs_with_dependent_nullify.delete_all
+        assert_equal 1, person.reload.jobs_with_dependent_nullify.delete_all
       end
     end
   end
@@ -222,7 +222,7 @@ def test_delete_all_for_with_dependent_option_delete_all
 
     assert_no_difference "Job.count" do
       assert_difference "Reference.count", -1 do
-        person.reload.jobs_with_dependent_delete_all.delete_all
+        assert_equal 1, person.reload.jobs_with_dependent_delete_all.delete_all
       end
     end
   end
