Pass array to references()

We are now passing an array of table names into references.

We were passing a nested hash of relations into references which is not
valid.

This is undefined behavior, and not guaranteed to work in the future.
Currently, tails treats this as "just use references for all".

We are introducing includes_to_references.
It  converts an includes compatible (hash of references)
to a references compatible (array of table names)

app/models/application_record.rb

@@ -13,6 +13,7 @@ class ApplicationRecord < ActiveRecord::Base
   include ToModelHash
 
   extend ArTableLock
+  extend ArReferences
 
   # FIXME: UI code - decorator support
   if defined?(ManageIQ::Decorators::Engine)

app/models/miq_report/generator.rb

@@ -258,7 +258,7 @@ def generate_daily_metric_rollup_results(options = {})
                             .where(options[:where_clause])
                             .where(:timestamp => time_range)
                             .includes(db_includes)
-                            .references(db_includes)
+                            .references(db_klass.includes_to_references(db_includes))
                             .includes(exp_includes || [])
                             .limit(options[:limit])
     results = Rbac.filtered(results, :class        => db,

lib/acts_as_ar_model.rb

@@ -21,6 +21,10 @@ def self.base_class
     superclass == ActsAsArModel ? self : superclass.base_class
   end
 
+  def self.includes_to_references(_inc)
+    []
+  end
+
   class << self; alias_method :base_model, :base_class; end
 
   #

lib/extensions/ar_references.rb

@@ -0,0 +1,21 @@
+module ArReferences
+  # Given a nested hash of associations (used by includes)
+  #   convert into an array of table names (used by references)
+  # If given an array of table names, will output the same array
+  def includes_to_references(inc)
+    return [] unless inc
+
+    inc = Array(inc) unless inc.kind_of?(Hash)
+    inc.flat_map do |n, v|
+      if (ref = reflect_on_association(n.to_sym)) && !ref.polymorphic?
+        n_table = ref.table_name
+        v_tables = v ? ref.klass.try(:includes_to_references, v) : []
+        [n_table] + v_tables
+      elsif reflection_with_virtual(n.to_sym) # ignore polymorphic and virtual attribute
+        []
+      else # it is probably a table name - keep it
+        n
+      end
+    end
+  end
+end

lib/rbac/filterer.rb

@@ -173,7 +173,7 @@ def self.accessible_tenant_ids_strategy(klass)
     # @option options :conditions    [Hash|String|Array<String>]
     # @option options :where_clause  []
     # @option options :sub_filter
-    # @option options :include_for_find [Array<Symbol>]
+    # @option options :include_for_find [Array<Symbol>, Hash{Symbol => Symbol,Hash,Array}] models included but not in query
     # @option options :filter       [MiqExpression] (optional)
 
     # @option options :user         [User]     (default: current_user)
@@ -413,7 +413,7 @@ def include_references(scope, klass, include_for_find, exp_includes, skip)
 
       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)
+        scope = scope.references(klass.includes_to_references(include_for_find)).references(klass.includes_to_references(exp_includes))
       end
       scope
     end

spec/lib/extensions/ar_references_spec.rb

@@ -0,0 +1,23 @@
+describe "ar_references" do
+  describe ".includes_to_references" do
+    it "supports none" do
+      expect(Vm.includes_to_references(nil)).to eq([])
+      expect(Vm.includes_to_references([])).to eq([])
+    end
+
+    it "supports arrays" do
+      expect(Vm.includes_to_references(%w[host operating_system])).to eq(%w[hosts operating_systems])
+      expect(Vm.includes_to_references(%i[host])).to eq(%w[hosts])
+    end
+
+    it "supports hashes" do
+      expect(Hardware.includes_to_references(:vm => {})).to eq(%w[vms])
+      expect(Hardware.includes_to_references(:vm => :ext_management_system)).to eq(%w[vms ext_management_systems])
+      expect(Hardware.includes_to_references(:vm => {:ext_management_system => {}})).to eq(%w[vms ext_management_systems])
+    end
+
+    it "supports table array" do
+      expect(Vm.includes_to_references(%w[hosts operating_systems])).to eq(%w[hosts operating_systems])
+    end
+  end
+end

spec/lib/rbac/filterer_spec.rb

@@ -621,11 +621,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
       end
 
       it "does not add references without includes" do
-        # empty string here is basically passing `.references(nil)`, and the
-        # extra empty hash here is from the MiqExpression (which will result in
-        # the same), both of which will no-op to when determining if there are
-        # joins in ActiveRecord, and will not create a JoinDependency query
-        expect(results.references_values).to match_array ["", "{}"]
+        expect(results.references_values).to eq []
       end
 
       context "with :include_for_find" do
@@ -638,7 +634,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
         end
 
         it "adds references" do
-          expect(results.references_values).to match_array ["{:evm_owner=>{}}", "{}"]
+          expect(results.references_values).to match_array %w[users]
         end
       end
     end
@@ -655,11 +651,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
       end
 
       it "does not add references with no includes" do
-        # The single empty string is the result of a nil from both the lack of
-        # a MiqExpression filter and the user filter, which is deduped in
-        # ActiveRecord's internals and results in a `.references(nil)`
-        # effectively
-        expect(results.references_values).to match_array [""]
+        expect(results.references_values).to eq []
       end
 
       context "with :include_for_find" do
@@ -671,7 +663,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
         end
 
         it "adds references" do
-          expect(results.references_values).to match_array ["", "{:evm_owner=>{}}"]
+          expect(results.references_values).to match_array %w[users]
         end
       end
     end
@@ -2461,21 +2453,21 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
       method_args      = [scope, klass, include_for_find, nil, skip]
       resulting_scope  = subject.send(:include_references, *method_args)
 
-      expect(resulting_scope.references_values).to eq(["{:miq_server=>{}}", ""])
+      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]
       resulting_scope  = subject.send(:include_references, *method_args)
 
-      expect(resulting_scope.references_values).to eq(["", "{:host=>{}}"])
+      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]
       resulting_scope  = subject.send(:include_references, *method_args)
 
-      expect(resulting_scope.references_values).to eq(["{:miq_server=>{}}", "{:host=>{}}"])
+      expect(resulting_scope.references_values).to eq(%w[miq_servers hosts])
     end
 
     context "if the include is polymorphic" do
@@ -2516,7 +2508,7 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
 
         it "adds .references to the scope" do
           allow(subject).to receive(:warn)
-          expect(resulting_scope.references_values).to eq(["{:miq_server=>{}}", "{:host=>{}}"])
+          expect(resulting_scope.references_values).to eq(%w[miq_servers hosts])
         end
 
         it "warns that there was an issue in test mode" do