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

7 files changed, 145 insertions, 41 deletions

diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb
index 693e2f6365c..e18778cdf80 100644
--- a/app/controllers/concerns/issuable_collections.rb
+++ b/app/controllers/concerns/issuable_collections.rb
@@ -1,6 +1,7 @@
 module IssuableCollections
   extend ActiveSupport::Concern
   include SortingHelper
+  include Gitlab::IssuableMetadata
 
   included do
     helper_method :issues_finder
@@ -9,39 +10,6 @@ module IssuableCollections
 
   private
 
-  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
-
   def issues_collection
     issues_finder.execute.preload(:project, :author, :assignees, :labels, :milestone, project: :namespace)
   end
diff --git a/changelogs/unreleased/sh-optimize-mr-api-emojis-and-labels.yml b/changelogs/unreleased/sh-optimize-mr-api-emojis-and-labels.yml
new file mode 100644
index 00000000000..9589659cdc2
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-mr-api-emojis-and-labels.yml
@@ -0,0 +1,4 @@
+---
+title: Remove remaining N+1 queries in merge requests API with emojis and labels
+merge_request:
+author:
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
diff --git a/spec/lib/gitlab/issuable_metadata_spec.rb b/spec/lib/gitlab/issuable_metadata_spec.rb
new file mode 100644
index 00000000000..f9f4b290dbf
--- /dev/null
+++ b/spec/lib/gitlab/issuable_metadata_spec.rb
@@ -0,0 +1,59 @@
+require 'spec_helper'
+
+describe Gitlab::IssuableMetadata, lib: true do
+  let(:user)        { create(:user) }
+  let!(:project)    { create(:project, :public, :repository, creator: user, namespace: user.namespace) }
+
+  subject { Class.new { include Gitlab::IssuableMetadata }.new }
+
+  it 'returns an empty Hash if an empty collection is provided' do
+    expect(subject.issuable_meta_data(Issue.none, 'Issue')).to eq({})
+  end
+
+  context 'issues' do
+    let!(:issue) { create(:issue, author: user, project: project) }
+    let!(:closed_issue) { create(:issue, state: :closed, author: user, project: project) }
+    let!(:downvote) { create(:award_emoji, :downvote, awardable: closed_issue) }
+    let!(:upvote) { create(:award_emoji, :upvote, awardable: issue) }
+    let!(:merge_request) { create(:merge_request, :simple, author: user, assignee: user, source_project: project, target_project: project, title: "Test") }
+    let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) }
+
+    it 'aggregates stats on issues' do
+      data = subject.issuable_meta_data(Issue.all, 'Issue')
+
+      expect(data.count).to eq(2)
+      expect(data[issue.id].upvotes).to eq(1)
+      expect(data[issue.id].downvotes).to eq(0)
+      expect(data[issue.id].notes_count).to eq(0)
+      expect(data[issue.id].merge_requests_count).to eq(1)
+
+      expect(data[closed_issue.id].upvotes).to eq(0)
+      expect(data[closed_issue.id].downvotes).to eq(1)
+      expect(data[closed_issue.id].notes_count).to eq(0)
+      expect(data[closed_issue.id].merge_requests_count).to eq(0)
+    end
+  end
+
+  context 'merge requests' do
+    let!(:merge_request) { create(:merge_request, :simple, author: user, assignee: user, source_project: project, target_project: project, title: "Test") }
+    let!(:merge_request_closed) { create(:merge_request, state: "closed", source_project: project, target_project: project, title: "Closed Test") }
+    let!(:downvote) { create(:award_emoji, :downvote, awardable: merge_request) }
+    let!(:upvote) { create(:award_emoji, :upvote, awardable: merge_request) }
+    let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") }
+
+    it 'aggregates stats on merge requests' do
+      data = subject.issuable_meta_data(MergeRequest.all, 'MergeRequest')
+
+      expect(data.count).to eq(2)
+      expect(data[merge_request.id].upvotes).to eq(1)
+      expect(data[merge_request.id].downvotes).to eq(1)
+      expect(data[merge_request.id].notes_count).to eq(1)
+      expect(data[merge_request.id].merge_requests_count).to eq(0)
+
+      expect(data[merge_request_closed.id].upvotes).to eq(0)
+      expect(data[merge_request_closed.id].downvotes).to eq(0)
+      expect(data[merge_request_closed.id].notes_count).to eq(0)
+      expect(data[merge_request_closed.id].merge_requests_count).to eq(0)
+    end
+  end
+end
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 4d0bd67c571..360a82196a8 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -16,7 +16,11 @@ describe API::MergeRequests do
   let!(:label) do
     create(:label, title: 'label', color: '#FFAABB', project: project)
   end
+  let!(:label2) { create(:label, title: 'a-test', color: '#FFFFFF', project: project) }
   let!(:label_link) { create(:label_link, label: label, target: merge_request) }
+  let!(:label_link2) { create(:label_link, label: label2, target: merge_request) }
+  let!(:downvote) { create(:award_emoji, :downvote, awardable: merge_request) }
+  let!(:upvote) { create(:award_emoji, :upvote, awardable: merge_request) }
 
   before do
     project.team << [user, :reporter]
@@ -32,6 +36,18 @@ describe API::MergeRequests do
     end
 
     context "when authenticated" do
+      it 'avoids N+1 queries' do
+        control_count = ActiveRecord::QueryRecorder.new do
+          get api("/projects/#{project.id}/merge_requests", user)
+        end.count
+
+        create(:merge_request, state: 'closed', milestone: milestone1, author: user, assignee: user, source_project: project, target_project: project, title: "Test", created_at: base_time)
+
+        expect do
+          get api("/projects/#{project.id}/merge_requests", user)
+        end.not_to exceed_query_limit(control_count)
+      end
+
       it "returns an array of all merge_requests" do
         get api("/projects/#{project.id}/merge_requests", user)
 
@@ -44,6 +60,9 @@ describe API::MergeRequests do
         expect(json_response.last['sha']).to eq(merge_request.diff_head_sha)
         expect(json_response.last['merge_commit_sha']).to be_nil
         expect(json_response.last['merge_commit_sha']).to eq(merge_request.merge_commit_sha)
+        expect(json_response.last['downvotes']).to eq(1)
+        expect(json_response.last['upvotes']).to eq(1)
+        expect(json_response.last['labels']).to eq([label2.title, label.title])
         expect(json_response.first['title']).to eq(merge_request_merged.title)
         expect(json_response.first['sha']).to eq(merge_request_merged.diff_head_sha)
         expect(json_response.first['merge_commit_sha']).not_to be_nil
@@ -145,7 +164,7 @@ describe API::MergeRequests do
         expect(response).to have_http_status(200)
         expect(json_response).to be_an Array
         expect(json_response.length).to eq(1)
-        expect(json_response.first['labels']).to eq([label.title])
+        expect(json_response.first['labels']).to eq([label2.title, label.title])
       end
 
       it 'returns an array of labeled merge requests where all labels match' do
@@ -236,8 +255,8 @@ describe API::MergeRequests do
       expect(json_response['author']).to be_a Hash
       expect(json_response['target_branch']).to eq(merge_request.target_branch)
       expect(json_response['source_branch']).to eq(merge_request.source_branch)
-      expect(json_response['upvotes']).to eq(0)
-      expect(json_response['downvotes']).to eq(0)
+      expect(json_response['upvotes']).to eq(1)
+      expect(json_response['downvotes']).to eq(1)
       expect(json_response['source_project_id']).to eq(merge_request.source_project.id)
       expect(json_response['target_project_id']).to eq(merge_request.target_project.id)
       expect(json_response['work_in_progress']).to be_falsy