From 42b0158b1ce60d69f40a877f1a14ad6437794b71 Mon Sep 17 00:00:00 2001
From: Dmitry Gruzd <dgruzd@gitlab.com>
Date: Sat, 18 Jul 2020 00:43:48 +0000
Subject: [PATCH] Resolve "Search API merge_requests scope preload project
features"
---
app/models/merge_request.rb | 19 +++++++------
ee/app/models/merge_train.rb | 4 +--
.../unreleased/219309-fix-mr-n-plus-1.yml | 5 ++++
spec/services/search_service_spec.rb | 27 +++++++++++++++++++
4 files changed, 43 insertions(+), 12 deletions(-)
create mode 100644 ee/changelogs/unreleased/219309-fix-mr-n-plus-1.yml
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index b788577178180..cf7c40b97d8f8 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -251,17 +251,12 @@ def public_merge_status
end
scope :join_project, -> { joins(:target_project) }
scope :references_project, -> { references(:target_project) }
-
- PROJECT_ROUTE_AND_NAMESPACE_ROUTE = [
- target_project: [:route, { namespace: :route }],
- source_project: [:route, { namespace: :route }]
- ].freeze
-
scope :with_api_entity_associations, -> {
- preload(:assignees, :author, :unresolved_notes, :labels, :milestone,
- :timelogs, :latest_merge_request_diff,
- *PROJECT_ROUTE_AND_NAMESPACE_ROUTE,
- metrics: [:latest_closed_by, :merged_by])
+ preload_routables
+ .preload(:assignees, :author, :unresolved_notes, :labels, :milestone,
+ :timelogs, :latest_merge_request_diff,
+ target_project: :project_feature,
+ metrics: [:latest_closed_by, :merged_by])
}
scope :by_target_branch_wildcard, ->(wildcard_branch_name) do
@@ -269,6 +264,10 @@ def public_merge_status
end
scope :by_target_branch, ->(branch_name) { where(target_branch: branch_name) }
scope :preload_source_project, -> { preload(:source_project) }
+ scope :preload_routables, -> do
+ preload(target_project: [:route, { namespace: :route }],
+ source_project: [:route, { namespace: :route }])
+ end
scope :with_auto_merge_enabled, -> do
with_state(:opened).where(auto_merge_enabled: true)
diff --git a/ee/app/models/merge_train.rb b/ee/app/models/merge_train.rb
index 8372742bda742..68b8eb139ebe2 100644
--- a/ee/app/models/merge_train.rb
+++ b/ee/app/models/merge_train.rb
@@ -71,8 +71,8 @@ class MergeTrain < ApplicationRecord
scope :by_id, -> (sort = :asc) { order(id: sort) }
scope :preload_api_entities, -> do
- preload(:user, merge_request: MergeRequest::PROJECT_ROUTE_AND_NAMESPACE_ROUTE,
- pipeline: Ci::Pipeline::PROJECT_ROUTE_AND_NAMESPACE_ROUTE)
+ preload(:user, :merge_request, pipeline: Ci::Pipeline::PROJECT_ROUTE_AND_NAMESPACE_ROUTE)
+ .merge(MergeRequest.preload_routables)
end
class << self
diff --git a/ee/changelogs/unreleased/219309-fix-mr-n-plus-1.yml b/ee/changelogs/unreleased/219309-fix-mr-n-plus-1.yml
new file mode 100644
index 0000000000000..4ecdd46cca905
--- /dev/null
+++ b/ee/changelogs/unreleased/219309-fix-mr-n-plus-1.yml
@@ -0,0 +1,5 @@
+---
+title: Avoid N+1 of merge requests associations in Search
+merge_request: 36712
+author:
+type: performance
diff --git a/spec/services/search_service_spec.rb b/spec/services/search_service_spec.rb
index 52aef73ac77ed..f6bb7acee5728 100644
--- a/spec/services/search_service_spec.rb
+++ b/spec/services/search_service_spec.rb
@@ -374,6 +374,19 @@
subject(:result) { search_service.search_objects }
+ shared_examples "redaction limits N+1 queries" do |limit:|
+ it 'does not exceed the query limit' do
+ # issuing the query to remove the data loading call
+ unredacted_results.to_a
+
+ # only the calls from the redaction are left
+ query = ActiveRecord::QueryRecorder.new { result }
+
+ # these are the project authorization calls, which are not preloaded
+ expect(query.count).to be <= limit
+ end
+ end
+
def found_blob(project)
Gitlab::Search::FoundBlob.new(project: project)
end
@@ -427,6 +440,12 @@ def kaminari_array(*objects)
it 'redacts the inaccessible merge request' do
expect(result).to contain_exactly(readable)
end
+
+ context 'with :with_api_entity_associations' do
+ let(:unredacted_results) { ar_relation(MergeRequest.with_api_entity_associations, readable, unreadable) }
+
+ it_behaves_like "redaction limits N+1 queries", limit: 7
+ end
end
context 'project repository blobs' do
@@ -460,6 +479,10 @@ def kaminari_array(*objects)
it 'redacts the inaccessible snippet' do
expect(result).to contain_exactly(readable)
end
+
+ context 'with :with_api_entity_associations' do
+ it_behaves_like "redaction limits N+1 queries", limit: 12
+ end
end
context 'personal snippets' do
@@ -471,6 +494,10 @@ def kaminari_array(*objects)
it 'redacts the inaccessible snippet' do
expect(result).to contain_exactly(readable)
end
+
+ context 'with :with_api_entity_associations' do
+ it_behaves_like "redaction limits N+1 queries", limit: 3
+ end
end
context 'commits' do
--
GitLab