Add improvements to the global search processfj-59522-improve-search-controller-performance

Removed the conditions added to
Project.with_feature_available_for_user, and moved to the
IssuableFinder. Now, we ensure that, in the projects retrieved
in the Finder, the user has enough access for the feature.

14 files changed, 182 insertions, 113 deletions

diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index f1dd040515f..52b6e828cfa 100644
--- a/app/finders/issuable_finder.rb
+++ b/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
diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index e6a82f55856..58a01d598ba 100644
--- a/app/finders/issues_finder.rb
+++ b/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
 
diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb
index 93d3c991846..23b731b1aed 100644
--- a/app/finders/projects_finder.rb
+++ b/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
diff --git a/app/models/project.rb b/app/models/project.rb
index 626ff9e1389..d5e4a0e0bc0 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -460,10 +460,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
@@ -473,30 +475,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
diff --git a/app/models/user.rb b/app/models/user.rb
index 43039f3760e..4a1bf5514fe 100644
--- a/app/models/user.rb
+++ b/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?
 
diff --git a/changelogs/unreleased/fj-59522-improve-search-controller-performance.yml b/changelogs/unreleased/fj-59522-improve-search-controller-performance.yml
new file mode 100644
index 00000000000..c513f3c3aeb
--- /dev/null
+++ b/changelogs/unreleased/fj-59522-improve-search-controller-performance.yml
@@ -0,0 +1,5 @@
+---
+title: Add improvements to global search of issues and merge requests
+merge_request: 27817
+author:
+type: performance
diff --git a/lib/gitlab/group_search_results.rb b/lib/gitlab/group_search_results.rb
index 7255293b194..334642f252e 100644
--- a/lib/gitlab/group_search_results.rb
+++ b/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
diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb
index 58f06b6708c..78337518988 100644
--- a/lib/gitlab/project_search_results.rb
+++ b/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
diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb
index a29517e068f..4a097a00101 100644
--- a/lib/gitlab/search_results.rb
+++ b/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
diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb
index ab185ab3972..743ec322885 100644
--- a/spec/factories/projects.rb
+++ b/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 }
diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb
index 6a47cd013f8..89fdaceaa9f 100644
--- a/spec/finders/issues_finder_spec.rb
+++ b/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
 
diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb
index 117f4a03735..da5e9dab058 100644
--- a/spec/finders/merge_requests_finder_spec.rb
+++ b/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)
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index bb0257e7456..2a17bd6002e 100644
--- a/spec/models/project_spec.rb
+++ b/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
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index 577f61ae8d0..16d306f39cd 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/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