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
"skippable" 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 transaction SAVEPOINT is necessary so we don't pollute
the transaction in the process.  More info on that here:

https://stackoverflow.com/a/31146267/3574689

lib/rbac/filterer.rb

@@ -299,7 +299,35 @@ def skip_references?(options, attrs)
     end
 
     def include_references(scope, klass, include_for_find, exp_includes, skip)
-      return scope if skip
+      if skip
+        # We want to no-op here, since the rescue means we should just execute
+        # the rest of the method as usual and skip the `return`.
+        begin
+          # If we are in a transaction, we don't want to polute that
+          # transaction with a failed EXPLAIN.  We use the savepoint to prevent
+          # that from being an issue (happens in tests with transactional
+          # fixtures)
+          #
+          # See https://stackoverflow.com/a/31146267/3574689
+          if ActiveRecord::Base.connection.transaction_open?
+            ActiveRecord::Base.connection.execute("SAVEPOINT rbac_pre_explain_check")
+          end
+          ActiveRecord::Base.connection.explain(scope.to_sql)
+          return scope
+        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
+          # See above
+          if ActiveRecord::Base.connection.transaction_open?
+            ActiveRecord::Base.connection.execute("ROLLBACK TO SAVEPOINT rbac_pre_explain_check")
+          end
+        end
+      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)

spec/lib/rbac/filterer_spec.rb

@@ -305,6 +305,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
 
@@ -1913,6 +1924,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 + 14}: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 + 14}: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