Skip to content

Commit

Permalink
Merge pull request #19127 from kbrock/rbac_no_references
Browse files Browse the repository at this point in the history 
Improve references
  • Loading branch information
@jrafanie
jrafanie committed Oct 10, 2019
2 parents cd215c7 + 296b73f commit e24682e
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 167 deletions.
25 changes: 11 additions & 14 deletions app/models/miq_report/generator.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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],
Expand Down Expand Up @@ -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
)
Expand Down
1 change: 1 addition & 0 deletions app/models/miq_report/search.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions app/models/vim_performance_analysis.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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],
}
Expand Down
70 changes: 5 additions & 65 deletions lib/rbac/filterer.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Symbol>, Hash{Symbol => Symbol,Hash,Array}] models included but not in query
# @option options :references [Array<Symbol>], 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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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]
Expand All @@ -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
Expand Down
97 changes: 9 additions & 88 deletions spec/lib/rbac/filterer_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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

Expand All @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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])
Expand All @@ -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
Expand Down

0 comments on commit e24682e

Please sign in to comment.