summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/finders/issuable_finder.rb26
-rw-r--r--app/finders/issues_finder.rb4
-rw-r--r--app/finders/projects_finder.rb8
-rw-r--r--app/models/project.rb28
-rw-r--r--app/models/user.rb8
-rw-r--r--changelogs/unreleased/fj-59522-improve-search-controller-performance.yml5
-rw-r--r--lib/gitlab/group_search_results.rb6
-rw-r--r--lib/gitlab/project_search_results.rb4
-rw-r--r--lib/gitlab/search_results.rb88
-rw-r--r--spec/factories/projects.rb1
-rw-r--r--spec/finders/issues_finder_spec.rb8
-rw-r--r--spec/finders/merge_requests_finder_spec.rb2
-rw-r--r--spec/models/project_spec.rb104
-rw-r--r--spec/requests/api/projects_spec.rb3
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 da72186c8a0..61d245478ca 100644
--- a/app/models/project.rb
+++ b/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
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
View Lorry Controller Status