Commit e4158b7c authored by Francisco Javier López's avatar Francisco Javier López
Browse files

Add improvements to the global search process

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.
parent 3d3d8b9320cd
......@@ -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 @@
@project = project
end
# rubocop: disable CodeReuse/ActiveRecord
def projects
return @projects if defined?(@projects)
......@@ -192,5 +192,5 @@
projects =
if current_user && params[:authorized_only].presence && !current_user_related?
current_user.authorized_projects
current_user.authorized_projects(min_access_level)
elsif group
......@@ -196,4 +196,3 @@
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
......@@ -199,4 +198,4 @@
else
ProjectsFinder.new(current_user: current_user).execute # rubocop: disable CodeReuse/Finder
Project.public_or_visible_to_user(current_user, min_access_level)
end
......@@ -201,4 +200,4 @@
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
......@@ -204,5 +203,14 @@
end
# rubocop: enable CodeReuse/ActiveRecord
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
def search
params[:search].presence
......@@ -570,4 +578,8 @@
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
......@@ -48,5 +48,5 @@
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,
......@@ -52,5 +52,5 @@
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
......
......@@ -62,7 +62,7 @@
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,8 +71,7 @@
collection
end
# rubocop: disable CodeReuse/ActiveRecord
def collection_with_user
if owned_projects?
current_user.owned_projects
elsif min_access_level?
......@@ -75,8 +74,8 @@
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 @@
end
end
end
# rubocop: enable CodeReuse/ActiveRecord
# Builds a collection for an anonymous user.
def collection_without_user
......@@ -131,7 +129,7 @@
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
......
......@@ -464,6 +464,8 @@
# 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 (?)',
......@@ -468,6 +470,6 @@
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,9 +479,12 @@
# 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]
......@@ -480,11 +485,10 @@
#
# 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
......@@ -487,7 +491,8 @@
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
......@@ -491,13 +496,10 @@
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
......@@ -503,4 +505,6 @@
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
......
......@@ -757,6 +757,8 @@
# 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)`
......@@ -760,8 +762,10 @@
#
# 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?
......
---
title: Add improvements to global search of issues and merge requests
merge_request: 27817
author:
type: performance
......@@ -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 @@
.where(id: groups.select('members.user_id'))
end
# rubocop:enable CodeReuse/ActiveRecord
def issuable_params
super.merge(group_id: group.id)
end
end
end
......@@ -145,5 +145,9 @@
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
......@@ -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,5 +27,5 @@
def objects(scope, page = nil, without_count = true)
collection = case scope
when 'projects'
projects.page(page).per(per_page)
projects
when 'issues'
......@@ -29,3 +31,3 @@
when 'issues'
issues.page(page).per(per_page)
issues
when 'merge_requests'
......@@ -31,3 +33,3 @@
when 'merge_requests'
merge_requests.page(page).per(per_page)
merge_requests
when 'milestones'
......@@ -33,3 +35,3 @@
when 'milestones'
milestones.page(page).per(per_page)
milestones
when 'users'
......@@ -35,3 +37,3 @@
when 'users'
users.page(page).per(per_page)
users
else
......@@ -37,7 +39,7 @@
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
......@@ -40,6 +42,5 @@
without_count ? collection.without_count : collection
end
# rubocop: disable CodeReuse/ActiveRecord
def limited_projects_count
......@@ -45,3 +46,3 @@
def limited_projects_count
@limited_projects_count ||= projects.limit(count_limit).count
@limited_projects_count ||= limited_count(projects)
end
......@@ -47,3 +48,2 @@
end
# rubocop: enable CodeReuse/ActiveRecord
......@@ -49,5 +49,4 @@
# rubocop: disable CodeReuse/ActiveRecord
def limited_issues_count
return @limited_issues_count if @limited_issues_count
......@@ -56,6 +55,6 @@
# 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
......@@ -61,3 +60,2 @@
end
# rubocop: enable CodeReuse/ActiveRecord
......@@ -63,3 +61,2 @@
# rubocop: disable CodeReuse/ActiveRecord
def limited_merge_requests_count
......@@ -65,3 +62,3 @@
def limited_merge_requests_count
@limited_merge_requests_count ||= merge_requests.limit(count_limit).count
@limited_merge_requests_count ||= limited_count(merge_requests)
end
......@@ -67,3 +64,2 @@
end
# rubocop: enable CodeReuse/ActiveRecord
......@@ -69,3 +65,2 @@
# rubocop: disable CodeReuse/ActiveRecord
def limited_milestones_count
......@@ -71,3 +66,3 @@
def limited_milestones_count
@limited_milestones_count ||= milestones.limit(count_limit).count
@limited_milestones_count ||= limited_count(milestones)
end
......@@ -73,3 +68,2 @@
end
# rubocop: enable CodeReuse/ActiveRecord
......@@ -75,3 +69,2 @@
# rubocop:disable CodeReuse/ActiveRecord
def limited_users_count
......@@ -77,3 +70,3 @@
def limited_users_count
@limited_users_count ||= users.limit(count_limit).count
@limited_users_count ||= limited_count(users)
end
......@@ -79,8 +72,7 @@
end
# rubocop:enable CodeReuse/ActiveRecord
def single_commit_result?
false
end
def count_limit
......@@ -81,10 +73,10 @@
def single_commit_result?
false
end
def count_limit
1001
COUNT_LIMIT
end
def users
......@@ -99,5 +91,4 @@
limit_projects.search(query)
end
# rubocop: disable CodeReuse/ActiveRecord
def issues(finder_params = {})
......@@ -103,3 +94,4 @@
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
......@@ -105,4 +97,4 @@
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
......@@ -107,11 +99,4 @@
end
issues =
if query =~ /#(\d+)\z/
issues.where(iid: $1)
else
issues.full_search(query)
end
issues.reorder('issues.updated_at DESC')
issues
end
......@@ -117,5 +102,4 @@
end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def milestones
......@@ -125,5 +109,4 @@
end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def merge_requests
......@@ -129,6 +112,7 @@
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
......@@ -131,13 +115,6 @@
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
......@@ -143,5 +120,4 @@
end
# rubocop: enable CodeReuse/ActiveRecord
def default_scope
'projects'
......@@ -152,5 +128,23 @@
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
......@@ -260,6 +260,7 @@
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 }
......
......@@ -641,9 +641,7 @@
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 @@
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
......
......@@ -31,7 +31,7 @@
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)
......
......@@ -3164,9 +3164,8 @@
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) }
......@@ -3170,10 +3169,42 @@
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
......@@ -3176,11 +3207,18 @@
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
......@@ -3184,11 +3222,9 @@
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
......@@ -3192,9 +3228,5 @@
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
......@@ -3199,11 +3231,18 @@
end
end
context 'when private project' do
let!(:project) { create(:project) }
it 'returns project when user has access to the feature' do
project.add_maintainer(user)
end
context 'user is an admin' do