[WIP] Remove <value> nodes from MiqExpression#to_ruby

[kbrock]

Overview

For now, this is only changes simple expressions like Host-name and not those with an association Vm.host-name.
It also does not change <find> clauses.

Looking for feedback on whether this is a good route. (I'm happy to merge code)

exp = MiqExpression.new({"=" => {"field" => "Vm-platform", "value" => "foo"}})
tmp_ruby_exp = exp.to_ruby
records.select do |rec|
  ruby_exp = Condition.subst(tmp_ruby_exp, rec)
  # Condition.do_eval(ruby_exp, rec)
  eval(ruby_exp)
end

Before

# to_ruby produces psudo ruby
tmp_ruby_exp = "!(<value ref=vm, type=string>/virtual/platform</value> == \"foo\"")
# for each record, `subst` runs `tmp_ruby_exp.gsub(/<value/>/, rec.platform)`.
ruby_exp = "!(\"windows\" == \"foo\")"
# eval = false

After

# to_ruby produces pseudo ruby
tmp_ruby_exp = "!(rec.platform == \"foo\")"
# subst is a noop
ruby_exp = "!(rec.platform == \"foo\")"
# eval = false

[kbrock]

@Fryguy When @jrafanie was working through performance for MiqExpression (in the api?), we saw a lot of time spent in subst on large record sets.

So I had started modifying subst but realized modifying to_ruby to not producing the <value> in the first place was a lot easier than untangling the code in subst.

Got a little bit of a wrench for the "hash" mode. It is used in 2 spots for the reports and the ui. Wondering why we need a whole separate encoding scheme for that use case.

Question: Do you feel this is a good direction or too much of a tangent?
If so, I can fix the spec and style

[miq-bot]

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

[miq-bot]

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

[miq-bot]

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

[kbrock]

def lenient
  ruby_exp = to_ruby(timezone, :prune_sql => prune_sql)
  ruby_exp.nil? ? true : Condition.subst_matches?(ruby_exp, obj)
end

def evaluate
  ruby_exp = to_ruby(tz)
  # ruby_exp is never null?
  ruby_exp.nil? ? nil : Condition.subst_matches?(ruby_exp, obj)
end

def evaluate
  ruby_exp = to_ruby(timezone, :prune_sql => prune_sql)
  if ruby_exp.nil?
    if lenient
      return true
    else
      return false # noop ??? verify
    end
  end

  Condition.subst_matches?(ruby_exp, obj)
end

def evaluate
  ruby_exp = to_ruby(timezone, :prune_sql => prune_sql)
  ruby_exp.nil? ? lenient : Condition.subst_matches?(ruby_exp, obj)
end

Since we know non-lenient ruby_exp must always be present (it throws an exception so we know the path is not valid) I wonder if we can drop the non-lenient form of evaluate.

[kbrock]

This code moved to #23467

[miq-bot]

Some comments on commit kbrock@b7f07cc

spec/lib/miq_expression_spec.rb

• ⚠️ - 1531 - Detected puts. Remove all debugging statements.
• ⚠️ - 1538 - Detected puts. Remove all debugging statements.
• ⚠️ - 1545 - Detected puts. Remove all debugging statements.
• ⚠️ - 1587 - Detected puts. Remove all debugging statements.
• ⚠️ - 1594 - Detected puts. Remove all debugging statements.
• ⚠️ - 1601 - Detected puts. Remove all debugging statements.

[miq-bot]

Checked commit kbrock@b7f07cc with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.62.0, and yamllint
4 files checked, 2 offenses detected

app/models/condition.rb

• ⚠️ - Line 88, Col 26 - Lint/UnusedMethodArgument - Unused method argument - rec. If it's necessary, use _ or _rec as an argument name to indicate that it won't be used. If it's unnecessary, remove it.

lib/miq_expression.rb

• ❗ - Line 193, Col 40 - Naming/MethodParameterName - Method parameter must be at least 3 characters long.