Optimize fetching issues closed by a merge requestmerge-request-closes-issues-performance

Instead of running ClosingIssueExtractor for every commit in a merge
request we can gather all the commit messages (and the merge request
description), concatenate all this together and then run
ClosingIssueExtractor only once.

The result of this is that MergeRequest#closes_issues is now between
3.5x and 4x faster than the old setup. Using a merge request with 10
commits (each referencing a number of issues to close) this reduced the
call duration from around 200 milliseconds to around 50 milliseconds.

As a result of these changes the Jira related tests for
MergeRequest#closes_issues have been removed. These tests stubbed
Commit#closes_issues meaning that the only code that was really tested
was the call to Array#uniq to filter out duplicate issues. As this code
is no longer used (nor present) the corresponding tests were removed.

Related: gitlab-org/gitlab-ce#12419

3 files changed, 12 insertions, 19 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 14f2f14becd..051bc033117 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -26,6 +26,7 @@ v 8.4.2
     track them in Performance Monitoring.
   - Increase contrast between highlighted code comments and inline diff marker
   - Fix method undefined when using external commit status in builds
+  - Optimized performance of finding issues to be closed by a merge request (Yorick Peterse)
 
 v 8.4.1
   - Apply security updates for Rails (4.2.5.1), rails-html-sanitizer (1.0.3),
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 41dd248d80a..09af60a2016 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -346,10 +346,10 @@ class MergeRequest < ActiveRecord::Base
   # Return the set of issues that will be closed if this merge request is accepted.
   def closes_issues(current_user = self.author)
     if target_branch == project.default_branch
-      issues = commits.flat_map { |c| c.closes_issues(current_user) }
-      issues.push(*Gitlab::ClosingIssueExtractor.new(project, current_user).
-                  closed_by_message(description))
-      issues.uniq(&:id)
+      messages = commits.map(&:safe_message) << description
+
+      Gitlab::ClosingIssueExtractor.new(project, current_user).
+        closed_by_message(messages.join("\n"))
     else
       []
     end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 291e6200a5b..46f2f20b986 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -137,9 +137,10 @@ describe MergeRequest, models: true do
   describe 'detection of issues to be closed' do
     let(:issue0) { create :issue, project: subject.project }
     let(:issue1) { create :issue, project: subject.project }
-    let(:commit0) { double('commit0', closes_issues: [issue0]) }
-    let(:commit1) { double('commit1', closes_issues: [issue0]) }
-    let(:commit2) { double('commit2', closes_issues: [issue1]) }
+
+    let(:commit0) { double('commit0', safe_message: "Fixes #{issue0.to_reference}") }
+    let(:commit1) { double('commit1', safe_message: "Fixes #{issue0.to_reference}") }
+    let(:commit2) { double('commit2', safe_message: "Fixes #{issue1.to_reference}") }
 
     before do
       allow(subject).to receive(:commits).and_return([commit0, commit1, commit2])
@@ -149,7 +150,9 @@ describe MergeRequest, models: true do
       allow(subject.project).to receive(:default_branch).
         and_return(subject.target_branch)
 
-      expect(subject.closes_issues).to eq([issue0, issue1].sort_by(&:id))
+      closed = subject.closes_issues
+
+      expect(closed).to include(issue0, issue1)
     end
 
     it 'only lists issues as to be closed if it targets the default branch' do
@@ -167,17 +170,6 @@ describe MergeRequest, models: true do
 
       expect(subject.closes_issues).to include(issue2)
     end
-
-    context 'for a project with JIRA integration' do
-      let(:issue0) { JiraIssue.new('JIRA-123', subject.project) }
-      let(:issue1) { JiraIssue.new('FOOBAR-4567', subject.project) }
-
-      it 'returns sorted JiraIssues' do
-        allow(subject.project).to receive_messages(default_branch: subject.target_branch)
-
-        expect(subject.closes_issues).to eq([issue0, issue1])
-      end
-    end
   end
 
   describe "#work_in_progress?" do