Remove taggings and polymorphic from references

kbrock · kbrock · commit d7ac8d9cab3d · 2025-10-06T22:38:28.000-04:00
Now that we've moved form references.includes to preload and eager_load,
They are respecting the difference between those 2.

So we can pass polymorphic to preload and it won't blow up.
We can pass taggings to preload and it won't join and bring back too much data.

This commit updates our ar_references methods to filter out polymorphic and taggings from an includes hash.
Also removed virtual attributes since we can't access ruby virtual attributes in sql
(and sql virtual attributes are sub-selects so they shouldn't be in an includes anyway)

We can drop the PolymorphicError issues (handled in rbac filterer and report generator)

We could drop `includes_to_references` support but it is hanging around for legacy purposes and can be dropped soon.
diff --git a/app/models/miq_report/generator.rb b/app/models/miq_report/generator.rb
@@ -88,27 +88,12 @@ def get_include
     include_as_hash(include.presence || invent_report_includes)
   end
 
-  def polymorphic_includes
-    @polymorphic_includes ||= begin
-      top_level_rels = Array(get_include.try(:keys)) + Array(get_include_for_find.try(:keys))
-
-      top_level_rels.uniq.each_with_object([]) do |assoc, polymorphic_rels|
-        reflection = db_class.reflect_on_association(assoc)
-        polymorphic_rels << assoc if reflection && reflection.polymorphic?
-      end
-    end
-  end
-
   def get_ar_includes
-    polymorphic_includes.each_with_object(get_include_for_find.dup) do |key, includes|
-      includes.delete(key)
-    end
+    include_for_find
   end
 
   def get_ar_references
-    polymorphic_includes.each_with_object(get_include.dup) do |key, includes|
-      includes.delete(key)
-    end
+    db_class.prune_references(get_include)
   end
 
   def invent_includes
diff --git a/lib/extensions/ar_references.rb b/lib/extensions/ar_references.rb
@@ -1,20 +1,24 @@
 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
+  # Tags bring back too many rows, since we can not limit on the particular tag
+  # So dont require them (meaning we don't join to the taggings table)
+  SKIP_TABLES = [:tags, :taggings].freeze
 
-    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
+  # Given a nested hash of associations (used by includes, preload, and eager_load)
+  #   prune out polymorphic, and references to tags
+  def prune_references(inc)
+    return {} unless inc
+
+    inc = Array(inc) unless inc.kind_of?(Hash) || inc.kind_of?(Array)
+    inc.each_with_object({}) do |(n, v), ret|
+      n = n.to_sym
+      if (ref = reflect_on_association(n))
+        if !ref.polymorphic? && !SKIP_TABLES.include?(n)
+          ret[n] = (v.present? && ref.klass.try(:prune_references, v)) || {}
+        end
+      # ignore virtual collections and virtual attribute
+      elsif !reflection_with_virtual(n) && !virtual_attribute?(n)
+        # Think this is an error. letting it slide (assuming it will throw an error elsewhere)
+        ret[n] = {}
       end
     end
   end
diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb
@@ -325,12 +325,15 @@ def search(options = {})
       if inline_view?(options, scope)
         inner_scope = scope.except(:select, :includes, :references, :eager_load, :preload)
         # similar to include_references but using joins
-        # TODO: optimization: Can we remove these from the outer query?
+        # TODO: optimization: Can we remove these references from the outer query?
+        #       To do this, we may need to introduce join() to complete the triad: include/reference/join
         inner_scope = add_joins(klass, inner_scope, references)
+        # TODO: do we want to klass.prune_references(exp_includes)? (see also include_references)
         inner_scope = add_joins(klass, inner_scope, exp_includes)
         if inner_scope.order_values.present?
           inner_scope = apply_select(klass, inner_scope, select_from_order_columns(inner_scope.order_values))
         end
+        # TODO: Convert from(inner_scope.to_sql) to from(inner_scope)
         scope = scope.from(Arel.sql("(#{inner_scope.to_sql})").as(scope.table_name))
                      .except(:offset, :limit, :where)
 
@@ -428,6 +431,7 @@ def select_from_order_columns(columns)
 
     def include_references(scope, klass, includes, references, exp_includes)
       if scope.respond_to?(:eager_load)
+        # TODO: do we want to klass.prune_references(exp_includes)? (see same comment for inline_view? section)
         scope.eager_load(references || {}).eager_load(exp_includes || {}).preload(includes)
       else
         # This is the AAAR / QueryRelation branch
@@ -440,13 +444,16 @@ def include_references(scope, klass, includes, references, exp_includes)
     def add_joins(klass, scope, includes)
       return scope unless includes
 
+      # NOTE: We should be pre-pruning polymorphic out of here, since they shouldn't be in references.
+      # TODO: do we want to be less lenient? i.e.: raise PolymorphicError if reflection&.polymorphic?
       includes = Array(includes) unless includes.kind_of?(Enumerable)
       includes.each do |association, value|
         reflection = klass.reflect_on_association(association)
         if reflection && !reflection.polymorphic?
           scope = value ? scope.left_outer_joins(association => value) : scope.left_outer_joins(association)
         end
       end
+      # This adds a DISTINCT to safeguard "JOIN has_many" screwing up the LIMIT.
       scope = scope.distinct if !scope.distinct_value && is_collection?(klass, includes)
       scope
     end
diff --git a/spec/lib/extensions/ar_references_spec.rb b/spec/lib/extensions/ar_references_spec.rb
@@ -1,23 +1,40 @@
 RSpec.describe "ar_references" do
-  describe ".includes_to_references" do
+  describe ".prune_references" do
     it "supports none" do
-      expect(Vm.includes_to_references(nil)).to eq([])
-      expect(Vm.includes_to_references([])).to eq([])
+      expect(Vm.prune_references(nil)).to eq({})
+      expect(Vm.prune_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])
+      expect(Vm.prune_references(%w[host operating_system])).to eq(:host => {}, :operating_system => {})
+      expect(Vm.prune_references(%i[host])).to eq(:host => {})
     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])
+      expect(Hardware.prune_references(:vm => {})).to eq(:vm => {})
+      expect(Hardware.prune_references(:vm => :ext_management_system)).to eq(:vm => {:ext_management_system => {}})
+      expect(Hardware.prune_references(:vm => {:ext_management_system => {}})).to eq(:vm => {:ext_management_system => {}})
     end
 
-    it "supports table array" do
-      expect(Vm.includes_to_references(%w[hosts operating_systems])).to eq(%w[hosts operating_systems])
+    it "skips tags" do
+      expect(Vm.prune_references(:taggings => {})).to eq({})
+    end
+
+    it "skips virtual has many" do
+      expect(Vm.prune_references(:processes => {})).to eq({})
+    end
+
+    it "skips virtual attributes" do
+      expect(Vm.prune_references(:archived => {}, :platform => {})).to eq({})
+    end
+
+    it "skips polymorphic references" do
+      expect(MetricRollup.prune_references(:resource=>{})).to eq({})
+      expect(MiqGroup.prune_references(:tenant => :source)).to eq({:tenant => {}})
+    end
+
+    it "skips uses with a polymorphic reference" do
+      expect(MetricRollup.prune_references(:v_derived_storage_used => {})).to eq({})
     end
   end
 end
diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb
@@ -1169,51 +1169,42 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
         }
       end
 
-      # NOTE:  Think long and hard before you consider removing this test.
-      # Many-a-hours wasted here determining this bug that resulted in
-      # re-adding this test again.
+      # We've had an on and off again success with polymorphics (first documented in 2015 by Greg Tanzillo)
+      # references().includes() with polymorphic does not work
+      # now that we've converted to preload and eager_load, this is no longer an issue
       #
-      # 2nd NOTE:  I did think (and wrote the above comment as well), however,
-      # now it seems we DO NOT SUPPORT polymorphic code, since we removed it
-      # here:
-      #
-      #   https://github.com/ManageIQ/manageiq/commit/8cc2277b
-      #
-      # That said, we should make sure this is erroring and no other callers
-      # are trying to do this (example:  MiqReport), so make sure this raises
-      # an error to inform the caller that fixing needs to happen.
+      # Since we've been so mix and match, please leave this test in here
       #
       it "raises an error when a polymorphic reflection is included and referenced" do
-        # NOTE: Fails if :references is passed with a value, or with no key,
-        # which it uses :include_for_find as the default.
+        # This is not valid. You should not be able to pass a polymorphic to references
         expect do
           described_class.search :class            => "MetricRollup",
                                  :include_for_find => @include,
                                  :references       => @include
         end.to raise_error(Rbac::PolymorphicError)
 
+        # defaults to includes when no :references passed in
         expect do
           described_class.search :class            => "MetricRollup",
                                  :include_for_find => @include
         end.to raise_error(Rbac::PolymorphicError)
       end
 
       it "does not raise an error when a polymorphic reflection is only included" do
-        # NOTE:  if references is passed in, but is blank, then doesn't override and is fine
         expect do
           described_class.search :class            => "MetricRollup",
                                  :include_for_find => @include,
-                                 :references       => {}
+                                 :references       => MetricRollup.prune_references(@include)
         end.not_to raise_error
 
+        # NOTE:  if a nil or blank references is passed in, then it doesn't default to the include_for_find and is fine
         expect do
           described_class.search :class            => "MetricRollup",
                                  :include_for_find => @include,
-                                 :references       => nil
+                                 :references       => {}
         end.not_to raise_error
 
-        # ensure going forward that polymorphics can be passed as references
-        # this has been broken and fixed many times. Please don't remove
+        # Any references with a polymorphic in the includes (and a supported references value should work. (it wasn't working before)
         expect do
           described_class.search :class            => "MetricRollup",
                                  :include_for_find => @include,
@@ -2627,6 +2618,7 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
 
         recs, attrs = Rbac.search(:targets          => MetricRollup,
                                   :include_for_find => {:resource => {}, :parent_host => {}},
+                                  :references       => MetricRollup.prune_references({:resource => {}, :parent_host => {}}),
                                   :extra_cols       => [:v_derived_storage_used],
                                   :use_sql_view     => true,
                                   :limit            => 20,
@@ -2769,19 +2761,6 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
         expect(resulting_scope.eager_load_values).to match_array([:miq_server, :host])
       end
     end
-
-    context "if the include is polymorphic" do
-      let(:klass)            { MetricRollup }
-      let(:include_for_find) { {:resource => {}} }
-
-      it "does not add .references to the scope" do
-        method_args      = [scope, klass, include_for_find, include_for_find, nil]
-        resulting_scope  = subject.send(:include_references, *method_args)
-
-        expect(resulting_scope.preload_values).to eq([include_for_find])
-        expect(resulting_scope.eager_load_values).to eq([include_for_find])
-      end
-    end
   end
 
   describe "using right RBAC rules to VM and Templates" do
