Commit 1aca5fe9 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin

Optimize read merge request permission check for references

Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/271242

**Problem**

'#can_read_reference?' is an expensive call especially when we trigger
it thousands of times.

**Solution**

`:read_merge_request_iid` depends on the user and the project. This
allows us to cache the user-project pair result and apply it to all
merge requests of the same project.

Changelog: performance
parent 77a544a4
---
title: Optimize merge request permission check for references
merge_request: 61591
author:
type: performance
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
module Banzai module Banzai
module ReferenceParser module ReferenceParser
class MergeRequestParser < IssuableParser class MergeRequestParser < IssuableParser
include Gitlab::Utils::StrongMemoize
self.reference_type = :merge_request self.reference_type = :merge_request
def records_for_nodes(nodes) def records_for_nodes(nodes)
...@@ -27,6 +29,16 @@ module Banzai ...@@ -27,6 +29,16 @@ module Banzai
self.class.data_attribute self.class.data_attribute
) )
end end
def can_read_reference?(user, merge_request)
memo = strong_memoize(:can_read_reference) { {} }
project_id = merge_request.project_id
return memo[project_id] if memo.key?(project_id)
memo[project_id] = can?(user, :read_merge_request_iid, merge_request.project)
end
end end
end end
end end
...@@ -8,7 +8,7 @@ RSpec.describe Banzai::ReferenceParser::MergeRequestParser do ...@@ -8,7 +8,7 @@ RSpec.describe Banzai::ReferenceParser::MergeRequestParser do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project, :public) } let(:project) { create(:project, :public) }
let(:merge_request) { create(:merge_request, source_project: project) } let(:merge_request) { create(:merge_request, source_project: project) }
subject { described_class.new(Banzai::RenderContext.new(merge_request.target_project, user)) } subject(:parser) { described_class.new(Banzai::RenderContext.new(merge_request.target_project, user)) }
let(:link) { empty_html_link } let(:link) { empty_html_link }
...@@ -65,4 +65,49 @@ RSpec.describe Banzai::ReferenceParser::MergeRequestParser do ...@@ -65,4 +65,49 @@ RSpec.describe Banzai::ReferenceParser::MergeRequestParser do
it_behaves_like 'no N+1 queries' it_behaves_like 'no N+1 queries'
end end
describe '#can_read_reference?' do
subject { parser.can_read_reference?(user, merge_request) }
it { is_expected.to be_truthy }
context 'when merge request belongs to the private project' do
let(:project) { create(:project, :private) }
it 'prevents user from reading merge request references' do
is_expected.to be_falsey
end
context 'when user has access to the project' do
before do
project.add_developer(user)
end
it { is_expected.to be_truthy }
end
end
context 'with memoization' do
context 'when project is the same' do
it 'calls #can? only once' do
expect(parser).to receive(:can?).once
2.times { parser.can_read_reference?(user, merge_request) }
end
end
context 'when merge requests belong to different projects' do
it 'calls #can? for each project' do
expect(parser).to receive(:can?).twice
another_merge_request = create(:merge_request)
2.times do
parser.can_read_reference?(user, merge_request)
parser.can_read_reference?(user, another_merge_request)
end
end
end
end
end
end end
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