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

2 files changed, 81 insertions, 3 deletions

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