Commit 9b58848a authored by Dylan Griffith's avatar Dylan Griffith

Fix performance issue with global search + refactor

A performance regression was introduced in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38095#note_412661217
. This reverts the relevant code that was necessarily passing through a
limited set of project IDs for Elasticsearch.

This also takes an extra refactoring step. Since we are passing project
IDs through to this method it no longer has the projects which was
necessary for the `#generic_search_results` method. But this was only
ever used for user search because users are not indexed in Elasticsearch
and as such cannot actually search using Elasticsearch. I decided it
kept things simpler to move this logic further up the calling stack so
we don't need the `Elastic::SearchResults` objects to be delegating back
again to the normal `SearchResults` objects. This is much simpler
because the calling code is already responsible for determining whether
or not Elasticsearch should be used anyway.
parent eb8418a8
......@@ -4,6 +4,7 @@ module Search
module Elasticsearchable
def use_elasticsearch?
return false if params[:basic_search]
return false if params[:scope] == 'users'
::Gitlab::CurrentSettings.search_using_elasticsearch?(scope: elasticsearchable_scope)
end
......
......@@ -14,7 +14,7 @@ module EE
::Gitlab::Elastic::SearchResults.new(
current_user,
params[:search],
projects,
elastic_projects,
public_and_internal_projects: elastic_global,
filters: { state: params[:state] }
)
......@@ -28,6 +28,25 @@ module EE
true
end
def elastic_projects
# For elasticsearch we need the list of projects to be as small as
# possible since they are loaded from the DB and sent in the
# Elasticsearch query. It should only be strictly the project IDs the
# user has been given authorization for. The Elasticsearch query will
# additionally take care of public projects. This behaves differently
# to the searching Postgres case in which this list of projects is
# intended to be all projects that should appear in the results.
strong_memoize(:elastic_projects) do
if current_user&.can_read_all_resources?
:any
elsif current_user
current_user.authorized_projects.pluck(:id) # rubocop: disable CodeReuse/ActiveRecord
else
[]
end
end
end
override :allowed_scopes
def allowed_scopes
return super unless use_elasticsearch?
......
......@@ -15,6 +15,11 @@ module EE
false
end
override :elastic_projects
def elastic_projects
@elastic_projects ||= projects.pluck_primary_key
end
override :execute
def execute
return super unless use_elasticsearch?
......@@ -22,7 +27,7 @@ module EE
::Gitlab::Elastic::GroupSearchResults.new(
current_user,
params[:search],
projects,
elastic_projects,
group: group,
public_and_internal_projects: elastic_global,
filters: { state: params[:state] }
......
---
title: Fix poor performance with global search across entire instance
merge_request: 42437
author:
type: performance
......@@ -6,27 +6,14 @@ module Gitlab
# superclass inside a module, because autoloading can occur in a
# different order between execution environments.
class GroupSearchResults < Gitlab::Elastic::SearchResults
delegate :users, to: :generic_search_results
delegate :limited_users_count, to: :generic_search_results
attr_reader :group, :default_project_filter, :filters
def initialize(current_user, query, limit_projects = nil, group:, public_and_internal_projects: false, default_project_filter: false, filters: {})
def initialize(current_user, query, limit_project_ids = nil, group:, public_and_internal_projects: false, default_project_filter: false, filters: {})
@group = group
@default_project_filter = default_project_filter
@filters = filters
super(current_user, query, limit_projects, public_and_internal_projects: public_and_internal_projects, filters: filters)
end
def generic_search_results
@generic_search_results ||= Gitlab::GroupSearchResults.new(
current_user,
query,
limit_projects,
group: group,
filters: filters
)
super(current_user, query, limit_project_ids, public_and_internal_projects: public_and_internal_projects, filters: filters)
end
end
end
......
......@@ -8,24 +8,11 @@ module Gitlab
class ProjectSearchResults < Gitlab::Elastic::SearchResults
attr_reader :project, :repository_ref, :filters
delegate :users, to: :generic_search_results
delegate :limited_users_count, to: :generic_search_results
def initialize(current_user, query, project:, repository_ref: nil, filters: {})
@project = project
@repository_ref = repository_ref.presence || project.default_branch
super(current_user, query, [project], public_and_internal_projects: false, filters: filters)
end
def generic_search_results
@generic_search_results ||= Gitlab::ProjectSearchResults.new(
current_user,
query,
project: project,
repository_ref: repository_ref,
filters: filters
)
super(current_user, query, [project.id], public_and_internal_projects: false, filters: filters)
end
private
......
......@@ -11,15 +11,12 @@ module Gitlab
# Limit search results by passed projects
# It allows us to search only for projects user has access to
attr_reader :limit_projects
attr_reader :limit_project_ids
delegate :users, to: :generic_search_results
delegate :limited_users_count, to: :generic_search_results
def initialize(current_user, query, limit_projects = nil, public_and_internal_projects: true, filters: {})
def initialize(current_user, query, limit_project_ids = nil, public_and_internal_projects: true, filters: {})
@current_user = current_user
@query = query
@limit_projects = limit_projects
@limit_project_ids = limit_project_ids
@public_and_internal_projects = public_and_internal_projects
@filters = filters
end
......@@ -44,17 +41,11 @@ module Gitlab
wiki_blobs(page: page, per_page: per_page)
when 'commits'
commits(page: page, per_page: per_page, preload_method: preload_method)
when 'users'
users.page(page).per(per_page)
else
Kaminari.paginate_array([])
end
end
def generic_search_results
@generic_search_results ||= Gitlab::SearchResults.new(current_user, query, limit_projects)
end
def formatted_count(scope)
case scope
when 'projects'
......@@ -73,8 +64,6 @@ module Gitlab
merge_requests_count.to_s
when 'milestones'
milestones_count.to_s
when 'users'
generic_search_results.formatted_count('users')
end
end
......@@ -176,20 +165,6 @@ module Gitlab
private
# Convert the `limit_projects` to a list of ids for Elasticsearch
def limit_project_ids
strong_memoize(:limit_project_ids) do
case limit_projects
when :any then :any
when ActiveRecord::Relation
limit_projects.pluck_primary_key if limit_projects.model == Project
when Array
limit_projects.all? { |x| x.is_a?(Project) } ? limit_projects.map(&:id) : []
else []
end
end
end
# Apply some eager loading to the `records` of an ES result object without
# losing pagination information. Also, take advantage of preload method if
# provided by the caller.
......
......@@ -9,7 +9,7 @@ RSpec.describe Gitlab::Elastic::GroupSearchResults, :elastic do
let(:filters) { {} }
let(:query) { '*' }
subject(:results) { described_class.new(user, query, Project.all, group: group, filters: filters) }
subject(:results) { described_class.new(user, query, Project.all.pluck_primary_key, group: group, filters: filters) }
before do
stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
......@@ -45,32 +45,6 @@ RSpec.describe Gitlab::Elastic::GroupSearchResults, :elastic do
end
end
context 'user search' do
let(:query) { guest.username }
before do
expect(Gitlab::GroupSearchResults).to receive(:new).and_call_original
end
it { expect(results.objects('users')).to contain_exactly(guest) }
it { expect(results.limited_users_count).to eq(1) }
describe 'pagination' do
let(:query) {}
let_it_be(:user2) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::REPORTER) } }
it 'returns the correct page of results' do
expect(results.objects('users', page: 1, per_page: 1)).to contain_exactly(user2)
expect(results.objects('users', page: 2, per_page: 1)).to contain_exactly(guest)
end
it 'returns the correct number of results for one page' do
expect(results.objects('users', page: 1, per_page: 2).count).to eq(2)
end
end
end
context 'query performance' do
include_examples 'does not hit Elasticsearch twice for objects and counts', %w|projects notes blobs wiki_blobs commits issues merge_requests milestones|
end
......
......@@ -167,33 +167,6 @@ RSpec.describe Gitlab::Elastic::ProjectSearchResults, :elastic do
end
end
context 'user search' do
let(:query) { project.owner.username }
before do
expect(Gitlab::ProjectSearchResults).to receive(:new).and_call_original
end
it { expect(results.objects('users')).to eq([project.owner]) }
it { expect(results.limited_users_count).to eq(1) }
describe 'pagination' do
let(:query) { }
let_it_be(:user2) { create(:user).tap { |u| project.add_user(u, Gitlab::Access::REPORTER) } }
it 'returns the correct page of results' do
# UsersFinder defaults to order_id_desc, the newer result will be first
expect(results.objects('users', page: 1, per_page: 1)).to eq([user2])
expect(results.objects('users', page: 2, per_page: 1)).to eq([project.owner])
end
it 'returns the correct number of results for one page' do
expect(results.objects('users', page: 1, per_page: 2).count).to eq(2)
end
end
end
context 'query performance' do
let(:project) { create(:project, :public, :repository, :wiki_repo) }
let(:query) { '*' }
......
......@@ -210,6 +210,41 @@ RSpec.describe Search::GlobalService do
end
end
describe '#elastic_projects' do
let_it_be(:group) { create(:group) }
let_it_be(:project) { create(:project, namespace: group) }
let_it_be(:another_project) { create(:project) }
let_it_be(:non_admin_user) { create_user_from_membership(project, :developer) }
let_it_be(:admin) { create(:admin) }
let(:service) { described_class.new(user, {}) }
let(:elastic_projects) { service.elastic_projects }
context 'when the user is an admin' do
let(:user) { admin }
it 'returns :any' do
expect(elastic_projects).to eq(:any)
end
end
context 'when the user is not an admin' do
let(:user) { non_admin_user }
it 'returns the projects the user has access to' do
expect(elastic_projects).to eq([project.id])
end
end
context 'when there is no user' do
let(:user) { nil }
it 'returns empty array' do
expect(elastic_projects).to eq([])
end
end
end
context 'confidential notes' do
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