From c408be48ca3d6840076c6f16c7910411cdfca24c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 22 Mar 2019 06:39:58 -0700 Subject: [PATCH] Optimize /api/v4/projects endpoint for visibility level Previously when a user requested a list of projects, `Project#public_or_visible_to_user` would search all authorized projects and public/internal projects as well. However, when a user requests a specific `visibility_level` (e.g. private), that should reduce the search space, and we shouldn't need to load public/internal projects. Improves https://gitlab.com/gitlab-org/gitlab-ce/issues/59329 --- app/finders/projects_finder.rb | 2 +- app/models/project.rb | 22 +++++++-- .../unreleased/sh-optimize-projects-api.yml | 5 ++ spec/models/project_spec.rb | 49 ++++++++++++++++++- 4 files changed, 71 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/sh-optimize-projects-api.yml diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 93d3c991846..0319e95d439 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -81,7 +81,7 @@ class ProjectsFinder < UnionFinder if private_only? current_user.authorized_projects else - Project.public_or_visible_to_user(current_user) + Project.public_or_visible_to_user(current_user, params[:visibility_level]) end end end diff --git a/app/models/project.rb b/app/models/project.rb index 97a17d120c6..6667076b359 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -459,13 +459,25 @@ class Project < ActiveRecord::Base # 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) - if user + # + # requested_visiblity_levels: Normally all projects that are visible + # to the user (e.g. internal and public) are queried, but this + # parameter allows the caller to narrow the search space to optimize + # database queries. For instance, a caller may only want to see + # internal projects. Instead of querying for internal and public + # projects and throwing away public projects, this parameter allows + # the query to be targeted for only internal projects. + def self.public_or_visible_to_user(user = nil, requested_visibility_levels = []) + return public_to_user unless user + + visible_levels = Gitlab::VisibilityLevel.levels_for_user(user) + visible_levels &= Array(requested_visibility_levels) if requested_visibility_levels.present? + + if visible_levels.present? where('EXISTS (?) OR projects.visibility_level IN (?)', - user.authorizations_for_projects, - Gitlab::VisibilityLevel.levels_for_user(user)) + user.authorizations_for_projects, visible_levels) else - public_to_user + where('EXISTS (?)', user.authorizations_for_projects) end end diff --git a/changelogs/unreleased/sh-optimize-projects-api.yml b/changelogs/unreleased/sh-optimize-projects-api.yml new file mode 100644 index 00000000000..2f2459be77f --- /dev/null +++ b/changelogs/unreleased/sh-optimize-projects-api.yml @@ -0,0 +1,5 @@ +--- +title: Optimize /api/v4/projects endpoint for visibility level +merge_request: 26481 +author: +type: performance diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index fc9d9a28b9a..90dcf861849 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2710,7 +2710,7 @@ describe Project do end describe '#any_lfs_file_locks?', :request_store do - set(:project) { create(:project) } + let!(:project) { create(:project) } it 'returns false when there are no LFS file locks' do expect(project.any_lfs_file_locks?).to be_falsey @@ -3148,6 +3148,53 @@ describe Project do expect(projects).to eq([public_project]) end end + + context 'with requested visibility levels' do + set(:internal_project) { create(:project, :internal, :repository) } + set(:private_project_2) { create(:project, :private) } + + context 'with admin user' do + set(:admin) { create(:admin) } + + it 'returns all projects' do + projects = described_class.all.public_or_visible_to_user(admin, []) + + expect(projects).to match_array([public_project, private_project, private_project_2, internal_project]) + end + + it 'returns all public and private projects' do + projects = described_class.all.public_or_visible_to_user(admin, [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::PRIVATE]) + + expect(projects).to match_array([public_project, private_project, private_project_2]) + end + + it 'returns all private projects' do + projects = described_class.all.public_or_visible_to_user(admin, [Gitlab::VisibilityLevel::PRIVATE]) + + expect(projects).to match_array([private_project, private_project_2]) + end + end + + context 'with regular user' do + it 'returns authorized projects' do + projects = described_class.all.public_or_visible_to_user(user, []) + + expect(projects).to match_array([public_project, private_project, internal_project]) + end + + it "returns user's public and private projects" do + projects = described_class.all.public_or_visible_to_user(user, [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::PRIVATE]) + + expect(projects).to match_array([public_project, private_project]) + end + + it 'returns one private project' do + projects = described_class.all.public_or_visible_to_user(user, [Gitlab::VisibilityLevel::PRIVATE]) + + expect(projects).to eq([private_project]) + end + end + end end describe '.with_feature_available_for_user' do