Skip to content

Commit

Permalink
drop rbac skip_references
Browse files Browse the repository at this point in the history 
: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
  • Loading branch information
@kbrock
kbrock committed Oct 3, 2019
1 parent fbe1d20 commit 8cc2277
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 154 deletions.
69 changes: 3 additions & 66 deletions lib/rbac/filterer.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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]
Expand All @@ -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
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 8cc2277

Please sign in to comment.