Using subselect for `delete_all` with `limit` or `offset`

Arel doesn't support subselect generation for DELETE unlike UPDATE yet,
but we already have that generation in connection adapters. We can
simply use the subselect generated by that one.

4 files changed, 29 insertions, 6 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index c1a6d7fd3a..73970c99d0 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   Using subselect for `delete_all` with `limit` or `offset`.
+
+    *Ryuta Kamizono*
+
 *   Undefine attribute methods on descendants when resetting column
     information.
 
@@ -553,4 +557,5 @@
 
     *Kevin McPhillips*
 
+
 Please check [5-1-stable](https://github.com/rails/rails/blob/5-1-stable/activerecord/CHANGELOG.md) for previous changes.
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index 66fa98f172..4df3864d07 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -10,7 +10,7 @@ module ActiveRecord
     SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :reordering,
                             :reverse_order, :distinct, :create_with, :skip_query_cache]
     CLAUSE_METHODS = [:where, :having, :from]
-    INVALID_METHODS_FOR_DELETE_ALL = [:limit, :distinct, :offset, :group, :having]
+    INVALID_METHODS_FOR_DELETE_ALL = [:distinct, :group, :having]
 
     VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + CLAUSE_METHODS
 
@@ -365,8 +365,8 @@ module ActiveRecord
     #
     # If an invalid method is supplied, #delete_all raises an ActiveRecordError:
     #
-    #   Post.limit(100).delete_all
-    #   # => ActiveRecord::ActiveRecordError: delete_all doesn't support limit
+    #   Post.distinct.delete_all
+    #   # => ActiveRecord::ActiveRecordError: delete_all doesn't support distinct
     def delete_all
       invalid_methods = INVALID_METHODS_FOR_DELETE_ALL.select do |method|
         value = get_value(method)
@@ -384,7 +384,7 @@ module ActiveRecord
       stmt = Arel::DeleteManager.new
       stmt.from(table)
 
-      if has_join_values?
+      if has_join_values? || has_limit_or_offset?
         @klass.connection.join_to_delete(stmt, arel, arel_attribute(primary_key))
       else
         stmt.wheres = arel.constraints
diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb
index 3440781fee..07c4a17fb1 100644
--- a/activerecord/test/cases/persistence_test.rb
+++ b/activerecord/test/cases/persistence_test.rb
@@ -76,6 +76,26 @@ class PersistenceTest < ActiveRecord::TestCase
       assert_equal "bulk update!", posts(:thinking).body
       assert_not_equal "bulk update!", posts(:welcome).body
     end
+
+    def test_delete_all_with_order_and_limit_deletes_subset_only
+      author = authors(:david)
+      limited_posts = Post.where(author: author).order(:id).limit(1)
+      assert_equal 1, limited_posts.size
+      assert_equal 2, limited_posts.limit(2).size
+      assert_equal 1, limited_posts.delete_all
+      assert_raise(ActiveRecord::RecordNotFound) { posts(:welcome) }
+      assert posts(:thinking)
+    end
+
+    def test_delete_all_with_order_and_limit_and_offset_deletes_subset_only
+      author = authors(:david)
+      limited_posts = Post.where(author: author).order(:id).limit(1).offset(1)
+      assert_equal 1, limited_posts.size
+      assert_equal 2, limited_posts.limit(2).size
+      assert_equal 1, limited_posts.delete_all
+      assert_raise(ActiveRecord::RecordNotFound) { posts(:thinking) }
+      assert posts(:welcome)
+    end
   end
 
   def test_update_many
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 675aafabda..915ff03c2c 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -896,11 +896,9 @@ class RelationTest < ActiveRecord::TestCase
   end
 
   def test_delete_all_with_unpermitted_relation_raises_error
-    assert_raises(ActiveRecord::ActiveRecordError) { Author.limit(10).delete_all }
     assert_raises(ActiveRecord::ActiveRecordError) { Author.distinct.delete_all }
     assert_raises(ActiveRecord::ActiveRecordError) { Author.group(:name).delete_all }
     assert_raises(ActiveRecord::ActiveRecordError) { Author.having("SUM(id) < 3").delete_all }
-    assert_raises(ActiveRecord::ActiveRecordError) { Author.offset(10).delete_all }
   end
 
   def test_select_with_aggregates