diff --git a/config/feature_flags/development/optimize_merge_request_parser.yml b/config/feature_flags/development/optimize_merge_request_parser.yml
new file mode 100644
index 0000000000000000000000000000000000000000..9e65f5412c445631c8e297d6f874cec5886ba05f
--- /dev/null
+++ b/config/feature_flags/development/optimize_merge_request_parser.yml
@@ -0,0 +1,8 @@
+---
+name: optimize_merge_request_parser
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/62490/
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/331893
+milestone: '14.0'
+type: development
+group: group::source code
+default_enabled: false
diff --git a/lib/banzai/reference_parser/merge_request_parser.rb b/lib/banzai/reference_parser/merge_request_parser.rb
index 78cbf4a807d54d00cb0d95aa2bc9152c82f6da12..1664fa1f9ff75d9e98c5f92559b5e1791fccaaa4 100644
--- a/lib/banzai/reference_parser/merge_request_parser.rb
+++ b/lib/banzai/reference_parser/merge_request_parser.rb
@@ -7,6 +7,19 @@ class MergeRequestParser < IssuableParser
self.reference_type = :merge_request
+ def nodes_visible_to_user(user, nodes)
+ return super if Feature.disabled?(:optimize_merge_request_parser, user, default_enabled: :yaml)
+
+ merge_request_nodes = nodes.select { |node| node.has_attribute?(self.class.data_attribute) }
+ records = projects_for_nodes(merge_request_nodes)
+
+ merge_request_nodes.select do |node|
+ project = records[node]
+
+ project && can_read_reference?(user, project)
+ end
+ end
+
def records_for_nodes(nodes)
@merge_requests_for_nodes ||= grouped_objects_for_nodes(
nodes,
@@ -23,14 +36,19 @@ def records_for_nodes(nodes)
)
end
- def can_read_reference?(user, merge_request)
+ def can_read_reference?(user, object)
memo = strong_memoize(:can_read_reference) { {} }
- project_id = merge_request.project_id
+ project_id = object.project_id
return memo[project_id] if memo.key?(project_id)
- memo[project_id] = can?(user, :read_merge_request_iid, merge_request.project)
+ memo[project_id] = can?(user, :read_merge_request_iid, object)
+ end
+
+ def projects_for_nodes(nodes)
+ @projects_for_nodes ||=
+ grouped_objects_for_nodes(nodes, Project.includes(:project_feature, :group, :namespace), 'data-project')
end
end
end
diff --git a/spec/lib/banzai/reference_parser/commit_parser_spec.rb b/spec/lib/banzai/reference_parser/commit_parser_spec.rb
index 612ce6b93f1f51ecdbeff1439a26f15dcac42a2a..31cece108bf78d9a3c712ead6d643ced3d1e91cb 100644
--- a/spec/lib/banzai/reference_parser/commit_parser_spec.rb
+++ b/spec/lib/banzai/reference_parser/commit_parser_spec.rb
@@ -130,11 +130,11 @@
end
context 'when checking commits on another projects' do
- let(:control_links) do
+ let!(:control_links) do
[commit_link]
end
- let(:actual_links) do
+ let!(:actual_links) do
control_links + [commit_link, commit_link]
end
diff --git a/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb b/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb
index a57e4e52501d632fd9c0fb15a0bedbd02677f8b7..04c35c8b0829d702099eb8835522a555c0131bac 100644
--- a/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb
+++ b/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb
@@ -18,10 +18,19 @@
context 'when the link has a data-issue attribute' do
before do
project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
+ link['data-project'] = merge_request.project_id.to_s
link['data-merge-request'] = merge_request.id.to_s
end
it_behaves_like "referenced feature visibility", "merge_requests"
+
+ context 'when optimize_merge_request_parser feature flag is off' do
+ before do
+ stub_feature_flags(optimize_merge_request_parser: false)
+ end
+
+ it_behaves_like "referenced feature visibility", "merge_requests"
+ end
end
end
@@ -29,6 +38,7 @@
describe 'when the link has a data-merge-request attribute' do
context 'using an existing merge request ID' do
it 'returns an Array of merge requests' do
+ link['data-project'] = merge_request.project_id.to_s
link['data-merge-request'] = merge_request.id.to_s
expect(subject.referenced_by([link])).to eq([merge_request])
@@ -37,6 +47,7 @@
context 'using a non-existing merge request ID' do
it 'returns an empty Array' do
+ link['data-project'] = merge_request.project_id.to_s
link['data-merge-request'] = ''
expect(subject.referenced_by([link])).to eq([])
@@ -49,16 +60,16 @@
let(:other_project) { create(:project, :public) }
let(:other_merge_request) { create(:merge_request, source_project: other_project) }
- let(:control_links) do
+ let!(:control_links) do
[merge_request_link(other_merge_request)]
end
- let(:actual_links) do
+ let!(:actual_links) do
control_links + [merge_request_link(create(:merge_request, :conflict, source_project: other_project))]
end
def merge_request_link(merge_request)
- Nokogiri::HTML.fragment(%Q{}).children[0]
+ Nokogiri::HTML.fragment(%Q{}).children[0]
end
before do
diff --git a/spec/support/helpers/reference_parser_helpers.rb b/spec/support/helpers/reference_parser_helpers.rb
index f42410d8102f1f8b7bd7184b40ca45a77d8dcf72..a6a7948d9d95bdf38c08c39e4790d26b3bc3cbb0 100644
--- a/spec/support/helpers/reference_parser_helpers.rb
+++ b/spec/support/helpers/reference_parser_helpers.rb
@@ -11,25 +11,20 @@ def expect_gathered_references(result, visible, not_visible_count)
end
RSpec.shared_examples 'no project N+1 queries' do
- it 'avoids N+1 queries in #nodes_visible_to_user', :request_store do
+ it 'avoids N+1 queries in #nodes_visible_to_user' do
context = Banzai::RenderContext.new(project, user)
- record_queries = lambda do |links|
- ActiveRecord::QueryRecorder.new do
- described_class.new(context).nodes_visible_to_user(user, links)
- end
+ request = lambda do |links|
+ described_class.new(context).nodes_visible_to_user(user, links)
end
- control = record_queries.call(control_links)
+ control = ActiveRecord::QueryRecorder.new { request.call(control_links) }
create(:group_member, group: project.group) if project.group
create(:project_member, project: project)
create(:project_group_link, project: project)
- actual = record_queries.call(actual_links)
-
- expect(actual.count).to be <= control.count
- expect(actual.cached_count).to be <= control.cached_count
+ expect { request.call(actual_links) }.not_to exceed_query_limit(control)
end
end