Merge branch 'projects-api-sort-by-statistics' into 'master'

Allow advanced API projects filtering for admins

See merge request gitlab-org/gitlab!32879

app/finders/projects_finder.rb

@@ -23,6 +23,7 @@
 #     min_access_level: integer
 #     last_activity_after: datetime
 #     last_activity_before: datetime
+#     repository_storage: string
 #
 class ProjectsFinder < UnionFinder
   include CustomAttributesFilter
@@ -75,6 +76,7 @@ class ProjectsFinder < UnionFinder
     collection = by_deleted_status(collection)
     collection = by_last_activity_after(collection)
     collection = by_last_activity_before(collection)
+    collection = by_repository_storage(collection)
     collection
   end
 
@@ -197,6 +199,14 @@ class ProjectsFinder < UnionFinder
     end
   end
 
+  def by_repository_storage(items)
+    if params[:repository_storage].present?
+      items.where(repository_storage: params[:repository_storage]) # rubocop: disable CodeReuse/ActiveRecord
+    else
+      items
+    end
+  end
+
   def sort(items)
     params[:sort].present? ? items.sort_by_attribute(params[:sort]) : items.projects_order_id_desc
   end

app/policies/global_policy.rb

@@ -105,6 +105,9 @@ class GlobalPolicy < BasePolicy
     enable :update_custom_attribute
   end
 
+  # We can't use `read_statistics` because the user may have different permissions for different projects
+  rule { admin }.enable :use_project_statistics_filters
+
   rule { external_user }.prevent :create_snippet
 end
 

changelogs/unreleased/projects-api-sort-by-statistics.yml

+---
+title: Allow advanced API projects filtering for admins
+merge_request: 32879
+author:
+type: added

db/migrate/20200605160806_add_index_on_repository_size_and_project_id_to_project_statistics.rb

+# frozen_string_literal: true
+
+class AddIndexOnRepositorySizeAndProjectIdToProjectStatistics < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :project_statistics, [:repository_size, :project_id]
+  end
+
+  def down
+    remove_concurrent_index :project_statistics, [:repository_size, :project_id]
+  end
+end

db/migrate/20200605160836_add_index_on_storage_size_and_project_id_to_project_statistics.rb

+# frozen_string_literal: true
+
+class AddIndexOnStorageSizeAndProjectIdToProjectStatistics < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :project_statistics, [:storage_size, :project_id]
+  end
+
+  def down
+    remove_concurrent_index :project_statistics, [:storage_size, :project_id]
+  end
+end

db/migrate/20200605160851_add_index_on_wiki_size_and_project_id_to_project_statistics.rb

+# frozen_string_literal: true
+
+class AddIndexOnWikiSizeAndProjectIdToProjectStatistics < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :project_statistics, [:wiki_size, :project_id]
+  end
+
+  def down
+    remove_concurrent_index :project_statistics, [:wiki_size, :project_id]
+  end
+end

db/structure.sql

@@ -10643,6 +10643,12 @@ CREATE INDEX index_project_statistics_on_namespace_id ON public.project_statisti
 
 CREATE UNIQUE INDEX index_project_statistics_on_project_id ON public.project_statistics USING btree (project_id);
 
+CREATE INDEX index_project_statistics_on_repository_size_and_project_id ON public.project_statistics USING btree (repository_size, project_id);
+
+CREATE INDEX index_project_statistics_on_storage_size_and_project_id ON public.project_statistics USING btree (storage_size, project_id);
+
+CREATE INDEX index_project_statistics_on_wiki_size_and_project_id ON public.project_statistics USING btree (wiki_size, project_id);
+
 CREATE UNIQUE INDEX index_project_tracing_settings_on_project_id ON public.project_tracing_settings USING btree (project_id);
 
 CREATE INDEX index_projects_api_created_at_id_desc ON public.projects USING btree (created_at, id DESC);
@@ -14055,6 +14061,9 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200604174558
 20200605003204
 20200605093113
+20200605160806
+20200605160836
+20200605160851
 20200608072931
 20200608075553
 20200608214008

doc/api/projects.md

@@ -45,7 +45,7 @@ GET /projects
 | --------- | ---- | -------- | ----------- |
 | `archived`                    | boolean  | no | Limit by archived status |
 | `visibility`                  | string   | no | Limit by visibility `public`, `internal`, or `private` |
-| `order_by`                    | string   | no | Return projects ordered by `id`, `name`, `path`, `created_at`, `updated_at`, or `last_activity_at` fields. Default is `created_at` |
+| `order_by`                    | string   | no | Return projects ordered by `id`, `name`, `path`, `created_at`, `updated_at`, or `last_activity_at` fields. `repository_size`, `storage_size`, or `wiki_size` fields are only allowed for admins. Default is `created_at` |
 | `sort`                        | string   | no | Return projects sorted in `asc` or `desc` order. Default is `desc` |
 | `search`                      | string   | no | Return list of projects matching the search criteria |
 | `search_namespaces`           | boolean  | no | Include ancestor namespaces when matching search criteria. Default is `false` |
@@ -65,6 +65,7 @@ GET /projects
 | `id_before`                   | integer  | no | Limit results to projects with IDs less than the specified ID |
 | `last_activity_after`         | datetime | no | Limit results to projects with last_activity after specified time. Format: ISO 8601 YYYY-MM-DDTHH:MM:SSZ |
 | `last_activity_before`        | datetime | no | Limit results to projects with last_activity before specified time. Format: ISO 8601 YYYY-MM-DDTHH:MM:SSZ |
+| `repository_storage`          | string   | no | Limit results to projects stored on repository_storage. Available for admins only. |
 
 NOTE: **Note:**
 This endpoint supports [keyset pagination](README.md#keyset-based-pagination) for selected `order_by` options.

lib/api/helpers.rb

@@ -543,6 +543,7 @@ module API
       finder_params[:id_before] = params[:id_before] if params[:id_before]
       finder_params[:last_activity_after] = params[:last_activity_after] if params[:last_activity_after]
       finder_params[:last_activity_before] = params[:last_activity_before] if params[:last_activity_before]
+      finder_params[:repository_storage] = params[:repository_storage] if params[:repository_storage]
       finder_params
     end
 

lib/api/helpers/projects_helpers.rb

@@ -6,6 +6,8 @@ module API
       extend ActiveSupport::Concern
       extend Grape::API::Helpers
 
+      STATISTICS_SORT_PARAMS = %w[storage_size repository_size wiki_size].freeze
+
       params :optional_project_params_ce do
         optional :description, type: String, desc: 'The description of the project'
         optional :build_git_strategy, type: String, values: %w(fetch clone), desc: 'The Git strategy. Defaults to `fetch`'

lib/api/projects.rb

@@ -17,6 +17,7 @@ module API
         projects = projects.with_issues_available_for_user(current_user) if params[:with_issues_enabled]
         projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled]
         projects = projects.with_statistics if params[:statistics]
+        projects = projects.joins(:statistics) if params[:order_by].include?('project_statistics') # rubocop: disable CodeReuse/ActiveRecord
 
         lang = params[:with_programming_language]
         projects = projects.with_programming_language(lang) if lang
@@ -28,6 +29,20 @@ module API
         attrs.delete(:repository_storage) unless can?(current_user, :change_repository_storage, project)
       end
 
+      def verify_project_filters!(attrs)
+        attrs.delete(:repository_storage) unless can?(current_user, :use_project_statistics_filters)
+      end
+
+      def verify_statistics_order_by_projects!
+        return unless Helpers::ProjectsHelpers::STATISTICS_SORT_PARAMS.include?(params[:order_by])
+
+        params[:order_by] = if can?(current_user, :use_project_statistics_filters)
+                              "project_statistics.#{params[:order_by]}"
+                            else
+                              route.params['order_by'][:default]
+                            end
+      end
+
       def delete_project(user_project)
         destroy_conditionally!(user_project) do
           ::Projects::DestroyService.new(user_project, current_user, {}).async_execute
@@ -52,8 +67,9 @@ module API
       end
 
       params :sort_params do
-        optional :order_by, type: String, values: %w[id name path created_at updated_at last_activity_at],
-                            default: 'created_at', desc: 'Return projects ordered by field'
+        optional :order_by, type: String,
+                            values: %w[id name path created_at updated_at last_activity_at] + Helpers::ProjectsHelpers::STATISTICS_SORT_PARAMS,
+                            default: 'created_at', desc: "Return projects ordered by field. #{Helpers::ProjectsHelpers::STATISTICS_SORT_PARAMS.join(', ')} are only available to admins."
         optional :sort, type: String, values: %w[asc desc], default: 'desc',
                         desc: 'Return projects sorted in ascending and descending order'
       end
@@ -75,6 +91,7 @@ module API
         optional :id_before, type: Integer, desc: 'Limit results to projects with IDs less than the specified ID'
         optional :last_activity_after, type: DateTime, desc: 'Limit results to projects with last_activity after specified time. Format: ISO 8601 YYYY-MM-DDTHH:MM:SSZ'
         optional :last_activity_before, type: DateTime, desc: 'Limit results to projects with last_activity before specified time. Format: ISO 8601 YYYY-MM-DDTHH:MM:SSZ'
+        optional :repository_storage, type: String, desc: 'Which storage shard the repository is on. Available only to admins'
 
         use :optional_filter_params_ee
       end
@@ -88,10 +105,15 @@ module API
       end
 
       def load_projects
-        ProjectsFinder.new(current_user: current_user, params: project_finder_params).execute
+        params = project_finder_params
+        verify_project_filters!(params)
+
+        ProjectsFinder.new(current_user: current_user, params: params).execute
       end
 
       def present_projects(projects, options = {})
+        verify_statistics_order_by_projects!
+
         projects = reorder_projects(projects)
         projects = apply_filters(projects)
 

spec/finders/projects_finder_spec.rb

@@ -262,6 +262,17 @@ RSpec.describe ProjectsFinder, :do_not_mock_admin_mode do
         it { is_expected.to match_array([public_project]) }
       end
 
+      describe 'filter by repository_storage' do
+        let(:params) { { repository_storage: 'nfs-05' } }
+        let!(:project) { create(:project, :public) }
+
+        before do
+          project.update_columns(repository_storage: 'nfs-05')
+        end
+
+        it { is_expected.to match_array([project]) }
+      end
+
       describe 'sorting' do
         let(:params) { { sort: 'name_asc' } }
 

spec/policies/global_policy_spec.rb

@@ -130,6 +130,24 @@ describe GlobalPolicy do
     end
   end
 
+  describe 'using project statistics filters' do
+    context 'regular user' do
+      it { is_expected.not_to be_allowed(:use_project_statistics_filters) }
+    end
+
+    context 'admin' do
+      let(:current_user) { create(:user, :admin) }
+
+      context 'when admin mode is enabled', :enable_admin_mode do
+        it { is_expected.to be_allowed(:use_project_statistics_filters) }
+      end
+
+      context 'when admin mode is disabled' do
+        it { is_expected.to be_disallowed(:use_project_statistics_filters) }
+      end
+    end
+  end
+
   shared_examples 'access allowed when terms accepted' do |ability|
     it { is_expected.not_to be_allowed(ability) }
 

spec/requests/api/projects_spec.rb

@@ -584,6 +584,85 @@ describe API::Projects do
       end
     end
 
+    context 'sorting by project statistics' do
+      %w(repository_size storage_size wiki_size).each do |order_by|
+        context "sorting by #{order_by}" do
+          before do
+            ProjectStatistics.update_all(order_by => 100)
+            project4.statistics.update_columns(order_by => 10)
+            project.statistics.update_columns(order_by => 200)
+          end
+
+          context 'admin user' do
+            let(:current_user) { admin }
+
+            context "when sorting by #{order_by} ascendingly" do
+              it 'returns a properly sorted list of projects' do
+                get api('/projects', current_user), params: { order_by: order_by, sort: :asc }
+
+                expect(response).to have_gitlab_http_status(:ok)
+                expect(response).to include_pagination_headers
+                expect(json_response).to be_an Array
+                expect(json_response.first['id']).to eq(project4.id)
+              end
+            end
+
+            context "when sorting by #{order_by} descendingly" do
+              it 'returns a properly sorted list of projects' do
+                get api('/projects', current_user), params: { order_by: order_by, sort: :desc }
+
+                expect(response).to have_gitlab_http_status(:ok)
+                expect(response).to include_pagination_headers
+                expect(json_response).to be_an Array
+                expect(json_response.first['id']).to eq(project.id)
+              end
+            end
+          end
+
+          context 'non-admin user' do
+            let(:current_user) { user }
+            let(:projects) { [public_project, project, project2, project3] }
+
+            it 'returns projects ordered normally' do
+              get api('/projects', current_user), params: { order_by: order_by }
+
+              expect(response).to have_gitlab_http_status(:ok)
+              expect(response).to include_pagination_headers
+              expect(json_response).to be_an Array
+              expect(json_response.map { |project| project['id'] }).to eq(projects.map(&:id).reverse)
+            end
+          end
+        end
+      end
+    end
+
+    context 'filtering by repository_storage' do
+      before do
+        [project, project3].each { |proj| proj.update_columns(repository_storage: 'nfs-11') }
+        # Since we don't actually have Gitaly configured with an nfs-11 storage, an error would be raised
+        # when we present the projects in a response, as we ask Gitaly for stuff like default branch and Gitaly
+        # is not configured for a nfs-11 storage. So we trick Rails into thinking the storage for these projects
+        # is still default (in reality, it is).
+        allow_any_instance_of(Project).to receive(:repository_storage).and_return('default')
+      end
+
+      context 'admin user' do
+        it_behaves_like 'projects response' do
+          let(:filter) { { repository_storage: 'nfs-11' } }
+          let(:current_user) { admin }
+          let(:projects) { [project, project3] }
+        end
+      end
+
+      context 'non-admin user' do
+        it_behaves_like 'projects response' do
+          let(:filter) { { repository_storage: 'nfs-11' } }
+          let(:current_user) { user }
+          let(:projects) { [public_project, project, project2, project3] }
+        end
+      end
+    end
+
     context 'with keyset pagination' do
       let(:current_user) { user }
       let(:projects) { [public_project, project, project2, project3] }