Fixes to Rbac::Filterer#skip_references

lib/rbac/filterer.rb

@@ -248,7 +248,7 @@ 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?(options, attrs)
+      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
@@ -299,11 +299,19 @@ def is_sti?(klass)
     # 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?(options, attrs)
-      options[:extra_cols].blank? && !attrs[:apply_limit_in_sql]
+    def skip_references?(scope, options, attrs, exp_sql, exp_includes)
+      return false if scope.singleton_class.included_modules.include?(ActiveRecord::NullRelation)
+      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)

spec/lib/rbac/filterer_spec.rb

@@ -332,7 +332,9 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
     context "with non-sql filter" do
       subject { described_class.new }
 
-      let(:expression)        { MiqExpression.new("=" => {"field" => "Vm-vendor_display", "value" => "VMware"}) }
+      let(:nonsql_expression) { {"=" => {"field" => "Vm-vendor_display", "value" => "VMware"}} }
+      let(:raw_expression)    { nonsql_expression }
+      let(:expression)        { MiqExpression.new(raw_expression) }
       let(:search_attributes) { { :class => "Vm", :filter => expression } }
       let(:results)           { subject.search(search_attributes).first }
 
@@ -348,6 +350,23 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
         results
       end
 
+      context "with a partial non-sql filter" do
+        let(:sql_expression) { { "IS EMPTY" => { "field" => "Vm.host-name" } } }
+        let(:raw_expression) { { "AND" => [nonsql_expression, sql_expression] } }
+
+        it "finds the Vms" do
+          expect(results.to_a).to match_array [owned_vm, other_vm]
+          expect(results.count).to eq 2
+        end
+
+        it "includes references" do
+          expect(subject).to receive(:include_references).with(anything, ::Vm, nil, {:host => {}}, false)
+                                                         .and_call_original
+          expect(subject).to receive(:warn).never
+          results
+        end
+      end
+
       context "with :include_for_find" do
         let(:include_search) { search_attributes.merge(:include_for_find => {:evm_owner => {}}) }
         let(:results)        { subject.search(include_search).first }
@@ -371,6 +390,18 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
             expect(subject).to receive(:warn).exactly(4).times
             results
           end
+
+          context "and targets is a NullRelation scope" do
+            let(:targets)     { Vm.none }
+            let(:null_search) { search_with_where.merge(:targets => targets) }
+            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
+              results
+            end
+          end
         end
       end
     end