Merge pull request #22347 from kbrock/miq_expression_to_ruby

Miq expression removes sql only fields for filters

lib/miq_expression.rb

@@ -1,4 +1,10 @@
 class MiqExpression
+  # bit array of the types of nodes available/desired
+  MODE_NONE = 0
+  MODE_RUBY = 1
+  MODE_SQL  = 2
+  MODE_BOTH = MODE_RUBY | MODE_SQL
+
   require_nested :Tag
   include Vmdb::Logging
   attr_accessor :exp, :context_type, :preprocess_options
@@ -155,13 +161,28 @@ def self._to_human(exp, options = {})
     clause
   end
 
-  def to_ruby(tz = nil)
-    # TODO: we cache the ruby expression regardless if the tz is different
-    if @ruby
+  # @param timezone [String]   (default: "UTC")
+  # @param prune_sql [boolean] (default: false) remove expressions that are sql friendly
+  #
+  # when prune_sql is true, then the sql friendly expression was used to filter the
+  #                         records already. no reason to do that again in ruby
+  def to_ruby(timezone = nil, prune_sql: false)
+    timezone ||= "UTC".freeze
+    cached_args = prune_sql ? "#{timezone}P" : timezone
+    # clear out the cache if the args changed
+    if @chached_args != cached_args
+      @ruby = nil
+      @chached_args = cached_args
+    end
+    if @ruby == true
+      nil
+    elsif @ruby
       @ruby.dup
     elsif valid?
-      @ruby ||= self.class._to_ruby(exp.deep_clone, context_type, tz || "UTC".freeze)
-      @ruby.dup
+      pexp = preprocess_exp!(exp.deep_clone)
+      pexp, _ = prune_exp(pexp, MODE_RUBY) if prune_sql
+      @ruby = self.class._to_ruby(pexp, context_type, timezone) || true
+      @ruby == true ? nil : @ruby.dup
     else
       ""
     end
@@ -301,7 +322,8 @@ def self._to_ruby(exp, context_type, tz)
   def to_sql(tz = nil)
     tz ||= "UTC"
     pexp = preprocess_exp!(exp.deep_clone)
-    pexp, attrs = preprocess_for_sql(pexp)
+    pexp, seen = prune_exp(pexp, MODE_SQL)
+    attrs = {:supported_by_sql => (seen == MODE_SQL)}
     sql = to_arel(pexp, tz).to_sql if pexp.present?
     incl = includes_for_sql unless sql.blank?
     [sql, incl, attrs]
@@ -323,31 +345,121 @@ def preprocess_exp!(exp)
     exp
   end
 
-  def preprocess_for_sql(exp, attrs = nil)
-    attrs ||= {:supported_by_sql => true}
+  # @param operator [String]      operator (i.e.: AND, OR, NOT)
+  # @param children [Array[Hash]] array of child nodes
+  # @param unary [boolean]       true if we are dealing with a unary operator (i.e.: not)
+  #            unary:true  (i.e.: NOT),     don't collapse single child nodes
+  #            unary:false (i.e.: AND, OR), drop binary operators with a single node
+  def operator_hash(operator, children, unary: false)
+    case children&.size
+    when nil, 0
+      nil
+    when 1
+      unary ? {operator => children} : children.first
+    else
+      {operator => children}
+    end
+  end
+
+  # prune child nodes (OR, NOT, AND) using prune_exp
+  # This method simplifies the aggregate of the modes seen in the children
+  #
+  # @param children [Array<Hash>] child nodes
+  # @param mode [MODE_SQL|MODE_RUBY]  which nodes we want to keep
+  #
+  # @return
+  #   [Array] children that can be used in the given mode
+  #   [MODE_SQL|MODE_RUBY|MODE_BOTH]: mode summary for the children
+  #
+  # filtered_children:
+  #
+  #   children     | mode=sql   | mode=ruby    |
+  #   -------------|------------|--------------|
+  #   sql1, sql2   | sql1, sql2 |              |
+  #   sql1, ruby1  | sql1       | ruby1        |
+  #   ruby1, ruby2 |            | ruby1, ruby1 |
+  #
+  def prune_exp_children(children, mode, swap:)
+    seen = MODE_NONE
+    filtered_children = []
+    children.each do |child|
+      child_exp, child_seen = prune_exp(child, mode, :swap => swap)
+      seen |= child_seen
+      filtered_children << child_exp if child_exp
+    end
+    [filtered_children, seen]
+  end
+  private :prune_exp_children
+
+  # Cut up an expression into 2 expressions that can be applied sequentially:
+  # orig_exp == (exp mode sql) AND (exp mode=ruby)
+  #
+  # the sql expression is applied in the db
+  #
+  # @param exp  [Hash]               ast for miq_expression
+  # @param mode [MODE_RUBY|MODE_SQL] whether we are pruning for a sql or ruby generation
+  # @param swap [boolean]            true if we are in a NOT clause and applying Demorgan's law
+  #
+  # @returns [Hash, mode]
+  #   Hash: expression that works for the given mode
+  #   [MODE_SQL|MODE_RUBY|MODE_BOTH]: mode summary for the children
+  #
+  # NOTE on Compound nodes:
+  #
+  # exp             |==>| output (mode=sql) |and| output (mode=ruby)
+  # ----------------|---|-------------------|---|-----------------
+  # sql1  AND sql2  |==>| sql1 AND sql2     |AND|
+  # sql1  AND ruby1 |==>| sql1              |AND| ruby1
+  # ruby1 AND ruby2 |==>|                   |AND| ruby1 AND ruby2
+  #
+  # The AND case uses all nodes that match the input mode
+  #
+  # exp            |==>| output (mode=sql) |and| output (mode=ruby)
+  # ---------------|---|-------------------|---|-----------------
+  # sql1  OR sql2  |==>| sql1 OR sql2      |AND|
+  # sql1  OR ruby1 |==>|                   |AND| sql1 OR ruby1
+  # ruby1 OR ruby2 |==>|                   |AND| ruby1 OR ruby2
+  #
+  # The OR case uses all nodes that match the input mode with one exception:
+  # mixed mode expressions are completely applied in ruby to keep the same logical result.
+  #
+  #
+  # exp                |==>| exp (mode=sql) |and| exp (mode=ruby)
+  # -------------------|---|----------------|---|-----------------
+  # !(sql  OR sql)     |==>| !(sql OR sql)  |AND|
+  # !(sql  OR ruby)    |==>| !(sql)         |AND| !(ruby)
+  # !(ruby1 OR ruby2)  |==>|                |AND| !(ruby1 OR ruby2)
+  #
+  # exp                |==>| exp (mode=sql) |and| exp (mode=ruby)
+  # -------------------|---|----------------|---|-----------------
+  # !(sql   AND sql)   |==>| !(sql AND sql) |AND|
+  # !(sql   AND ruby)  |==>|                |AND| !(sql AND ruby)
+  # !(ruby1 AND ruby2) |==>|                |AND| !(ruby1 AND ruby2)
+  #
+  # Inside a NOT, the OR acts like the AND, and the AND acts like the OR
+  # so follow the AND logic if we are not swapping (and vice versa)
+  #
+  def prune_exp(exp, mode, swap: false)
     operator = exp.keys.first
-    case operator.downcase
-    when "and"
-      exp[operator].dup.each { |atom| preprocess_for_sql(atom, attrs) }
-      exp[operator].reject!(&:blank?)
-      exp.delete(operator) if exp[operator].empty?
-    when "or"
-      or_attrs = {:supported_by_sql => true}
-      exp[operator].each { |atom| preprocess_for_sql(atom, or_attrs) }
-      exp[operator].reject!(&:blank?)
-      attrs.merge!(or_attrs)
-      exp.delete(operator) if !or_attrs[:supported_by_sql] || exp[operator].empty? # Clean out unsupported or empty operands
+    down_operator = operator.downcase
+    case down_operator
+    when "and", "or"
+      children, seen = prune_exp_children(exp[operator], mode, :swap => swap)
+      if (down_operator == "and") != swap || seen != MODE_BOTH
+        [operator_hash(operator, children), seen]
+      else
+        [mode == MODE_RUBY ? exp : nil, seen]
+      end
     when "not", "!"
-      preprocess_for_sql(exp[operator], attrs)
-      exp.delete(operator) if exp[operator].empty? # Clean out empty operands
+      children, seen = prune_exp(exp[operator], mode, :swap => !swap)
+      [operator_hash(operator, children, :unary => true), seen]
     else
-      unless sql_supports_atom?(exp)
-        attrs[:supported_by_sql] = false
-        exp.delete(operator)
+      if sql_supports_atom?(exp)
+        [mode == MODE_SQL ? exp : nil, MODE_SQL]
+      else
+        [mode == MODE_RUBY ? exp : nil, MODE_RUBY]
       end
     end
-
-    exp.empty? ? [nil, attrs] : [exp, attrs]
   end
 
   def sql_supports_atom?(exp)
@@ -467,8 +579,8 @@ def self.get_col_info(field, options = {})
     }
   end
 
-  def lenient_evaluate(obj, tz = nil)
-    ruby_exp = to_ruby(tz)
+  def lenient_evaluate(obj, timezone = nil, prune_sql: false)
+    ruby_exp = to_ruby(timezone, :prune_sql => prune_sql)
     ruby_exp.nil? || Condition.subst_matches?(ruby_exp, obj)
   end
 

lib/rbac/filterer.rb

@@ -214,6 +214,7 @@ def self.accessible_tenant_ids_strategy(klass)
     # @option options :limit        [Numeric] (default: no limit)
     # @option options :offset       [Numeric] (default: no offset)
     # @option options :skip_count   [Boolean] (default: false)
+    # @option options :prune_sql    [Boolean] (default: true) true to remove sql from ruby filter
     #
     # @return [Array<Array<Object>,Hash>] list of object and the associated search options
     #   Array<Object> list of object in the same order as input targets if possible
@@ -240,6 +241,8 @@ def search(options = {})
       offset            = options[:offset] || targets.try(:offset_value)
       order             = options[:order]  || targets.try(:order_values)
 
+      prune_sql = options.key?(:prune_sql) ? options[:prune_sql] : true
+
       user, miq_group, user_filters = get_user_info(options[:user],
                                                     options[:userid],
                                                     options[:miq_group],
@@ -341,7 +344,7 @@ def search(options = {})
       #
       # NOTE: if supported_by_sql is false then apply_limit_in_sql is also false
       if search_filter && targets && (!exp_attrs || !exp_attrs[:supported_by_sql])
-        targets = targets.lazy.select { |obj| matches_search_filters?(obj, search_filter, tz) }
+        targets = targets.lazy.select { |obj| matches_search_filters?(obj, search_filter, tz, :prune_sql => prune_sql) }
         unless options[:skip_counts]
           # use to_a to run filters once. otherwise will filter for:
           # length, (possible) pagination ids, and final taget results
@@ -877,8 +880,8 @@ def get_belongsto_matches_for_storage(blist)
       MiqPreloader.preload_and_map(sources, :storages)
     end
 
-    def matches_search_filters?(obj, filter, tz)
-      filter.nil? || filter.lenient_evaluate(obj, tz)
+    def matches_search_filters?(obj, filter, timezone, prune_sql: true)
+      filter.nil? || filter.lenient_evaluate(obj, timezone, :prune_sql => prune_sql)
     end
   end
 end