From fcc46332d7d8c329bd43d0ce258c2a3a76a81f48 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Fri, 23 Mar 2018 17:14:42 -0500 Subject: [PATCH] Skip references if sql EXPLAIN passes Currently, there are a lot of factors that determine whether or not a query can be executed without the `.references` call. ActiveRecord itself is even smart enough to figured it out and add it for you if you use the hash `.where` syntax: ContainerImage.includes(:containers => {}) .where(:containers => {:name => "foo"}) .to_sql So as a fail safe, if `skip` is passed to method `Rbac::Filterer#include_references`, then we can do an much cheaper explain to figure out if it is a valid query before deciding if skipping is a bad idea. This means 1 extra query to every RBac call search that is currently "skip-able" by our criteria, but they should hopefully be quick to execute and be a fail safe for what we miss. Note, doing a `scope.explain` will both execute the regular query and the explain, and since we have a lot of heavy Rbac calls, this would not be ideal. We work around this by calling a private API with `.to_sql` to only execute the EXPLAIN query, which still returns an error of `ActiveRecord::StatementInvalid` when it is malformed. In that same vain, a check if we are in a transaction is also required, and setting up a sub transaction SAVEPOINT (via `transaction(:requires_new => true)`)is necessary so we don't pollute an existing transaction in the process. More info on that here: https://stackoverflow.com/a/31146267/3574689 --- lib/rbac/filterer.rb | 28 ++++++++++++- spec/lib/rbac/filterer_spec.rb | 76 ++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb index 1d5d89dab50..2b3752354ae 100644 --- a/lib/rbac/filterer.rb +++ b/lib/rbac/filterer.rb @@ -307,7 +307,33 @@ def skip_references?(options, attrs) end def include_references(scope, klass, include_for_find, exp_includes, skip) - return scope if 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(include_for_find).references(exp_includes) diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb index cad0a703231..d4f73040f0d 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -361,6 +361,17 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil, true).and_call_original results end + + context "with a references based where_clause" do + let(:search_with_where) { include_search.merge(:where_clause => ['"users"."id" = ?', owner_user.id]) } + 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 + results + end + end end end @@ -1969,6 +1980,71 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) 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(["{:miq_server=>{}}", "{:host=>{}}"]) + 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