From 6005c51553980dceb80cb65104140a6b8a154f30 Mon Sep 17 00:00:00 2001
From: Keenan Brock <keenan@thebrocks.net>
Date: Fri, 24 Apr 2020 20:47:37 -0400
Subject: [PATCH] convert from a traverser arel to a visitor

---
 lib/miq_expression.rb                         |  2 +-
 lib/miq_expression/query_helper.rb            | 59 ------------
 lib/miq_expression/remove_equality_visitor.rb | 93 +++++++++++++++++++
 ...pec.rb => remove_equality_visitor_spec.rb} | 30 +++---
 4 files changed, 109 insertions(+), 75 deletions(-)
 delete mode 100644 lib/miq_expression/query_helper.rb
 create mode 100644 lib/miq_expression/remove_equality_visitor.rb
 rename spec/lib/miq_expression/{query_helper_spec.rb => remove_equality_visitor_spec.rb} (83%)

diff --git a/lib/miq_expression.rb b/lib/miq_expression.rb
index 27a8e3c490d..eb24b06033a 100644
--- a/lib/miq_expression.rb
+++ b/lib/miq_expression.rb
@@ -1475,7 +1475,7 @@ def subquery_for_contains(field, limiter_query)
     #    Typically there will be no extra part (A5) of the join query, and this will return nil
     # b) Also Convert binds to sql values in the join clause (A5)
     binds = ActiveRecord.version.to_s >= "5.2" ? [] : query_relation.bound_attributes
-    join_query = QueryHelper.accept(join_query, primary_attribute.eq(secondary_attribute), binds)
+    join_query = RemoveEqualityVisitor.accept(join_query, primary_attribute.eq(secondary_attribute), binds)
 
     # If there is a join clause (A5), add it to the query producing B7
     query.where(join_query) if join_query
diff --git a/lib/miq_expression/query_helper.rb b/lib/miq_expression/query_helper.rb
deleted file mode 100644
index fb10de7b42d..00000000000
--- a/lib/miq_expression/query_helper.rb
+++ /dev/null
@@ -1,59 +0,0 @@
-class MiqExpression
-  class QueryHelper
-    # This method does 2 things:
-    # - remove node from the query
-    # - replace bind variables in the query
-    #
-    # Active record has the concept of the bind param values, but arel does not.
-    # So converting a query into arel will loose the values for the bind parameters.
-    # The arel becomes incomplete and can not generate valid sql.
-    #
-    # This method takes the values from they query's bound_attributes and plugs them into the arel.
-    # The arel once again can generate valid sql.
-    #
-    # @param query [Arel::Nodes::Node] a where clause
-    # @param node_to_remove [Arel::Nodes::Equality] the clause to remove
-    # @param bind [Array[ActiveModel::Attribute]] values to populate in the BindParam slots.
-    # @return query without the node_to_remove
-    def self.remove_node(query, node_to_remove, binds = [])
-      traverse_and_replace(query) do |q|
-        if (q.left == node_to_remove.left && q.right == node_to_remove.right) ||
-          (q.right == node_to_remove.left && q.left == node_to_remove.right)
-          # remove this node from the query
-          nil
-         elsif q.right.kind_of?(Arel::Nodes::BindParam)
-          # replace this node with "field = $(next value from the binds array)"
-          value = object.right.respond_to?(:value) ? object.right.value.value : collector.next_bind_value.value_for_database
-          q.left.eq(value)
-        else
-          # leave this node in the query unchanged
-          q
-        end
-      end
-    end
-
-    def self.traverse_and_replace(query, &bind)
-      return if query.nil?
-
-      if query.kind_of?(Arel::Nodes::And)
-        children = query.children.compact.flatten.map { |q| traverse_and_replace(q, &bind) }
-        children.compact!
-
-        if children.empty?
-          nil
-        elsif children.size == 1
-          children.first
-        else
-          query.class.new(children)
-        end
-      elsif query.kind_of?(Arel::Nodes::Grouping)
-        q2 = traverse_and_replace(query.expr, &bind)
-        q2 ? query.class.new(q2) : q2
-      elsif query.kind_of?(Arel::Nodes::Equality)
-        yield(query)
-      else
-        query
-      end
-    end
-  end
-end
diff --git a/lib/miq_expression/remove_equality_visitor.rb b/lib/miq_expression/remove_equality_visitor.rb
new file mode 100644
index 00000000000..866fe2193da
--- /dev/null
+++ b/lib/miq_expression/remove_equality_visitor.rb
@@ -0,0 +1,93 @@
+class MiqExpression
+  class RemoveEqualityCollector
+    respond_to :binds
+    def initialize(node_to_remove, binds)
+      @node_to_remove = node_to_remove
+      @binds = binds
+    end
+
+    attr_reader :node_to_remove
+
+    def next_bind_value
+      @binds.shift
+    end
+  end
+  # rubocop:disable Naming/MethodName
+  class RemoveEqualityVisitor < Arel::Visitors::Reduce
+    # @return [Arel::Node|nil] the replacement node from this grouping
+    def visit_nil(object, _collector)
+      object
+    end
+
+    # @return [Arel::Node|nil] the replacement node from this grouping
+    def visit_Arel_Nodes_Equality(object, collector)
+      field = collector.node_to_remove
+      if (object.left == field.left && object.right == field.right) ||
+         (object.right == field.left && object.left == field.right)
+        # remove this node from the query
+        nil
+      elsif object.right.kind_of?(Arel::Nodes::BindParam)
+        # replace this node with "field = $(next value from the binds array)"
+        value = object.right
+        value = collector.next_bind_value if !value.respond_to?(:value_for_database)
+        object.left.eq(value.value_for_database)
+      else
+        # leave this node in the query unchanged
+        object
+      end
+    end
+
+    # @return [Arel::Node|nil] the replacement node from this grouping
+    def visit_Arel_Nodes_And(object, collector)
+      children = object.children.compact.flatten.map { |q| visit(q, collector) }
+      children.compact!
+
+      if children.blank?
+        nil
+      elsif children.size == 1
+        children.first
+      elsif children == object.children
+        object
+      else
+        Arel::Nodes::And.new(children)
+      end
+    end
+
+    # @return [Arel::Node|nil] the replacement node from this grouping
+    def visit_Arel_Nodes_Grouping(object, collector)
+      expr = visit(object.expr, collector)
+      if expr == object.expr
+        object
+      elsif expr.nil?
+        nil
+      else
+        Arel::Nodes::Grouping.new(expr)
+      end
+    end
+
+    # This method does 2 things:
+    # - remove node_to_remove from the query
+    # - replace bind variables with static variables in the query
+    #
+    # collector is just a tuple of the node_to_remove and the bound_attributes that are passed in,
+    #   and not a ActiveRecord/Arel construct.
+    #
+    # Active record has the concept of the bind param values, but arel does not.
+    # So converting a query into arel will lose the values for the bind parameters.
+    # The arel becomes incomplete and can not generate valid sql.
+    #
+    # This method takes the values from the query's bound_attributes and plugs them into the arel.
+    # The arel once again can generate valid sql.
+    #
+    # @param query [Arel::Nodes::Node] a where clause (this will be mutated)
+    # @param node_to_remove [Arel::Nodes::Equality] the clause to remove
+    # @param bind [Array[ActiveModel::Attribute]] values to populate in the BindParam slots (this will be mutated)
+    # @return [Arel::Nodes::Node|nil] query without the node_to_remove
+    def self.accept(query, node_to_remove, binds = [])
+      # TODO: use collector to receive nodes (vs returning the value)
+      collector = RemoveEqualityCollector.new(node_to_remove, binds)
+      new.accept(query, collector) unless query.nil?
+    end
+  end
+  # rubocop:enable Naming/MethodName
+end
diff --git a/spec/lib/miq_expression/query_helper_spec.rb b/spec/lib/miq_expression/remove_equality_visitor_spec.rb
similarity index 83%
rename from spec/lib/miq_expression/query_helper_spec.rb
rename to spec/lib/miq_expression/remove_equality_visitor_spec.rb
index 28cfa7f7178..d5930b58ad7 100644
--- a/spec/lib/miq_expression/query_helper_spec.rb
+++ b/spec/lib/miq_expression/remove_equality_visitor_spec.rb
@@ -1,5 +1,5 @@
-RSpec.describe MiqExpression::QueryHelper do
-  describe ".remove_node" do
+RSpec.describe MiqExpression::RemoveEqualityVisitor do
+  describe ".accept" do
     let(:vm_host_id) { Vm.arel_table[:host_id] }
     let(:host_id) { Host.arel_table[:id] }
     let(:host_type) { Host.arel_table[:type] }
@@ -16,7 +16,7 @@
     # result: nil
     it "handles an empty query" do
       query = nil
-      result = described_class.remove_node(query, matching_query)
+      result = described_class.accept(query, matching_query)
       expect(result).to be_nil
     end
 
@@ -24,7 +24,7 @@
     # result: nil
     it "removes a matching query" do
       query = matching_query
-      result = described_class.remove_node(query, matching_query)
+      result = described_class.accept(query, matching_query)
       expect(result).to be_nil
     end
 
@@ -32,7 +32,7 @@
     # result: nil
     it "removes a matching query (opposite order)" do
       query = host_id.eq(vm_host_id)
-      result = described_class.remove_node(query, matching_query)
+      result = described_class.accept(query, matching_query)
       expect(result).to be_nil
     end
 
@@ -40,7 +40,7 @@
     # result: "hosts"."type" = 'abc'
     it "keeps a query that does not have `matching_query` in it" do
       query = host_type.eq('abc')
-      result = described_class.remove_node(query, matching_query, [])
+      result = described_class.accept(query, matching_query, [])
       expect(result).to eq(query)
     end
 
@@ -49,7 +49,7 @@
     it "keeps a query that does not have `matching_query` in it, replacing the binds" do
       query = host_type.eq(bind_param)
       expected = host_type.eq(Arel::Nodes::Casted.new(5, host_type))
-      result = described_class.remove_node(query, matching_query, bind_values(5))
+      result = described_class.accept(query, matching_query, bind_values(5))
       expect(result).to eq(expected)
     end
 
@@ -58,7 +58,7 @@
     # A single element in an and doesn't make sense, but we saw it come into this method so we test it
     it "removes a matching query in an AND (single node)" do
       query = arel_and(matching_query)
-      result = described_class.remove_node(query, matching_query)
+      result = described_class.accept(query, matching_query)
       expect(result).to be_nil
     end
 
@@ -67,7 +67,7 @@
     # The input form is not quite valid, but we saw it come into this method so we test it
     it "removes matching and nil nodes in an AND" do
       query = arel_and(nil, matching_query)
-      result = described_class.remove_node(query, matching_query)
+      result = described_class.accept(query, matching_query)
       expect(result).to be_nil
     end
 
@@ -76,7 +76,7 @@
     it "removes matching query at the beginning of a tripple AND" do
       query = arel_and(matching_query, host_type.eq('abc'), host_type.eq('def'))
       expected = host_type.eq('abc').and(host_type.eq('def'))
-      result = described_class.remove_node(query, matching_query)
+      result = described_class.accept(query, matching_query)
       expect(result).to eq(expected)
     end
 
@@ -85,7 +85,7 @@
     it "removes matching query in the middle of a tripple AND" do
       query = arel_and(host_type.eq('abc'), matching_query, host_type.eq('def'))
       expected = host_type.eq('abc').and(host_type.eq('def'))
-      result = described_class.remove_node(query, matching_query, [])
+      result = described_class.accept(query, matching_query, [])
       expect(result).to eq(expected)
     end
 
@@ -94,7 +94,7 @@
     it "removes matching query at the end of a tripple AND" do
       query = matching_query.and(host_type.eq('abc').and(host_type.eq('def')))
       expected = host_type.eq('abc').and(host_type.eq('def'))
-      result = described_class.remove_node(query, matching_query)
+      result = described_class.accept(query, matching_query)
       expect(result).to eq(expected)
     end
 
@@ -103,7 +103,7 @@
     it "removes matching query from multiple layers where our equality is at the base" do
       query = host_type.eq('abc').and(matching_query.and(host_type.eq('def')))
       expected = host_type.eq('abc').and(host_type.eq('def'))
-      result = described_class.remove_node(query, matching_query)
+      result = described_class.accept(query, matching_query)
       expect(result).to eq(expected)
     end
 
@@ -111,7 +111,7 @@
     # result: ("hosts"."type" = 'abc')
     it "removes matching query with grouped other node" do
       query = arel_group(host_type.eq('abc')).and(matching_query)
-      result = described_class.remove_node(query, matching_query, [])
+      result = described_class.accept(query, matching_query, [])
       expect(result).to eq(arel_group(host_type.eq('abc')))
     end
 
@@ -119,7 +119,7 @@
     # result: "hosts"."type" = 'abc'
     it "removes matching node with grouped matchng node" do
       query = host_type.eq('abc').and(arel_group(matching_query))
-      result = described_class.remove_node(query, matching_query)
+      result = described_class.accept(query, matching_query)
       expect(result).to eq(host_type.eq('abc'))
     end