Merge pull request rails#51609 from ClearlyClaire/fixes/arel-select-union-wrapping

Arel: only wrap SELECT statements in UNION if they involve ORDER BY, LIMIT or OFFSET

Author: byroot

activerecord/lib/arel/visitors/to_sql.rb

@@ -965,21 +965,21 @@ def infix_value_with_paren(o, collector, value, suppress_parens = false)
           collector = if o.left.class == o.class
             infix_value_with_paren(o.left, collector, value, true)
           else
-            grouping_parentheses o.left, collector
+            grouping_parentheses o.left, collector, false
           end
           collector << value
           collector = if o.right.class == o.class
             infix_value_with_paren(o.right, collector, value, true)
           else
-            grouping_parentheses o.right, collector
+            grouping_parentheses o.right, collector, false
           end
           collector << " )" unless suppress_parens
           collector
         end
 
         # Used by some visitors to enclose select queries in parentheses
-        def grouping_parentheses(o, collector)
-          if o.is_a?(Nodes::SelectStatement)
+        def grouping_parentheses(o, collector, always_wrap_selects = true)
+          if o.is_a?(Nodes::SelectStatement) && (always_wrap_selects || o.orders.present? || o.limit.present? || o.offset.present?)
             collector << "("
             visit o, collector
             collector << ")"

activerecord/test/cases/arel/attributes/attribute_test.rb

@@ -963,7 +963,7 @@ class AttributeTest < Arel::Spec
           union = mgr1.union(mgr2)
           node = relation[:id].in(union)
           _(node.to_sql).must_be_like %{
-            "users"."id" IN (( (SELECT "users"."id" FROM "users") UNION (SELECT "users"."id" FROM "users") ))
+            "users"."id" IN (( SELECT "users"."id" FROM "users" UNION SELECT "users"."id" FROM "users" ))
           }
         end
 

activerecord/test/cases/arel/select_manager_test.rb

@@ -253,15 +253,15 @@ def test_join_sources
 
         # maybe FIXME: decide when wrapper parens are needed
         _(node.to_sql).must_be_like %{
-          ( (SELECT * FROM "users"  WHERE "users"."age" < 18) UNION (SELECT * FROM "users"  WHERE "users"."age" > 99) )
+          ( SELECT * FROM "users"  WHERE "users"."age" < 18 UNION SELECT * FROM "users"  WHERE "users"."age" > 99 )
         }
       end
 
       it "should union all" do
         node = @m1.union :all, @m2
 
         _(node.to_sql).must_be_like %{
-          ( (SELECT * FROM "users"  WHERE "users"."age" < 18) UNION ALL (SELECT * FROM "users"  WHERE "users"."age" > 99) )
+          ( SELECT * FROM "users"  WHERE "users"."age" < 18 UNION ALL SELECT * FROM "users"  WHERE "users"."age" > 99 )
         }
       end
     end
@@ -354,9 +354,9 @@ def test_join_sources
         sql = manager.to_sql
         _(sql).must_be_like %{
           WITH RECURSIVE "replies" AS (
-              (SELECT "comments"."id", "comments"."parent_id" FROM "comments" WHERE "comments"."id" = 42)
+              SELECT "comments"."id", "comments"."parent_id" FROM "comments" WHERE "comments"."id" = 42
             UNION
-              (SELECT "comments"."id", "comments"."parent_id" FROM "comments" INNER JOIN "replies" ON "comments"."parent_id" = "replies"."id")
+              SELECT "comments"."id", "comments"."parent_id" FROM "comments" INNER JOIN "replies" ON "comments"."parent_id" = "replies"."id"
           )
           SELECT * FROM "replies"
         }