From 126eeb6c4a4b042060d07252b18e7a706c5bbd60 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 4 May 2023 18:12:15 -0400 Subject: [PATCH 1/5] bust cache for to_ruby when timezone changes --- lib/miq_expression.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/miq_expression.rb b/lib/miq_expression.rb index 2fdc5bc2595..7b06b9685ca 100644 --- a/lib/miq_expression.rb +++ b/lib/miq_expression.rb @@ -155,8 +155,14 @@ 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 + def to_ruby(timezone = nil) + timezone ||= "UTC".freeze + cached_args = timezone + # clear out the cache if the args changed + if @chached_args != cached_args + @ruby = nil + @chached_args = cached_args + end if @ruby @ruby.dup elsif valid? From 0db6fc391f89aff26117fb392be3ea0a69c80565 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Wed, 3 May 2023 11:34:45 -0400 Subject: [PATCH 2/5] introduce MiqExpression#prune_exp The goal is to prune nodes from an expression tree that are not applicable to our target language When generating sql, we only want to process nodes that are sql friendly. In the case that both sql and ruby are in and OR clause, we will need to run this in ruby, so both are removed. Logically you can not split up the entries in the OR clause. When generating ruby, we only want to process ruby nodes. Or in the case that both sql and ruby are needed together, we deal with both. This is based upon the original preprocess_for_sql, but it allows flexibility for choosing to keep ruby OR sql. This all came about because in Rbac we are filtering in both sql and ruby. The sql filter was reduced, but the ruby filter contained both the ruby and sql filters. It doesn't make sense to run the same sql filters when processing ruby since those have already been run. In a followup commit, there is a note around demorgan's law which may help you understand a little more about how these various components are splitup --- lib/miq_expression.rb | 126 ++++++++++++++++++++++++++++++-- spec/lib/miq_expression_spec.rb | 98 +++++++++++++++++++++++-- 2 files changed, 211 insertions(+), 13 deletions(-) diff --git a/lib/miq_expression.rb b/lib/miq_expression.rb index 7b06b9685ca..1c1af31445b 100644 --- a/lib/miq_expression.rb +++ b/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,19 +161,28 @@ def self._to_human(exp, options = {}) clause end - def to_ruby(timezone = nil) + # @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 = timezone + 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 + 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 @@ -356,6 +371,107 @@ def preprocess_for_sql(exp, attrs = nil) exp.empty? ? [nil, attrs] : [exp, attrs] end + # @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] 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) + seen = MODE_NONE + filtered_children = [] + children.each do |child| + child_exp, child_seen = prune_exp(child, mode) + 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. + # + def prune_exp(exp, mode) + operator = exp.keys.first + down_operator = operator.downcase + case down_operator + when "and", "or" + children, seen = prune_exp_children(exp[operator], mode) + if down_operator == "and" || seen != MODE_BOTH + [operator_hash(operator, children), seen] + else + [mode == MODE_RUBY ? exp : nil, seen] + end + when "not", "!" + children, seen = prune_exp(exp[operator], mode) + [operator_hash(operator, children, :unary => true), seen] + else + if sql_supports_atom?(exp) + [mode == MODE_SQL ? exp : nil, MODE_SQL] + else + [mode == MODE_RUBY ? exp : nil, MODE_RUBY] + end + end + end + def sql_supports_atom?(exp) operator = exp.keys.first case operator.downcase diff --git a/spec/lib/miq_expression_spec.rb b/spec/lib/miq_expression_spec.rb index 8525c767ed9..276c0ba24d1 100644 --- a/spec/lib/miq_expression_spec.rb +++ b/spec/lib/miq_expression_spec.rb @@ -247,37 +247,106 @@ context "mode: :sql" do it "(sql AND ruby) => (sql)" do - expect(sql_reduced_exp("AND" => [sql_field, ruby_field.clone])).to eq("AND" => [sql_field]) + expect(sql_pruned_exp("AND" => [sql_field, ruby_field.clone])).to eq("AND" => [sql_field]) end it "(ruby AND ruby) => ()" do - expect(sql_reduced_exp("AND" => [ruby_field.clone, ruby_field.clone])).to be_nil + expect(sql_pruned_exp("AND" => [ruby_field.clone, ruby_field.clone])).to be_nil end it "(sql OR sql) => (sql OR sql)" do - expect(sql_reduced_exp("OR" => [sql_field, sql_field])).to eq("OR" => [sql_field, sql_field]) + expect(sql_pruned_exp("OR" => [sql_field, sql_field])).to eq("OR" => [sql_field, sql_field]) end it "(sql OR ruby) => ()" do - expect(sql_reduced_exp("OR" => [sql_field, ruby_field])).to be_nil + expect(sql_pruned_exp("OR" => [sql_field, ruby_field])).to be_nil end it "(ruby OR ruby) => ()" do - expect(sql_reduced_exp("OR" => [ruby_field.clone, ruby_field.clone].deep_clone)).to be_nil + expect(sql_pruned_exp("OR" => [ruby_field.clone, ruby_field.clone].deep_clone)).to be_nil end it "!(sql OR ruby) => (!(sql) AND !(ruby)) => !(sql)" do - expect(sql_reduced_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to be_nil + expect(sql_pruned_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to be_nil # TODO: eq("NOT" => sql_field) end it "!(sql AND ruby) => (!(sql) OR !(ruby)) => nil" do - expect(sql_reduced_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => {"AND" => [sql_field]}) + expect(sql_pruned_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => {"AND" => [sql_field]}) # TODO: be_nil end end end + describe "#prune_exp" do + let(:sql_field) { {"=" => {"field" => "Vm-name", "value" => "foo"}.freeze}.freeze } + let(:ruby_field) { {"=" => {"field" => "Vm-platform", "value" => "bar"}.freeze}.freeze } + + context "mode: ruby" do + it "(sql) => ()" do + expect(ruby_pruned_exp(sql_field.clone)).to be_nil + end + + it "(ruby) => (ruby)" do + expect(ruby_pruned_exp(ruby_field.clone)).to eq(ruby_field) + end + + it "(sql and sql) => ()" do + expect(ruby_pruned_exp("AND" => [sql_field.clone, sql_field.clone])).to be_nil + end + + it "(sql and ruby) => (ruby)" do + expect(ruby_pruned_exp("AND" => [sql_field.clone, ruby_field.clone])).to eq(ruby_field) + end + + it "(ruby or ruby) => (ruby or ruby)" do + expect(ruby_pruned_exp("OR" => [ruby_field.clone, ruby_field.clone])).to eq("OR" => [ruby_field, ruby_field]) + end + + it "(sql or sql) => ()" do + expect(ruby_pruned_exp("OR" => [sql_field.clone, sql_field.clone])).to be_nil + end + + it "(sql or ruby) => (sql or ruby)" do + expect(ruby_pruned_exp("OR" => [ruby_field.clone, sql_field.clone])).to eq("OR" => [ruby_field, sql_field]) + end + + it "(ruby or ruby) => (ruby or ruby)" do + expect(ruby_pruned_exp("OR" => [ruby_field.clone, ruby_field.clone])).to eq("OR" => [ruby_field, ruby_field]) + end + + it "(sql AND sql) or ruby => keep all expressions" do + expect(ruby_pruned_exp("OR" => [{"AND" => [sql_field.clone, sql_field.clone]}, ruby_field.clone])).to eq("OR" => [{"AND" => [sql_field, sql_field]}, ruby_field]) + end + + # ensuring that the OR/AND is treating each sub expression independently + # it was getting this wrong + it "(sql or sql) and ruby => ruby" do + expect(ruby_pruned_exp("AND" => [{"OR" => [sql_field.clone, sql_field.clone]}, ruby_field.clone])).to eq(ruby_field) + end + + it "ruby and (sql or sql) => ruby" do + expect(ruby_pruned_exp("AND" => [ruby_field.clone, {"OR" => [sql_field.clone, sql_field.clone]}])).to eq(ruby_field) + end + + it "!(ruby) => keep all expressions" do + exp1 = {"=" => {"field" => "Vm-platform", "value" => "foo"}} + ruby = MiqExpression.new("NOT" => exp1).to_ruby(:prune_sql => true) + expect(ruby).to eq("!(/virtual/platform == \"foo\")") + end + + it "!(sql OR ruby) => (!(sql) AND !(ruby)) => !(ruby)" do + expect(ruby_pruned_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to eq("NOT" => {"OR" => [sql_field, ruby_field.clone]}) + # TODO: eq("NOT" => ruby_field) + end + + it "!(sql AND ruby) => (!(sql) OR !(ruby)) => !(sql AND ruby)" do + expect(ruby_pruned_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => ruby_field) + # TODO: eq("NOT" => {"AND" => [sql_field, ruby_field]}) + end + end + end + describe "#to_sql" do it "returns nil if SQL generation for that expression is not supported" do sql, * = MiqExpression.new("=" => {"field" => "Service-custom_1", "value" => ""}).to_sql @@ -1036,6 +1105,13 @@ end end end + + context "caching" do + it "clears caching if prune_sql value changes" do + exp = MiqExpression.new({"=" => {"field" => "Vm-name", "value" => "foo"}}) + expect(exp.to_ruby(:prune_sql => true)).not_to eq(exp.to_ruby(:prune_sql => false)) + end + end end describe "#lenient_evaluate" do @@ -3472,9 +3548,15 @@ private - def sql_reduced_exp(input) + def sql_pruned_exp(input) mexp = MiqExpression.new(input) pexp = mexp.preprocess_exp!(mexp.exp.deep_clone) mexp.preprocess_for_sql(pexp).first end + + def ruby_pruned_exp(input) + mexp = MiqExpression.new(input) + pexp = mexp.preprocess_exp!(mexp.exp.deep_clone) + mexp.prune_exp(pexp, MiqExpression::MODE_RUBY).first + end end From c4afefd33e8d8c74c00e3baa1a316b6bc8f73225 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Mon, 13 Feb 2023 12:47:34 -0500 Subject: [PATCH 3/5] use MiqExpression#reduce_exp for sql --- lib/miq_expression.rb | 30 ++---------------------------- spec/lib/miq_expression_spec.rb | 17 ++++++----------- 2 files changed, 8 insertions(+), 39 deletions(-) diff --git a/lib/miq_expression.rb b/lib/miq_expression.rb index 1c1af31445b..44dff20a00d 100644 --- a/lib/miq_expression.rb +++ b/lib/miq_expression.rb @@ -322,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] @@ -344,33 +345,6 @@ def preprocess_exp!(exp) exp end - def preprocess_for_sql(exp, attrs = nil) - attrs ||= {:supported_by_sql => true} - 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 - when "not", "!" - preprocess_for_sql(exp[operator], attrs) - exp.delete(operator) if exp[operator].empty? # Clean out empty operands - else - unless sql_supports_atom?(exp) - attrs[:supported_by_sql] = false - exp.delete(operator) - end - end - - exp.empty? ? [nil, attrs] : [exp, attrs] - end - # @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) diff --git a/spec/lib/miq_expression_spec.rb b/spec/lib/miq_expression_spec.rb index 276c0ba24d1..6a2ce186ab1 100644 --- a/spec/lib/miq_expression_spec.rb +++ b/spec/lib/miq_expression_spec.rb @@ -241,13 +241,13 @@ end end - describe "#preprocess_for_sql" do + describe "#reduce_exp" do let(:sql_field) { {"=" => {"field" => "Vm-name", "value" => "foo"}.freeze}.freeze } let(:ruby_field) { {"=" => {"field" => "Vm-platform", "value" => "bar"}.freeze}.freeze } context "mode: :sql" do it "(sql AND ruby) => (sql)" do - expect(sql_pruned_exp("AND" => [sql_field, ruby_field.clone])).to eq("AND" => [sql_field]) + expect(sql_pruned_exp("AND" => [sql_field, ruby_field.clone])).to eq(sql_field) end it "(ruby AND ruby) => ()" do @@ -272,15 +272,10 @@ end it "!(sql AND ruby) => (!(sql) OR !(ruby)) => nil" do - expect(sql_pruned_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => {"AND" => [sql_field]}) + expect(sql_pruned_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => sql_field) # TODO: be_nil end end - end - - describe "#prune_exp" do - let(:sql_field) { {"=" => {"field" => "Vm-name", "value" => "foo"}.freeze}.freeze } - let(:ruby_field) { {"=" => {"field" => "Vm-platform", "value" => "bar"}.freeze}.freeze } context "mode: ruby" do it "(sql) => ()" do @@ -360,7 +355,7 @@ it "does not raise error for SQL generation if expression has a count in it" do sql, _ = MiqExpression.new("AND" => [{"=" => {"field" => "Vm-name", "value" => "foo"}}, {"=" => {"count" => "Vm.snapshots", "value" => "1"}}]).to_sql - expect(sql).to eq("(\"vms\".\"name\" = 'foo')") + expect(sql).to eq("\"vms\".\"name\" = 'foo'") end it "generates the SQL for an = expression if SQL generation for expression supported and 'token' key present in expression's Hash" do @@ -513,7 +508,7 @@ exp1 = {"STARTS WITH" => {"field" => "Vm-name", "value" => "foo"}} exp2 = {"ENDS WITH" => {"field" => "Vm-platform", "value" => "bar"}} sql, * = MiqExpression.new("AND" => [exp1, exp2]).to_sql - expect(sql).to eq("(\"vms\".\"name\" LIKE 'foo%')") + expect(sql).to eq("\"vms\".\"name\" LIKE 'foo%'") end it "returns nil for an AND expression where none is supported by SQL" do @@ -3551,7 +3546,7 @@ def sql_pruned_exp(input) mexp = MiqExpression.new(input) pexp = mexp.preprocess_exp!(mexp.exp.deep_clone) - mexp.preprocess_for_sql(pexp).first + mexp.prune_exp(pexp, MiqExpression::MODE_SQL).first end def ruby_pruned_exp(input) From 400a3b11f171c7886314fc1da026f8ffcda30031 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Mon, 13 Feb 2023 13:37:48 -0500 Subject: [PATCH 4/5] Rbac#filter prunes ruby in miq expression rbac#filter/search can use an miq expression to filter the results. These expressions can be ruby only, sql, or a combination of both. The issue is the code was smart enough to send the sql the WHERE clause that was sql friendly. But when we were filtering in ruby, we performed all the sql filtering and ruby filtering. For 1k records, this takes a hit. After We send the sql only filters to sql and the ruby only filters to ruby. In cases where the OR mixes sql friendly and ruby friendly parameters, we still perform the sql friendly filters in ruby. (logically it is the only way to do it) --- lib/miq_expression.rb | 4 ++-- lib/rbac/filterer.rb | 9 ++++++--- spec/lib/rbac/filterer_spec.rb | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/miq_expression.rb b/lib/miq_expression.rb index 44dff20a00d..1f690699c22 100644 --- a/lib/miq_expression.rb +++ b/lib/miq_expression.rb @@ -563,8 +563,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 diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb index 451e83af0af..365ba47e0b2 100644 --- a/lib/rbac/filterer.rb +++ b/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,Hash>] list of object and the associated search options # Array 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 diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb index 375349ba2b1..29eec311b13 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -2502,7 +2502,7 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) it "supports limits in ruby with limits on scopes" do filter = MiqExpression.new("=" => {"field" => "Vm-location", "value" => "b"}) # force this filter to be evaluated in ruby - expect(filter).to receive(:sql_supports_atom?).and_return(false) + expect(filter).to receive(:sql_supports_atom?).at_least(:once).and_return(false) FactoryBot.create_list(:vm_vmware, 3, :location => "a") FactoryBot.create_list(:vm_vmware, 3, :location => "b") From 029bc1b8e37baacf348223dceed5c4f5f615e249 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Mon, 13 Feb 2023 14:22:12 -0500 Subject: [PATCH 5/5] MiqExpression introduce demorgan's law Without this change, we were incorrectly filtering records This mostly worked when the ruby expression handled all expressions but it especially breaks down when the two are kept separate --- lib/miq_expression.rb | 28 ++++++++++++++++++++++------ spec/lib/miq_expression_spec.rb | 12 ++++-------- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/lib/miq_expression.rb b/lib/miq_expression.rb index 1f690699c22..725cc7aa5c7 100644 --- a/lib/miq_expression.rb +++ b/lib/miq_expression.rb @@ -379,11 +379,11 @@ def operator_hash(operator, children, unary: false) # sql1, ruby1 | sql1 | ruby1 | # ruby1, ruby2 | | ruby1, ruby1 | # - def prune_exp_children(children, mode) + def prune_exp_children(children, mode, swap:) seen = MODE_NONE filtered_children = [] children.each do |child| - child_exp, child_seen = prune_exp(child, mode) + child_exp, child_seen = prune_exp(child, mode, :swap => swap) seen |= child_seen filtered_children << child_exp if child_exp end @@ -423,19 +423,35 @@ def prune_exp_children(children, mode) # 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. # - def prune_exp(exp, mode) + # + # 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 down_operator = operator.downcase case down_operator when "and", "or" - children, seen = prune_exp_children(exp[operator], mode) - if down_operator == "and" || seen != MODE_BOTH + 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", "!" - children, seen = prune_exp(exp[operator], mode) + children, seen = prune_exp(exp[operator], mode, :swap => !swap) [operator_hash(operator, children, :unary => true), seen] else if sql_supports_atom?(exp) diff --git a/spec/lib/miq_expression_spec.rb b/spec/lib/miq_expression_spec.rb index 6a2ce186ab1..c686dc375b8 100644 --- a/spec/lib/miq_expression_spec.rb +++ b/spec/lib/miq_expression_spec.rb @@ -267,13 +267,11 @@ end it "!(sql OR ruby) => (!(sql) AND !(ruby)) => !(sql)" do - expect(sql_pruned_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to be_nil - # TODO: eq("NOT" => sql_field) + expect(sql_pruned_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to eq("NOT" => sql_field) end it "!(sql AND ruby) => (!(sql) OR !(ruby)) => nil" do - expect(sql_pruned_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => sql_field) - # TODO: be_nil + expect(sql_pruned_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to be_nil end end @@ -331,13 +329,11 @@ end it "!(sql OR ruby) => (!(sql) AND !(ruby)) => !(ruby)" do - expect(ruby_pruned_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to eq("NOT" => {"OR" => [sql_field, ruby_field.clone]}) - # TODO: eq("NOT" => ruby_field) + expect(ruby_pruned_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to eq("NOT" => ruby_field) end it "!(sql AND ruby) => (!(sql) OR !(ruby)) => !(sql AND ruby)" do - expect(ruby_pruned_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => ruby_field) - # TODO: eq("NOT" => {"AND" => [sql_field, ruby_field]}) + expect(ruby_pruned_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => {"AND" => [sql_field, ruby_field]}) end end end