Commit fdb5f285 authored by Felipe Artur's avatar Felipe Artur

Retrieve merge request closing issues from database cache

parent cb2e0730
...@@ -52,6 +52,8 @@ class MergeRequest < ActiveRecord::Base ...@@ -52,6 +52,8 @@ class MergeRequest < ActiveRecord::Base
class_name: 'MergeRequestsClosingIssues', class_name: 'MergeRequestsClosingIssues',
dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue
belongs_to :assignee, class_name: "User" belongs_to :assignee, class_name: "User"
serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize
...@@ -755,8 +757,9 @@ class MergeRequest < ActiveRecord::Base ...@@ -755,8 +757,9 @@ class MergeRequest < ActiveRecord::Base
# Calculating this information for a number of merge requests requires # Calculating this information for a number of merge requests requires
# running `ReferenceExtractor` on each of them separately. # running `ReferenceExtractor` on each of them separately.
# This optimization does not apply to issues from external sources. # This optimization does not apply to issues from external sources.
def cache_merge_request_closes_issues!(current_user) def cache_merge_request_closes_issues!(current_user = self.author)
return unless project.issues_enabled? return unless project.issues_enabled?
return if closed? || merged?
transaction do transaction do
self.merge_requests_closing_issues.delete_all self.merge_requests_closing_issues.delete_all
...@@ -769,6 +772,18 @@ class MergeRequest < ActiveRecord::Base ...@@ -769,6 +772,18 @@ class MergeRequest < ActiveRecord::Base
end end
end end
def visible_closing_issues_for(current_user = self.author)
strong_memoize(:visible_closing_issues_for) do
if self.target_project.has_external_issue_tracker?
closes_issues(current_user)
else
cached_closes_issues.select do |issue|
Ability.allowed?(current_user, :read_issue, issue)
end
end
end
end
# 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
...@@ -788,7 +803,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -788,7 +803,7 @@ class MergeRequest < ActiveRecord::Base
ext = Gitlab::ReferenceExtractor.new(project, current_user) ext = Gitlab::ReferenceExtractor.new(project, current_user)
ext.analyze("#{title}\n#{description}") ext.analyze("#{title}\n#{description}")
ext.issues - closes_issues(current_user) ext.issues - visible_closing_issues_for(current_user)
end end
def target_project_path def target_project_path
...@@ -836,7 +851,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -836,7 +851,7 @@ class MergeRequest < ActiveRecord::Base
end end
def merge_commit_message(include_description: false) def merge_commit_message(include_description: false)
closes_issues_references = closes_issues.map do |issue| closes_issues_references = visible_closing_issues_for.map do |issue|
issue.to_reference(target_project) issue.to_reference(target_project)
end end
......
...@@ -206,7 +206,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated ...@@ -206,7 +206,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
end end
def closing_issues def closing_issues
@closing_issues ||= closes_issues(current_user) @closing_issues ||= visible_closing_issues_for(current_user)
end end
def pipeline def pipeline
......
...@@ -25,7 +25,7 @@ module MergeRequests ...@@ -25,7 +25,7 @@ module MergeRequests
def close_issues(merge_request) def close_issues(merge_request)
return unless merge_request.target_branch == project.default_branch return unless merge_request.target_branch == project.default_branch
closed_issues = merge_request.closes_issues(current_user) closed_issues = merge_request.visible_closing_issues_for(current_user)
closed_issues.each do |issue| closed_issues.each do |issue|
if can?(current_user, :update_issue, issue) if can?(current_user, :update_issue, issue)
......
...@@ -14,6 +14,7 @@ module MergeRequests ...@@ -14,6 +14,7 @@ module MergeRequests
merge_request.mark_as_unchecked merge_request.mark_as_unchecked
invalidate_cache_counts(merge_request, users: merge_request.assignees) invalidate_cache_counts(merge_request, users: merge_request.assignees)
merge_request.update_project_counter_caches merge_request.update_project_counter_caches
merge_request.cache_merge_request_closes_issues!(current_user)
end end
merge_request merge_request
......
---
title: Retrieve merge request closing issues from database cache
merge_request: 20911
author:
type: fixed
...@@ -380,7 +380,7 @@ module API ...@@ -380,7 +380,7 @@ module API
end end
get ':id/merge_requests/:merge_request_iid/closes_issues' do get ':id/merge_requests/:merge_request_iid/closes_issues' do
merge_request = find_merge_request_with_access(params[:merge_request_iid]) merge_request = find_merge_request_with_access(params[:merge_request_iid])
issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user)) issues = ::Kaminari.paginate_array(merge_request.visible_closing_issues_for(current_user))
issues = paginate(issues) issues = paginate(issues)
external_issues, internal_issues = issues.partition { |issue| issue.is_a?(ExternalIssue) } external_issues, internal_issues = issues.partition { |issue| issue.is_a?(ExternalIssue) }
......
...@@ -100,6 +100,10 @@ FactoryBot.define do ...@@ -100,6 +100,10 @@ FactoryBot.define do
end end
end end
after(:create) do |merge_request, evaluator|
merge_request.cache_merge_request_closes_issues!
end
factory :merged_merge_request, traits: [:merged] factory :merged_merge_request, traits: [:merged]
factory :closed_merge_request, traits: [:closed] factory :closed_merge_request, traits: [:closed]
factory :reopened_merge_request, traits: [:opened] factory :reopened_merge_request, traits: [:opened]
......
...@@ -87,6 +87,7 @@ merge_requests: ...@@ -87,6 +87,7 @@ merge_requests:
- merge_request_diff - merge_request_diff
- events - events
- merge_requests_closing_issues - merge_requests_closing_issues
- cached_closes_issues
- metrics - metrics
- timelogs - timelogs
- head_pipeline - head_pipeline
......
...@@ -310,6 +310,51 @@ describe MergeRequest do ...@@ -310,6 +310,51 @@ describe MergeRequest do
end end
end end
describe '#visible_closing_issues_for' do
let(:guest) { create(:user) }
let(:developer) { create(:user) }
let(:issue_1) { create(:issue, project: subject.source_project) }
let(:issue_2) { create(:issue, project: subject.source_project) }
let(:confidential_issue) { create(:issue, :confidential, project: subject.source_project) }
before do
subject.project.add_developer(subject.author)
subject.target_branch = subject.project.default_branch
commit = double('commit1', safe_message: "Fixes #{issue_1.to_reference} #{issue_2.to_reference} #{confidential_issue.to_reference}")
allow(subject).to receive(:commits).and_return([commit])
end
it 'shows only allowed issues to guest' do
subject.project.add_guest(guest)
subject.cache_merge_request_closes_issues!
expect(subject.visible_closing_issues_for(guest)).to match_array([issue_1, issue_2])
end
it 'shows only allowed issues to developer' do
subject.project.add_developer(developer)
subject.cache_merge_request_closes_issues!
expect(subject.visible_closing_issues_for(developer)).to match_array([issue_1, confidential_issue, issue_2])
end
context 'when external issue tracker is enabled' do
before do
subject.project.has_external_issue_tracker = true
subject.project.save!
end
it 'calls non #closes_issues to retrieve data' do
expect(subject).to receive(:closes_issues)
expect(subject).not_to receive(:cached_closes_issues)
subject.visible_closing_issues_for
end
end
end
describe '#cache_merge_request_closes_issues!' do describe '#cache_merge_request_closes_issues!' do
before do before do
subject.project.add_developer(subject.author) subject.project.add_developer(subject.author)
...@@ -324,6 +369,25 @@ describe MergeRequest do ...@@ -324,6 +369,25 @@ describe MergeRequest do
expect { subject.cache_merge_request_closes_issues!(subject.author) }.to change(subject.merge_requests_closing_issues, :count).by(1) expect { subject.cache_merge_request_closes_issues!(subject.author) }.to change(subject.merge_requests_closing_issues, :count).by(1)
end end
it 'does not cache closed issues when merge request is closed' do
issue = create :issue, project: subject.project
commit = double('commit1', safe_message: "Fixes #{issue.to_reference}")
allow(subject).to receive(:commits).and_return([commit])
allow(subject).to receive(:state).and_return("closed")
expect { subject.cache_merge_request_closes_issues!(subject.author) }.not_to change(subject.merge_requests_closing_issues, :count)
end
it 'does not cache closed issues when merge request is merged' do
issue = create :issue, project: subject.project
commit = double('commit1', safe_message: "Fixes #{issue.to_reference}")
allow(subject).to receive(:commits).and_return([commit])
allow(subject).to receive(:state).and_return("merged")
expect { subject.cache_merge_request_closes_issues!(subject.author) }.not_to change(subject.merge_requests_closing_issues, :count)
end
context 'when both internal and external issue trackers are enabled' do context 'when both internal and external issue trackers are enabled' do
before do before do
subject.project.has_external_issue_tracker = true subject.project.has_external_issue_tracker = true
...@@ -632,6 +696,7 @@ describe MergeRequest do ...@@ -632,6 +696,7 @@ describe MergeRequest do
allow(subject).to receive(:commits).and_return([commit]) allow(subject).to receive(:commits).and_return([commit])
allow(subject.project).to receive(:default_branch) allow(subject.project).to receive(:default_branch)
.and_return(subject.target_branch) .and_return(subject.target_branch)
subject.cache_merge_request_closes_issues!
expect(subject.issues_mentioned_but_not_closing(subject.author)).to match_array([mentioned_issue]) expect(subject.issues_mentioned_but_not_closing(subject.author)).to match_array([mentioned_issue])
end end
...@@ -649,6 +714,8 @@ describe MergeRequest do ...@@ -649,6 +714,8 @@ describe MergeRequest do
end end
it 'detects issues mentioned in description but not closed' do it 'detects issues mentioned in description but not closed' do
subject.cache_merge_request_closes_issues!
expect(subject.issues_mentioned_but_not_closing(subject.author).map(&:to_s)).to match_array(['TEST-2']) expect(subject.issues_mentioned_but_not_closing(subject.author).map(&:to_s)).to match_array(['TEST-2'])
end end
end end
...@@ -779,9 +846,8 @@ describe MergeRequest do ...@@ -779,9 +846,8 @@ describe MergeRequest do
subject.project.add_developer(subject.author) subject.project.add_developer(subject.author)
subject.description = "This issue Closes #{issue.to_reference}" subject.description = "This issue Closes #{issue.to_reference}"
allow(subject.project).to receive(:default_branch).and_return(subject.target_branch)
allow(subject.project).to receive(:default_branch) subject.cache_merge_request_closes_issues!
.and_return(subject.target_branch)
expect(subject.merge_commit_message) expect(subject.merge_commit_message)
.to match("Closes #{issue.to_reference}") .to match("Closes #{issue.to_reference}")
......
...@@ -117,9 +117,9 @@ describe MergeRequestPresenter do ...@@ -117,9 +117,9 @@ describe MergeRequestPresenter do
before do before do
project.add_developer(user) project.add_developer(user)
allow(resource.project).to receive(:default_branch) allow(resource.project).to receive(:default_branch)
.and_return(resource.target_branch) .and_return(resource.target_branch)
resource.cache_merge_request_closes_issues!
end end
describe '#closing_issues_links' do describe '#closing_issues_links' do
......
...@@ -954,6 +954,7 @@ describe API::MergeRequests do ...@@ -954,6 +954,7 @@ describe API::MergeRequests do
issue = create(:issue, project: project) issue = create(:issue, project: project)
mr = merge_request.tap do |mr| mr = merge_request.tap do |mr|
mr.update_attribute(:description, "Closes #{issue.to_reference(mr.project)}") mr.update_attribute(:description, "Closes #{issue.to_reference(mr.project)}")
mr.cache_merge_request_closes_issues!
end end
get api("/projects/#{project.id}/merge_requests/#{mr.iid}/closes_issues", user) get api("/projects/#{project.id}/merge_requests/#{mr.iid}/closes_issues", user)
......
...@@ -20,7 +20,7 @@ describe Issues::ReopenService do ...@@ -20,7 +20,7 @@ describe Issues::ReopenService do
end end
end end
context 'when user is authrized to reopen issue' do context 'when user is authorized to reopen issue' do
let(:user) { create(:user) } let(:user) { create(:user) }
before do before do
......
...@@ -49,6 +49,7 @@ describe MergeRequests::MergeService do ...@@ -49,6 +49,7 @@ describe MergeRequests::MergeService do
issue = create :issue, project: project issue = create :issue, project: project
commit = double('commit', safe_message: "Fixes #{issue.to_reference}") commit = double('commit', safe_message: "Fixes #{issue.to_reference}")
allow(merge_request).to receive(:commits).and_return([commit]) allow(merge_request).to receive(:commits).and_return([commit])
merge_request.cache_merge_request_closes_issues!
service.execute(merge_request) service.execute(merge_request)
......
...@@ -53,7 +53,7 @@ describe MergeRequests::PostMergeService do ...@@ -53,7 +53,7 @@ describe MergeRequests::PostMergeService do
allow(project).to receive(:default_branch).and_return('foo') allow(project).to receive(:default_branch).and_return('foo')
issue = create(:issue, project: project) issue = create(:issue, project: project)
allow(merge_request).to receive(:closes_issues).and_return([issue]) allow(merge_request).to receive(:visible_closing_issues_for).and_return([issue])
allow_any_instance_of(Issues::CloseService).to receive(:execute).with(issue, commit: merge_request).and_raise allow_any_instance_of(Issues::CloseService).to receive(:execute).with(issue, commit: merge_request).and_raise
expect { described_class.new(project, user, {}).execute(merge_request) }.to raise_error expect { described_class.new(project, user, {}).execute(merge_request) }.to raise_error
......
...@@ -47,6 +47,12 @@ describe MergeRequests::ReopenService do ...@@ -47,6 +47,12 @@ describe MergeRequests::ReopenService do
end end
end end
it 'caches merge request closing issues' do
expect(merge_request).to receive(:cache_merge_request_closes_issues!)
described_class.new(project, user, {}).execute(merge_request)
end
it 'updates metrics' do it 'updates metrics' do
metrics = merge_request.metrics metrics = merge_request.metrics
service = double(MergeRequestMetricsService) service = double(MergeRequestMetricsService)
......
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