Merge branch '249515-global-search-admin-should-use-any' into 'master'

Fix performance issue with global search + refactor

Closes #249515

See merge request gitlab-org/gitlab!42437

ee/app/services/concerns/search/elasticsearchable.rb

@@ -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

ee/app/services/ee/search/global_service.rb

@@ -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?

ee/app/services/ee/search/group_service.rb

@@ -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] }

ee/changelogs/unreleased/249515-global-search-admin-should-use-any.yml

+---
+title: Fix poor performance with global search across entire instance
+merge_request: 42437
+author:
+type: performance

ee/lib/gitlab/elastic/group_search_results.rb

@@ -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

ee/lib/gitlab/elastic/project_search_results.rb

@@ -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

ee/lib/gitlab/elastic/search_results.rb

@@ -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.

ee/spec/lib/gitlab/elastic/group_search_results_spec.rb

@@ -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

ee/spec/lib/gitlab/elastic/project_search_results_spec.rb

@@ -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) { '*' }

ee/spec/services/search/global_service_spec.rb

@@ -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) }