Commit d4a9126c authored by Tan Le's avatar Tan Le

Preload project associations on web search results

This is to mirror the preloads logic on Projects::Dashboard
controller. The dashboard and search view both share the same project
view partial (i.e. `_project.html.haml`) and require similar project
data for presentation.

The N+1 test was failing to pick up this N+1 issue as it uses the same
associations for project test data and these queries are cached. This
change ensures that we skip cached queries and count everything.
parent 59122560
......@@ -5,6 +5,10 @@ class SearchController < ApplicationController
include SearchHelper
include RendersCommits
SCOPE_PRELOAD_METHOD = {
projects: :with_web_entity_associations
}.freeze
around_action :allow_gitaly_ref_name_caching
skip_before_action :authenticate_user!
......@@ -28,7 +32,7 @@ class SearchController < ApplicationController
@scope = search_service.scope
@show_snippets = search_service.show_snippets?
@search_results = search_service.search_results
@search_objects = search_service.search_objects
@search_objects = search_service.search_objects(preload_method)
render_commits if @scope == 'commits'
eager_load_user_status if @scope == 'users'
......@@ -64,6 +68,10 @@ class SearchController < ApplicationController
private
def preload_method
SCOPE_PRELOAD_METHOD[@scope.to_sym]
end
def search_term_valid?
unless search_service.valid_query_length?
flash[:alert] = t('errors.messages.search_chars_too_long', count: SearchService::SEARCH_CHAR_LIMIT)
......
......@@ -508,6 +508,7 @@ class Project < ApplicationRecord
left_outer_joins(:pages_metadatum)
.where(project_pages_metadata: { project_id: nil })
end
scope :with_api_entity_associations, -> {
preload(:project_feature, :route, :tags,
group: :ip_restrictions, namespace: [:route, :owner])
......@@ -527,6 +528,10 @@ class Project < ApplicationRecord
# Used by Projects::CleanupService to hold a map of rewritten object IDs
mount_uploader :bfg_object_map, AttachmentUploader
def self.with_web_entity_associations
preload(:project_feature, :route, :creator, :group, namespace: [:route, :owner])
end
def self.eager_load_namespace_and_owner
includes(namespace: :owner)
end
......
......@@ -192,6 +192,8 @@ module EE
end
class_methods do
extend ::Gitlab::Utils::Override
def search_by_visibility(level)
where(visibility_level: ::Gitlab::VisibilityLevel.string_options[level])
end
......@@ -206,6 +208,11 @@ module EE
# see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24063#note_282435524 for details
joins(:service_desk_setting).find_by('service_desk_settings.project_key' => key)
end
override :with_web_entity_associations
def with_web_entity_associations
super.preload(:compliance_framework_setting)
end
end
def can_store_security_reports?
......
---
title: Fix N+1 queries for Elastic Web Search projects scope
merge_request: 30346
author:
type: performance
......@@ -29,7 +29,7 @@ module Gitlab
case scope
when 'projects'
eager_load(projects, page, per_page, preload_method, [:route, :namespace, :compliance_framework_setting])
eager_load(projects, page, per_page, preload_method, [:route, :namespace])
when 'issues'
eager_load(issues, page, per_page, preload_method, project: [:route, :namespace], labels: [], timelogs: [], assignees: [])
when 'merge_requests'
......
......@@ -5,7 +5,7 @@ require 'spec_helper'
describe SearchController, type: :request do
let_it_be(:user) { create(:user) }
let_it_be(:group) { create(:group) }
let(:project) { create(:project, :public, :repository, :wiki_repo, name: 'awesome project', group: group) }
let_it_be(:project) { create(:project, :public, :repository, :wiki_repo, name: 'awesome project', group: group) }
before_all do
login_as(user)
......@@ -20,6 +20,7 @@ describe SearchController, type: :request do
before do
stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
end
context 'for merge_request scope' do
before do
create(:merge_request, target_branch: 'feature_1', source_project: project)
......@@ -28,6 +29,7 @@ describe SearchController, type: :request do
create(:merge_request, target_branch: 'feature_4', source_project: project)
ensure_elasticsearch_index!
end
it 'avoids N+1 queries' do
control = ActiveRecord::QueryRecorder.new(skip_cached: false) { send_search_request(scope: 'merge_requests', search: '*') }
......@@ -40,7 +42,30 @@ describe SearchController, type: :request do
# some N+1 queries still exist
expect { send_search_request(scope: 'merge_requests', search: '*') }
.not_to exceed_all_query_limit(control.count + 2)
.not_to exceed_all_query_limit(control)
end
end
context 'for project scope' do
before do
create(:project, :public)
ensure_elasticsearch_index!
end
it 'avoids N+1 queries' do
control = ActiveRecord::QueryRecorder.new(skip_cached: false) { send_search_request(scope: 'project', search: '*') }
create_list(:project, 3, :public)
ensure_elasticsearch_index!
# some N+1 queries still exist
# each project requires 3 extra queries
# - one count for forks
# - one count for open MRs
# - one count for open Issues
expect { send_search_request(scope: 'project', search: '*') }
.not_to exceed_all_query_limit(control).with_threshold(9)
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