From 63dd8d8e12edb25d8d5bac324aacb1caf05bbe22 Mon Sep 17 00:00:00 2001
From: Keenan Brock <keenan@thebrocks.net>
Date: Tue, 13 Nov 2018 15:22:08 -0500
Subject: Emit single pair of parens for UNION and UNION ALL

mysql has a great implementation to suppress multiple parens for union
sql statements.

This moves that functionality to the generic implementation
This also introduces that functionality for UNION ALL
---
 activerecord/lib/arel/visitors/mysql.rb            | 28 ----------------------
 activerecord/lib/arel/visitors/to_sql.rb           | 23 ++++++++++++++----
 .../test/cases/arel/visitors/mysql_test.rb         | 10 --------
 .../test/cases/arel/visitors/to_sql_test.rb        | 22 +++++++++++++++++
 4 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/activerecord/lib/arel/visitors/mysql.rb b/activerecord/lib/arel/visitors/mysql.rb
index 32f6705d04..4e7b2456aa 100644
--- a/activerecord/lib/arel/visitors/mysql.rb
+++ b/activerecord/lib/arel/visitors/mysql.rb
@@ -4,34 +4,6 @@ module Arel # :nodoc: all
   module Visitors
     class MySQL < Arel::Visitors::ToSql
       private
-        def visit_Arel_Nodes_Union(o, collector, suppress_parens = false)
-          unless suppress_parens
-            collector << "( "
-          end
-
-          case o.left
-          when Arel::Nodes::Union
-            visit_Arel_Nodes_Union o.left, collector, true
-          else
-            visit o.left, collector
-          end
-
-          collector << " UNION "
-
-          case o.right
-          when Arel::Nodes::Union
-            visit_Arel_Nodes_Union o.right, collector, true
-          else
-            visit o.right, collector
-          end
-
-          if suppress_parens
-            collector
-          else
-            collector << " )"
-          end
-        end
-
         def visit_Arel_Nodes_Bin(o, collector)
           collector << "BINARY "
           visit o.expr, collector
diff --git a/activerecord/lib/arel/visitors/to_sql.rb b/activerecord/lib/arel/visitors/to_sql.rb
index 8e56fb55a2..34e82975ad 100644
--- a/activerecord/lib/arel/visitors/to_sql.rb
+++ b/activerecord/lib/arel/visitors/to_sql.rb
@@ -268,13 +268,11 @@ module Arel # :nodoc: all
         end
 
         def visit_Arel_Nodes_Union(o, collector)
-          collector << "( "
-          infix_value(o, collector, " UNION ") << " )"
+          infix_value_with_paren(o, collector, " UNION ")
         end
 
         def visit_Arel_Nodes_UnionAll(o, collector)
-          collector << "( "
-          infix_value(o, collector, " UNION ALL ") << " )"
+          infix_value_with_paren(o, collector, " UNION ALL ")
         end
 
         def visit_Arel_Nodes_Intersect(o, collector)
@@ -845,6 +843,23 @@ module Arel # :nodoc: all
           visit o.right, collector
         end
 
+        def infix_value_with_paren(o, collector, value, suppress_parens = false)
+          collector << '( ' unless suppress_parens
+          collector = if o.left.class == o.class
+                        infix_value_with_paren(o.left, collector, value, true)
+                      else
+                        visit 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
+                      end
+          collector << ' )' unless suppress_parens
+          collector
+        end
+
         def aggregate(name, o, collector)
           collector << "#{name}("
           if o.distinct
diff --git a/activerecord/test/cases/arel/visitors/mysql_test.rb b/activerecord/test/cases/arel/visitors/mysql_test.rb
index 9d3bad8516..dae98b0583 100644
--- a/activerecord/test/cases/arel/visitors/mysql_test.rb
+++ b/activerecord/test/cases/arel/visitors/mysql_test.rb
@@ -13,16 +13,6 @@ module Arel
         @visitor.accept(node, Collectors::SQLString.new).value
       end
 
-      it "squashes parenthesis on multiple unions" do
-        subnode = Nodes::Union.new Arel.sql("left"), Arel.sql("right")
-        node    = Nodes::Union.new subnode, Arel.sql("topright")
-        assert_equal 1, compile(node).scan("(").length
-
-        subnode = Nodes::Union.new Arel.sql("left"), Arel.sql("right")
-        node    = Nodes::Union.new Arel.sql("topleft"), subnode
-        assert_equal 1, compile(node).scan("(").length
-      end
-
       ###
       # :'(
       # http://dev.mysql.com/doc/refman/5.0/en/select.html#id3482214
diff --git a/activerecord/test/cases/arel/visitors/to_sql_test.rb b/activerecord/test/cases/arel/visitors/to_sql_test.rb
index b6426a211e..680c26a9ab 100644
--- a/activerecord/test/cases/arel/visitors/to_sql_test.rb
+++ b/activerecord/test/cases/arel/visitors/to_sql_test.rb
@@ -480,6 +480,28 @@ module Arel
         end
       end
 
+      describe "Nodes::Union" do
+        it "squashes parenthesis on multiple unions" do
+          subnode = Nodes::Union.new Arel.sql("left"), Arel.sql("right")
+          node = Nodes::Union.new subnode, Arel.sql("topright")
+          assert_equal("( left UNION right UNION topright )", compile(node))
+          subnode = Nodes::Union.new Arel.sql("left"), Arel.sql("right")
+          node = Nodes::Union.new Arel.sql("topleft"), subnode
+          assert_equal("( topleft UNION left UNION right )", compile(node))
+        end
+      end
+
+      describe "Nodes::UnionAll" do
+        it "squashes parenthesis on multiple union alls" do
+          subnode = Nodes::UnionAll.new Arel.sql("left"), Arel.sql("right")
+          node = Nodes::UnionAll.new subnode, Arel.sql("topright")
+          assert_equal("( left UNION ALL right UNION ALL topright )", compile(node))
+          subnode = Nodes::UnionAll.new Arel.sql("left"), Arel.sql("right")
+          node = Nodes::UnionAll.new Arel.sql("topleft"), subnode
+          assert_equal("( topleft UNION ALL left UNION ALL right )", compile(node))
+        end
+      end
+
       describe "Nodes::NotIn" do
         it "should know how to visit" do
           node = @attr.not_in [1, 2, 3]
-- 
cgit v1.2.3