convert from a traverser arel to a visitor

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

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

spec/lib/miq_expression/query_helper_spec.rb → 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,31 +16,31 @@
     # 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
 
     # query:  "vms"."host_id" = "hosts"."id"
     # 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
 
     # query:  "hosts"."id" = "vms"."host_id"
     # 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
 
     # query:  "hosts"."type" = 'abc'
     # 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,23 +103,23 @@
     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
 
     # query:  ("hosts"."type" = 'abc') AND "vms"."host_id" = "hosts"."id"
     # 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
 
     # query:  "hosts"."type" = 'abc' AND ("vms"."host_id" = "hosts"."id")
     # 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