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.

Author: kamipo

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.

activerecord/lib/active_record/relation.rb

@@ -10,7 +10,7 @@ class Relation
     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 @@ def destroy_all
     #
     # 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 @@ def delete_all
       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

activerecord/test/cases/persistence_test.rb

@@ -76,6 +76,26 @@ def test_update_all_with_order_and_limit_and_offset_updates_subset_only
       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

activerecord/test/cases/relations_test.rb

@@ -896,11 +896,9 @@ def test_delete_all_loaded
   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