Merge branch '47988-improve-milestone-queries-with-subq' into 'master'

Improve MilestonesFinder to accept project and group relations

Closes #47988

See merge request gitlab-org/gitlab-ce!24325

9 files changed, 21 insertions, 23 deletions

diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb
index 8e68014a30d..8bc59d8a305 100644
--- a/app/controllers/projects/milestones_controller.rb
+++ b/app/controllers/projects/milestones_controller.rb
@@ -144,7 +144,7 @@ class Projects::MilestonesController < Projects::ApplicationController
 
   def search_params
     if request.format.json? && project_group && can?(current_user, :read_group, project_group)
-      groups = project_group.self_and_ancestors_ids
+      groups = project_group.self_and_ancestors.select(:id)
     end
 
     params.permit(:state).merge(project_ids: @project.id, group_ids: groups)
diff --git a/app/finders/milestones_finder.rb b/app/finders/milestones_finder.rb
index 9c477978f60..fcd54b6106e 100644
--- a/app/finders/milestones_finder.rb
+++ b/app/finders/milestones_finder.rb
@@ -3,8 +3,8 @@
 # Search for milestones
 #
 # params - Hash
-#   project_ids: Array of project ids or single project id.
-#   group_ids: Array of group ids or single group id.
+#   project_ids: Array of project ids or single project id or ActiveRecord relation.
+#   group_ids: Array of group ids or single group id or ActiveRecord relation.
 #   order - Orders by field default due date asc.
 #   title - filter by title.
 #   state - filters by state.
@@ -12,17 +12,13 @@
 class MilestonesFinder
   include FinderMethods
 
-  attr_reader :params, :project_ids, :group_ids
+  attr_reader :params
 
   def initialize(params = {})
-    @project_ids = Array(params[:project_ids])
-    @group_ids = Array(params[:group_ids])
     @params = params
   end
 
   def execute
-    return Milestone.none if project_ids.empty? && group_ids.empty?
-
     items = Milestone.all
     items = by_groups_and_projects(items)
     items = by_title(items)
@@ -34,7 +30,7 @@ class MilestonesFinder
   private
 
   def by_groups_and_projects(items)
-    items.for_projects_and_groups(project_ids, group_ids)
+    items.for_projects_and_groups(params[:project_ids], params[:group_ids])
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 1ebcbcda0d8..b21edce3aad 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -45,7 +45,7 @@ class Milestone < ActiveRecord::Base
     groups = groups.compact if groups.is_a? Array
     groups = [] if groups.nil?
 
-    where(project: projects).or(where(group: groups))
+    where(project_id: projects).or(where(group_id: groups))
   end
 
   scope :order_by_name_asc, -> { order(Arel::Nodes::Ascending.new(arel_table[:title].lower)) }
@@ -191,7 +191,7 @@ class Milestone < ActiveRecord::Base
     return STATE_COUNT_HASH unless projects || groups
 
     counts = Milestone
-               .for_projects_and_groups(projects&.map(&:id), groups&.map(&:id))
+               .for_projects_and_groups(projects, groups)
                .reorder(nil)
                .group(:state)
                .count
@@ -275,8 +275,7 @@ class Milestone < ActiveRecord::Base
     if project
       relation = Milestone.for_projects_and_groups([project_id], [project.group&.id])
     elsif group
-      project_ids = group.projects.map(&:id)
-      relation = Milestone.for_projects_and_groups(project_ids, [group.id])
+      relation = Milestone.for_projects_and_groups(group.projects.select(:id), [group.id])
     end
 
     title_exists = relation.find_by_title(title)
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index c7e7bb55e4b..805bb5b510d 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -61,10 +61,10 @@ class IssuableBaseService < BaseService
     return unless milestone_id
 
     params[:milestone_id] = '' if milestone_id == IssuableFinder::NONE
-    group_ids = project.group&.self_and_ancestors&.pluck(:id)
+    groups = project.group&.self_and_ancestors&.select(:id)
 
     milestone =
-      Milestone.for_projects_and_groups([project.id], group_ids).find_by_id(milestone_id)
+      Milestone.for_projects_and_groups([project.id], groups).find_by_id(milestone_id)
 
     params[:milestone_id] = '' unless milestone
   end
diff --git a/app/services/milestones/promote_service.rb b/app/services/milestones/promote_service.rb
index 39071b5dc14..cbe5996e8ca 100644
--- a/app/services/milestones/promote_service.rb
+++ b/app/services/milestones/promote_service.rb
@@ -82,11 +82,9 @@ module Milestones
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def group_project_ids
-      @group_project_ids ||= group.projects.pluck(:id)
+      group.projects.select(:id)
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     def raise_error(message)
       raise PromoteMilestoneError, "Promotion failed - #{message}"
diff --git a/app/services/projects/autocomplete_service.rb b/app/services/projects/autocomplete_service.rb
index 61f6402a810..3dad90188cf 100644
--- a/app/services/projects/autocomplete_service.rb
+++ b/app/services/projects/autocomplete_service.rb
@@ -14,7 +14,7 @@ module Projects
         order: { due_date: :asc, title: :asc }
       }
 
-      finder_params[:group_ids] = @project.group.self_and_ancestors_ids if @project.group
+      finder_params[:group_ids] = @project.group.self_and_ancestors.select(:id) if @project.group
 
       MilestonesFinder.new(finder_params).execute.select([:iid, :title])
     end
diff --git a/changelogs/unreleased/47988-improve-milestone-queries-with-subqueries.yml b/changelogs/unreleased/47988-improve-milestone-queries-with-subqueries.yml
new file mode 100644
index 00000000000..d1a80ab43cf
--- /dev/null
+++ b/changelogs/unreleased/47988-improve-milestone-queries-with-subqueries.yml
@@ -0,0 +1,5 @@
+---
+title: Improve milestone queries using subqueries instead of separate queries for ids
+merge_request: 24325
+author:
+type: performance
diff --git a/lib/banzai/filter/milestone_reference_filter.rb b/lib/banzai/filter/milestone_reference_filter.rb
index c70c3f0c04e..fce042e8946 100644
--- a/lib/banzai/filter/milestone_reference_filter.rb
+++ b/lib/banzai/filter/milestone_reference_filter.rb
@@ -101,9 +101,9 @@ module Banzai
 
       def self_and_ancestors_ids(parent)
         if group_context?(parent)
-          parent.self_and_ancestors_ids
+          parent.self_and_ancestors.select(:id)
         elsif project_context?(parent)
-          parent.group&.self_and_ancestors_ids
+          parent.group&.self_and_ancestors&.select(:id)
         end
       end
 
diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb
index 015db4d4e96..2e436f2cc8a 100644
--- a/spec/models/milestone_spec.rb
+++ b/spec/models/milestone_spec.rb
@@ -286,8 +286,8 @@ describe Milestone do
     end
 
     context 'relations as params' do
-      let(:projects) { Project.where(id: project.id) }
-      let(:groups) { Group.where(id: group.id) }
+      let(:projects) { Project.where(id: project.id).select(:id) }
+      let(:groups) { Group.where(id: group.id).select(:id) }
 
       it_behaves_like 'filters by projects and groups'
     end