From 98043aeedf1fe3241d8362d8036f28b709e6d468 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 22 May 2017 16:42:10 +0200 Subject: [PATCH 01/12] Copy `filter_projects` helper to V3 The helper will be modified in V4, so copy the original to V4 to keep the current behavior in V3. --- lib/api/v3/helpers.rb | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/lib/api/v3/helpers.rb b/lib/api/v3/helpers.rb index 0f234d4cdad..06af286ef50 100644 --- a/lib/api/v3/helpers.rb +++ b/lib/api/v3/helpers.rb @@ -14,6 +14,33 @@ module API authorize! access_level, merge_request merge_request end + + # project helpers + + def filter_projects(projects) + if params[:membership] + projects = projects.merge(current_user.authorized_projects) + end + + if params[:owned] + projects = projects.merge(current_user.owned_projects) + end + + if params[:starred] + projects = projects.merge(current_user.starred_projects) + end + + if params[:search].present? + projects = projects.search(params[:search]) + end + + if params[:visibility].present? + projects = projects.search_by_visibility(params[:visibility]) + end + + projects = projects.where(archived: params[:archived]) + projects.reorder(params[:order_by] => params[:sort]) + end end end end From 44fdf0a1e314da517d189e356b0e44bb63cf0f4b Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 22 May 2017 16:47:16 +0200 Subject: [PATCH 02/12] Move ProjectsFinder to `present_projects` for simplification To avoid passing parameters double, move all filtering to the `present_projects` helper. --- lib/api/projects.rb | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/lib/api/projects.rb b/lib/api/projects.rb index d4fe5c023bf..fce496308c3 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -67,20 +67,18 @@ module API optional :import_url, type: String, desc: 'URL from which the project is imported' end - def present_projects(projects, options = {}) + def present_projects(options = {}) options = options.reverse_merge( - with: Entities::Project, - current_user: current_user, - simple: params[:simple], - with_issues_enabled: params[:with_issues_enabled], - with_merge_requests_enabled: params[:with_merge_requests_enabled] + with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails, + current_user: current_user ) + projects = ProjectsFinder.new(current_user: current_user).execute projects = filter_projects(projects) - projects = projects.with_statistics if options[:statistics] - projects = projects.with_issues_enabled if options[:with_issues_enabled] - projects = projects.with_merge_requests_enabled if options[:with_merge_requests_enabled] - options[:with] = Entities::BasicProjectDetails if options[:simple] + projects = projects.with_statistics if params[:statistics] + projects = projects.with_issues_enabled if params[:with_issues_enabled] + projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled] + options[:with] = Entities::BasicProjectDetails if params[:simple] present paginate(projects), options end @@ -94,8 +92,7 @@ module API use :statistics_params end get do - entity = current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails - present_projects ProjectsFinder.new(current_user: current_user).execute, with: entity, statistics: params[:statistics] + present_projects end desc 'Create new project' do From 4fda13b68eb7da27be5e74cac6d3d537502b19f7 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 22 May 2017 16:48:38 +0200 Subject: [PATCH 03/12] Build options hash after finding the list of projects Because this order makes more sense and makes the code easier to read. --- lib/api/projects.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/api/projects.rb b/lib/api/projects.rb index fce496308c3..ef09dfa0102 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -68,16 +68,17 @@ module API end def present_projects(options = {}) - options = options.reverse_merge( - with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails, - current_user: current_user - ) - projects = ProjectsFinder.new(current_user: current_user).execute projects = filter_projects(projects) projects = projects.with_statistics if params[:statistics] projects = projects.with_issues_enabled if params[:with_issues_enabled] projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled] + + options = options.reverse_merge( + with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails, + statistics: params[:statistics], + current_user: current_user + ) options[:with] = Entities::BasicProjectDetails if params[:simple] present paginate(projects), options From 07fc79e7c53a4fa7c4dd33835b905dfa8a609ff8 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 23 May 2017 15:48:13 +0200 Subject: [PATCH 04/12] Handle `membership` in ProjectFinder The ProjectFinder supports the `non_public` parameter. This can be used to find only projects the user is member of. --- lib/api/helpers.rb | 4 ---- lib/api/projects.rb | 5 ++++- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 226a7ddd50e..0dc33eb2097 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -257,10 +257,6 @@ module API # project helpers def filter_projects(projects) - if params[:membership] - projects = projects.merge(current_user.authorized_projects) - end - if params[:owned] projects = projects.merge(current_user.owned_projects) end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index ef09dfa0102..bb03480f1ee 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -68,7 +68,10 @@ module API end def present_projects(options = {}) - projects = ProjectsFinder.new(current_user: current_user).execute + finder_params = {} + finder_params[:non_public] = true if params[:membership].present? + + projects = ProjectsFinder.new(current_user: current_user, params: finder_params).execute projects = filter_projects(projects) projects = projects.with_statistics if params[:statistics] projects = projects.with_issues_enabled if params[:with_issues_enabled] From 8e72ad70bd2479ae5a465eac1df74f99f03ea731 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 23 May 2017 22:40:07 +0200 Subject: [PATCH 05/12] Add starred_by scope to Project Add a scope to search for the projects that are starred by a certain user. --- app/models/project.rb | 3 ++- app/models/user.rb | 2 +- spec/models/project_spec.rb | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index a59095cb25c..963fd594d46 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -242,6 +242,7 @@ class Project < ActiveRecord::Base scope :in_namespace, ->(namespace_ids) { where(namespace_id: namespace_ids) } scope :personal, ->(user) { where(namespace_id: user.namespace_id) } scope :joined, ->(user) { where('namespace_id != ?', user.namespace_id) } + scope :starred_by, ->(user) { joins(:users_star_projects).where('users_star_projects.user_id': user.id) } scope :visible_to_user, ->(user) { where(id: user.authorized_projects.select(:id).reorder(nil)) } scope :non_archived, -> { where(archived: false) } scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct } @@ -350,7 +351,7 @@ class Project < ActiveRecord::Base where("projects.id IN (#{union.to_sql})") end - def search_by_visibility(level) + def search_by_visibility(level) # DEPRECATED: remove with API V3 where(visibility_level: Gitlab::VisibilityLevel.string_options[level]) end diff --git a/app/models/user.rb b/app/models/user.rb index 3f816a250c2..20894ce269a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -557,7 +557,7 @@ class User < ActiveRecord::Base authorized_projects(Gitlab::Access::REPORTER).where(id: projects) end - def viewable_starred_projects + def viewable_starred_projects # DEPRECATED: Use ProjectFinder instead. Remove together with API V3 starred_projects.where("projects.visibility_level IN (?) OR projects.id IN (?)", [Project::PUBLIC, Project::INTERNAL], authorized_projects.select(:project_id)) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2ef3d5654d5..da1b29a2bda 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -948,6 +948,20 @@ describe Project, models: true do end end + describe '.starred_by' do + it 'returns only projects starred by the given user' do + user1 = create(:user) + user2 = create(:user) + project1 = create(:empty_project) + project2 = create(:empty_project) + create(:empty_project) + user1.toggle_star(project1) + user2.toggle_star(project2) + + expect(Project.starred_by(user1)).to contain_exactly(project1) + end + end + describe '.visible_to_user' do let!(:project) { create(:empty_project, :private) } let!(:user) { create(:user) } From 0725050802dd30d4c235b6a2d28dd494d2d7429b Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 23 May 2017 22:38:12 +0200 Subject: [PATCH 06/12] Change ProjectFinder so starred can be combined with other filters The `starred` parameter couldn't be used in combination with `trending` or `non_public`. But this is changed now. --- app/finders/projects_finder.rb | 7 +++++-- spec/finders/projects_finder_spec.rb | 8 +++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index f6d8226bf3f..588406982d7 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -31,6 +31,7 @@ class ProjectsFinder < UnionFinder items = by_ids(items) items = union(items) items = by_personal(items) + items = by_starred(items) items = by_visibilty_level(items) items = by_tags(items) items = by_search(items) @@ -45,8 +46,6 @@ class ProjectsFinder < UnionFinder if params[:trending].present? projects << Project.trending - elsif params[:starred].present? && current_user - projects << current_user.viewable_starred_projects else projects << current_user.authorized_projects if current_user projects << Project.unscoped.public_to_user(current_user) unless params[:non_public].present? @@ -67,6 +66,10 @@ class ProjectsFinder < UnionFinder (params[:personal].present? && current_user) ? items.personal(current_user) : items end + def by_starred(items) + (params[:starred].present? && current_user) ? items.starred_by(current_user) : items + end + def by_visibilty_level(items) params[:visibility_level].present? ? items.where(visibility_level: params[:visibility_level]) : items end diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index 148adcffe3b..077f2624388 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -146,13 +146,19 @@ describe ProjectsFinder do it { is_expected.to eq([private_project]) } end - describe 'filter by viewable_starred_projects' do + describe 'filter by starred' do let(:params) { { starred: true } } before do current_user.toggle_star(public_project) end it { is_expected.to eq([public_project]) } + + it 'returns only projects the user has access to' do + current_user.toggle_star(private_project) + + is_expected.to eq([public_project]) + end end describe 'sorting' do From a1deed629e03d8db47deb1bcf795ae8abaf2c847 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 23 May 2017 22:40:39 +0200 Subject: [PATCH 07/12] Use ProjectFinder to filter the projects Instead of trying to do the heavy lifting in the API itself, use the existing features of the ProjectFinder. --- lib/api/helpers.rb | 13 ------------- lib/api/projects.rb | 4 ++++ 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 0dc33eb2097..2855bd7385d 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -261,19 +261,6 @@ module API projects = projects.merge(current_user.owned_projects) end - if params[:starred] - projects = projects.merge(current_user.starred_projects) - end - - if params[:search].present? - projects = projects.search(params[:search]) - end - - if params[:visibility].present? - projects = projects.search_by_visibility(params[:visibility]) - end - - projects = projects.where(archived: params[:archived]) projects.reorder(params[:order_by] => params[:sort]) end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index bb03480f1ee..7610e3cbacc 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -70,6 +70,10 @@ module API def present_projects(options = {}) finder_params = {} finder_params[:non_public] = true if params[:membership].present? + finder_params[:starred] = true if params[:starred].present? + finder_params[:visibility_level] = Gitlab::VisibilityLevel.level_value(params[:visibility]) if params[:visibility] + finder_params[:archived] = params[:archived] + finder_params[:search] = params[:search] if params[:search] projects = ProjectsFinder.new(current_user: current_user, params: finder_params).execute projects = filter_projects(projects) From 01c6323d238706bc8d0acb0273552a094c996ca0 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 24 May 2017 15:03:45 +0200 Subject: [PATCH 08/12] UNION of SELECT/WHERE is faster than WHERE on UNION Instead of applying WHERE on a UNION, apply the WHERE on each of the seperate SELECT statements, and do UNION on that. Local tests with about 2_000_000 projects: - 1_500_000 private projects - 40_000 internal projects - 400_000 public projects For the API endpoint `/api/v4/projects?visibility=private` the slowest query was: ```sql SELECT "projects".* FROM "projects" WHERE ... ``` The original query took 1073.8ms. The query refactored to UNION of SELECT/WHERE took 2.3ms. The original query was: ```sql SELECT "projects".* FROM "projects" WHERE "projects"."pending_delete" = $1 AND (projects.id IN (SELECT "projects"."id" FROM "projects" INNER JOIN "project_authorizations" ON "projects"."id" = "project_authorizations"."project_id" WHERE "projects"."pending_delete" = 'f' AND "project_authorizations"."user_id" = 23 UNION SELECT "projects"."id" FROM "projects" WHERE "projects"."visibility_level" IN (20, 10))) AND "projects"."visibility_level" = $2 AND "projects"."archived" = $3 ORDER BY "projects"."created_at" DESC LIMIT 20 OFFSET 0 [["pending_delete", "f"], ["visibility_level", 0], ["archived", "f"]] ``` The refactored query: ```sql SELECT "projects".* FROM "projects" WHERE "projects"."pending_delete" = $1 AND (projects.id IN (SELECT "projects"."id" FROM "projects" INNER JOIN "project_authorizations" ON "projects"."id" = "project_authorizations"."project_id" WHERE "projects"."pending_delete" = 'f' AND "project_authorizations"."user_id" = 23 AND "projects"."visibility_level" = 0 AND "projects"."archived" = 'f' UNION SELECT "projects"."id" FROM "projects" WHERE "projects"."visibility_level" IN (20, 10) AND "projects"."visibility_level" = 0 AND "projects"."archived" = 'f')) ORDER BY "projects"."created_at" DESC LIMIT 20 OFFSET 0 [["pending_delete", "f"]] ``` --- app/finders/projects_finder.rb | 18 ++++++++++-------- .../unreleased/tc-improve-project-api-perf.yml | 4 ++++ 2 files changed, 14 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/tc-improve-project-api-perf.yml diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 588406982d7..1d365e7cd7d 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -28,14 +28,16 @@ class ProjectsFinder < UnionFinder def execute items = init_collection - items = by_ids(items) + items = items.map do |item| + item = by_ids(item) + item = by_personal(item) + item = by_starred(item) + item = by_visibilty_level(item) + item = by_tags(item) + item = by_search(item) + by_archived(item) + end items = union(items) - items = by_personal(items) - items = by_starred(items) - items = by_visibilty_level(items) - items = by_tags(items) - items = by_search(items) - items = by_archived(items) sort(items) end @@ -55,7 +57,7 @@ class ProjectsFinder < UnionFinder end def by_ids(items) - project_ids_relation ? items.map { |item| item.where(id: project_ids_relation) } : items + project_ids_relation ? items.where(id: project_ids_relation) : items end def union(items) diff --git a/changelogs/unreleased/tc-improve-project-api-perf.yml b/changelogs/unreleased/tc-improve-project-api-perf.yml new file mode 100644 index 00000000000..7e88466c058 --- /dev/null +++ b/changelogs/unreleased/tc-improve-project-api-perf.yml @@ -0,0 +1,4 @@ +--- +title: Improve performance of ProjectFinder used in /projects API endpoint +merge_request: 11666 +author: From 0f0b9a8466747f69e210fc27778f96ab8ef628bc Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 24 May 2017 22:12:40 +0200 Subject: [PATCH 09/12] Use helper to construct Finder params The ProjectsFinder and GroupFinder both support the same set of params. And the `/api/v4/projects` and `/api/v4/group/:id/projects` also support the same set of params. But they do not match the Finder params. So use a helper method to transform them. --- lib/api/groups.rb | 2 +- lib/api/helpers.rb | 10 ++++++++++ lib/api/projects.rb | 9 +-------- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/api/groups.rb b/lib/api/groups.rb index ee85b777aff..aacc3356a0e 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -151,7 +151,7 @@ module API end get ":id/projects" do group = find_group!(params[:id]) - projects = GroupProjectsFinder.new(group: group, current_user: current_user).execute + projects = GroupProjectsFinder.new(group: group, current_user: current_user, params: project_finder_params).execute projects = filter_projects(projects) entity = params[:simple] ? Entities::BasicProjectDetails : Entities::Project present paginate(projects), with: entity, current_user: current_user diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 2855bd7385d..17f57cfb8d7 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -264,6 +264,16 @@ module API projects.reorder(params[:order_by] => params[:sort]) end + def project_finder_params + finder_params = {} + finder_params[:non_public] = true if params[:membership].present? + finder_params[:starred] = true if params[:starred].present? + finder_params[:visibility_level] = Gitlab::VisibilityLevel.level_value(params[:visibility]) if params[:visibility] + finder_params[:archived] = params[:archived] + finder_params[:search] = params[:search] if params[:search] + finder_params + end + # file helpers def uploaded_file(field, uploads_path) diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 7610e3cbacc..267dd2a74d7 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -68,14 +68,7 @@ module API end def present_projects(options = {}) - finder_params = {} - finder_params[:non_public] = true if params[:membership].present? - finder_params[:starred] = true if params[:starred].present? - finder_params[:visibility_level] = Gitlab::VisibilityLevel.level_value(params[:visibility]) if params[:visibility] - finder_params[:archived] = params[:archived] - finder_params[:search] = params[:search] if params[:search] - - projects = ProjectsFinder.new(current_user: current_user, params: finder_params).execute + projects = ProjectsFinder.new(current_user: current_user, params: project_finder_params).execute projects = filter_projects(projects) projects = projects.with_statistics if params[:statistics] projects = projects.with_issues_enabled if params[:with_issues_enabled] From 5654ac877df5b6007606e0e1827d965bdf8e552b Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Fri, 26 May 2017 15:39:38 +0200 Subject: [PATCH 10/12] Make it possible to combine :trending with other params Now it is possible to combine the :non_public parameter. This might be useful when a user wants to know the trending projects they are member of. --- app/finders/projects_finder.rb | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 1d365e7cd7d..866d95ea1d7 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -32,6 +32,7 @@ class ProjectsFinder < UnionFinder item = by_ids(item) item = by_personal(item) item = by_starred(item) + item = by_trending(item) item = by_visibilty_level(item) item = by_tags(item) item = by_search(item) @@ -46,12 +47,8 @@ class ProjectsFinder < UnionFinder def init_collection projects = [] - if params[:trending].present? - projects << Project.trending - else - projects << current_user.authorized_projects if current_user - projects << Project.unscoped.public_to_user(current_user) unless params[:non_public].present? - end + projects << current_user.authorized_projects if current_user + projects << Project.unscoped.public_to_user(current_user) unless params[:non_public].present? projects end @@ -72,6 +69,10 @@ class ProjectsFinder < UnionFinder (params[:starred].present? && current_user) ? items.starred_by(current_user) : items end + def by_trending(items) + params[:trending].present? ? items.trending : items + end + def by_visibilty_level(items) params[:visibility_level].present? ? items.where(visibility_level: params[:visibility_level]) : items end From db679788e46d55984a4af71034c6db11aed919e4 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Fri, 26 May 2017 16:31:37 +0200 Subject: [PATCH 11/12] Add :owned param to ProjectFinder And use it in the API. --- app/finders/projects_finder.rb | 9 +++++++-- lib/api/groups.rb | 2 +- lib/api/helpers.rb | 7 ++----- lib/api/projects.rb | 2 +- spec/finders/projects_finder_spec.rb | 7 +++++++ 5 files changed, 18 insertions(+), 9 deletions(-) diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 866d95ea1d7..5bf722d1ec6 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -7,6 +7,7 @@ # project_ids_relation: int[] - project ids to use # params: # trending: boolean +# owned: boolean # non_public: boolean # starred: boolean # sort: string @@ -47,8 +48,12 @@ class ProjectsFinder < UnionFinder def init_collection projects = [] - projects << current_user.authorized_projects if current_user - projects << Project.unscoped.public_to_user(current_user) unless params[:non_public].present? + if params[:owned].present? + projects << current_user.owned_projects if current_user + else + projects << current_user.authorized_projects if current_user + projects << Project.unscoped.public_to_user(current_user) unless params[:non_public].present? + end projects end diff --git a/lib/api/groups.rb b/lib/api/groups.rb index aacc3356a0e..e14a988a153 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -152,7 +152,7 @@ module API get ":id/projects" do group = find_group!(params[:id]) projects = GroupProjectsFinder.new(group: group, current_user: current_user, params: project_finder_params).execute - projects = filter_projects(projects) + projects = reorder_projects(projects) entity = params[:simple] ? Entities::BasicProjectDetails : Entities::Project present paginate(projects), with: entity, current_user: current_user end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 17f57cfb8d7..d61450f8258 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -256,16 +256,13 @@ module API # project helpers - def filter_projects(projects) - if params[:owned] - projects = projects.merge(current_user.owned_projects) - end - + def reorder_projects(projects) projects.reorder(params[:order_by] => params[:sort]) end def project_finder_params finder_params = {} + finder_params[:owned] = true if params[:owned].present? finder_params[:non_public] = true if params[:membership].present? finder_params[:starred] = true if params[:starred].present? finder_params[:visibility_level] = Gitlab::VisibilityLevel.level_value(params[:visibility]) if params[:visibility] diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 267dd2a74d7..1356f959e70 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -69,7 +69,7 @@ module API def present_projects(options = {}) projects = ProjectsFinder.new(current_user: current_user, params: project_finder_params).execute - projects = filter_projects(projects) + projects = reorder_projects(projects) projects = projects.with_statistics if params[:statistics] projects = projects.with_issues_enabled if params[:with_issues_enabled] projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled] diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index 077f2624388..03d98459e8c 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -137,6 +137,13 @@ describe ProjectsFinder do it { is_expected.to eq([public_project]) } end + describe 'filter by owned' do + let(:params) { { owned: true } } + let!(:owned_project) { create(:empty_project, :private, namespace: current_user.namespace) } + + it { is_expected.to eq([owned_project]) } + end + describe 'filter by non_public' do let(:params) { { non_public: true } } before do From 1e5506d01619780da68fc51ada58188a9070255b Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 30 May 2017 23:24:17 +0200 Subject: [PATCH 12/12] Remove some deprecated methods To avoid the use of slow queries, remove some deprecated methods and encourage the use of ProjectFinder to find projects. --- app/controllers/dashboard_controller.rb | 2 +- app/models/project.rb | 4 ---- app/models/user.rb | 6 ------ lib/api/v3/helpers.rb | 2 +- lib/api/v3/projects.rb | 2 +- spec/models/user_spec.rb | 19 ------------------- 6 files changed, 3 insertions(+), 32 deletions(-) diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 6195121b931..f9c31920302 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -24,7 +24,7 @@ class DashboardController < Dashboard::ApplicationController def load_events projects = if params[:filter] == "starred" - current_user.viewable_starred_projects + ProjectsFinder.new(current_user: current_user, params: { starred: true }).execute else current_user.authorized_projects end diff --git a/app/models/project.rb b/app/models/project.rb index 963fd594d46..457399cb60e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -351,10 +351,6 @@ class Project < ActiveRecord::Base where("projects.id IN (#{union.to_sql})") end - def search_by_visibility(level) # DEPRECATED: remove with API V3 - where(visibility_level: Gitlab::VisibilityLevel.string_options[level]) - end - def search_by_title(query) pattern = "%#{query}%" table = Project.arel_table diff --git a/app/models/user.rb b/app/models/user.rb index 20894ce269a..9aad327b592 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -557,12 +557,6 @@ class User < ActiveRecord::Base authorized_projects(Gitlab::Access::REPORTER).where(id: projects) end - def viewable_starred_projects # DEPRECATED: Use ProjectFinder instead. Remove together with API V3 - starred_projects.where("projects.visibility_level IN (?) OR projects.id IN (?)", - [Project::PUBLIC, Project::INTERNAL], - authorized_projects.select(:project_id)) - end - def owned_projects @owned_projects ||= Project.where('namespace_id IN (?) OR namespace_id = ?', diff --git a/lib/api/v3/helpers.rb b/lib/api/v3/helpers.rb index 06af286ef50..d9e76560d03 100644 --- a/lib/api/v3/helpers.rb +++ b/lib/api/v3/helpers.rb @@ -35,7 +35,7 @@ module API end if params[:visibility].present? - projects = projects.search_by_visibility(params[:visibility]) + projects = projects.where(visibility_level: Gitlab::VisibilityLevel.level_value(params[:visibility])) end projects = projects.where(archived: params[:archived]) diff --git a/lib/api/v3/projects.rb b/lib/api/v3/projects.rb index 164612cb8dd..896c00b88e7 100644 --- a/lib/api/v3/projects.rb +++ b/lib/api/v3/projects.rb @@ -147,7 +147,7 @@ module API get '/starred' do authenticate! - present_projects current_user.viewable_starred_projects + present_projects ProjectsFinder.new(current_user: current_user, params: { starred: true }).execute end desc 'Get all projects for admin user' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 9edf34b05ad..fe9df3360ff 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1496,25 +1496,6 @@ describe User, models: true do end end - describe '#viewable_starred_projects' do - let(:user) { create(:user) } - let(:public_project) { create(:empty_project, :public) } - let(:private_project) { create(:empty_project, :private) } - let(:private_viewable_project) { create(:empty_project, :private) } - - before do - private_viewable_project.team << [user, Gitlab::Access::MASTER] - - [public_project, private_project, private_viewable_project].each do |project| - user.toggle_star(project) - end - end - - it 'returns only starred projects the user can view' do - expect(user.viewable_starred_projects).not_to include(private_project) - end - end - describe '#projects_with_reporter_access_limited_to' do let(:project1) { create(:empty_project) } let(:project2) { create(:empty_project) }