Merge branch 'fj-59522-improve-search-controller-performance' into 'master'

Improve performance of the global search for issuables

Closes #59522

See merge request gitlab-org/gitlab-ce!27817

app/finders/issuable_finder.rb

@@ -29,6 +29,7 @@
 #     updated_after: datetime
 #     updated_before: datetime
 #     attempt_group_search_optimizations: boolean
+#     attempt_project_search_optimizations: boolean
 #
 class IssuableFinder
   prepend FinderWithCrossProjectAccess
@@ -184,7 +185,6 @@ class IssuableFinder
     @project = project
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def projects
     return @projects if defined?(@projects)
 
@@ -192,17 +192,25 @@ class IssuableFinder
 
     projects =
       if current_user && params[:authorized_only].presence && !current_user_related?
-        current_user.authorized_projects
+        current_user.authorized_projects(min_access_level)
       elsif group
-        finder_options = { include_subgroups: params[:include_subgroups], only_owned: true }
-        GroupProjectsFinder.new(group: group, current_user: current_user, options: finder_options).execute # rubocop: disable CodeReuse/Finder
+        find_group_projects
       else
-        ProjectsFinder.new(current_user: current_user).execute # rubocop: disable CodeReuse/Finder
+        Project.public_or_visible_to_user(current_user, min_access_level)
       end
 
-    @projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil)
+    @projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil) # rubocop: disable CodeReuse/ActiveRecord
+  end
+
+  def find_group_projects
+    return Project.none unless group
+
+    if params[:include_subgroups]
+      Project.where(namespace_id: group.self_and_descendants) # rubocop: disable CodeReuse/ActiveRecord
+    else
+      group.projects
+    end.public_or_visible_to_user(current_user, min_access_level)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def search
     params[:search].presence
@@ -570,4 +578,8 @@ class IssuableFinder
     scope = params[:scope]
     scope == 'created_by_me' || scope == 'authored' || scope == 'assigned_to_me'
   end
+
+  def min_access_level
+    ProjectFeature.required_minimum_access_level(klass)
+  end
 end

app/finders/issues_finder.rb

@@ -48,9 +48,9 @@ class IssuesFinder < IssuableFinder
       OR (issues.confidential = TRUE
         AND (issues.author_id = :user_id
           OR EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = :user_id AND issue_id = issues.id)
-          OR issues.project_id IN(:project_ids)))',
+          OR EXISTS (:authorizations)))',
       user_id: current_user.id,
-      project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id))
+      authorizations: current_user.authorizations_for_projects(min_access_level: CONFIDENTIAL_ACCESS_LEVEL, related_project_column: "issues.project_id"))
   end
   # rubocop: enable CodeReuse/ActiveRecord
 

app/finders/projects_finder.rb

@@ -62,7 +62,7 @@ class ProjectsFinder < UnionFinder
     collection = by_personal(collection)
     collection = by_starred(collection)
     collection = by_trending(collection)
-    collection = by_visibilty_level(collection)
+    collection = by_visibility_level(collection)
     collection = by_tags(collection)
     collection = by_search(collection)
     collection = by_archived(collection)
@@ -71,12 +71,11 @@ class ProjectsFinder < UnionFinder
     collection
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def collection_with_user
     if owned_projects?
       current_user.owned_projects
     elsif min_access_level?
-      current_user.authorized_projects.where('project_authorizations.access_level >= ?', params[:min_access_level])
+      current_user.authorized_projects(params[:min_access_level])
     else
       if private_only?
         current_user.authorized_projects
@@ -85,7 +84,6 @@ class ProjectsFinder < UnionFinder
       end
     end
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   # Builds a collection for an anonymous user.
   def collection_without_user
@@ -131,7 +129,7 @@ class ProjectsFinder < UnionFinder
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
-  def by_visibilty_level(items)
+  def by_visibility_level(items)
     params[:visibility_level].present? ? items.where(visibility_level: params[:visibility_level]) : items
   end
   # rubocop: enable CodeReuse/ActiveRecord

app/models/project.rb

@@ -464,10 +464,12 @@ class Project < ApplicationRecord
 
   # Returns a collection of projects that is either public or visible to the
   # logged in user.
-  def self.public_or_visible_to_user(user = nil)
+  def self.public_or_visible_to_user(user = nil, min_access_level = nil)
+    min_access_level = nil if user&.admin?
+
     if user
       where('EXISTS (?) OR projects.visibility_level IN (?)',
-            user.authorizations_for_projects,
+            user.authorizations_for_projects(min_access_level: min_access_level),
             Gitlab::VisibilityLevel.levels_for_user(user))
     else
       public_to_user
@@ -477,30 +479,32 @@ class Project < ApplicationRecord
   # project features may be "disabled", "internal", "enabled" or "public". If "internal",
   # they are only available to team members. This scope returns projects where
   # the feature is either public, enabled, or internal with permission for the user.
+  # Note: this scope doesn't enforce that the user has access to the projects, it just checks
+  # that the user has access to the feature. It's important to use this scope with others
+  # that checks project authorizations first.
   #
   # This method uses an optimised version of `with_feature_access_level` for
   # logged in users to more efficiently get private projects with the given
   # feature.
   def self.with_feature_available_for_user(feature, user)
     visible = [ProjectFeature::ENABLED, ProjectFeature::PUBLIC]
-    min_access_level = ProjectFeature.required_minimum_access_level(feature)
 
     if user&.admin?
       with_feature_enabled(feature)
     elsif user
+      min_access_level = ProjectFeature.required_minimum_access_level(feature)
       column = ProjectFeature.quoted_access_level_column(feature)
 
       with_project_feature
-        .where(
-          "(projects.visibility_level > :private AND (#{column} IS NULL OR #{column} >= (:public_visible) OR (#{column} = :private_visible AND EXISTS(:authorizations))))"\
-          " OR (projects.visibility_level = :private AND (#{column} IS NULL OR #{column} >= :private_visible) AND EXISTS(:authorizations))",
-          {
-            private: Gitlab::VisibilityLevel::PRIVATE,
-            public_visible: ProjectFeature::ENABLED,
-            private_visible: ProjectFeature::PRIVATE,
-            authorizations: user.authorizations_for_projects(min_access_level: min_access_level)
-          })
+      .where("#{column} IS NULL OR #{column} IN (:public_visible) OR (#{column} = :private_visible AND EXISTS (:authorizations))",
+            {
+              public_visible: visible,
+              private_visible: ProjectFeature::PRIVATE,
+              authorizations: user.authorizations_for_projects(min_access_level: min_access_level)
+            })
     else
+      # This has to be added to include features whose value is nil in the db
+      visible << nil
       with_feature_access_level(feature, visible)
     end
   end

app/models/user.rb

@@ -757,11 +757,15 @@ class User < ApplicationRecord
 
   # Typically used in conjunction with projects table to get projects
   # a user has been given access to.
+  # The param `related_project_column` is the column to compare to the
+  # project_authorizations. By default is projects.id
   #
   # Example use:
   # `Project.where('EXISTS(?)', user.authorizations_for_projects)`
-  def authorizations_for_projects(min_access_level: nil)
-    authorizations = project_authorizations.select(1).where('project_authorizations.project_id = projects.id')
+  def authorizations_for_projects(min_access_level: nil, related_project_column: 'projects.id')
+    authorizations = project_authorizations
+                      .select(1)
+                      .where("project_authorizations.project_id = #{related_project_column}")
 
     return authorizations unless min_access_level.present?
 

changelogs/unreleased/fj-59522-improve-search-controller-performance.yml

+---
+title: Add improvements to global search of issues and merge requests
+merge_request: 27817
+author:
+type: performance

lib/gitlab/group_search_results.rb

@@ -2,6 +2,8 @@
 
 module Gitlab
   class GroupSearchResults < SearchResults
+    attr_reader :group
+
     def initialize(current_user, limit_projects, group, query, default_project_filter: false, per_page: 20)
       super(current_user, limit_projects, query, default_project_filter: default_project_filter, per_page: per_page)
 
@@ -26,5 +28,9 @@ module Gitlab
         .where(id: groups.select('members.user_id'))
     end
     # rubocop:enable CodeReuse/ActiveRecord
+
+    def issuable_params
+      super.merge(group_id: group.id)
+    end
   end
 end

lib/gitlab/project_search_results.rb

@@ -145,5 +145,9 @@ module Gitlab
     def repository_wiki_ref
       @repository_wiki_ref ||= repository_ref || project.wiki.default_branch
     end
+
+    def issuable_params
+      super.merge(project_id: project.id)
+    end
   end
 end

lib/gitlab/search_results.rb

@@ -2,6 +2,8 @@
 
 module Gitlab
   class SearchResults
+    COUNT_LIMIT = 1001
+
     attr_reader :current_user, :query, :per_page
 
     # Limit search results by passed projects
@@ -25,29 +27,26 @@ module Gitlab
     def objects(scope, page = nil, without_count = true)
       collection = case scope
                    when 'projects'
-                     projects.page(page).per(per_page)
+                     projects
                    when 'issues'
-                     issues.page(page).per(per_page)
+                     issues
                    when 'merge_requests'
-                     merge_requests.page(page).per(per_page)
+                     merge_requests
                    when 'milestones'
-                     milestones.page(page).per(per_page)
+                     milestones
                    when 'users'
-                     users.page(page).per(per_page)
+                     users
                    else
-                     Kaminari.paginate_array([]).page(page).per(per_page)
-                   end
+                     Kaminari.paginate_array([])
+                   end.page(page).per(per_page)
 
       without_count ? collection.without_count : collection
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def limited_projects_count
-      @limited_projects_count ||= projects.limit(count_limit).count
+      @limited_projects_count ||= limited_count(projects)
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def limited_issues_count
       return @limited_issues_count if @limited_issues_count
 
@@ -56,35 +55,28 @@ module Gitlab
       # and confidential issues user has access to, is too complex.
       # It's faster to try to fetch all public issues first, then only
       # if necessary try to fetch all issues.
-      sum = issues(public_only: true).limit(count_limit).count
-      @limited_issues_count = sum < count_limit ? issues.limit(count_limit).count : sum
+      sum = limited_count(issues(public_only: true))
+      @limited_issues_count = sum < count_limit ? limited_count(issues) : sum
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def limited_merge_requests_count
-      @limited_merge_requests_count ||= merge_requests.limit(count_limit).count
+      @limited_merge_requests_count ||= limited_count(merge_requests)
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def limited_milestones_count
-      @limited_milestones_count ||= milestones.limit(count_limit).count
+      @limited_milestones_count ||= limited_count(milestones)
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
-    # rubocop:disable CodeReuse/ActiveRecord
     def limited_users_count
-      @limited_users_count ||= users.limit(count_limit).count
+      @limited_users_count ||= limited_count(users)
     end
-    # rubocop:enable CodeReuse/ActiveRecord
 
     def single_commit_result?
       false
     end
 
     def count_limit
-      1001
+      COUNT_LIMIT
     end
 
     def users
@@ -99,23 +91,15 @@ module Gitlab
       limit_projects.search(query)
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def issues(finder_params = {})
-      issues = IssuesFinder.new(current_user, finder_params).execute
+      issues = IssuesFinder.new(current_user, issuable_params.merge(finder_params)).execute
+
       unless default_project_filter
-        issues = issues.where(project_id: project_ids_relation)
+        issues = issues.where(project_id: project_ids_relation) # rubocop: disable CodeReuse/ActiveRecord
       end
 
-      issues =
-        if query =~ /#(\d+)\z/
-          issues.where(iid: $1)
-        else
-          issues.full_search(query)
-        end
-
-      issues.reorder('issues.updated_at DESC')
+      issues
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     # rubocop: disable CodeReuse/ActiveRecord
     def milestones
@@ -125,23 +109,15 @@ module Gitlab
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def merge_requests
-      merge_requests = MergeRequestsFinder.new(current_user).execute
+      merge_requests = MergeRequestsFinder.new(current_user, issuable_params).execute
+
       unless default_project_filter
         merge_requests = merge_requests.in_projects(project_ids_relation)
       end
 
-      merge_requests =
-        if query =~ /[#!](\d+)\z/
-          merge_requests.where(iid: $1)
-        else
-          merge_requests.full_search(query)
-        end
-
-      merge_requests.reorder('merge_requests.updated_at DESC')
+      merge_requests
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     def default_scope
       'projects'
@@ -152,5 +128,23 @@ module Gitlab
       limit_projects.select(:id).reorder(nil)
     end
     # rubocop: enable CodeReuse/ActiveRecord
+
+    def issuable_params
+      {}.tap do |params|
+        params[:sort] = 'updated_desc'
+
+        if query =~ /#(\d+)\z/
+          params[:iids] = $1
+        else
+          params[:search] = query
+        end
+      end
+    end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def limited_count(relation)
+      relation.reorder(nil).limit(count_limit).size
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
   end
 end

spec/factories/projects.rb

@@ -260,6 +260,7 @@ FactoryBot.define do
     trait(:merge_requests_enabled)  { merge_requests_access_level ProjectFeature::ENABLED }
     trait(:merge_requests_disabled) { merge_requests_access_level ProjectFeature::DISABLED }
     trait(:merge_requests_private)  { merge_requests_access_level ProjectFeature::PRIVATE }
+    trait(:merge_requests_public)   { merge_requests_access_level ProjectFeature::PUBLIC }
     trait(:repository_enabled)      { repository_access_level ProjectFeature::ENABLED }
     trait(:repository_disabled)     { repository_access_level ProjectFeature::DISABLED }
     trait(:repository_private)      { repository_access_level ProjectFeature::PRIVATE }

spec/finders/issues_finder_spec.rb

@@ -641,9 +641,7 @@ describe IssuesFinder do
         end
 
         it 'filters by confidentiality' do
-          expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything)
-
-          subject
+          expect(subject.to_sql).to match("issues.confidential")
         end
       end
 
@@ -660,9 +658,7 @@ describe IssuesFinder do
         end
 
         it 'filters by confidentiality' do
-          expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything)
-
-          subject
+          expect(subject.to_sql).to match("issues.confidential")
         end
       end
 

spec/finders/merge_requests_finder_spec.rb

@@ -31,7 +31,7 @@ describe MergeRequestsFinder do
       end
 
       context 'filtering by group' do
-        it 'includes all merge requests when user has access exceluding merge requests from projects the user does not have access to' do
+        it 'includes all merge requests when user has access excluding merge requests from projects the user does not have access to' do
           private_project = allow_gitaly_n_plus_1 { create(:project, :private, group: group) }
           private_project.add_guest(user)
           create(:merge_request, :simple, author: user, source_project: private_project, target_project: private_project)

spec/models/project_spec.rb

@@ -3164,61 +3164,105 @@ describe Project do
   end
 
   describe '.with_feature_available_for_user' do
-    let!(:user) { create(:user) }
-    let!(:feature) { MergeRequest }
-    let!(:project) { create(:project, :public, :merge_requests_enabled) }
+    let(:user) { create(:user) }
+    let(:feature) { MergeRequest }
 
     subject { described_class.with_feature_available_for_user(feature, user) }
 
-    context 'when user has access to project' do
-      subject { described_class.with_feature_available_for_user(feature, user) }
+    shared_examples 'feature disabled' do
+      let(:project) { create(:project, :public, :merge_requests_disabled) }
+
+      it 'does not return projects with the project feature disabled' do
+        is_expected.not_to include(project)
+      end
+    end
+
+    shared_examples 'feature public' do
+      let(:project) { create(:project, :public, :merge_requests_public) }
+
+      it 'returns projects with the project feature public' do
+        is_expected.to include(project)
+      end
+    end
+
+    shared_examples 'feature enabled' do
+      let(:project) { create(:project, :public, :merge_requests_enabled) }
+
+      it 'returns projects with the project feature enabled' do
+        is_expected.to include(project)
+      end
+    end
+
+    shared_examples 'feature access level is nil' do
+      let(:project) { create(:project, :public) }
+
+      it 'returns projects with the project feature access level nil' do
+        project.project_feature.update(merge_requests_access_level: nil)
+
+        is_expected.to include(project)
+      end
+    end
 
+    context 'with user' do
       before do
         project.add_guest(user)
       end
 
-      context 'when public project' do
-        context 'when feature is public' do
-          it 'returns project' do
-            is_expected.to include(project)
+      it_behaves_like 'feature disabled'
+      it_behaves_like 'feature public'
+      it_behaves_like 'feature enabled'
+      it_behaves_like 'feature access level is nil'
+
+      context 'when feature is private' do
+        let(:project) { create(:project, :public, :merge_requests_private) }
+
+        context 'when user does not has access to the feature' do
+          it 'does not return projects with the project feature private' do
+            is_expected.not_to include(project)
           end
         end
 
-        context 'when feature is private' do
-          let!(:project) { create(:project, :public, :merge_requests_private) }
-
-          it 'returns project when user has access to the feature' do
-            project.add_maintainer(user)
+        context 'when user has access to the feature' do
+          it 'returns projects with the project feature private' do
+            project.add_reporter(user)
 
             is_expected.to include(project)
           end
-
-          it 'does not return project when user does not have the minimum access level required' do
-            is_expected.not_to include(project)
-          end
         end
       end
+    end
 
-      context 'when private project' do
-        let!(:project) { create(:project) }
+    context 'user is an admin' do
+      let(:user) { create(:user, :admin) }
 
-        it 'returns project when user has access to the feature' do
-          project.add_maintainer(user)
+      it_behaves_like 'feature disabled'
+      it_behaves_like 'feature public'
+      it_behaves_like 'feature enabled'
+      it_behaves_like 'feature access level is nil'
 
-          is_expected.to include(project)
-        end
+      context 'when feature is private' do
+        let(:project) { create(:project, :public, :merge_requests_private) }
 
-        it 'does not return project when user does not have the minimum access level required' do
-          is_expected.not_to include(project)
+        it 'returns projects with the project feature private' do
+          is_expected.to include(project)
         end
       end
     end
 
-    context 'when user does not have access to project' do
-      let!(:project) { create(:project) }
+    context 'without user' do
+      let(:user) { nil }
 
-      it 'does not return project when user cant access project' do
-        is_expected.not_to include(project)
+      it_behaves_like 'feature disabled'
+      it_behaves_like 'feature public'
+      it_behaves_like 'feature enabled'
+      it_behaves_like 'feature access level is nil'
+
+      context 'when feature is private' do
+        let(:project) { create(:project, :public, :merge_requests_private) }
+
+        it 'does not return projects with the project feature private' do
+          is_expected.not_to include(project)
+        end
       end
     end
   end

spec/requests/api/projects_spec.rb

@@ -504,8 +504,9 @@ describe API::Projects do
           project4.add_reporter(user2)
         end
 
-        it 'returns an array of groups the user has at least developer access' do
+        it 'returns an array of projects the user has at least developer access' do
           get api('/projects', user2), params: { min_access_level: 30 }
+
           expect(response).to have_gitlab_http_status(200)
           expect(response).to include_pagination_headers
           expect(json_response).to be_an Array