Arel: wrap SELECT statements in parentheses when generating SQL for UNION

This commit fixes generated SQL for `UNION` and `UNION ALL` involving `LIMIT`
or `ORDER BY` by wrapping `SELECT` statements appearing in an `UNION` or
`UNION ALL` in parentheses.

Author: ClearlyClaire

activerecord/lib/arel/visitors/postgresql.rb

@@ -63,7 +63,7 @@ def visit_Arel_Nodes_GroupingSet(o, collector)
 
         def visit_Arel_Nodes_Lateral(o, collector)
           collector << "LATERAL "
-          grouping_parentheses o, collector
+          grouping_parentheses o.expr, collector
         end
 
         def visit_Arel_Nodes_IsNotDistinctFrom(o, collector)
@@ -83,17 +83,6 @@ def visit_Arel_Nodes_IsDistinctFrom(o, collector)
 
         def bind_block; BIND_BLOCK; end
 
-        # Used by Lateral visitor to enclose select queries in parentheses
-        def grouping_parentheses(o, collector)
-          if o.expr.is_a? Nodes::SelectStatement
-            collector << "("
-            visit o.expr, collector
-            collector << ")"
-          else
-            visit o.expr, collector
-          end
-        end
-
         # Utilized by GroupingSet, Cube & RollUp visitors to
         # handle grouping aggregation semantics
         def grouping_array_or_grouping_element(o, collector)

activerecord/lib/arel/visitors/to_sql.rb

@@ -965,18 +965,30 @@ 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
-            visit o.left, collector
+            grouping_parentheses o.left, collector
           end
           collector << value
           collector = if o.right.class == o.class
             infix_value_with_paren(o.right, collector, value, true)
           else
-            visit o.right, collector
+            grouping_parentheses o.right, collector
           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)
+            collector << "("
+            visit o, collector
+            collector << ")"
+            collector
+          else
+            visit o, collector
+          end
+        end
+
         def aggregate(name, o, collector)
           collector << "#{name}("
           if o.distinct

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"
         }

activerecord/test/cases/arel/visitors/to_sql_test.rb

@@ -635,6 +635,14 @@ def dispatch
           node = Nodes::Union.new Arel.sql("topleft"), subnode
           assert_equal("( topleft UNION left UNION right )", compile(node))
         end
+
+        it "encloses SELECT statements with parentheses" do
+          table = Table.new(:users)
+          left = table.where(table[:name].eq(0)).take(1).ast
+          right = table.where(table[:name].eq(1)).take(1).ast
+          node = Nodes::Union.new left, right
+          assert_match(/LIMIT 1\) UNION \(/, compile(node))
+        end
       end
 
       describe "Nodes::UnionAll" do
@@ -646,6 +654,14 @@ def dispatch
           node = Nodes::UnionAll.new Arel.sql("topleft"), subnode
           assert_equal("( topleft UNION ALL left UNION ALL right )", compile(node))
         end
+
+        it "encloses SELECT statements with parentheses" do
+          table = Table.new(:users)
+          left = table.where(table[:name].eq(0)).take(1).ast
+          right = table.where(table[:name].eq(1)).take(1).ast
+          node = Nodes::UnionAll.new left, right
+          assert_match(/LIMIT 1\) UNION ALL \(/, compile(node))
+        end
       end
 
       describe "Nodes::NotIn" do