From 4d7354aa352b94ec3a10ad053cff04890c534080 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Tue, 4 Dec 2018 14:33:31 +0900 Subject: Ensure that `delete_all` on collection proxy returns affected count 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. --- activerecord/CHANGELOG.md | 4 ++++ .../active_record/associations/has_many_association.rb | 1 + .../associations/has_many_through_association.rb | 2 ++ .../cases/associations/has_many_associations_test.rb | 16 +++++++++++++--- .../associations/has_many_through_associations_test.rb | 6 +++--- 5 files changed, 23 insertions(+), 6 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index d9488bd0f0..ba1b9a304f 100644 --- 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 index e224d3456a..f6fdbcde54 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -99,6 +99,7 @@ module ActiveRecord def delete_or_nullify_all_records(method) count = delete_count(method, scope) update_counter(-count) + count end # Deletes the records according to the :dependent 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 index 2322a49931..84a9797aa5 100644 --- 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 @@ module ActiveRecord 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 index 6024a9e354..b4bb6e7227 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -264,7 +264,7 @@ class HasManyAssociationsTest < ActiveRecord::TestCase 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 @@ class HasManyAssociationsTest < ActiveRecord::TestCase 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 @@ class HasManyAssociationsTest < ActiveRecord::TestCase 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 index cf514957ca..3827729ac4 100644 --- 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 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase 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 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase 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 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase 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 -- cgit v1.2.3