Merge pull request rails#43580 from ignacio-chiazzo/ignacio/delete_all

Use nested queries when doing DELETE and GROUP_BY and HAVINAG clauses…

Author: rafaelfranca

activerecord/CHANGELOG.md

@@ -1,3 +1,17 @@
+*   Use subquery for DELETE with GROUP_BY and HAVING clauses.
+
+    Prior to this change, deletes with GROUP_BY and HAVING were returning an error.
+
+    After this change, GROUP_BY and HAVING are valid clauses in DELETE queries, generating the following query: 
+
+    ```sql
+    DELETE FROM "posts" WHERE "posts"."id" IN (
+        SELECT "posts"."id" FROM "posts" INNER JOIN "comments" ON "comments"."post_id" = "posts"."id" GROUP BY "posts"."id" HAVING (count(comments.id) >= 2))
+    )  [["flagged", "t"]]
+    ```
+
+    *Ignacio Chiazzo Cardarello*
+
 *   Use subquery for UPDATE with GROUP_BY and HAVING clauses.
 
     Prior to this change, updates with GROUP_BY and HAVING were being ignored, generating a SQL like this:

activerecord/lib/active_record/relation.rb

@@ -11,7 +11,7 @@ class Relation
                             :reverse_order, :distinct, :create_with, :skip_query_cache]
 
     CLAUSE_METHODS = [:where, :having, :from]
-    INVALID_METHODS_FOR_DELETE_ALL = [:distinct, :group, :having]
+    INVALID_METHODS_FOR_DELETE_ALL = [:distinct]
 
     VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + CLAUSE_METHODS
 
@@ -616,7 +616,9 @@ def delete_all
       arel = eager_loading? ? apply_join_dependency.arel : build_arel
       arel.source.left = table
 
-      stmt = arel.compile_delete(table[primary_key])
+      group_values_arel_columns = arel_columns(group_values.uniq)
+      having_clause_ast = having_clause.ast unless having_clause.empty?
+      stmt = arel.compile_delete(table[primary_key], having_clause_ast, group_values_arel_columns)
 
       klass.connection.delete(stmt, "#{klass} Delete All").tap { reset }
     end

activerecord/lib/arel/crud.rb

@@ -33,13 +33,15 @@ def compile_update(
       um
     end
 
-    def compile_delete(key = nil)
+    def compile_delete(key = nil, having_clause = nil, group_values_columns = [])
       dm = DeleteManager.new(source)
       dm.take(limit)
       dm.offset(offset)
       dm.order(*orders)
       dm.wheres = constraints
       dm.key = key
+      dm.group(group_values_columns) unless group_values_columns.empty?
+      dm.having(having_clause) unless having_clause.nil?
       dm
     end
   end

activerecord/lib/arel/delete_manager.rb

@@ -12,5 +12,21 @@ def from(relation)
       @ast.relation = relation
       self
     end
+
+    def group(columns)
+      columns.each do |column|
+        column = Nodes::SqlLiteral.new(column) if String === column
+        column = Nodes::SqlLiteral.new(column.to_s) if Symbol === column
+
+        @ast.groups.push Nodes::Group.new column
+      end
+
+      self
+    end
+
+    def having(expr)
+      @ast.havings << expr
+      self
+    end
   end
 end

activerecord/test/cases/relation/delete_all_test.rb

@@ -5,9 +5,10 @@
 require "models/post"
 require "models/pet"
 require "models/toy"
+require "models/comment"
 
 class DeleteAllTest < ActiveRecord::TestCase
-  fixtures :authors, :author_addresses, :posts, :pets, :toys
+  fixtures :authors, :author_addresses, :comments, :posts, :pets, :toys
 
   def test_destroy_all
     davids = Author.where(name: "David")
@@ -53,10 +54,22 @@ def test_delete_all_loaded
     assert_predicate davids, :loaded?
   end
 
+  def test_delete_all_with_group_by_and_having
+    minimum_comments_count = 2
+    posts_to_be_deleted = Post.most_commented(minimum_comments_count).all.to_a
+    assert_operator posts_to_be_deleted.length, :>, 0
+
+    assert_difference("Post.count", -posts_to_be_deleted.length) do
+      Post.most_commented(minimum_comments_count).delete_all
+    end
+
+    posts_to_be_deleted.each do |deleted_post|
+      assert_raise(ActiveRecord::RecordNotFound) { deleted_post.reload }
+    end
+  end
+
   def test_delete_all_with_unpermitted_relation_raises_error
     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 }
   end
 
   def test_delete_all_with_joins_and_where_part_is_hash