Merge pull request #23601 from kbrock/report_requires3

convert MiqReport from references to eager_load

Author: Fryguy

app/models/cloud_tenant.rb

@@ -187,7 +187,6 @@ def self.post_refresh_ems(ems_id, _)
   end
 
   def self.tenant_joins_clause(scope)
-    scope.includes(:source_tenant, :ext_management_system)
-         .references(:source_tenant, :ext_management_system)
+    scope.left_outer_joins(:source_tenant, :ext_management_system)
   end
 end

app/models/flavor.rb

@@ -43,9 +43,9 @@ def self.class_by_ems(ext_management_system)
     ext_management_system&.class_by_ems(:Flavor)
   end
 
+  # NOTE: tenant (source_tenant) is far away, so it brings back a lot of models. can we use left_joins instead?
   def self.tenant_joins_clause(scope)
-    scope.includes(:cloud_tenants => "source_tenant", :ext_management_system => {})
-         .references(:cloud_tenants, :tenants, :ext_management_system)
+    scope.left_outer_joins(:cloud_tenants => "source_tenant", :ext_management_system => {})
   end
 
   # Create a flavor as a queued task and return the task id. The queue name and

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_include_for_find_rbac
-    polymorphic_includes.each_with_object(get_include_for_find.dup) do |key, includes|
-      includes.delete(key)
-    end
+  def get_ar_includes
+    include_for_find
   end
 
-  def get_include_rbac
-    polymorphic_includes.each_with_object(get_include.dup) do |key, includes|
-      includes.delete(key)
-    end
+  def get_ar_references
+    db_class.prune_references(get_include)
   end
 
   def invent_includes
@@ -118,16 +103,14 @@ def invent_includes
   # would like this format to go away
   # will go away when we drop build_reportable_data
   def invent_report_includes
-    return {} unless col_order
-
-    col_order.each_with_object({}) do |col, ret|
-      next unless col.include?(".")
+    (Array(col_order) + Array(sortby)).each_with_object({}) do |col, ret|
+      next if !col.include?(".") || col.include?("virtual_custom")
 
       *rels, column = col.split(".")
-      if col !~ /managed\./ && col !~ /virtual_custom/
-        (rels.inject(ret) { |h, rel| h[rel] ||= {} }["columns"] ||= []) << column
-      end
-    end
+      dest = rels.inject(ret) { |h, rel| (h["include"] ||= {})[rel] ||= {} }["columns"] ||= []
+      # sortby tends to be a duplicate. So suppress duplicates for all
+      dest << column unless dest.include?(column)
+    end["include"]
   end
 
   def include_as_hash(includes = include, klass = db_class, klass_cols = cols)
@@ -290,8 +273,8 @@ def generate_daily_metric_rollup_results(options = {})
                             .where(where_clause)
                             .where(options[:where_clause])
                             .where(:timestamp => performance_report_time_range)
-                            .includes(get_include_for_find)
-                            .references(db_class.includes_to_references(get_include))
+                            .preload(get_ar_includes)
+                            .eager_load(get_ar_references)
                             .limit(options[:limit])
     results = Rbac.filtered(results, :class        => db,
                                      :filter       => conditions,
@@ -305,8 +288,8 @@ def generate_interval_metric_results(options = {})
     results = db_class.with_interval_and_time_range(interval, performance_report_time_range)
                       .where(where_clause)
                       .where(options[:where_clause])
-                      .includes(get_include_for_find)
-                      .references(db_class.includes_to_references(get_include))
+                      .preload(get_ar_includes)
+                      .eager_load(get_ar_references)
                       .limit(options[:limit])
 
     # Rbac will only add miq_expression for hourly report. It will not work properly for daily because many values are rolled up from hourly.
@@ -339,8 +322,8 @@ def generate_basic_results(options = {})
     rbac_opts = options.merge(
       :targets          => targets,
       :filter           => conditions,
-      :include_for_find => get_include_for_find_rbac,
-      :references       => get_include_rbac,
+      :include_for_find => get_ar_includes,
+      :references       => get_ar_references,
       :skip_counts      => true
     )
 

app/models/miq_report/search.rb

@@ -86,7 +86,7 @@ def paged_view_search(options = {})
     self.display_filter = options.delete(:display_filter_hash)  if options[:display_filter_hash]
     self.display_filter = options.delete(:display_filter_block) if options[:display_filter_block]
 
-    includes = get_include_for_find
+    includes = get_ar_includes
     self.extras ||= {}
     if extras[:target_ids_for_paging] && db_class.column_names.include?('id')
       return get_cached_page(limited_ids(limit, offset), includes, options)
@@ -97,7 +97,7 @@ def paged_view_search(options = {})
     search_options = options.merge(:class            => db,
                                    :conditions       => conditions,
                                    :include_for_find => includes,
-                                   :references       => get_include
+                                   :references       => get_ar_references
                                   )
     search_options.merge!(:limit => limit, :offset => offset, :order => order) if order
     search_options[:extra_cols] = va_sql_cols if va_sql_cols.present?

app/models/mixins/cloud_tenancy_mixin.rb

@@ -13,8 +13,7 @@ def tenant_id_clause_format(tenant_ids)
     end
 
     def tenant_joins_clause(scope)
-      scope.includes(:cloud_tenant => "source_tenant", :ext_management_system => {})
-           .references(:cloud_tenants, :tenants, :ext_management_systems) # needed for the where to work
+      scope.left_outer_joins(:cloud_tenant => "source_tenant", :ext_management_system => {})
     end
   end
 end

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

lib/rbac/filterer.rb

@@ -296,10 +296,9 @@ def search(options = {})
       scope = scope.except(:offset, :limit, :order)
       scope = scope_targets(klass, scope, user_filters, user, miq_group)
               .where(conditions).where(sub_filter).where(where_clause).where(exp_sql).where(ids_clause)
-              .includes(include_for_find).includes(exp_includes)
               .order(order)
 
-      scope = include_references(scope, klass, references, exp_includes)
+      scope = include_references(scope, klass, include_for_find, references, exp_includes)
       scope = scope.limit(limit).offset(offset) if attrs[:apply_limit_in_sql]
 
       #      SELECT col1, (SELECT "abc") AS virtual_col
@@ -324,11 +323,17 @@ def search(options = {})
       #        Hence auth_count calculated from inner_scope.
       #
       if inline_view?(options, scope)
-        inner_scope = scope.except(:select, :includes, :references)
-        scope.includes_values.each { |hash| inner_scope = add_joins(klass, inner_scope, hash) }
+        inner_scope = scope.except(:select, :includes, :references, :eager_load, :preload)
+        # similar to include_references but using joins
+        # 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)
 
@@ -424,25 +429,42 @@ def select_from_order_columns(columns)
       end
     end
 
-    def include_references(scope, klass, references, exp_includes)
-      scope.references(klass.includes_to_references(references)).references(klass.includes_to_references(exp_includes))
+    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
+        # TODO: drop this fallback once https://github.com/ManageIQ/query_relation/pull/43 is merged
+        scope.references(references || {}).references(exp_includes || {}).includes(includes)
+      end
     end
 
     # @param includes [Array, Hash]
     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)
-          scope = scope.distinct if reflection.try(:collection?)
         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
 
+    def is_collection?(klass, includes)
+      Array(includes).any? do |association, value|
+        reflection = klass.reflect_on_association(association)
+        reflection&.collection? || !reflection.polymorphic? && is_collection?(reflection.klass, value)
+      end
+    end
+
     def filtered(objects, options = {})
       Rbac.search(options.reverse_merge(:targets => objects, :skip_counts => true)).first
     end

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