Merge branch '225998-advanced-search-result-page-is-slow' into 'master'

Optimize the ES Query for confidentiality check

Closes #225998

See merge request gitlab-org/gitlab!38095

app/services/search/global_service.rb

@@ -5,16 +5,15 @@ module Search
     include Gitlab::Utils::StrongMemoize
 
     attr_accessor :current_user, :params
-    attr_reader :default_project_filter
 
     def initialize(user, params)
       @current_user, @params = user, params.dup
-      @default_project_filter = true
     end
 
     def execute
-      Gitlab::SearchResults.new(current_user, projects, params[:search],
-                                default_project_filter: default_project_filter)
+      Gitlab::SearchResults.new(current_user,
+                                params[:search],
+                                projects)
     end
 
     def projects

app/services/search/group_service.rb

@@ -7,13 +7,15 @@ module Search
     def initialize(user, group, params)
       super(user, params)
 
-      @default_project_filter = false
       @group = group
     end
 
     def execute
       Gitlab::GroupSearchResults.new(
-        current_user, projects, group, params[:search], default_project_filter: default_project_filter
+        current_user,
+        params[:search],
+        projects,
+        group: group
       )
     end
 

app/services/search/project_service.rb

@@ -10,9 +10,9 @@ module Search
 
     def execute
       Gitlab::ProjectSearchResults.new(current_user,
-                                       project,
                                        params[:search],
-                                       params[:repository_ref])
+                                       project: project,
+                                       repository_ref: params[:repository_ref])
     end
 
     def scope

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

@@ -14,9 +14,8 @@ module EE
         ::Gitlab::Elastic::SearchResults.new(
           current_user,
           params[:search],
-          elastic_projects,
           projects,
-          elastic_global
+          public_and_internal_projects: elastic_global
         )
       end
 
@@ -24,18 +23,6 @@ module EE
         nil
       end
 
-      def elastic_projects
-        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
-
       def elastic_global
         true
       end

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

@@ -10,11 +10,6 @@ module EE
         group
       end
 
-      override :elastic_projects
-      def elastic_projects
-        @elastic_projects ||= projects.pluck(:id) # rubocop:disable CodeReuse/ActiveRecord
-      end
-
       override :elastic_global
       def elastic_global
         false
@@ -26,12 +21,10 @@ module EE
 
         ::Gitlab::Elastic::GroupSearchResults.new(
           current_user,
-          elastic_projects,
-          projects,
-          group,
           params[:search],
-          elastic_global,
-          default_project_filter: default_project_filter
+          projects,
+          group: group,
+          public_and_internal_projects: elastic_global
         )
       end
     end

ee/app/services/ee/search/project_service.rb

@@ -13,8 +13,8 @@ module EE
         ::Gitlab::Elastic::ProjectSearchResults.new(
           current_user,
           params[:search],
-          project,
-          repository_ref
+          project: project,
+          repository_ref: repository_ref
         )
       end
 

ee/app/services/ee/search/snippet_service.rb

@@ -9,7 +9,9 @@ module EE
       def execute
         return super unless use_elasticsearch?
 
-        ::Gitlab::Elastic::SnippetSearchResults.new(current_user, params[:search], elastic_projects, nil, true)
+        ::Gitlab::Elastic::SnippetSearchResults.new(current_user,
+                                                    params[:search],
+                                                    projects)
       end
 
       override :elasticsearchable_scope

ee/changelogs/unreleased/225998-advanced-search-result-page-is-slow.yml

+---
+title: Optimize the Advanced Search query for Issues and Notes.
+merge_request: 38095
+author:
+type: performance

ee/lib/elastic/latest/application_class_proxy.rb

@@ -119,22 +119,20 @@ module Elastic
       # documents gated by that project feature - e.g., "issues". The feature's
       # visibility level must be taken into account.
       def project_ids_query(user, project_ids, public_and_internal_projects, features = nil)
-        # When reading cross project is not allowed, only allow searching a
-        # a single project, so the `:read_*` ability is only checked once.
-        unless Ability.allowed?(user, :read_cross_project)
-          project_ids = [] if project_ids.is_a?(Array) && project_ids.size > 1
-        end
+        scoped_project_ids = scoped_project_ids(user, project_ids)
 
         # At least one condition must be present, so pick no projects for
         # anonymous users.
         # Pick private, internal and public projects the user is a member of.
         # Pick all private projects for admins & auditors.
-        conditions = pick_projects_by_membership(project_ids, user, features)
+        conditions = pick_projects_by_membership(scoped_project_ids, user, features)
 
         if public_and_internal_projects
           # Skip internal projects for anonymous and external users.
-          # Others are given access to all internal projects. Admins & auditors
-          # get access to internal projects where the feature is private.
+          # Others are given access to all internal projects.
+          #
+          # Admins & auditors get access to internal projects even
+          # if the feature is private.
           conditions += pick_projects_by_visibility(Project::INTERNAL, user, features) if user && !user.external?
 
           # All users, including anonymous, can access public projects.
@@ -219,6 +217,33 @@ module Elastic
           .filter_by_feature_visibility(feature, user)
           .pluck_primary_key
       end
+
+      def scoped_project_ids(current_user, project_ids)
+        return :any if project_ids == :any
+
+        project_ids ||= []
+
+        # When reading cross project is not allowed, only allow searching a
+        # a single project, so the `:read_*` ability is only checked once.
+        unless Ability.allowed?(current_user, :read_cross_project)
+          return [] if project_ids.size > 1
+        end
+
+        project_ids
+      end
+
+      def authorized_project_ids(current_user, options = {})
+        return [] unless current_user
+
+        scoped_project_ids = scoped_project_ids(current_user, options[:project_ids])
+        authorized_project_ids = current_user.authorized_projects(Gitlab::Access::REPORTER).pluck_primary_key.to_set
+
+        # if the current search is limited to a subset of projects, we should do
+        # confidentiality check for these projects.
+        authorized_project_ids &= scoped_project_ids.to_set unless scoped_project_ids == :any
+
+        authorized_project_ids.to_a
+      end
     end
   end
 end

ee/lib/elastic/latest/git_class_proxy.rb

@@ -10,13 +10,15 @@ module Elastic
 
         case type
         when 'all'
-          results[:blobs] = search_blob(query, page: page, per: per, options: options)
-          results[:commits] = search_commit(query, page: page, per: per, options: options)
-          results[:wiki_blobs] = search_blob(query, type: 'wiki_blob', page: page, per: per, options: options)
+          results[:commits] = search_commit(query, page: page, per: per, options: options.merge(features: 'repository'))
+          results[:blobs] = search_blob(query, type: 'blob', page: page, per: per, options: options.merge(features: 'repository'))
+          results[:wiki_blobs] = search_blob(query, type: 'wiki_blob', page: page, per: per, options: options.merge(features: 'wiki'))
         when 'commit'
-          results[:commits] = search_commit(query, page: page, per: per, options: options)
-        when 'blob', 'wiki_blob'
-          results[type.pluralize.to_sym] = search_blob(query, type: type, page: page, per: per, options: options)
+          results[:commits] = search_commit(query, page: page, per: per, options: options.merge(features: 'repository'))
+        when 'blob'
+          results[:blobs] = search_blob(query, type: type, page: page, per: per, options: options.merge(features: 'repository'))
+        when 'wiki_blob'
+          results[:wiki_blobs] = search_blob(query, type: type, page: page, per: per, options: options.merge(features: 'wiki'))
         end
 
         results
@@ -54,12 +56,17 @@ module Elastic
         bool_expr = Gitlab::Elastic::BoolExpr.new
         query_hash = {
           query: { bool: bool_expr },
-
           size: per,
           from: per * (page - 1),
           sort: [:_score]
         }
 
+        # If there is a :current_user set in the `options`, we can assume
+        # we need to do a project visibility check.
+        #
+        # Note that `:current_user` might be `nil` for a anonymous user
+        query_hash = project_ids_filter(query_hash, options) if options.key?(:current_user)
+
         if query.blank?
           bool_expr[:must] = { match_all: {} }
           query_hash[:track_scores] = true
@@ -73,11 +80,15 @@ module Elastic
           }
         end
 
-        options_filter_context = options_filter_context(:commit, options)
-
+        # add the document type filter
         bool_expr[:filter] << { term: { type: 'commit' } }
+
+        # add filters extracted from the options
+        options_filter_context = options_filter_context(:commit, options)
         bool_expr[:filter] += options_filter_context[:filter] if options_filter_context[:filter].any?
 
+        options[:order] = :default if options[:order].blank?
+
         if options[:highlight]
           es_fields = fields.map { |field| field.split('^').first }.each_with_object({}) do |field, memo|
             memo[field.to_sym] = {}
@@ -90,8 +101,6 @@ module Elastic
           }
         end
 
-        options[:order] = :default if options[:order].blank?
-
         res = search(query_hash, options)
         {
           results: res.results,
@@ -125,6 +134,12 @@ module Elastic
           }
         }
 
+        # If there is a :current_user set in the `options`, we can assume
+        # we need to do a project visibility check.
+        #
+        # Note that `:current_user` might be `nil` for a anonymous user
+        query_hash = project_ids_filter(query_hash, options) if options.key?(:current_user)
+
         # add the document type filter
         bool_expr[:filter] << { term: { type: type } }
 

ee/lib/elastic/latest/issue_class_proxy.rb

@@ -13,29 +13,32 @@ module Elastic
 
         options[:features] = 'issues'
         query_hash = project_ids_filter(query_hash, options)
-        query_hash = confidentiality_filter(query_hash, options[:current_user], options[:project_ids])
+        query_hash = confidentiality_filter(query_hash, options)
 
         search(query_hash, options)
       end
 
       private
 
-      def user_has_access_to_confidential_issues?(authorized_project_ids, project_ids)
-        # is_a?(Array) is needed because we might receive project_ids: :any
-        return false unless authorized_project_ids && project_ids.is_a?(Array)
+      def confidentiality_filter(query_hash, options)
+        current_user = options[:current_user]
+        project_ids = options[:project_ids]
 
-        (project_ids - authorized_project_ids).empty?
-      end
-
-      def confidentiality_filter(query_hash, current_user, project_ids)
         return query_hash if current_user&.can_read_all_resources?
 
-        authorized_project_ids = current_user&.authorized_projects(Gitlab::Access::REPORTER)&.pluck_primary_key
-        return query_hash if user_has_access_to_confidential_issues?(authorized_project_ids, project_ids)
+        scoped_project_ids = scoped_project_ids(current_user, project_ids)
+        authorized_project_ids = authorized_project_ids(current_user, options)
+
+        # we can shortcut the filter if the user is authorized to see
+        # all the projects for which this query is scoped on
+        unless scoped_project_ids == :any || scoped_project_ids.empty?
+          return query_hash if authorized_project_ids.to_set == scoped_project_ids.to_set
+        end
+
+        filter = { term: { confidential: false } }
 
-        filter =
         if current_user
-            {
+          filter = {
               bool: {
                 should: [
                   { term: { confidential: false } },
@@ -58,8 +61,6 @@ module Elastic
                 ]
               }
             }
-          else
-            { term: { confidential: false } }
         end
 
         query_hash[:query][:bool][:filter] << filter

ee/lib/elastic/latest/note_class_proxy.rb

@@ -14,7 +14,7 @@ module Elastic
 
         query_hash = basic_query_hash(%w[note], query)
         query_hash = project_ids_filter(query_hash, options)
-        query_hash = confidentiality_filter(query_hash, options[:current_user])
+        query_hash = confidentiality_filter(query_hash, options)
 
         query_hash[:highlight] = highlight_options(options[:in])
 
@@ -23,7 +23,9 @@ module Elastic
 
       private
 
-      def confidentiality_filter(query_hash, current_user)
+      def confidentiality_filter(query_hash, options)
+        current_user = options[:current_user]
+
         return query_hash if current_user&.can_read_all_resources?
 
         filter = {
@@ -43,7 +45,7 @@ module Elastic
                     bool: {
                     should: [
                       { bool: { must_not: [{ exists: { field: :confidential } }] } },
-                      { term: { "confidential" => false } }
+                      { term: { confidential: false } }
                     ]
                     }
                   }
@@ -61,7 +63,7 @@ module Elastic
                   bool: {
                     should: [
                       { term: { "issue.confidential" => true } },
-                      { term: { "confidential" => true } }
+                      { term: { confidential: true } }
                     ]
                   }
                 },
@@ -70,7 +72,7 @@ module Elastic
                     should: [
                       { term: { "issue.author_id" => current_user.id } },
                       { term: { "issue.assignee_id" => current_user.id } },
-                      { terms: { "project_id" => current_user.authorized_projects(Gitlab::Access::REPORTER).pluck_primary_key } }
+                      { terms: { project_id: authorized_project_ids(current_user, options) } }
                     ]
                   }
                 }

ee/lib/gitlab/elastic/group_search_results.rb

@@ -11,15 +11,20 @@ module Gitlab
 
       attr_reader :group, :default_project_filter
 
-      def initialize(current_user, limit_project_ids, limit_projects, group, query, public_and_internal_projects, default_project_filter: false)
-        super(current_user, query, limit_project_ids, limit_projects, public_and_internal_projects)
-
-        @default_project_filter = default_project_filter
+      def initialize(current_user, query, limit_projects = nil, group:, public_and_internal_projects: false, default_project_filter: false)
         @group = group
+        @default_project_filter = default_project_filter
+
+        super(current_user, query, limit_projects, public_and_internal_projects: public_and_internal_projects)
       end
 
       def generic_search_results
-        @generic_search_results ||= Gitlab::GroupSearchResults.new(current_user, limit_projects, group, query, default_project_filter: default_project_filter)
+        @generic_search_results ||= Gitlab::GroupSearchResults.new(
+          current_user,
+          query,
+          limit_projects,
+          group: group
+        )
       end
     end
   end

ee/lib/gitlab/elastic/project_search_results.rb

@@ -11,16 +11,20 @@ module Gitlab
       delegate :users, to: :generic_search_results
       delegate :limited_users_count, to: :generic_search_results
 
-      def initialize(current_user, query, project, repository_ref = nil)
-        @current_user = current_user
+      def initialize(current_user, query, project:, repository_ref: nil)
         @project = project
         @repository_ref = repository_ref.presence || project.default_branch
-        @query = query
-        @public_and_internal_projects = false
+
+        super(current_user, query, [project], public_and_internal_projects: false)
       end
 
       def generic_search_results
-        @generic_search_results ||= Gitlab::ProjectSearchResults.new(current_user, project, query, repository_ref)
+        @generic_search_results ||= Gitlab::ProjectSearchResults.new(
+          current_user,
+          query,
+          project: project,
+          repository_ref: repository_ref
+        )
       end
 
       private
@@ -95,10 +99,6 @@ module Gitlab
         end
       end
 
-      def limit_project_ids
-        [project.id]
-      end
-
       def root_ref?
         project.root_ref?(repository_ref)
       end

ee/lib/gitlab/elastic/search_results.rb

@@ -9,18 +9,17 @@ module Gitlab
 
       attr_reader :current_user, :query, :public_and_internal_projects
 
-      # Limit search results by passed project ids
+      # Limit search results by passed projects
       # It allows us to search only for projects user has access to
-      attr_reader :limit_project_ids, :limit_projects
+      attr_reader :limit_projects
 
       delegate :users, to: :generic_search_results
       delegate :limited_users_count, to: :generic_search_results
 
-      def initialize(current_user, query, limit_project_ids, limit_projects = nil, public_and_internal_projects = true)
+      def initialize(current_user, query, limit_projects = nil, public_and_internal_projects: true)
         @current_user = current_user
-        @limit_project_ids = limit_project_ids
-        @limit_projects = limit_projects
         @query = query
+        @limit_projects = limit_projects
         @public_and_internal_projects = public_and_internal_projects
       end
 
@@ -52,7 +51,7 @@ module Gitlab
       end
 
       def generic_search_results
-        @generic_search_results ||= Gitlab::SearchResults.new(current_user, limit_projects, query)
+        @generic_search_results ||= Gitlab::SearchResults.new(current_user, query, limit_projects)
       end
 
       def formatted_count(scope)
@@ -110,6 +109,17 @@ module Gitlab
         @milestones_count ||= milestones.total_count
       end
 
+      # mbergeron: these aliases act as an adapter to the Gitlab::SearchResults
+      # interface, which is mostly implemented by this class.
+      alias_method :limited_projects_count, :projects_count
+      alias_method :limited_notes_count, :notes_count
+      alias_method :limited_blobs_count, :blobs_count
+      alias_method :limited_wiki_blobs_count, :wiki_blobs_count
+      alias_method :limited_commits_count, :commits_count
+      alias_method :limited_issues_count, :issues_count
+      alias_method :limited_merge_requests_count, :merge_requests_count
+      alias_method :limited_milestones_count, :milestones_count
+
       def single_commit_result?
         false
       end
@@ -165,6 +175,20 @@ 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.
@@ -216,11 +240,7 @@ module Gitlab
 
       def merge_requests
         strong_memoize(:merge_requests) do
-          options = base_options.merge(
-            project_ids: filter_project_ids_by_feature(:merge_requests, limit_project_ids)
-          )
-
-          MergeRequest.elastic_search(query, options: options)
+          MergeRequest.elastic_search(query, options: base_options)
         end
       end
 
@@ -234,15 +254,11 @@ module Gitlab
         return Kaminari.paginate_array([]) if query.blank?
 
         strong_memoize(:blobs) do
-          options = base_options.merge(
-            additional_filter: repository_filter(limit_project_ids)
-          )
-
           Repository.__elasticsearch__.elastic_search_as_found_blob(
             query,
             page: (page || 1).to_i,
             per: per_page,
-            options: options
+            options: base_options
           )
         end
       end
@@ -251,15 +267,11 @@ module Gitlab
         return Kaminari.paginate_array([]) if query.blank?
 
         strong_memoize(:wiki_blobs) do
-          options = base_options.merge(
-            additional_filter: wiki_filter(limit_project_ids)
-          )
-
           ProjectWiki.__elasticsearch__.elastic_search_as_wiki_page(
             query,
             page: (page || 1).to_i,
             per: per_page,
-            options: options
+            options: base_options
           )
         end
       end
@@ -274,104 +286,16 @@ module Gitlab
         return Kaminari.paginate_array([]) if query.blank?
 
         strong_memoize(:commits) do
-          options = base_options.merge(
-            additional_filter: repository_filter(limit_project_ids)
-          )
-
           Repository.find_commits_by_message_with_elastic(
             query,
             page: (page || 1).to_i,
             per_page: per_page,
-            options: options,
+            options: base_options,
             preload_method: preload_method
           )
         end
       end
 
-      def wiki_filter(project_ids)
-        blob_filter(:wiki, project_ids)
-      end
-
-      def repository_filter(project_ids)
-        blob_filter(:repository, project_ids)
-      end
-
-      def filter_project_ids_by_feature(feature, project_ids)
-        return project_ids if project_ids == :any
-
-        Project
-          .id_in(project_ids)
-          .filter_by_feature_visibility(feature, current_user)
-          .pluck_primary_key
-      end
-
-      def blob_filter(feature, project_ids)
-        key_name = "#{feature}_access_level"
-
-        project_ids = filter_project_ids_by_feature(feature, project_ids)
-
-        conditions =
-          if project_ids == :any
-            [{ exists: { field: "id" } }]
-          else
-            [{ terms: { id: project_ids } }]
-          end
-
-        if public_and_internal_projects
-          conditions << {
-                          bool: {
-                            filter: [
-                              { term: { visibility_level: Project::PUBLIC } },
-                              { term: { key_name => ProjectFeature::ENABLED } }
-                            ]
-                          }
-                        }
-
-          if current_user && !current_user.external?
-            conditions << {
-                            bool: {
-                              filter: [
-                                { term: { visibility_level: Project::INTERNAL } },
-                                { term: { key_name => ProjectFeature::ENABLED } }
-                              ]
-                            }
-                          }
-          end
-        end
-
-        {
-          has_parent: {
-            parent_type: 'project',
-            query: {
-              bool: {
-                should: conditions,
-                must_not: { term: { key_name => ProjectFeature::DISABLED } }
-              }
-            }
-          }
-        }
-      end
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def guest_project_ids
-        if current_user
-          current_user.authorized_projects
-            .where('project_authorizations.access_level = ?', Gitlab::Access::GUEST)
-            .pluck(:id)
-        else
-          []
-        end
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-
-      def non_guest_project_ids
-        if limit_project_ids == :any
-          :any
-        else
-          @non_guest_project_ids ||= limit_project_ids - guest_project_ids
-        end
-      end
-
       def default_scope
         'projects'
       end

ee/spec/elastic_integration/global_search_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe 'GlobalSearch', :elastic do
     project.add_guest(guest)
   end
 
-  context "Respect feature visibility levels" do
+  context "Respect feature visibility levels", :aggregate_failures do
     context "Private projects" do
       let(:project) { create(:project, :private, :repository, :wiki_repo) }
 
@@ -127,7 +127,7 @@ RSpec.describe 'GlobalSearch', :elastic do
         expect_items_to_be_found(nil)
       end
 
-      it "shows items to member only if features are private" do
+      it "shows items to member only if features are private", :aggregate_failures do
         create_items(project, feature_settings(:private))
 
         expect_items_to_be_found(admin)
@@ -178,9 +178,9 @@ RSpec.describe 'GlobalSearch', :elastic do
 
     check_count = lambda do |feature, c|
       if arr.include?(feature)
-        expect(c).to be > 0
+        expect(c).to be > 0, "Search returned no #{feature} for #{user}"
       else
-        expect(c).to eq(0)
+        expect(c).to eq(0), "Search returned #{feature} for #{user}"
       end
     end
 

ee/spec/lib/ee/gitlab/search_results_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe Gitlab::SearchResults do
 
   let_it_be(:compliance_project) { create(:project, :with_compliance_framework, name: 'foo') }
 
-  subject { described_class.new(user, Project.all, 'foo') }
+  subject { described_class.new(user, 'foo') }
 
   describe '#projects' do
     it 'avoid N+1 queries' do

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

@@ -3,12 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Elastic::GroupSearchResults do
-  subject(:results) { described_class.new(user, nil, nil, group, query, nil) }
-
   let_it_be(:user) { create(:user) }
   let_it_be(:group) { create(:group) }
   let_it_be(:guest) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::GUEST) } }
 
+  subject(:results) { described_class.new(user, query, group: group) }
+
   before do
     stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
   end
@@ -20,7 +20,7 @@ RSpec.describe Gitlab::Elastic::GroupSearchResults do
       expect(Gitlab::GroupSearchResults).to receive(:new).and_call_original
     end
 
-    it { expect(results.objects('users')).to eq([guest]) }
+    it { expect(results.objects('users')).to contain_exactly(guest) }
     it { expect(results.limited_users_count).to eq(1) }
 
     describe 'pagination' do
@@ -29,8 +29,8 @@ RSpec.describe Gitlab::Elastic::GroupSearchResults do
       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 eq([user2])
-        expect(results.objects('users', page: 2, per_page: 1)).to eq([guest])
+        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

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

@@ -6,13 +6,16 @@ RSpec.describe Gitlab::Elastic::ProjectSearchResults, :elastic do
   let(:user) { create(:user) }
   let(:project) { create(:project, :public, :repository) }
   let(:query) { 'hello world' }
+  let(:repository_ref) { nil }
+
+  subject(:results) { described_class.new(user, query, project: project, repository_ref: repository_ref) }
 
   before do
     stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
   end
 
   describe 'initialize with empty ref' do
-    subject(:results) { described_class.new(user, query, project, '') }
+    let(:repository_ref) { '' }
 
     it { expect(results.project).to eq(project) }
     it { expect(results.repository_ref).to eq('master') }
@@ -20,58 +23,48 @@ RSpec.describe Gitlab::Elastic::ProjectSearchResults, :elastic do
   end
 
   describe 'initialize with ref' do
-    let(:ref) { 'refs/heads/test' }
-
-    subject(:results) { described_class.new(user, query, project, ref) }
+    let(:repository_ref) { 'refs/heads/test' }
 
     it { expect(results.project).to eq(project) }
-    it { expect(results.repository_ref).to eq(ref) }
+    it { expect(results.repository_ref).to eq(repository_ref) }
     it { expect(results.query).to eq('hello world') }
   end
 
   describe "search", :sidekiq_might_not_need_inline do
-    it "returns correct amounts" do
-      project = create :project, :public, :repository, :wiki_repo
-      project1 = create :project, :public, :repository, :wiki_repo
-
-      project.repository.index_commits_and_blobs
-
-      # Notes
-      create :note, note: 'bla-bla term', project: project
-      # The note in the project you have no access to
-      create :note, note: 'bla-bla term', project: project1
+    let(:project) { create(:project, :public, :repository, :wiki_repo) }
+    let(:private_project) { create(:project, :repository, :wiki_repo) }
 
-      # Wiki
+    before do
+      [project, private_project].each do |project|
+        create(:note, note: 'bla-bla term', project: project)
         project.wiki.create_page('index_page', 'term')
         project.wiki.index_wiki_blobs
-      project1.wiki.create_page('index_page', ' term')
-      project1.wiki.index_wiki_blobs
+      end
 
+      project.repository.index_commits_and_blobs
       ensure_elasticsearch_index!
+    end
 
-      result = described_class.new(user, 'term', project)
+    it "returns correct amounts" do
+      result = described_class.new(user, 'term', project: project)
       expect(result.notes_count).to eq(1)
       expect(result.wiki_blobs_count).to eq(1)
       expect(result.blobs_count).to eq(1)
 
-      result1 = described_class.new(user, 'initial', project)
-      expect(result1.commits_count).to eq(1)
+      result = described_class.new(user, 'initial', project: project)
+      expect(result.commits_count).to eq(1)
     end
 
     context 'visibility checks' do
-      it 'shows wiki for guests' do
-        project = create :project, :public, :wiki_repo
-        guest = create :user
-        project.add_guest(guest)
-
-        # Wiki
-        project.wiki.create_page('index_page', 'term')
-        project.wiki.index_wiki_blobs
+      let(:project) { create(:project, :public, :wiki_repo) }
+      let(:query) { 'term' }
 
-        ensure_elasticsearch_index!
+      before do
+        project.add_guest(user)
+      end
 
-        result = described_class.new(guest, 'term', project)
-        expect(result.wiki_blobs_count).to eq(1)
+      it 'shows wiki for guests' do
+        expect(results.wiki_blobs_count).to eq(1)
       end
     end
   end
@@ -79,12 +72,13 @@ RSpec.describe Gitlab::Elastic::ProjectSearchResults, :elastic do
   describe "search for commits in non-default branch" do
     let(:project) { create(:project, :public, :repository, visibility) }
     let(:visibility) { :repository_enabled }
-    let(:result) { described_class.new(user, 'initial', project, 'test') }
+    let(:query) { 'initial' }
+    let(:repository_ref) { 'test' }
 
-    subject(:commits) { result.objects('commits') }
+    subject(:commits) { results.objects('commits') }
 
     it 'finds needed commit' do
-      expect(result.commits_count).to eq(1)
+      expect(results.commits_count).to eq(1)
     end
 
     it 'responds to total_pages method' do
@@ -122,9 +116,10 @@ RSpec.describe Gitlab::Elastic::ProjectSearchResults, :elastic do
 
   describe 'search for blobs in non-default branch' do
     let(:project) { create(:project, :public, :repository, :repository_private) }
-    let(:result) { described_class.new(user, 'initial', project, 'test') }
+    let(:query) { 'initial' }
+    let(:repository_ref) { 'test' }
 
-    subject(:blobs) { result.objects('blobs') }
+    subject(:blobs) { results.objects('blobs') }
 
     it 'always returns zero results' do
       expect_any_instance_of(Gitlab::FileFinder).to receive(:find).never
@@ -134,88 +129,14 @@ RSpec.describe Gitlab::Elastic::ProjectSearchResults, :elastic do
   end
 
   describe 'confidential issues', :sidekiq_might_not_need_inline do
-    let(:query) { 'issue' }
-    let(:author) { create(:user) }
-    let(:assignee) { create(:user) }
-    let(:non_member) { create(:user) }
-    let(:member) { create(:user) }
-    let(:admin) { create(:admin) }
-    let!(:issue) { create(:issue, project: project, title: 'Issue 1') }
-    let!(:security_issue_1) { create(:issue, :confidential, project: project, title: 'Security issue 1', author: author) }
-    let!(:security_issue_2) { create(:issue, :confidential, title: 'Security issue 2', project: project, assignees: [assignee]) }
-
+    include_examples 'access restricted confidential issues' do
       before do
         ensure_elasticsearch_index!
       end
-
-    it 'does not list project confidential issues for non project members' do
-      results = described_class.new(non_member, query, project)
-      issues = results.objects('issues')
-
-      expect(issues).to include issue
-      expect(issues).not_to include security_issue_1
-      expect(issues).not_to include security_issue_2
-      expect(results.issues_count).to eq 1
-    end
-
-    it 'lists project confidential issues for author' do
-      results = described_class.new(author, query, project)
-      issues = results.objects('issues')
-
-      expect(issues).to include issue
-      expect(issues).to include security_issue_1
-      expect(issues).not_to include security_issue_2
-      expect(results.issues_count).to eq 2
-    end
-
-    it 'lists project confidential issues for assignee' do
-      results = described_class.new(assignee, query, project)
-      issues = results.objects('issues')
-
-      expect(issues).to include issue
-      expect(issues).not_to include security_issue_1
-      expect(issues).to include security_issue_2
-      expect(results.issues_count).to eq 2
-    end
-
-    it 'lists project confidential issues for project members' do
-      project.add_developer(member)
-
-      results = described_class.new(member, query, project)
-      issues = results.objects('issues')
-
-      expect(issues).to include issue
-      expect(issues).to include security_issue_1
-      expect(issues).to include security_issue_2
-      expect(results.issues_count).to eq 3
-    end
-
-    it 'does not list project confidential issues for project members with guest role' do
-      project.add_guest(member)
-
-      results = described_class.new(member, query, project)
-      issues = results.objects('issues')
-
-      expect(issues).to include issue
-      expect(issues).not_to include security_issue_1
-      expect(issues).not_to include security_issue_2
-      expect(results.issues_count).to eq 1
-    end
-
-    it 'lists all project issues for admin' do
-      results = described_class.new(admin, query, project)
-      issues = results.objects('issues')
-
-      expect(issues).to include issue
-      expect(issues).to include security_issue_1
-      expect(issues).to include security_issue_2
-      expect(results.issues_count).to eq 3
     end
   end
 
   context 'user search' do
-    subject(:results) { described_class.new(user, query, project) }
-
     let(:query) { project.owner.username }
 
     before do
@@ -243,6 +164,7 @@ RSpec.describe Gitlab::Elastic::ProjectSearchResults, :elastic do
 
   context 'query performance' do
     let(:project) { create(:project, :public, :repository, :wiki_repo) }
+    let(:query) { '*' }
 
     before do
       # wiki_blobs method checks to see if there is a wiki page before doing
@@ -250,8 +172,6 @@ RSpec.describe Gitlab::Elastic::ProjectSearchResults, :elastic do
       create(:wiki_page, wiki: project.wiki)
     end
 
-    let(:results) { described_class.new(user, '*', project) }
-
     include_examples 'does not hit Elasticsearch twice for objects and counts', %w|notes blobs wiki_blobs commits issues merge_requests milestones|
   end
 end

ee/spec/support/helpers/search_results_helpers.rb

@@ -17,16 +17,12 @@ module SearchResultHelpers
       if expected_count
         actual_count = results.public_send("#{target}_count")
 
-        expect(actual_count).to eq(expected_count), "#{target} expected count to be #{expected_count} for #{user_name}, got #{actual_count}"
+        expect(actual_count).to eq(expected_count), "#{target} expected count to be #{expected_count} for #{user_name}, got #{actual_count}: #{objects}"
       end
 
       if expected_objects
-        if expected_objects.empty?
-          expect(objects.empty?).to eq(true)
-        else
         expect(objects).to contain_exactly(*expected_objects)
       end
     end
   end
-  end
 end

lib/gitlab/group_search_results.rb

@@ -4,10 +4,10 @@ module Gitlab
   class GroupSearchResults < SearchResults
     attr_reader :group
 
-    def initialize(current_user, limit_projects, group, query, default_project_filter: false)
-      super(current_user, limit_projects, query, default_project_filter: default_project_filter)
-
+    def initialize(current_user, query, limit_projects = nil, group:, default_project_filter: false)
       @group = group
+
+      super(current_user, query, limit_projects, default_project_filter: default_project_filter)
     end
 
     # rubocop:disable CodeReuse/ActiveRecord

lib/gitlab/project_search_results.rb

@@ -4,11 +4,11 @@ module Gitlab
   class ProjectSearchResults < SearchResults
     attr_reader :project, :repository_ref
 
-    def initialize(current_user, project, query, repository_ref = nil)
-      @current_user = current_user
+    def initialize(current_user, query, project:, repository_ref: nil)
       @project = project
       @repository_ref = repository_ref.presence
-      @query = query
+
+      super(current_user, query, [project])
     end
 
     def objects(scope, page: nil, per_page: DEFAULT_PER_PAGE, preload_method: nil)

lib/gitlab/search_results.rb

@@ -19,10 +19,10 @@ module Gitlab
     # query
     attr_reader :default_project_filter
 
-    def initialize(current_user, limit_projects, query, default_project_filter: false)
+    def initialize(current_user, query, limit_projects = nil, default_project_filter: false)
       @current_user = current_user
-      @limit_projects = limit_projects || Project.all
       @query = query
+      @limit_projects = limit_projects || Project.all
       @default_project_filter = default_project_filter
     end
 

lib/gitlab/snippet_search_results.rb

@@ -4,11 +4,8 @@ module Gitlab
   class SnippetSearchResults < SearchResults
     include SnippetsHelper
 
-    attr_reader :current_user
-
     def initialize(current_user, query)
-      @current_user = current_user
-      @query = query
+      super(current_user, query)
     end
 
     def objects(scope, page: nil, per_page: DEFAULT_PER_PAGE, preload_method: nil)

spec/lib/gitlab/group_search_results_spec.rb

@@ -4,9 +4,12 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::GroupSearchResults do
   let(:user) { create(:user) }
+  let(:group) { create(:group) }
+
+  subject(:results) { described_class.new(user, 'gob', anything, group: group) }
 
   describe 'user search' do
-    let(:group) { create(:group) }
+    subject(:objects) { results.objects('users') }
 
     it 'returns the users belonging to the group matching the search query' do
       user1 = create(:user, username: 'gob_bluth')
@@ -17,9 +20,7 @@ RSpec.describe Gitlab::GroupSearchResults do
 
       create(:user, username: 'gob_2018')
 
-      result = described_class.new(user, anything, group, 'gob').objects('users')
-
-      expect(result).to eq [user1]
+      is_expected.to eq [user1]
     end
 
     it 'returns the user belonging to the subgroup matching the search query' do
@@ -29,9 +30,7 @@ RSpec.describe Gitlab::GroupSearchResults do
 
       create(:user, username: 'gob_2018')
 
-      result = described_class.new(user, anything, group, 'gob').objects('users')
-
-      expect(result).to eq [user1]
+      is_expected.to eq [user1]
     end
 
     it 'returns the user belonging to the parent group matching the search query' do
@@ -41,9 +40,7 @@ RSpec.describe Gitlab::GroupSearchResults do
 
       create(:user, username: 'gob_2018')
 
-      result = described_class.new(user, anything, group, 'gob').objects('users')
-
-      expect(result).to eq [user1]
+      is_expected.to eq [user1]
     end
 
     it 'does not return the user belonging to the private subgroup' do
@@ -53,9 +50,7 @@ RSpec.describe Gitlab::GroupSearchResults do
 
       create(:user, username: 'gob_2018')
 
-      result = described_class.new(user, anything, group, 'gob').objects('users')
-
-      expect(result).to eq []
+      is_expected.to be_empty
     end
 
     it 'does not return the user belonging to an unrelated group' do
@@ -63,15 +58,13 @@ RSpec.describe Gitlab::GroupSearchResults do
       unrelated_group = create(:group)
       create(:group_member, :developer, user: user, group: unrelated_group)
 
-      result = described_class.new(user, anything, group, 'gob').objects('users')
-
-      expect(result).to eq []
+      is_expected.to be_empty
+    end
   end
 
+  describe "#issuable_params" do
     it 'sets include_subgroups flag by default' do
-      result = described_class.new(user, anything, group, 'gob')
-
-      expect(result.issuable_params[:include_subgroups]).to eq(true)
+      expect(results.issuable_params[:include_subgroups]).to eq(true)
     end
   end
 end

spec/lib/gitlab/search_results_spec.rb

@@ -9,13 +9,10 @@ RSpec.describe Gitlab::SearchResults do
   let(:user) { create(:user) }
   let!(:project) { create(:project, name: 'foo') }
   let!(:issue) { create(:issue, project: project, title: 'foo') }
-
-  let!(:merge_request) do
-    create(:merge_request, source_project: project, title: 'foo')
-  end
-
+  let!(:merge_request) { create(:merge_request, source_project: project, title: 'foo') }
   let!(:milestone) { create(:milestone, project: project, title: 'foo') }
-  let(:results) { described_class.new(user, Project.all, 'foo') }
+
+  subject(:results) { described_class.new(user, 'foo', Project.all) }
 
   context 'as a user with access' do
     before do
@@ -133,7 +130,7 @@ RSpec.describe Gitlab::SearchResults do
       forked_project = fork_project(project, user)
       merge_request_2 = create(:merge_request, target_project: project, source_project: forked_project, title: 'foo')
 
-      results = described_class.new(user, Project.where(id: forked_project.id), 'foo')
+      results = described_class.new(user, 'foo', Project.where(id: forked_project.id))
 
       expect(results.objects('merge_requests')).to include merge_request_2
     end
@@ -214,7 +211,7 @@ RSpec.describe Gitlab::SearchResults do
     let!(:security_issue_5) { create(:issue, :confidential, project: project_4, title: 'Security issue 5') }
 
     it 'does not list confidential issues for non project members' do
-      results = described_class.new(non_member, limit_projects, query)
+      results = described_class.new(non_member, query, limit_projects)
       issues = results.objects('issues')
 
       expect(issues).to include issue
@@ -230,7 +227,7 @@ RSpec.describe Gitlab::SearchResults do
       project_1.add_guest(member)
       project_2.add_guest(member)
 
-      results = described_class.new(member, limit_projects, query)
+      results = described_class.new(member, query, limit_projects)
       issues = results.objects('issues')
 
       expect(issues).to include issue
@@ -243,7 +240,7 @@ RSpec.describe Gitlab::SearchResults do
     end
 
     it 'lists confidential issues for author' do
-      results = described_class.new(author, limit_projects, query)
+      results = described_class.new(author, query, limit_projects)
       issues = results.objects('issues')
 
       expect(issues).to include issue
@@ -256,7 +253,7 @@ RSpec.describe Gitlab::SearchResults do
     end
 
     it 'lists confidential issues for assignee' do
-      results = described_class.new(assignee, limit_projects, query)
+      results = described_class.new(assignee, query, limit_projects)
       issues = results.objects('issues')
 
       expect(issues).to include issue
@@ -272,7 +269,7 @@ RSpec.describe Gitlab::SearchResults do
       project_1.add_developer(member)
       project_2.add_developer(member)
 
-      results = described_class.new(member, limit_projects, query)
+      results = described_class.new(member, query, limit_projects)
       issues = results.objects('issues')
 
       expect(issues).to include issue
@@ -285,7 +282,7 @@ RSpec.describe Gitlab::SearchResults do
     end
 
     it 'lists all issues for admin' do
-      results = described_class.new(admin, limit_projects, query)
+      results = described_class.new(admin, query, limit_projects)
       issues = results.objects('issues')
 
       expect(issues).to include issue
@@ -323,7 +320,7 @@ RSpec.describe Gitlab::SearchResults do
       # Global search scope takes user authorized projects, internal projects and public projects.
       limit_projects = ProjectsFinder.new(current_user: user).execute
 
-      milestones = described_class.new(user, limit_projects, 'milestone').objects('milestones')
+      milestones = described_class.new(user, 'milestone', limit_projects).objects('milestones')
 
       expect(milestones).to match_array([milestone_1, milestone_2, milestone_3])
     end

spec/spec_helper.rb

@@ -261,6 +261,7 @@ RSpec.configure do |config|
       ./spec/support/protected_tags
       ./spec/support/shared_examples/features
       ./spec/support/shared_examples/requests
+      ./spec/support/shared_examples/lib/gitlab
       ./spec/views
       ./spec/workers
     )

spec/support/shared_examples/lib/gitlab/project_search_results_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'access restricted confidential issues' do
+  let(:query) { 'issue' }
+  let(:author) { create(:user) }
+  let(:assignee) { create(:user) }
+  let(:project) { create(:project, :internal) }
+
+  let!(:issue) { create(:issue, project: project, title: 'Issue 1') }
+  let!(:security_issue_1) { create(:issue, :confidential, project: project, title: 'Security issue 1', author: author) }
+  let!(:security_issue_2) { create(:issue, :confidential, title: 'Security issue 2', project: project, assignees: [assignee]) }
+
+  subject(:objects) do
+    described_class.new(user, query, project: project).objects('issues')
+  end
+
+  context 'when the user is non-member' do
+    let(:user) { create(:user) }
+
+    it 'does not list project confidential issues for non project members' do
+      expect(objects).to contain_exactly(issue)
+      expect(results.limited_issues_count).to eq 1
+    end
+  end
+
+  context 'when the member is guest' do
+    let(:user) do
+      create(:user) { |guest| project.add_guest(guest) }
+    end
+
+    it 'does not list project confidential issues for project members with guest role' do
+      expect(objects).to contain_exactly(issue)
+      expect(results.limited_issues_count).to eq 1
+    end
+  end
+
+  context 'when the user is the author' do
+    let(:user) { author }
+
+    it 'lists project confidential issues' do
+      expect(objects).to contain_exactly(issue,
+                                         security_issue_1)
+      expect(results.limited_issues_count).to eq 2
+    end
+  end
+
+  context 'when the user is the assignee' do
+    let(:user) { assignee }
+
+    it 'lists project confidential issues for assignee' do
+      expect(objects).to contain_exactly(issue,
+                                         security_issue_2)
+      expect(results.limited_issues_count).to eq 2
+    end
+  end
+
+  context 'when the user is a developper' do
+    let(:user) do
+      create(:user) { |user| project.add_developer(user) }
+    end
+
+    it 'lists project confidential issues' do
+      expect(objects).to contain_exactly(issue,
+                                         security_issue_1,
+                                         security_issue_2)
+      expect(results.limited_issues_count).to eq 3
+    end
+  end
+
+  context 'when the user is admin', :request_store do
+    let(:user) { create(:user, admin: true) }
+
+    it 'lists all project issues' do
+      expect(objects).to contain_exactly(issue,
+                                         security_issue_1,
+                                         security_issue_2)
+    end
+  end
+end