Support UPDATE outer joins on PostgreSQL and SQLite with a self-join transform

When an OUTER JOIN references the updated table in the ON clause, the join
condition cannot be moved to the WHERE clause. Support this case by duplicating
the updated table into the FROM clause with a perfect self-join on the pk.
Roughly, transform a hypothetical

UPDATE "products"
LEFT JOIN "categories" ON "products"."category_id" = "categories"."id"
... other joins ...
WHERE ...

into

UPDATE "products" "alias"
FROM "products"
LEFT JOIN "categories" ON "products"."category_id" = "categories"."id"
... other joins ...
WHERE "alias"."id" = "products"."id" AND ...

This reinterprets top-level FULL or RIGHT OUTER JOIN as a LEFT or INNER JOIN.
This should be ok since we interpret the internal AST as joining with the
updated table, and rows with no "product" can be ignored in this context.

Unfortunately this introduces an ambiguity when the SET or WHERE references an
unqualified column of "products". It can be qualified as "products"."column",
it will reference the introduced table and the problem will be fixed.
This shouldn't happen with AR generated nodes.

Then PostgreSQL and SQLite use a subquery only when a LIMIT, OFFSET, ORDER BY,
GROUP BY, or HAVING is present, like MySQL does.

Author: brunodccarvalho

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