From fdb5f285f924e653400f7bbe18d13718c469d74f Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 30 Jul 2018 15:52:42 -0300 Subject: [PATCH] Retrieve merge request closing issues from database cache --- app/models/merge_request.rb | 21 +++++- app/presenters/merge_request_presenter.rb | 2 +- .../merge_requests/post_merge_service.rb | 2 +- app/services/merge_requests/reopen_service.rb | 1 + changelogs/unreleased/issue_44821.yml | 5 ++ lib/api/merge_requests.rb | 2 +- spec/factories/merge_requests.rb | 4 ++ spec/lib/gitlab/import_export/all_models.yml | 1 + spec/models/merge_request_spec.rb | 72 ++++++++++++++++++- .../merge_request_presenter_spec.rb | 2 +- spec/requests/api/merge_requests_spec.rb | 1 + spec/services/issues/reopen_service_spec.rb | 2 +- .../merge_requests/merge_service_spec.rb | 1 + .../merge_requests/post_merge_service_spec.rb | 2 +- .../merge_requests/reopen_service_spec.rb | 6 ++ 15 files changed, 112 insertions(+), 12 deletions(-) create mode 100644 changelogs/unreleased/issue_44821.yml diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b4090fd8baf..e390ab8ddf9 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -52,6 +52,8 @@ class MergeRequest < ActiveRecord::Base class_name: 'MergeRequestsClosingIssues', 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" serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize @@ -755,8 +757,9 @@ class MergeRequest < ActiveRecord::Base # Calculating this information for a number of merge requests requires # running `ReferenceExtractor` on each of them separately. # 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 if closed? || merged? transaction do self.merge_requests_closing_issues.delete_all @@ -769,6 +772,18 @@ class MergeRequest < ActiveRecord::Base 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. def closes_issues(current_user = self.author) if target_branch == project.default_branch @@ -788,7 +803,7 @@ class MergeRequest < ActiveRecord::Base ext = Gitlab::ReferenceExtractor.new(project, current_user) ext.analyze("#{title}\n#{description}") - ext.issues - closes_issues(current_user) + ext.issues - visible_closing_issues_for(current_user) end def target_project_path @@ -836,7 +851,7 @@ class MergeRequest < ActiveRecord::Base end 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) end diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index ffa238a63d5..8c4eac3c31d 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -206,7 +206,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end def closing_issues - @closing_issues ||= closes_issues(current_user) + @closing_issues ||= visible_closing_issues_for(current_user) end def pipeline diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index 3d2aea4e9b6..f26e3bee06f 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -25,7 +25,7 @@ module MergeRequests def close_issues(merge_request) 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| if can?(current_user, :update_issue, issue) diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb index f2fc13ad028..f6cbe769ef4 100644 --- a/app/services/merge_requests/reopen_service.rb +++ b/app/services/merge_requests/reopen_service.rb @@ -14,6 +14,7 @@ module MergeRequests merge_request.mark_as_unchecked invalidate_cache_counts(merge_request, users: merge_request.assignees) merge_request.update_project_counter_caches + merge_request.cache_merge_request_closes_issues!(current_user) end merge_request diff --git a/changelogs/unreleased/issue_44821.yml b/changelogs/unreleased/issue_44821.yml new file mode 100644 index 00000000000..b1807e069af --- /dev/null +++ b/changelogs/unreleased/issue_44821.yml @@ -0,0 +1,5 @@ +--- +title: Retrieve merge request closing issues from database cache +merge_request: 20911 +author: +type: fixed diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 2621c9f8fc2..abad418771c 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -380,7 +380,7 @@ module API end get ':id/merge_requests/:merge_request_iid/closes_issues' do 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) external_issues, internal_issues = issues.partition { |issue| issue.is_a?(ExternalIssue) } diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index f722bb9cb0d..f3bea17385b 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -100,6 +100,10 @@ FactoryBot.define do end end + after(:create) do |merge_request, evaluator| + merge_request.cache_merge_request_closes_issues! + end + factory :merged_merge_request, traits: [:merged] factory :closed_merge_request, traits: [:closed] factory :reopened_merge_request, traits: [:opened] diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index db5aab0cd76..375a93b28a6 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -87,6 +87,7 @@ merge_requests: - merge_request_diff - events - merge_requests_closing_issues +- cached_closes_issues - metrics - timelogs - head_pipeline diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index b0d9d03bf6c..5904f575f74 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -310,6 +310,51 @@ describe MergeRequest do 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 before do subject.project.add_developer(subject.author) @@ -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) 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 before do subject.project.has_external_issue_tracker = true @@ -632,6 +696,7 @@ describe MergeRequest do allow(subject).to receive(:commits).and_return([commit]) allow(subject.project).to receive(:default_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]) end @@ -649,6 +714,8 @@ describe MergeRequest do end 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']) end end @@ -779,9 +846,8 @@ describe MergeRequest do subject.project.add_developer(subject.author) 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).and_return(subject.target_branch) + subject.cache_merge_request_closes_issues! expect(subject.merge_commit_message) .to match("Closes #{issue.to_reference}") diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb index 46ba6f442f5..a1b52d8692d 100644 --- a/spec/presenters/merge_request_presenter_spec.rb +++ b/spec/presenters/merge_request_presenter_spec.rb @@ -117,9 +117,9 @@ describe MergeRequestPresenter do before do project.add_developer(user) - allow(resource.project).to receive(:default_branch) .and_return(resource.target_branch) + resource.cache_merge_request_closes_issues! end describe '#closing_issues_links' do diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 1716d182782..4de834bf93a 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -954,6 +954,7 @@ describe API::MergeRequests do issue = create(:issue, project: project) mr = merge_request.tap do |mr| mr.update_attribute(:description, "Closes #{issue.to_reference(mr.project)}") + mr.cache_merge_request_closes_issues! end get api("/projects/#{project.id}/merge_requests/#{mr.iid}/closes_issues", user) diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb index 3b617d4ca54..2a56075419b 100644 --- a/spec/services/issues/reopen_service_spec.rb +++ b/spec/services/issues/reopen_service_spec.rb @@ -20,7 +20,7 @@ describe Issues::ReopenService do end end - context 'when user is authrized to reopen issue' do + context 'when user is authorized to reopen issue' do let(:user) { create(:user) } before do diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 9dd235f6660..5d96b5ce27c 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -49,6 +49,7 @@ describe MergeRequests::MergeService do issue = create :issue, project: project commit = double('commit', safe_message: "Fixes #{issue.to_reference}") allow(merge_request).to receive(:commits).and_return([commit]) + merge_request.cache_merge_request_closes_issues! service.execute(merge_request) diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index ba2b062875b..5ad6f5528f9 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -53,7 +53,7 @@ describe MergeRequests::PostMergeService do allow(project).to receive(:default_branch).and_return('foo') 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 expect { described_class.new(project, user, {}).execute(merge_request) }.to raise_error diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb index e10eaa95da4..21e71509ed6 100644 --- a/spec/services/merge_requests/reopen_service_spec.rb +++ b/spec/services/merge_requests/reopen_service_spec.rb @@ -47,6 +47,12 @@ describe MergeRequests::ReopenService do 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 metrics = merge_request.metrics service = double(MergeRequestMetricsService)