Commit 6b2b89f3 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'osw-fix-n-plus-1-for-mrs-without-merge-info' into 'master'

Avoid N+1 on MRs page when metrics merging date cannot be found

Closes #47613

See merge request gitlab-org/gitlab-ce!21053
parents c8c38f94 49dc8215
...@@ -1089,23 +1089,29 @@ class MergeRequest < ActiveRecord::Base ...@@ -1089,23 +1089,29 @@ class MergeRequest < ActiveRecord::Base
def can_be_reverted?(current_user) def can_be_reverted?(current_user)
return false unless merge_commit return false unless merge_commit
return false unless merged_at
merged_at = metrics&.merged_at # It is not guaranteed that Note#created_at will be strictly later than
notes_association = notes_with_associations # MergeRequestMetric#merged_at. Nanoseconds on MySQL may break this
# comparison, as will a HA environment if clocks are not *precisely*
# synchronized. Add a minute's leeway to compensate for both possibilities
cutoff = merged_at - 1.minute
if merged_at notes_association = notes_with_associations.where('created_at >= ?', cutoff)
# It is not guaranteed that Note#created_at will be strictly later than
# MergeRequestMetric#merged_at. Nanoseconds on MySQL may break this
# comparison, as will a HA environment if clocks are not *precisely*
# synchronized. Add a minute's leeway to compensate for both possibilities
cutoff = merged_at - 1.minute
notes_association = notes_association.where('created_at >= ?', cutoff)
end
!merge_commit.has_been_reverted?(current_user, notes_association) !merge_commit.has_been_reverted?(current_user, notes_association)
end end
def merged_at
strong_memoize(:merged_at) do
next unless merged?
metrics&.merged_at ||
merge_event&.created_at ||
notes.system.reorder(nil).find_by(note: 'merged')&.created_at
end
end
def can_be_cherry_picked? def can_be_cherry_picked?
merge_commit.present? merge_commit.present?
end end
......
---
title: Avoid N+1 on MRs page when metrics merging date cannot be found
merge_request: 21053
author:
type: performance
...@@ -1354,6 +1354,16 @@ describe MergeRequest do ...@@ -1354,6 +1354,16 @@ describe MergeRequest do
project.default_branch == branch) project.default_branch == branch)
end end
context 'but merged at timestamp cannot be found' do
before do
allow(subject).to receive(:merged_at) { nil }
end
it 'returns false' do
expect(subject.can_be_reverted?(current_user)).to be_falsey
end
end
context 'when the revert commit is mentioned in a note after the MR was merged' do context 'when the revert commit is mentioned in a note after the MR was merged' do
it 'returns false' do it 'returns false' do
expect(subject.can_be_reverted?(current_user)).to be_falsey expect(subject.can_be_reverted?(current_user)).to be_falsey
...@@ -1393,6 +1403,63 @@ describe MergeRequest do ...@@ -1393,6 +1403,63 @@ describe MergeRequest do
end end
end end
describe '#merged_at' do
context 'when MR is not merged' do
let(:merge_request) { create(:merge_request, :closed) }
it 'returns nil' do
expect(merge_request.merged_at).to be_nil
end
end
context 'when metrics has merged_at data' do
let(:merge_request) { create(:merge_request, :merged) }
before do
merge_request.metrics.update!(merged_at: 1.day.ago)
end
it 'returns metrics merged_at' do
expect(merge_request.merged_at).to eq(merge_request.metrics.merged_at)
end
end
context 'when merged event is persisted, but no metrics merged_at is persisted' do
let(:user) { create(:user) }
let(:merge_request) { create(:merge_request, :merged) }
before do
EventCreateService.new.merge_mr(merge_request, user)
end
it 'returns merged event creation date' do
expect(merge_request.merge_event).to be_persisted
expect(merge_request.merged_at).to eq(merge_request.merge_event.created_at)
end
end
context 'when merging note is persisted, but no metrics or merge event exists' do
let(:user) { create(:user) }
let(:merge_request) { create(:merge_request, :merged) }
before do
merge_request.metrics.destroy!
SystemNoteService.change_status(merge_request,
merge_request.target_project,
user,
merge_request.state, nil)
end
it 'returns merging note creation date' do
expect(merge_request.reload.metrics).to be_nil
expect(merge_request.merge_event).to be_nil
expect(merge_request.notes.count).to eq(1)
expect(merge_request.merged_at).to eq(merge_request.notes.first.created_at)
end
end
end
describe '#participants' do describe '#participants' do
let(:project) { create(:project, :public) } let(:project) { create(:project, :public) }
......
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