From 0edb04166b996b2af52d54c627c8daddeef6dfb1 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Wed, 21 Aug 2019 15:40:33 -0400 Subject: [PATCH 1/4] distinguish report include and include_for_find includes are for columns in the query includes_for_find are for the models used by the report views Reports merge the two concepts. But only the miq expression (where) or includes (select) need to be in the references. While includes_for_find only needs to preload the models for use in ruby. This allows each to be accessed in a report. --- app/models/miq_report/generator.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/miq_report/generator.rb b/app/models/miq_report/generator.rb index 372818d939a..a0be1f5e892 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 From fbe1d2010c070c386e7cbb90428d8fac41ef5ad8 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Wed, 21 Aug 2019 16:54:56 -0400 Subject: [PATCH 2/4] Pass in references to rbac includes() are used in ruby references() are used by sql Tables used in the select or where clause need to be joined using references, joins, or outer_joins. Up until now, we've treated them as the same. But it is taxing to have so many tables in the query. So they are split apart. :include is used in the select column, so it is referenced miq_expression is used in the where clause, so it is referenced include_for_find is only used to preload models. --- If references are not passed in, it defaults to existing behavior. Meaning, all includes are in references. But if references are passed in, then only those tables are referenced() --- app/models/miq_report/generator.rb | 8 ++++---- app/models/miq_report/search.rb | 1 + app/models/vim_performance_analysis.rb | 1 + lib/rbac/filterer.rb | 5 ++++- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/app/models/miq_report/generator.rb b/app/models/miq_report/generator.rb index a0be1f5e892..7400356064d 100644 --- a/app/models/miq_report/generator.rb +++ b/app/models/miq_report/generator.rb @@ -255,14 +255,12 @@ def generate_daily_metric_rollup_results(options = {}) 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(options[:where_clause]) .where(:timestamp => time_range) - .includes(db_includes) - .references(db_klass.includes_to_references(db_includes)) + .includes(get_include_for_find) + .references(db_klass.includes_to_references(get_include)) .includes(exp_includes || []) .limit(options[:limit]) results = Rbac.filtered(results, :class => db, @@ -284,6 +282,7 @@ def generate_interval_metric_results(options = {}) .where(options[:where_clause]) .where(exp_sql) .includes(get_include_for_find) + .references(db_klass.includes_to_references(get_include)) .includes(exp_includes || []) .limit(options[:limit]) @@ -316,6 +315,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..0c78f88edf1 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) @@ -268,7 +270,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, skip_references) scope = scope.limit(limit).offset(offset) if attrs[:apply_limit_in_sql] if inline_view?(options, scope) @@ -380,6 +382,7 @@ 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? && + !options.key?(:references) && (!attrs[:apply_limit_in_sql] && (exp_sql.nil? || exp_includes.nil?))) end From 8cc2277b10605693dcd5d66701f475e511a6c33a Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 12 Sep 2019 14:11:37 -0400 Subject: [PATCH 3/4] drop rbac skip_references :references are now passed for hopefully all rbac that also passes in include_for_find. Since the references are explicitly passed in, we don't want the code second guessing these decisions. Also, since not all tables are passed to references, there is no longer the need to come up with complicated logic to reduce the references passed into ActiveRecord --- lib/rbac/filterer.rb | 69 ++---------------------- spec/lib/rbac/filterer_spec.rb | 97 ++++------------------------------ 2 files changed, 12 insertions(+), 154 deletions(-) diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb index 0c78f88edf1..3a5f27dd5dd 100644 --- a/lib/rbac/filterer.rb +++ b/lib/rbac/filterer.rb @@ -260,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 @@ -270,7 +269,7 @@ def search(options = {}) .includes(include_for_find).includes(exp_includes) .order(order) - scope = include_references(scope, klass, references, 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) @@ -362,63 +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? && - !options.key?(:references) && - (!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] @@ -435,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 From 296b73fa990ebee36e795582f757e1ccb4131797 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Fri, 9 Aug 2019 12:14:26 -0400 Subject: [PATCH 4/4] Don't run MiqExpression twice on reports 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. --- app/models/miq_report/generator.rb | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/app/models/miq_report/generator.rb b/app/models/miq_report/generator.rb index 7400356064d..fc1a98a31c1 100644 --- a/app/models/miq_report/generator.rb +++ b/app/models/miq_report/generator.rb @@ -250,18 +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) 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(get_include_for_find) .references(db_klass.includes_to_references(get_include)) - .includes(exp_includes || []) .limit(options[:limit]) results = Rbac.filtered(results, :class => db, :filter => conditions, @@ -274,18 +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) .references(db_klass.includes_to_references(get_include)) - .includes(exp_includes || []) .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],