Merge pull request rails#54722 from brunodccarvalho/repeat-in-join-clause

Use a self-join for UPDATE with outer joins on PostgreSQL and SQLite

Author: byroot

activerecord/lib/arel/visitors/postgresql.rb

@@ -12,46 +12,21 @@ def visit_Arel_Nodes_UpdateStatement(o, collector)
 
           # UPDATE with JOIN is in the form of:
           #
-          #   UPDATE t1
+          #   UPDATE t1 AS __active_record_update_alias
           #   SET ..
-          #   FROM t2
-          #   WHERE t1.join_id = t2.join_id
-          #
-          # Or if more than one join is present:
-          #
-          #   UPDATE t1
-          #   SET ..
-          #   FROM t2
-          #   JOIN t3 ON t2.join_id = t3.join_id
-          #   WHERE t1.join_id = t2.join_id
+          #   FROM t1 JOIN t2 ON t2.join_id = t1.join_id ..
+          #   WHERE t1.id = __active_record_update_alias.id AND ..
           if has_join_sources?(o)
-            visit o.relation.left, collector
+            collector = visit o.relation.left, collector
             collect_nodes_for o.values, collector, " SET "
             collector << " FROM "
-            first_join, *remaining_joins = o.relation.right
-            from_items = remaining_joins.extract! do |join|
-              join.right.expr.right.relation == o.relation.left
-            end
-
-            from_where = [first_join.left] + from_items.map(&:left)
-            collect_nodes_for from_where, collector, " ", ", "
-
-            if remaining_joins && !remaining_joins.empty?
-              collector << " "
-              remaining_joins.each do |join|
-                visit join, collector
-                collector << " "
-              end
-            end
-
-            from_where = [first_join.right.expr] + from_items.map { |i| i.right.expr }
-            collect_nodes_for from_where + o.wheres, collector, " WHERE ", " AND "
+            collector = inject_join o.relation.right, collector, " "
           else
             collector = visit o.relation, collector
             collect_nodes_for o.values, collector, " SET "
-            collect_nodes_for o.wheres, collector, " WHERE ", " AND "
           end
 
+          collect_nodes_for o.wheres, collector, " WHERE ", " AND "
           collect_nodes_for o.orders, collector, " ORDER BY "
           maybe_visit o.limit, collector
         end
@@ -60,12 +35,18 @@ def visit_Arel_Nodes_UpdateStatement(o, collector)
         # query. However, this does not allow for LIMIT, OFFSET and ORDER. To support
         # these, we must use a subquery.
         def prepare_update_statement(o)
-          if has_join_sources?(o) && !has_limit_or_offset_or_orders?(o) && !has_group_by_and_having?(o) &&
-            # The PostgreSQL dialect isn't flexible enough to allow anything other than a inner join
-            # for the first join:
-            #   UPDATE table SET .. FROM joined_table WHERE ...
-            (o.relation.right.all? { |join| join.is_a?(Arel::Nodes::InnerJoin) || join.right.expr.right.relation != o.relation.left })
-            o
+          if o.key && has_join_sources?(o) && !has_group_by_and_having?(o) && !has_limit_or_offset_or_orders?(o)
+            # Join clauses cannot reference the target table, so alias the
+            # updated table, place the entire relation in the FROM clause, and
+            # add a self-join (which requires the primary key)
+            stmt = o.clone
+            stmt.relation, stmt.wheres = o.relation.clone, o.wheres.clone
+            stmt.relation.right = [stmt.relation.left, *stmt.relation.right]
+            stmt.relation.left = stmt.relation.left.alias("__active_record_update_alias")
+            Array.wrap(o.key).each do |key|
+              stmt.wheres << key.eq(stmt.relation.left[key.name])
+            end
+            stmt
           else
             super
           end
@@ -133,6 +114,12 @@ def visit_Arel_Nodes_Lateral(o, collector)
           grouping_parentheses o.expr, collector
         end
 
+        def visit_Arel_Nodes_InnerJoin(o, collector)
+          return super if o.right
+          collector << "CROSS JOIN "
+          visit o.left, collector
+        end
+
         def visit_Arel_Nodes_IsNotDistinctFrom(o, collector)
           collector = visit o.left, collector
           collector << " IS NOT DISTINCT FROM "

activerecord/lib/arel/visitors/sqlite.rb

@@ -12,65 +12,52 @@ def visit_Arel_Nodes_UpdateStatement(o, collector)
 
           # UPDATE with JOIN is in the form of:
           #
-          #   UPDATE t1
+          #   UPDATE t1 AS __active_record_update_alias
           #   SET ..
-          #   FROM t2
-          #   WHERE t1.join_id = t2.join_id
-          #
-          # Or if more than one join is present:
-          #
-          #   UPDATE t1
-          #   SET ..
-          #   FROM t2
-          #   JOIN t3 ON t2.join_id = t3.join_id
-          #   WHERE t1.join_id = t2.join_id
+          #   FROM t1 JOIN t2 ON t2.join_id = t1.join_id ..
+          #   WHERE t1.id = __active_record_update_alias.id AND ..
           if has_join_sources?(o)
-            visit o.relation.left, collector
+            collector = visit o.relation.left, collector
             collect_nodes_for o.values, collector, " SET "
-
             collector << " FROM "
-            first_join, *remaining_joins = o.relation.right
-            from_items = remaining_joins.extract! do |join|
-              join.right.expr.right.relation == o.relation.left
-            end
-
-            from_where = [first_join.left] + from_items.map(&:left)
-            collect_nodes_for from_where, collector, " ", ", "
-
-            if remaining_joins && !remaining_joins.empty?
-              collector << " "
-              remaining_joins.each do |join|
-                visit join, collector
-                collector << " "
-              end
-            end
-
-            from_where = [first_join.right.expr] + from_items.map { |i| i.right.expr }
-            collect_nodes_for from_where + o.wheres, collector, " WHERE ", " AND "
+            collector = inject_join o.relation.right, collector, " "
           else
             collector = visit o.relation, collector
             collect_nodes_for o.values, collector, " SET "
-            collect_nodes_for o.wheres, collector, " WHERE ", " AND "
           end
 
+          collect_nodes_for o.wheres, collector, " WHERE ", " AND "
           collect_nodes_for o.orders, collector, " ORDER BY "
           maybe_visit o.limit, collector
         end
 
         def prepare_update_statement(o)
           # Sqlite need to be built with the SQLITE_ENABLE_UPDATE_DELETE_LIMIT compile-time option
           # to support LIMIT/OFFSET/ORDER in UPDATE and DELETE statements.
-          if has_join_sources?(o) && !has_limit_or_offset_or_orders?(o) && !has_group_by_and_having?(o) &&
-            # The SQLite3 dialect isn't flexible enough to allow anything other than a inner join
-            # for the first join:
-            #   UPDATE table SET .. FROM joined_table WHERE ...
-            (o.relation.right.all? { |join| join.is_a?(Arel::Nodes::InnerJoin) || join.right.expr.right.relation != o.relation.left })
-            o
+          if o.key && has_join_sources?(o) && !has_group_by_and_having?(o) && !has_limit_or_offset_or_orders?(o)
+            # Join clauses cannot reference the target table, so alias the
+            # updated table, place the entire relation in the FROM clause, and
+            # add a self-join (which requires the primary key)
+            stmt = o.clone
+            stmt.relation, stmt.wheres = o.relation.clone, o.wheres.clone
+            stmt.relation.right = [stmt.relation.left, *stmt.relation.right]
+            stmt.relation.left = stmt.relation.left.alias("__active_record_update_alias")
+            Array.wrap(o.key).each do |key|
+              stmt.wheres << key.eq(stmt.relation.left[key.name])
+            end
+            stmt
           else
             super
           end
         end
 
+        def visit_Arel_Nodes_TableAlias(o, collector)
+          # "AS" is not optional in "{UPDATE | DELETE} table AS alias ..."
+          collector = visit o.relation, collector
+          collector << " AS "
+          collector << quote_table_name(o.name)
+        end
+
         # Locks are not supported in SQLite
         def visit_Arel_Nodes_Lock(o, collector)
           collector

activerecord/test/cases/relation/update_all_test.rb

@@ -171,11 +171,57 @@ def test_update_all_with_left_joins
     assert_equal pets.count, pets.update_all(name: "Bob")
   end
 
-  def test_update_all_with_left_outer_joins
-    pets = Pet.left_outer_joins(:toys)
+  def test_update_all_with_left_outer_joins_can_reference_joined_table
+    pets = Pet.left_outer_joins(:toys).where(toys: { name: ["Bone", nil] })
 
     assert_equal true, pets.exists?
-    assert_equal pets.count, pets.update_all(name: "Boby")
+    assert_equal pets.count, pets.update_all(name: Arel.sql("COALESCE(toys.name, 'Toyless')"))
+    assert_equal "Toyless", Pet.where.missing(:toys).first.name
+    assert_not_equal "Toyless", Pet.joins(:toys).first.name
+  end
+
+  def test_update_all_with_string_joins_can_reference_joined_table
+    join = current_adapter?(:Mysql2Adapter, :TrilogyAdapter) ? "LEFT OUTER JOIN" : "FULL OUTER JOIN"
+    pets = Pet.joins("#{join} toys ON toys.pet_id = pets.pet_id").where(toys: { name: ["Bone", nil] })
+
+    assert_equal true, pets.exists?
+    assert_equal pets.count, pets.update_all(name: Arel.sql("COALESCE(toys.name, 'Toyless')"))
+    assert_equal "Toyless", Pet.where.missing(:toys).first.name
+    assert_not_equal "Toyless", Pet.joins(:toys).first.name
+  end
+
+  def test_update_all_with_self_left_joins_can_reference_joined_table
+    lvl2 = Comment.left_joins(parent: :parent).joins(:post).where(parent: { parent: nil }).where.not(parent: nil)
+
+    assert_equal true, lvl2.exists?
+    assert_equal lvl2.count, lvl2.update_all(body: Arel.sql("COALESCE(parent.body, posts.title)"))
+  end
+
+  def test_update_all_with_left_joins_composite_primary_key_can_reference_joined_table
+    orders = Cpk::Order.left_joins(:order_agreements).where(order_agreements: { order_id: nil })
+
+    assert_equal true, orders.exists?
+    assert_equal orders.count, orders.update_all(status: Arel.sql("COALESCE(order_agreements.signature, 'orphan')"))
+    assert_equal orders.count, Cpk::Order.where(status: "orphan").count
+  end
+
+  # Limitations of the implementation
+  if current_adapter?(:SQLite3Adapter, :PostgreSQLAdapter)
+    def test_update_all_with_left_joins_unqualified_set_reference_is_ambiguous
+      orders = Cpk::Order.left_joins(:order_agreements).where(order_agreements: { order_id: nil })
+
+      assert_raises(ActiveRecord::StatementInvalid, match: /ambiguous/) do
+        orders.update_all(status: Arel.sql("CONCAT(\"status\", 'orphan')"))
+      end
+    end
+
+    def test_update_all_with_left_joins_unqualified_where_reference_is_ambiguous
+      orders = Cpk::Order.left_joins(:order_agreements).where(order_agreements: { order_id: nil })
+
+      assert_raises(ActiveRecord::StatementInvalid, match: /ambiguous/) do
+        orders.where(Arel.sql("\"status\" != '123'")).update_all(status: "123")
+      end
+    end
   end
 
   def test_update_all_with_includes

activerecord/test/cases/relation_test.rb

@@ -268,7 +268,7 @@ def test_relation_merging_with_merged_symbol_joins_is_aliased
       assert_equal 3, nb_inner_join, "Wrong amount of INNER JOIN in query"
 
       # using `\W` as the column separator
-      assert queries.any? { |sql| %r[INNER\s+JOIN\s+#{Regexp.escape(Author.quoted_table_name)}\s+\Wauthors_categorizations\W]i.match?(sql) }, "Should be aliasing the child INNER JOINs in query"
+      assert queries.any? { |sql| %r[INNER\s+JOIN\s+#{Regexp.escape(Author.quoted_table_name)}\s+(AS\s+)?\Wauthors_categorizations\W]i.match?(sql) }, "Should be aliasing the child INNER JOINs in query"
     end
 
     def test_relation_with_merged_joins_aliased_works