BigW Consortium Gitlab

Commit 99492d6b by Yorick Peterse

Optimize fetching issues closed by a merge request

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
parent ca171b81
...@@ -26,6 +26,7 @@ v 8.4.2 ...@@ -26,6 +26,7 @@ v 8.4.2
track them in Performance Monitoring. track them in Performance Monitoring.
- Increase contrast between highlighted code comments and inline diff marker - Increase contrast between highlighted code comments and inline diff marker
- Fix method undefined when using external commit status in builds - 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 v 8.4.1
- Apply security updates for Rails (4.2.5.1), rails-html-sanitizer (1.0.3), - Apply security updates for Rails (4.2.5.1), rails-html-sanitizer (1.0.3),
......
...@@ -346,10 +346,10 @@ class MergeRequest < ActiveRecord::Base ...@@ -346,10 +346,10 @@ class MergeRequest < ActiveRecord::Base
# Return the set of issues that will be closed if this merge request is accepted. # Return the set of issues that will be closed if this merge request is accepted.
def closes_issues(current_user = self.author) def closes_issues(current_user = self.author)
if target_branch == project.default_branch if target_branch == project.default_branch
issues = commits.flat_map { |c| c.closes_issues(current_user) } messages = commits.map(&:safe_message) << description
issues.push(*Gitlab::ClosingIssueExtractor.new(project, current_user).
closed_by_message(description)) Gitlab::ClosingIssueExtractor.new(project, current_user).
issues.uniq(&:id) closed_by_message(messages.join("\n"))
else else
[] []
end end
......
...@@ -137,9 +137,10 @@ describe MergeRequest, models: true do ...@@ -137,9 +137,10 @@ describe MergeRequest, models: true do
describe 'detection of issues to be closed' do describe 'detection of issues to be closed' do
let(:issue0) { create :issue, project: subject.project } let(:issue0) { create :issue, project: subject.project }
let(:issue1) { 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(:commit0) { double('commit0', safe_message: "Fixes #{issue0.to_reference}") }
let(:commit2) { double('commit2', closes_issues: [issue1]) } let(:commit1) { double('commit1', safe_message: "Fixes #{issue0.to_reference}") }
let(:commit2) { double('commit2', safe_message: "Fixes #{issue1.to_reference}") }
before do before do
allow(subject).to receive(:commits).and_return([commit0, commit1, commit2]) allow(subject).to receive(:commits).and_return([commit0, commit1, commit2])
...@@ -149,7 +150,9 @@ describe MergeRequest, models: true do ...@@ -149,7 +150,9 @@ describe MergeRequest, models: true do
allow(subject.project).to receive(:default_branch). allow(subject.project).to receive(:default_branch).
and_return(subject.target_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 end
it 'only lists issues as to be closed if it targets the default branch' do it 'only lists issues as to be closed if it targets the default branch' do
...@@ -167,17 +170,6 @@ describe MergeRequest, models: true do ...@@ -167,17 +170,6 @@ describe MergeRequest, models: true do
expect(subject.closes_issues).to include(issue2) expect(subject.closes_issues).to include(issue2)
end 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 end
describe "#work_in_progress?" do describe "#work_in_progress?" do
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment