diff --git a/app/models/miq_report/generator.rb b/app/models/miq_report/generator.rb index bfa99a28666..643d09ee49b 100644 --- a/app/models/miq_report/generator.rb +++ b/app/models/miq_report/generator.rb @@ -81,7 +81,11 @@ def table2class(table) end def get_include_for_find - include_as_hash(include.presence || invent_report_includes).deep_merge(include_for_find || {}).presence + get_include.deep_merge(include_for_find || {}).presence + end + + def get_include + include_as_hash(include.presence || invent_report_includes) end def invent_includes @@ -246,20 +250,15 @@ 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_klass.includes_to_references(db_includes)) - .includes(exp_includes || []) + .includes(get_include_for_find) + .references(db_klass.includes_to_references(get_include)) .limit(options[:limit]) results = Rbac.filtered(results, :class => db, :filter => conditions, @@ -272,17 +271,14 @@ 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 || []) + .references(db_klass.includes_to_references(get_include)) .limit(options[:limit]) + # Rbac will only add miq_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], @@ -312,6 +308,7 @@ def generate_basic_results(options = {}) :targets => targets, :filter => conditions, :include_for_find => get_include_for_find, + :references => get_include, :where_clause => where_clause, :skip_counts => true ) diff --git a/app/models/miq_report/search.rb b/app/models/miq_report/search.rb index dda4f7fa2b0..0f5dd2f038e 100644 --- a/app/models/miq_report/search.rb +++ b/app/models/miq_report/search.rb @@ -93,6 +93,7 @@ def paged_view_search(options = {}) search_options = options.merge(:class => db, :conditions => conditions, :include_for_find => includes, + :references => get_include, :skip_references => skip_references ) search_options.merge!(:limit => limit, :offset => offset, :order => order) if order diff --git a/app/models/vim_performance_analysis.rb b/app/models/vim_performance_analysis.rb index 54a76fe35fa..8680ab23b84 100644 --- a/app/models/vim_performance_analysis.rb +++ b/app/models/vim_performance_analysis.rb @@ -66,6 +66,7 @@ def get_targets search_options = { :class => topts[:compute_type].to_s, :include_for_find => includes, + :references => includes, :userid => @options[:userid], :miq_group_id => @options[:miq_group_id], } diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb index 47ce09aa878..3a5f27dd5dd 100644 --- a/lib/rbac/filterer.rb +++ b/lib/rbac/filterer.rb @@ -174,6 +174,7 @@ def self.accessible_tenant_ids_strategy(klass) # @option options :where_clause [] # @option options :sub_filter # @option options :include_for_find [Array, Hash{Symbol => Symbol,Hash,Array}] models included but not in query + # @option options :references [Array], models used by select and where. If not passed, uses include_for_find instead # @option options :filter [MiqExpression] (optional) # @option options :user [User] (default: current_user) @@ -209,6 +210,7 @@ def search(options = {}) where_clause = options[:where_clause] sub_filter = options[:sub_filter] include_for_find = options[:include_for_find] + references = options.fetch(:references) { include_for_find } search_filter = options[:filter] limit = options[:limit] || targets.try(:limit_value) @@ -258,7 +260,6 @@ def search(options = {}) exp_sql, exp_includes, exp_attrs = search_filter.to_sql(tz) if search_filter && !klass.try(:instances_are_derived?) attrs[:apply_limit_in_sql] = (exp_attrs.nil? || exp_attrs[:supported_by_sql]) && user_filters["belongsto"].blank? - skip_references = skip_references?(scope, options, attrs, exp_sql, exp_includes) # for belongs_to filters, scope_targets uses scope to make queries. want to remove limits for those. # if you note, the limits are put back into scope a few lines down from here @@ -268,7 +269,7 @@ def search(options = {}) .includes(include_for_find).includes(exp_includes) .order(order) - scope = include_references(scope, klass, include_for_find, exp_includes, skip_references) + scope = include_references(scope, klass, references, exp_includes) scope = scope.limit(limit).offset(offset) if attrs[:apply_limit_in_sql] if inline_view?(options, scope) @@ -360,62 +361,8 @@ def select_from_order_columns(columns) end end - # This is a very primitive way of determining whether we want to skip - # adding references to the query. - # - # For now, basically it checks if the caller has not provided :extra_cols, - # or if the MiqExpression can't apply the limit in SQL. If both of those - # are true, then we don't add `.references` to the scope. - # - # Also, if for whatever reason we are passed a - # `ActiveRecord::NullRelation`, make sure that we don't skip references. - # This will cause the EXPLAIN to blow up since `.to_sql` gets changed to - # always return `""`... even though at the end of the day, we will always - # get back zero records from the query. - # - # If still invalid, there is an EXPLAIN check in #include_references that - # will make sure the query is valid and if not, will include the references - # as done previously. - def skip_references?(scope, options, attrs, exp_sql, exp_includes) - return false if scope.singleton_class.included_modules.include?(ActiveRecord::NullRelation) - options[:skip_references] || - (options[:extra_cols].blank? && - (!attrs[:apply_limit_in_sql] && (exp_sql.nil? || exp_includes.nil?))) - end - - def include_references(scope, klass, include_for_find, exp_includes, skip) - if skip - # If we are in a transaction, we don't want to polute that - # transaction with a failed EXPLAIN. We use a SQL SAVEPOINT (which is - # created via `transaction(:requires_new => true)`) to prevent that - # from being an issue (happens in tests with transactional fixtures) - # - # See https://stackoverflow.com/a/31146267/3574689 - valid_skip = MiqDatabase.transaction(:requires_new => true) do - begin - ActiveRecord::Base.connection.explain(scope.to_sql) - rescue ActiveRecord::StatementInvalid => e - unless Rails.env.production? - warn "There was an issue with the Rbac filter without references!" - warn "Consider trying to fix this edge case in Rbac::Filterer! Error Below:" - warn e.message - warn e.backtrace - end - # returns nil - raise ActiveRecord::Rollback - end - end - # If the result of the transaction is non-nil, then the block was - # successful and didn't trigger the ActiveRecord::Rollback, so we can - # return the scope as is. - return scope if valid_skip - end - - ref_includes = Hash(include_for_find).merge(Hash(exp_includes)) - unless polymorphic_include?(klass, ref_includes) - scope = scope.references(klass.includes_to_references(include_for_find)).references(klass.includes_to_references(exp_includes)) - end - scope + def include_references(scope, klass, include_for_find, exp_includes) + scope.references(klass.includes_to_references(include_for_find)).references(klass.includes_to_references(exp_includes)) end # @param includes [Array, Hash] @@ -432,13 +379,6 @@ def add_joins(klass, scope, includes) scope end - def polymorphic_include?(target_klass, includes) - includes.keys.any? do |incld| - reflection = target_klass.reflect_on_association(incld) - reflection && reflection.polymorphic? - end - end - def filtered(objects, options = {}) Rbac.search(options.reverse_merge(:targets => objects, :skip_counts => true)).first end diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb index 9de1db84cca..0485464147f 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -548,7 +548,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt end it "does not add references without includes" do - expect(subject).to receive(:include_references).with(anything, Vm, nil, nil, true).and_call_original + expect(subject).to receive(:include_references).with(anything, Vm, nil, nil).and_call_original results end @@ -562,7 +562,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt end it "includes references" do - expect(subject).to receive(:include_references).with(anything, ::Vm, nil, {:host => {}}, false) + expect(subject).to receive(:include_references).with(anything, ::Vm, nil, :host => {}) .and_call_original expect(subject).to receive(:warn).never results @@ -579,7 +579,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt end it "does not add references since there isn't a SQL filter" do - expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil, true).and_call_original + expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil).and_call_original results end @@ -588,8 +588,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt let(:results) { subject.search(search_with_where).first } it "will try to skip references to begin with" do - expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil, true).and_call_original - expect(subject).to receive(:warn).exactly(4).times + expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil).and_call_original results end @@ -599,8 +598,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt let(:results) { subject.search(null_search).first } it "will not try to skip references" do - expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil, false).and_call_original - expect(subject).to receive(:warn).never + expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil).and_call_original results end end @@ -2443,28 +2441,27 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) describe "#include_references (private)" do subject { described_class.new } - let(:skip) { false } let(:klass) { VmOrTemplate } let(:scope) { klass.all } let(:include_for_find) { { :miq_server => {} } } let(:exp_includes) { { :host => {} } } it "adds include_for_find .references to the scope" do - method_args = [scope, klass, include_for_find, nil, skip] + method_args = [scope, klass, include_for_find, nil] resulting_scope = subject.send(:include_references, *method_args) expect(resulting_scope.references_values).to eq(%w[miq_servers]) end it "adds exp_includes .references to the scope" do - method_args = [scope, klass, nil, exp_includes, skip] + method_args = [scope, klass, nil, exp_includes] resulting_scope = subject.send(:include_references, *method_args) expect(resulting_scope.references_values).to eq(%w[hosts]) end it "adds include_for_find and exp_includes .references to the scope" do - method_args = [scope, klass, include_for_find, exp_includes, skip] + method_args = [scope, klass, include_for_find, exp_includes] resulting_scope = subject.send(:include_references, *method_args) expect(resulting_scope.references_values).to eq(%w[miq_servers hosts]) @@ -2475,88 +2472,12 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) let(:include_for_find) { { :resource => {} } } it "does not add .references to the scope" do - method_args = [scope, klass, include_for_find, nil, skip] + method_args = [scope, klass, include_for_find, nil] resulting_scope = subject.send(:include_references, *method_args) expect(resulting_scope.references_values).to eq([]) end end - - context "when skip is passed as true" do - let(:skip) { true } - - it "does not add .references to the scope" do - method_args = [scope, klass, include_for_find, exp_includes, skip] - resulting_scope = subject.send(:include_references, *method_args) - - expect(resulting_scope.references_values).to eq([]) - end - - context "when the scope is invalid without .references" do - let(:scope) { klass.where("hosts.name = 'foo'") } - let(:method_args) { [scope, klass, include_for_find, exp_includes, skip] } - let(:resulting_scope) { subject.send(:include_references, *method_args) } - - let(:explain_error_match) do - Regexp.new(Regexp.escape(<<~PG_ERR.chomp)) - PG::UndefinedTable: ERROR: missing FROM-clause entry for table "hosts" - LINE 1: EXPLAIN SELECT "vms".* FROM "vms" WHERE (hosts.name = 'foo') - ^ - : EXPLAIN SELECT "vms".* FROM "vms" WHERE (hosts.name = 'foo') - PG_ERR - end - - it "adds .references to the scope" do - allow(subject).to receive(:warn) - expect(resulting_scope.references_values).to eq(%w[miq_servers hosts]) - end - - it "warns that there was an issue in test mode" do - # This next couple of lines is just used to check that some of the - # backtrace that we are dumping into the logs is what we expect will - # for sure be there, and not try to match the entire trace. - # - # Does a bit of line addition to avoid this being too brittle and - # breaking easily, but expect it to break if you update - # Rbac::Filterer#include_references - method_file, method_line = subject.method(:include_references).source_location - explain_stacktrace_includes = [ - "#{method_file}:#{method_line + 10}:in `block in include_references'", - Thread.current.backtrace[1].gsub(/:\d*:/) { |sub| ":#{sub.tr(":", "").to_i + 7}:" } - ] - - expect(subject).to receive(:warn).with("There was an issue with the Rbac filter without references!").ordered - expect(subject).to receive(:warn).with("Consider trying to fix this edge case in Rbac::Filterer! Error Below:").ordered - expect(subject).to receive(:warn).with(explain_error_match).ordered - expect(subject).to receive(:warn).with(array_including(explain_stacktrace_includes)).ordered - resulting_scope - end - - it "warns that there was an issue in development mode" do - expect(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("developement")) - - # See above - method_file, method_line = subject.method(:include_references).source_location - explain_stacktrace_includes = [ - "#{method_file}:#{method_line + 10}:in `block in include_references'", - Thread.current.backtrace[1].gsub(/:\d*:/) { |sub| ":#{sub.tr(":", "").to_i + 7}:" } - ] - - expect(subject).to receive(:warn).with("There was an issue with the Rbac filter without references!").ordered - expect(subject).to receive(:warn).with("Consider trying to fix this edge case in Rbac::Filterer! Error Below:").ordered - expect(subject).to receive(:warn).with(explain_error_match).ordered - expect(subject).to receive(:warn).with(array_including(explain_stacktrace_includes)).ordered - resulting_scope - end - - it "does not warn that there was an issue in production mode" do - expect(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("production")) - - expect(subject).to receive(:warn).never - resulting_scope - end - end - end end describe "using right RBAC rules to VM and Templates" do