Merge branch '217973-use-standard-finders-in-vsa-base-query' into 'master'

Always use the finders in Value Stream Analytics

See merge request gitlab-org/gitlab!33033

ee/lib/ee/gitlab/analytics/cycle_analytics/base_query_builder.rb

@@ -10,33 +10,25 @@ module EE::Gitlab::Analytics::CycleAnalytics::BaseQueryBuilder
 
   private
 
-  # rubocop: disable CodeReuse/ActiveRecord
-  def filter_by_project_ids(query)
-    project_ids = Array(params[:project_ids])
-
-    query = query.where(project_id: project_ids) if project_ids.any?
-    query
+  override :build_finder_params
+  def build_finder_params(params)
+    super.tap do |finder_params|
+      finder_params[:project_ids] = Array(params[:project_ids])
+    end
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
-  override :filter_by_parent_model
-  # rubocop: disable CodeReuse/ActiveRecord
-  def filter_by_parent_model(query)
+  override :add_parent_model_params!
+  def add_parent_model_params!(finder_params)
     return super unless parent_class.eql?(Group)
 
-    if subject_class.eql?(Issue)
-      join_groups(query.joins(:project))
-    elsif subject_class.eql?(MergeRequest)
-      join_groups(query.joins(:target_project))
-    else
-      raise ArgumentError, "unknown subject_class: #{subject_class}"
-    end
+    finder_params.merge!(group_id: stage.parent_id, include_subgroups: true)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   # rubocop: disable CodeReuse/ActiveRecord
-  def join_groups(query)
-    query.joins(Arel.sql("INNER JOIN (#{stage.parent.self_and_descendants.to_sql}) namespaces ON namespaces.id=projects.namespace_id"))
+  def filter_by_project_ids(query)
+    return query if params[:project_ids].empty?
+
+    query.where(project_id: params[:project_ids])
   end
   # rubocop: enable CodeReuse/ActiveRecord
 end

ee/lib/ee/gitlab/analytics/cycle_analytics/records_fetcher.rb

@@ -2,9 +2,4 @@
 
 module EE::Gitlab::Analytics::CycleAnalytics::RecordsFetcher
   extend ::Gitlab::Utils::Override
-
-  override :finder_params
-  def finder_params
-    super.merge({ ::Group => { group_id: stage.parent_id } })
-  end
 end

ee/lib/gitlab/analytics/cycle_analytics/summary/group/base_time.rb

@@ -43,7 +43,8 @@ module Gitlab
                 params: {
                   from: @options[:from],
                   to: @options[:to] || DateTime.now,
-                  project_ids: @options[:projects]
+                  project_ids: @options[:projects],
+                  current_user: @current_user
                 }
               )
             end

ee/spec/lib/ee/gitlab/analytics/cycle_analytics/base_query_builder_spec.rb

@@ -8,6 +8,12 @@ describe Gitlab::Analytics::CycleAnalytics::BaseQueryBuilder do
   let_it_be(:project_in_group) { create(:project, :repository, group: group) }
   let_it_be(:project_in_subgroup) { create(:project, :repository, group: subgroup) }
   let_it_be(:project_outside_group) { create(:project, :repository, group: create(:group)) }
+  let_it_be(:user) { create(:user) }
+
+  before do
+    group.add_maintainer(user)
+    project_outside_group.add_maintainer(user)
+  end
 
   context 'when the subject is `Issue`' do
     let(:issue_in_project) { create(:issue, project: project_in_group, created_at: 5.days.ago) }
@@ -27,7 +33,7 @@ describe Gitlab::Analytics::CycleAnalytics::BaseQueryBuilder do
         group: group
       })
 
-      result = described_class.new(stage: stage).build
+      result = described_class.new(stage: stage, params: { current_user: user }).build
 
       expect(result).to contain_exactly(issue_in_project, issue_in_subgroup_project)
     end
@@ -51,7 +57,7 @@ describe Gitlab::Analytics::CycleAnalytics::BaseQueryBuilder do
         group: group
       })
 
-      result = described_class.new(stage: stage).build
+      result = described_class.new(stage: stage, params: { current_user: user }).build
 
       expect(result).to contain_exactly(mr_in_project, mr_in_subgroup_project)
     end

lib/gitlab/analytics/cycle_analytics/base_query_builder.rb

@@ -8,21 +8,24 @@ module Gitlab
 
         delegate :subject_class, to: :stage
 
-        # rubocop: disable CodeReuse/ActiveRecord
+        FINDER_CLASSES = {
+          MergeRequest.to_s => MergeRequestsFinder,
+          Issue.to_s => IssuesFinder
+        }.freeze
 
         def initialize(stage:, params: {})
           @stage = stage
-          @params = params
+          @params = build_finder_params(params)
         end
 
+        # rubocop: disable CodeReuse/ActiveRecord
         def build
-          query = subject_class
-          query = filter_by_parent_model(query)
-          query = filter_by_time_range(query)
+          query = finder.execute
           query = stage.start_event.apply_query_customization(query)
           query = stage.end_event.apply_query_customization(query)
           query.where(duration_condition)
         end
+        # rubocop: enable CodeReuse/ActiveRecord
 
         private
 
@@ -32,38 +35,33 @@ module Gitlab
           stage.end_event.timestamp_projection.gteq(stage.start_event.timestamp_projection)
         end
 
-        def filter_by_parent_model(query)
-          if parent_class.eql?(Project)
-            if subject_class.eql?(Issue)
-              query.where(project_id: stage.parent_id)
-            elsif subject_class.eql?(MergeRequest)
-              query.where(target_project_id: stage.parent_id)
-            else
-              raise ArgumentError, "unknown subject_class: #{subject_class}"
-            end
-          else
-            raise ArgumentError, "unknown parent_class: #{parent_class}"
-          end
+        def finder
+          FINDER_CLASSES.fetch(subject_class.to_s).new(params[:current_user], params)
         end
 
-        def filter_by_time_range(query)
-          from = params.fetch(:from, 30.days.ago)
-          to = params[:to]
-
-          query = query.where(subject_table[:created_at].gteq(from))
-          query = query.where(subject_table[:created_at].lteq(to)) if to
-          query
+        def parent_class
+          stage.parent.class
         end
 
-        def subject_table
-          subject_class.arel_table
+        def build_finder_params(params)
+          {}.tap do |finder_params|
+            finder_params[:current_user] = params[:current_user]
+
+            add_parent_model_params!(finder_params)
+            add_time_range_params!(finder_params, params[:from], params[:to])
+          end
         end
 
-        def parent_class
-          stage.parent.class
+        def add_parent_model_params!(finder_params)
+          raise(ArgumentError, "unknown parent_class: #{parent_class}") unless parent_class.eql?(Project)
+
+          finder_params[:project_id] = stage.parent_id
         end
 
-        # rubocop: enable CodeReuse/ActiveRecord
+        def add_time_range_params!(finder_params, from, to)
+          finder_params[:created_after] = from || 30.days.ago
+          finder_params[:created_before] = to if to
+        end
       end
     end
   end

lib/gitlab/analytics/cycle_analytics/median.rb

@@ -11,12 +11,14 @@ module Gitlab
           @query = query
         end
 
+        # rubocop: disable CodeReuse/ActiveRecord
         def seconds
-          @query = @query.select(median_duration_in_seconds.as('median'))
+          @query = @query.select(median_duration_in_seconds.as('median')).reorder(nil)
           result = execute_query(@query).first || {}
 
           result['median'] || nil
         end
+        # rubocop: enable CodeReuse/ActiveRecord
 
         def days
           seconds ? seconds.fdiv(1.day) : nil

lib/gitlab/analytics/cycle_analytics/records_fetcher.rb

@@ -12,13 +12,11 @@ module Gitlab
 
         MAPPINGS = {
           Issue => {
-            finder_class: IssuesFinder,
             serializer_class: AnalyticsIssueSerializer,
             includes_for_query: { project: [:namespace], author: [] },
             columns_for_select: %I[title iid id created_at author_id project_id]
           },
           MergeRequest => {
-            finder_class: MergeRequestsFinder,
             serializer_class: AnalyticsMergeRequestSerializer,
             includes_for_query: { target_project: [:namespace], author: [] },
             columns_for_select: %I[title iid id created_at author_id state_id target_project_id]
@@ -56,27 +54,12 @@ module Gitlab
 
         attr_reader :stage, :query, :params
 
-        def finder_query
-          MAPPINGS
-            .fetch(subject_class)
-            .fetch(:finder_class)
-            .new(params.fetch(:current_user), finder_params.fetch(stage.parent.class))
-            .execute
-        end
-
         def columns
           MAPPINGS.fetch(subject_class).fetch(:columns_for_select).map do |column_name|
             subject_class.arel_table[column_name]
           end
         end
 
-        # EE will override this to include Group rules
-        def finder_params
-          {
-            Project => { project_id: stage.parent_id }
-          }
-        end
-
         def default_test_stage?
           stage.matches_with_stage_params?(Gitlab::Analytics::CycleAnalytics::DefaultStages.params_for_test_stage)
         end
@@ -113,8 +96,7 @@ module Gitlab
         end
 
         def records
-          results = finder_query
-            .merge(ordered_and_limited_query)
+          results = ordered_and_limited_query
             .select(*columns, round_duration_to_seconds.as('total_time'))
 
           # using preloader instead of includes to avoid AR generating a large column list

spec/lib/gitlab/analytics/cycle_analytics/base_query_builder_spec.rb

@@ -6,7 +6,8 @@ describe Gitlab::Analytics::CycleAnalytics::BaseQueryBuilder do
   let_it_be(:project) { create(:project, :empty_repo) }
   let_it_be(:mr1) { create(:merge_request, target_project: project, source_project: project, allow_broken: true, created_at: 3.months.ago) }
   let_it_be(:mr2) { create(:merge_request, target_project: project, source_project: project, allow_broken: true, created_at: 1.month.ago) }
-  let(:params) { {} }
+  let_it_be(:user) { create(:user) }
+  let(:params) { { current_user: user } }
   let(:records) do
     stage = build(:cycle_analytics_project_stage, {
       start_event_identifier: :merge_request_created,
@@ -17,6 +18,7 @@ describe Gitlab::Analytics::CycleAnalytics::BaseQueryBuilder do
   end
 
   before do
+    project.add_maintainer(user)
     mr1.metrics.update!(merged_at: 1.month.ago)
     mr2.metrics.update!(merged_at: Time.now)
   end

spec/lib/gitlab/analytics/cycle_analytics/records_fetcher_spec.rb

@@ -23,7 +23,7 @@ describe Gitlab::Analytics::CycleAnalytics::RecordsFetcher do
   describe '#serialized_records' do
     shared_context 'when records are loaded by maintainer' do
       before do
-        project.add_user(user, Gitlab::Access::MAINTAINER)
+        project.add_user(user, Gitlab::Access::DEVELOPER)
       end
 
       it 'returns all records' do
@@ -103,6 +103,8 @@ describe Gitlab::Analytics::CycleAnalytics::RecordsFetcher do
           latest_build_finished_at: 7.days.ago,
           pipeline: ci_build2.pipeline
         })
+
+        project.add_user(user, Gitlab::Access::MAINTAINER)
       end
 
       context 'returns build records' do