Skip to content

Commit

Permalink
Don't run MiqExpression twice on reports
Browse files Browse the repository at this point in the history
We are already passing conditions into Rbac.filtered, so the
MiqExpression will be applied in rbac. There is no reason to
also apply it to the where clause here.

The reason for making this change now, is we are changing the
way the references from an MiqExpression are added.
(instead of using a hash, we're converting into an array)
I prefer to make this code introduction in as few places as
possible.
  • Loading branch information
kbrock committed Sep 20, 2019
1 parent e8fed1d commit 91bcc8a
Showing 1 changed file with 2 additions and 10 deletions.
12 changes: 2 additions & 10 deletions app/models/miq_report/generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -246,20 +246,16 @@ def generate_performance_results(options = {})
def generate_daily_metric_rollup_results(options = {})
unless conditions.nil?
conditions.preprocess_options = {:vim_performance_daily_adhoc => (time_profile && time_profile.rollup_daily_metrics)}
exp_sql, exp_includes = conditions.to_sql
# only_cols += conditions.columns_for_sql # Add cols references in expression to ensure they are present for evaluation
end

time_range = Metric::Helper.time_range_from_offset(interval, db_options[:start_offset], db_options[:end_offset], tz)
# TODO: add .select(only_cols)
db_includes = get_include_for_find
results = Metric::Helper.find_for_interval_name('daily', time_profile || tz, db_klass)
.where(where_clause).where(exp_sql)
.where(where_clause)
.where(options[:where_clause])
.where(:timestamp => time_range)
.includes(db_includes)
.references(db_includes)
.includes(exp_includes || [])
.limit(options[:limit])
results = Rbac.filtered(results, :class => db,
:filter => conditions,
Expand All @@ -272,17 +268,13 @@ def generate_daily_metric_rollup_results(options = {})
def generate_interval_metric_results(options = {})
time_range = Metric::Helper.time_range_from_offset(interval, db_options[:start_offset], db_options[:end_offset])

# Only build where clause from expression for hourly report. It will not work properly for daily because many values are rolled up from hourly.
exp_sql, exp_includes = conditions.to_sql(tz) unless conditions.nil? || db_klass.respond_to?(:instances_are_derived?)

results = db_klass.with_interval_and_time_range(interval, time_range)
.where(where_clause)
.where(options[:where_clause])
.where(exp_sql)
.includes(get_include_for_find)
.includes(exp_includes || [])
.limit(options[:limit])

# Only build where clause from expression for hourly report. It will not work properly for daily because many values are rolled up from hourly.
results = Rbac.filtered(results, :class => db,
:filter => conditions,
:userid => options[:userid],
Expand Down

0 comments on commit 91bcc8a

Please sign in to comment.