Merge pull request #19137 from jrafanie/turbo_distinct_hammer

[HAMMER] Turbo distinct

app/models/miq_report.rb

@@ -252,6 +252,10 @@ def group_description=(_group_description)
     # which does not exist in the MiqReport class
   end
 
+  def self.default_use_sql_view
+    ::Settings.reporting.use_sql_view
+  end
+
   private
 
   def va_sql_cols

config/settings.yml

@@ -997,6 +997,7 @@
     :keep_reports: 6.months
     :purge_window_size: 100
   :queue_timeout: 1.hour
+  :use_sql_view: false
 :repository_scanning:
   :defaultsmartproxy:
 :server:

lib/rbac/filterer.rb

@@ -270,6 +270,9 @@ def search(options = {})
       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) }
+        if inner_scope.order_values.present?
+          inner_scope = apply_select(klass, inner_scope, select_from_order_columns(inner_scope.order_values))
+        end
         scope = scope.from(Arel.sql("(#{inner_scope.to_sql})").as(scope.table_name))
                      .except(:offset, :limit, :order, :where)
 
@@ -324,6 +327,35 @@ def inline_view?(options, scope)
         scope.respond_to?(:includes_values)
     end
 
+    # Convert an order by column to the select columns
+    #
+    # We assume that all traditional columns are already in the select
+    # So this just adds the non columns in the order (i.e.: functions and subqueries)
+    #
+    # @param [Array] columns the order by clause
+    # @return [Array] columns useable in the select clause
+    #
+    # this method is similar to Connection#columns_for_distinct, but more aggressive
+    #
+    # For a query with a DISTINCT, all columns in the ORDER BY clause
+    # need to be in the SELECT clause. This method gives us the columns for the SELECT.
+    # We currently assume that all regular columns are already in the SELECT.
+    # So this is just returning the functions (e.g. LOWER(name))
+    def select_from_order_columns(columns)
+      columns.compact.map do |column|
+        if column.kind_of?(Arel::Nodes::Ordering)
+          column.expr
+        else
+          column = column.to_sql if column.respond_to?(:to_sql)
+          column.kind_of?(String) ? column.gsub(/ (DESC|ASC)/i, '') : column
+        end
+      end.reject do |column|
+        column.kind_of?(Arel::Attributes::Attribute) ||
+          column.kind_of?(Symbol) ||
+          (column.kind_of?(String) && column =~ /^("?[a-z_0-9]+"?[.])?"?[a-z_0-9]+"?$/i)
+      end
+    end
+
     # This is a very primitive way of determining whether we want to skip
     # adding references to the query.
     #
@@ -386,19 +418,15 @@ def add_joins(klass, scope, includes)
       return scope unless includes
       includes = Array(includes) unless includes.kind_of?(Enumerable)
       includes.each do |association, value|
-        if table_include?(klass, association)
+        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
       scope
     end
 
-    # this is a reference to a non polymorphic table
-    def table_include?(target_klass, association)
-      reflection = target_klass.reflect_on_association(association)
-      reflection && !reflection.polymorphic?
-    end
-
     def polymorphic_include?(target_klass, includes)
       includes.keys.any? do |incld|
         reflection = target_klass.reflect_on_association(incld)
@@ -711,7 +739,9 @@ def apply_scope(klass, scope)
     end
 
     def apply_select(klass, scope, extra_cols)
-      scope.select(scope.select_values.blank? ? klass.arel_table[Arel.star] : nil).select(extra_cols)
+      return scope if extra_cols.blank?
+
+      (scope.select_values.blank? ? scope.select(klass.arel_table[Arel.star]) : scope).select(extra_cols)
     end
 
     def get_belongsto_matches(blist, klass)

spec/lib/rbac/filterer_spec.rb

@@ -2259,6 +2259,56 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
         expect(results.first).to eq(services1[0..2])
         expect(results.last[:auth_count]).to eq(4)
       end
+
+      it "respects order" do
+        # for some reason, while all models respect the order,
+        # MiqRequest sometimes did not. (before we put order in both inner and outer sql)
+        reqs = FactoryBot.create_list(:automation_requests, 3, :requester => user)
+        # move last created record to more recent
+        reqs.first.update(:description => "something")
+        expected_order = [reqs.second, reqs.last, reqs.first]
+
+        recs, attrs = Rbac.search(:targets      => MiqRequest,
+                                  :extra_cols   => %w[id],
+                                  :use_sql_view => true,
+                                  :limit        => 3,
+                                  :user         => user,
+                                  :order        => :updated_on)
+
+        expect(attrs[:auth_count]).to eq(3)
+        expect(recs.map(&:id)).to eq(expected_order.map(&:id))
+
+        recs, attrs = Rbac.search(:targets      => MiqRequest,
+                                  :extra_cols   => %w[],
+                                  :use_sql_view => true,
+                                  :limit        => 3,
+                                  :user         => user,
+                                  :order        => {:updated_on => :desc})
+        expect(attrs[:auth_count]).to eq(3)
+        expect(recs.map(&:id)).to eq(expected_order.reverse.map(&:id))
+      end
+
+      it "remembers distinct" do
+        # create vms with many disks. so a missing distinct would return 3*3 => 9
+        vms = FactoryBot.create_list(:vm_infra, 3, :miq_group => tagged_group)
+        vms.each do |vm|
+          # used by rbac filter
+          vm.tag_with("/managed/environment/prod", :ns => "*")
+          hw = FactoryBot.create(:hardware, :cpu_sockets => 4, :memory_mb => 3.megabytes, :vm => vm)
+          FactoryBot.create_list(:disk, 3, :device_type => "disk", :size => 10_000, :hardware_id => hw.id)
+        end
+
+        recs, attrs = Rbac.search(:targets          => Vm,
+                                  :include_for_find => {:ext_management_system => {}, :hardware => {:disks => {}}, :tags => {}},
+                                  :extra_cols       => %w[ram_size_in_bytes],
+                                  :use_sql_view     => true,
+                                  :limit            => 20,
+                                  :user             => User.super_admin,
+                                  :order            => :updated_on)
+
+        expect(attrs[:auth_count]).to eq(3)
+        expect(recs.map(&:id)).to eq(vms.sort_by(&:updated_on).reverse.map(&:id))
+      end
     end
   end
 
@@ -2768,6 +2818,54 @@ def id_for_model_in_region(model, other_region)
     end
   end
 
+  # private method
+  describe ".select_from_order_columns" do
+    subject { described_class.new }
+    it "removes empty" do
+      expect(subject.select_from_order_columns([])).to eq([])
+      expect(subject.select_from_order_columns([nil])).to eq([])
+    end
+
+    it "removes string columns" do
+      expect(subject.select_from_order_columns(["name", "id", "vms.name", '"vms"."name"'])).to eq([])
+    end
+
+    it "removes ascending string columns" do
+      expect(subject.select_from_order_columns(ascenders(["name", "id", "vms.name", '"vms"."name"']))).to eq([])
+    end
+
+    # services.name desc (spec)
+    it "removes strings with sorting" do
+      expect(subject.select_from_order_columns(["name asc", "\"vms\".\"id\" desc", "id asc"])).to eq([])
+    end
+
+    it "removes ordered nodes" do
+      attribute_id = Vm.arel_table["id"] # <Attribute<id>>
+      expect(subject.select_from_order_columns(ascenders([attribute_id]))).to eq([])
+    end
+
+    it "removes nodes" do
+      attribute_id = Vm.arel_table["id"] # <Attribute<id>>
+      expect(subject.select_from_order_columns([attribute_id])).to eq([])
+    end
+
+    it "keeps sorting in subselects" do
+      expect(subject.select_from_order_columns(ascenders(["(select a from x desc)"]))).to eq(["(select a from x desc)"])
+    end
+
+    it "keeps function in ascending node" do
+      expect(subject.select_from_order_columns(ascenders(["lower(name)"]))).to eq(["lower(name)"])
+    end
+
+    it "keeps functions as ordered strings" do
+      expect(subject.select_from_order_columns(["lower(name) asc"])).to eq(["lower(name)"])
+    end
+
+    def ascenders(cols)
+      cols.map { |c| Arel::Nodes::Ascending.new(c) }
+    end
+  end
+
   private
 
   # separate them to match easier for failures