Merge branch 'sh-optimize-mr-api-emojis-and-labels' into 'master'

Remove remaining N+1 queries in merge requests API with emojis and labels

Closes #34159

See merge request !12732

3 files changed, 59 insertions, 5 deletions

diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index f4796f311a5..945f2821d72 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -316,10 +316,26 @@ module API
 
     class MergeRequestBasic < ProjectEntity
       expose :target_branch, :source_branch
-      expose :upvotes, :downvotes
+      expose :upvotes do |merge_request, options|
+        if options[:issuable_metadata]
+          options[:issuable_metadata][merge_request.id].upvotes
+        else
+          merge_request.upvotes
+        end
+      end
+      expose :downvotes do |merge_request, options|
+        if options[:issuable_metadata]
+          options[:issuable_metadata][merge_request.id].downvotes
+        else
+          merge_request.downvotes
+        end
+      end
       expose :author, :assignee, using: Entities::UserBasic
       expose :source_project_id, :target_project_id
-      expose :label_names, as: :labels
+      expose :labels do |merge_request, options|
+        # Avoids an N+1 query since labels are preloaded
+        merge_request.labels.map(&:title).sort
+      end
       expose :work_in_progress?, as: :work_in_progress
       expose :milestone, using: Entities::Milestone
       expose :merge_when_pipeline_succeeds
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index d419d345ec5..4ad1eef4ff1 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -10,6 +10,8 @@ module API
     resource :projects, requirements: { id: %r{[^/]+} } do
       include TimeTrackingEndpoints
 
+      helpers ::Gitlab::IssuableMetadata
+
       helpers do
         def handle_merge_request_errors!(errors)
           if errors[:project_access].any?
@@ -42,8 +44,7 @@ module API
           args[:label_name] = args.delete(:labels)
 
           merge_requests = MergeRequestsFinder.new(current_user, args).execute
-                             .inc_notes_with_associations
-                             .preload(:target_project, :author, :assignee, :milestone, :merge_request_diff)
+                             .preload(:notes, :target_project, :author, :assignee, :milestone, :merge_request_diff, :labels)
 
           merge_requests.reorder(args[:order_by] => args[:sort])
         end
@@ -82,8 +83,9 @@ module API
         authorize! :read_merge_request, user_project
 
         merge_requests = find_merge_requests(project_id: user_project.id)
+        issuable_metadata = issuable_meta_data(merge_requests, 'MergeRequest')
 
-        present paginate(merge_requests), with: Entities::MergeRequestBasic, current_user: current_user, project: user_project
+        present paginate(merge_requests), with: Entities::MergeRequestBasic, current_user: current_user, project: user_project, issuable_metadata: issuable_metadata
       end
 
       desc 'Create a merge request' do
diff --git a/lib/gitlab/issuable_metadata.rb b/lib/gitlab/issuable_metadata.rb
new file mode 100644
index 00000000000..977c05910d3
--- /dev/null
+++ b/lib/gitlab/issuable_metadata.rb
@@ -0,0 +1,36 @@
+module Gitlab
+  module IssuableMetadata
+    def issuable_meta_data(issuable_collection, collection_type)
+      # map has to be used here since using pluck or select will
+      # throw an error when ordering issuables by priority which inserts
+      # a new order into the collection.
+      # We cannot use reorder to not mess up the paginated collection.
+      issuable_ids = issuable_collection.map(&:id)
+
+      return {} if issuable_ids.empty?
+
+      issuable_note_count = ::Note.count_for_collection(issuable_ids, collection_type)
+      issuable_votes_count = ::AwardEmoji.votes_for_collection(issuable_ids, collection_type)
+      issuable_merge_requests_count =
+        if collection_type == 'Issue'
+          ::MergeRequestsClosingIssues.count_for_collection(issuable_ids)
+        else
+          []
+        end
+
+      issuable_ids.each_with_object({}) do |id, issuable_meta|
+        downvotes = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.downvote? }
+        upvotes = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.upvote? }
+        notes = issuable_note_count.find { |notes| notes.noteable_id == id }
+        merge_requests = issuable_merge_requests_count.find { |mr| mr.first == id }
+
+        issuable_meta[id] = ::Issuable::IssuableMeta.new(
+          upvotes.try(:count).to_i,
+          downvotes.try(:count).to_i,
+          notes.try(:count).to_i,
+          merge_requests.try(:last).to_i
+        )
+      end
+    end
+  end
+end