Separate arel generation from field/tag parsing

When `Field` was extracted it was attractive to let it be a middleman
between `MiqExpression` and the Arel library since everything goes
through `Field#arel_attribute`. While this started out as
straightforward delegation, more supporting code was required to
bridge more complex expressions such as `CONTAINS`, or things that
require extra arguments like `matches`, etc..

I think that consequently `Field` (and `Tag`) have taken too much on
in terms of responsibility. It also introduced a cohesion problem in
that not all fields are concerned with columns in the database.

While this change might be a step back for `MiqExpression`, I think it
greatly improves `Field`. I think there are better patterns out there
for better separating responsibilities here, but that can be left to a
future revision.

lib/miq_expression.rb

@@ -1664,38 +1664,48 @@ def self.ruby_for_date_compare(col_ruby, col_type, tz, op1, val1, op2 = nil, val
   def to_arel(exp, tz)
     operator = exp.keys.first
     field = Field.parse(exp[operator]["field"]) if exp[operator].kind_of?(Hash) && exp[operator]["field"]
+    arel_attribute = field && field.target.arel_attribute(field.column)
     if(exp[operator].kind_of?(Hash) && exp[operator]["value"] && Field.is_field?(exp[operator]["value"]))
-      parsed_value = Field.parse(exp[operator]["value"]).arel_attribute
+      field_value = Field.parse(exp[operator]["value"])
+      parsed_value = field_value.target.arel_attribute(field_value.column)
     elsif exp[operator].kind_of?(Hash)
       parsed_value = exp[operator]["value"]
     end
     case operator.downcase
     when "equal", "="
-      field.eq(parsed_value)
+      arel_attribute.eq(parsed_value)
     when ">"
-      field.gt(parsed_value)
+      arel_attribute.gt(parsed_value)
     when "after"
       value = RelativeDatetime.normalize(parsed_value, tz, "end", field.date?)
-      field.gt(value)
+      arel_attribute.gt(value)
     when ">="
-      field.gteq(parsed_value)
+      arel_attribute.gteq(parsed_value)
     when "<"
-      field.lt(parsed_value)
+      arel_attribute.lt(parsed_value)
     when "before"
       value = RelativeDatetime.normalize(parsed_value, tz, "beginning", field.date?)
-      field.lt(value)
+      arel_attribute.lt(value)
     when "<="
-      field.lteq(parsed_value)
+      arel_attribute.lteq(parsed_value)
     when "!="
-      field.not_eq(parsed_value)
+      arel_attribute.not_eq(parsed_value)
     when "like", "includes"
-      field.matches("%#{parsed_value}%")
+      escape = nil
+      case_sensitive = true
+      arel_attribute.matches("%#{parsed_value}%", escape, case_sensitive)
     when "starts with"
-      field.matches("#{parsed_value}%")
+      escape = nil
+      case_sensitive = true
+      arel_attribute.matches("#{parsed_value}%", escape, case_sensitive)
     when "ends with"
-      field.matches("%#{parsed_value}")
+      escape = nil
+      case_sensitive = true
+      arel_attribute.matches("%#{parsed_value}", escape, case_sensitive)
     when "not like"
-      field.does_not_match("%#{parsed_value}%")
+      escape = nil
+      case_sensitive = true
+      arel_attribute.does_not_match("%#{parsed_value}%", escape, case_sensitive)
     when "and"
       operands = exp[operator].each_with_object([]) do |operand, result|
         next if operand.blank?
@@ -1716,45 +1726,72 @@ def to_arel(exp, tz)
     when "not", "!"
       Arel::Nodes::Not.new(to_arel(exp[operator], tz))
     when "is null"
-      field.eq(nil)
+      arel_attribute.eq(nil)
     when "is not null"
-      field.not_eq(nil)
+      arel_attribute.not_eq(nil)
     when "is empty"
-      arel = field.eq(nil)
-      arel = arel.or(field.eq("")) if field.string?
+      arel = arel_attribute.eq(nil)
+      arel = arel.or(arel_attribute.eq("")) if field.string?
       arel
     when "is not empty"
-      arel = field.not_eq(nil)
-      arel = arel.and(field.not_eq("")) if field.string?
+      arel = arel_attribute.not_eq(nil)
+      arel = arel.and(arel_attribute.not_eq("")) if field.string?
       arel
     when "contains"
       # Only support for tags of the main model
       if exp[operator].key?("tag")
         tag = Tag.parse(exp[operator]["tag"])
-        tag.contains(parsed_value)
+        ids = tag.model.find_tagged_with(:any => parsed_value, :ns => tag.namespace).pluck(:id)
+        tag.model.arel_attribute(:id).in(ids)
       else
-        field.contains(parsed_value)
+        raise unless field.associations.one?
+        reflection = field.reflections.first
+        arel = arel_attribute.eq(parsed_value)
+        arel = arel.and(Arel::Nodes::SqlLiteral.new(extract_where_values(reflection.klass, reflection.scope))) if reflection.scope
+        field.model.arel_attribute(:id).in(
+          field.target.arel_table.where(arel).project(field.target.arel_table[reflection.foreign_key]).distinct
+        )
       end
     when "is"
       value = parsed_value
       start_val = RelativeDatetime.normalize(value, tz, "beginning", field.date?)
       end_val = RelativeDatetime.normalize(value, tz, "end", field.date?)
 
       if !field.date? || RelativeDatetime.relative?(value)
-        field.between(start_val..end_val)
+        arel_attribute.between(start_val..end_val)
       else
-        field.eq(start_val)
+        arel_attribute.eq(start_val)
       end
     when "from"
       start_val, end_val = parsed_value
       start_val = RelativeDatetime.normalize(start_val, tz, "beginning", field.date?)
       end_val   = RelativeDatetime.normalize(end_val, tz, "end", field.date?)
-      field.between(start_val..end_val)
+      arel_attribute.between(start_val..end_val)
     else
       raise _("operator '%{operator_name}' is not supported") % {:operator_name => operator}
     end
   end
 
+  def extract_where_values(klass, scope)
+    relation = ActiveRecord::Relation.new klass, klass.arel_table, klass.predicate_builder
+    relation = relation.instance_eval(&scope)
+
+    begin
+      # This is basically ActiveRecord::Relation#to_sql, only using our
+      # custom visitor instance
+
+      connection = klass.connection
+      visitor    = WhereExtractionVisitor.new connection
+
+      arel  = relation.arel
+      binds = relation.bound_attributes
+      binds = connection.prepare_binds_for_database(binds)
+      binds.map! { |value| connection.quote(value) }
+      collect = visitor.accept(arel.ast, Arel::Collectors::Bind.new)
+      collect.substitute_binds(binds).join
+    end
+  end
+
   def self.determine_relat_path(ref)
     last_path = ref.name.to_s
     class_from_association_name = model_class(last_path)

lib/miq_expression/field.rb

@@ -10,8 +10,6 @@ class MiqExpression::Field < MiqExpression::Target
 )
 /x
 
-  delegate :eq, :not_eq, :lteq, :gteq, :lt, :gt, :between, :to => :arel_attribute
-
   def self.parse(field)
     parsed_params = parse_params(field) || return
     new(parsed_params[:model_name], parsed_params[:associations], parsed_params[:column] ||
@@ -39,28 +37,6 @@ def custom_attribute_column?
     column.include?(CustomAttributeMixin::CUSTOM_ATTRIBUTES_PREFIX)
   end
 
-  def matches(other)
-    escape = nil
-    case_sensitive = true
-    arel_attribute.matches(other, escape, case_sensitive)
-  end
-
-  def does_not_match(other)
-    escape = nil
-    case_sensitive = true
-    arel_attribute.does_not_match(other, escape, case_sensitive)
-  end
-
-  def contains(other)
-    raise unless associations.one?
-    reflection = reflections.first
-    arel = eq(other)
-    arel = arel.and(Arel::Nodes::SqlLiteral.new(extract_where_values(reflection.klass, reflection.scope))) if reflection.scope
-    model.arel_attribute(:id).in(
-      target.arel_table.where(arel).project(target.arel_table[reflection.foreign_key]).distinct
-    )
-  end
-
   def column_type
     if custom_attribute_column?
       CustomAttribute.where(:name => custom_attribute_column_name, :resource_type => model.to_s).first.try(:value_type)
@@ -77,10 +53,6 @@ def sub_type
     MiqReport::Formats.sub_type(column.to_sym) || column_type
   end
 
-  def arel_attribute
-    target.arel_attribute(column)
-  end
-
   def report_column
     (associations + [column]).join('.')
   end
@@ -90,44 +62,4 @@ def report_column
   def custom_attribute_column_name
     column.gsub(CustomAttributeMixin::CUSTOM_ATTRIBUTES_PREFIX, "")
   end
-
-  class WhereExtractionVisitor < Arel::Visitors::PostgreSQL
-    def visit_Arel_Nodes_SelectStatement(o, collector)
-      collector = o.cores.inject(collector) do |c, x|
-        visit_Arel_Nodes_SelectCore(x, c)
-      end
-    end
-
-    def visit_Arel_Nodes_SelectCore(o, collector)
-      unless o.wheres.empty?
-        len = o.wheres.length - 1
-        o.wheres.each_with_index do |x, i|
-          collector = visit(x, collector)
-          collector << AND unless len == i
-        end
-      end
-
-      collector
-    end
-  end
-
-  def extract_where_values(klass, scope)
-    relation = ActiveRecord::Relation.new klass, klass.arel_table, klass.predicate_builder
-    relation = relation.instance_eval(&scope)
-
-    begin
-      # This is basically ActiveRecord::Relation#to_sql, only using our
-      # custom visitor instance
-
-      connection = klass.connection
-      visitor    = WhereExtractionVisitor.new connection
-
-      arel  = relation.arel
-      binds = relation.bound_attributes
-      binds = connection.prepare_binds_for_database(binds)
-      binds.map! { |value| connection.quote(value) }
-      collect = visitor.accept(arel.ast, Arel::Collectors::Bind.new)
-      collect.substitute_binds(binds).join
-    end
-  end
 end

lib/miq_expression/tag.rb

@@ -23,11 +23,6 @@ def initialize(model, associations, column, managed = true)
     @namespace = "/#{@base_namespace}/#{column}"
   end
 
-  def contains(value)
-    ids = model.find_tagged_with(:any => value, :ns => namespace).pluck(:id)
-    model.arel_attribute(:id).in(ids)
-  end
-
   def numeric?
     false
   end

lib/miq_expression/where_extraction_visitor.rb

@@ -0,0 +1,21 @@
+class MiqExpression
+  class WhereExtractionVisitor < Arel::Visitors::PostgreSQL
+    def visit_Arel_Nodes_SelectStatement(o, collector)
+      collector = o.cores.inject(collector) do |c, x|
+        visit_Arel_Nodes_SelectCore(x, c)
+      end
+    end
+
+    def visit_Arel_Nodes_SelectCore(o, collector)
+      unless o.wheres.empty?
+        len = o.wheres.length - 1
+        o.wheres.each_with_index do |x, i|
+          collector = visit(x, collector)
+          collector << AND unless len == i
+        end
+      end
+
+      collector
+    end
+  end
+end