Merge pull request #34607 from gmcgibbon/clear_scope_on_delete

Reset scope after collection delete

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
       #   #       #<Pet id: 3, name: "Choo-Choo", person_id: 1>
       #   #    ]
       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)) }