From 67bca35a64e0fde994270b242f0b1c59d8e56b0b Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Mon, 3 Dec 2018 17:24:29 -0500 Subject: Reset scope after collection delete Reset scope after delete on collection association to clear stale offsets of removed records. --- activerecord/CHANGELOG.md | 4 +++ .../active_record/associations/collection_proxy.rb | 8 +++--- .../associations/has_many_associations_test.rb | 32 ++++++++++++++++++++++ .../has_many_through_associations_test.rb | 32 ++++++++++++++++++++++ 4 files changed, 72 insertions(+), 4 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index d9488bd0f0..d9394f1442 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,7 @@ +* Reset scope after delete on collection association to clear stale offsets of removed records. + + *Gannon McGibbon* + * 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/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb index 811fbfd927..4fbbc713e4 100644 --- a/activerecord/lib/active_record/associations/collection_proxy.rb +++ b/activerecord/lib/active_record/associations/collection_proxy.rb @@ -500,7 +500,7 @@ module ActiveRecord # Pet.find(1, 2, 3) # # => ActiveRecord::RecordNotFound: Couldn't find all Pets with 'id': (1, 2, 3) def delete_all(dependent = nil) - @association.delete_all(dependent) + @association.delete_all(dependent).tap { reset_scope } end # Deletes the records of the collection directly from the database @@ -527,7 +527,7 @@ module ActiveRecord # # Pet.find(1) # => Couldn't find Pet with id=1 def destroy_all - @association.destroy_all + @association.destroy_all.tap { reset_scope } end # Deletes the +records+ supplied from the collection according to the strategy @@ -646,7 +646,7 @@ module ActiveRecord # # # # # ] def delete(*records) - @association.delete(*records) + @association.delete(*records).tap { reset_scope } end # Destroys the +records+ supplied and removes them from the collection. @@ -718,7 +718,7 @@ module ActiveRecord # # Pet.find(4, 5, 6) # => ActiveRecord::RecordNotFound: Couldn't find all Pets with 'id': (4, 5, 6) def destroy(*records) - @association.destroy(*records) + @association.destroy(*records).tap { reset_scope } end ## diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 6024a9e354..257bab4635 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -294,6 +294,14 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_equal(expected_sql, loaded_sql) end + def test_delete_all_on_association_clears_scope + author = Author.create!(name: "Gannon") + posts = author.posts + posts.create!(title: "test", body: "body") + posts.delete_all + assert_nil posts.first + end + def test_building_the_associated_object_with_implicit_sti_base_class firm = DependentFirm.new company = firm.companies.build @@ -1710,6 +1718,30 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert companies(:first_firm).clients_of_firm.reload.empty?, "37signals has no clients after destroy all and refresh" end + def test_destroy_all_on_association_clears_scope + author = Author.create!(name: "Gannon") + posts = author.posts + posts.create!(title: "test", body: "body") + posts.destroy_all + assert_nil posts.first + end + + def test_destroy_on_association_clears_scope + author = Author.create!(name: "Gannon") + posts = author.posts + post = posts.create!(title: "test", body: "body") + posts.destroy(post) + assert_nil posts.first + end + + def test_delete_on_association_clears_scope + author = Author.create!(name: "Gannon") + posts = author.posts + post = posts.create!(title: "test", body: "body") + posts.delete(post) + assert_nil posts.first + end + def test_dependence firm = companies(:first_firm) assert_equal 3, firm.clients.size 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..03c7793e58 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -227,6 +227,14 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase end end + def test_delete_all_on_association_clears_scope + post = Post.create!(title: "Rails 6", body: "") + people = post.people + people.create!(first_name: "Jeb") + people.delete_all + assert_nil people.first + end + def test_concat person = people(:david) post = posts(:thinking) @@ -401,6 +409,30 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert_empty posts(:welcome).people.reload end + def test_destroy_all_on_association_clears_scope + post = Post.create!(title: "Rails 6", body: "") + people = post.people + people.create!(first_name: "Jeb") + people.destroy_all + assert_nil people.first + end + + def test_destroy_on_association_clears_scope + post = Post.create!(title: "Rails 6", body: "") + people = post.people + person = people.create!(first_name: "Jeb") + people.destroy(person) + assert_nil people.first + end + + def test_delete_on_association_clears_scope + post = Post.create!(title: "Rails 6", body: "") + people = post.people + person = people.create!(first_name: "Jeb") + people.delete(person) + assert_nil people.first + end + def test_should_raise_exception_for_destroying_mismatching_records assert_no_difference ["Person.count", "Reader.count"] do assert_raise(ActiveRecord::AssociationTypeMismatch) { posts(:welcome).people.destroy(posts(:thinking)) } -- cgit v1.2.3