Merge branch 'pedropombeiro/331600/add-sort-options-to-runner-api' into 'master'

Add 2 new sort options to runner API: CREATED_ASC and CONTACTED_DESC

See merge request gitlab-org/gitlab!62310

app/finders/ci/runners_finder.rb

@@ -4,6 +4,9 @@ module Ci
   class RunnersFinder < UnionFinder
     include Gitlab::Allowable
 
+    ALLOWED_SORTS = %w[contacted_asc contacted_desc created_at_asc created_at_desc created_date].freeze
+    DEFAULT_SORT = 'created_at_desc'
+
     def initialize(current_user:, group: nil, params:)
       @params = params
       @group = group
@@ -24,11 +27,7 @@ module Ci
     end
 
     def sort_key
-      if @params[:sort] == 'contacted_asc'
-        'contacted_asc'
-      else
-        'created_date'
-      end
+      ALLOWED_SORTS.include?(@params[:sort]) ? @params[:sort] : DEFAULT_SORT
     end
 
     private

app/graphql/types/ci/runner_sort_enum.rb

@@ -7,7 +7,9 @@ module Types
       description 'Values for sorting runners'
 
       value 'CONTACTED_ASC', 'Ordered by contacted_at in ascending order.', value: :contacted_asc
-      value 'CREATED_DESC', 'Ordered by created_date in descending order.', value: :created_date
+      value 'CONTACTED_DESC', 'Ordered by contacted_at in descending order.', value: :contacted_desc
+      value 'CREATED_ASC', 'Ordered by created_at in ascending order.', value: :created_at_asc
+      value 'CREATED_DESC', 'Ordered by created_at in descending order.', value: :created_at_desc
     end
   end
 end

app/models/ci/runner.rb

@@ -134,6 +134,8 @@ module Ci
     end
 
     scope :order_contacted_at_asc, -> { order(contacted_at: :asc) }
+    scope :order_contacted_at_desc, -> { order(contacted_at: :desc) }
+    scope :order_created_at_asc, -> { order(created_at: :asc) }
     scope :order_created_at_desc, -> { order(created_at: :desc) }
     scope :with_tags, -> { preload(:tags) }
 
@@ -190,8 +192,13 @@ module Ci
     end
 
     def self.order_by(order)
-      if order == 'contacted_asc'
+      case order
+      when 'contacted_asc'
         order_contacted_at_asc
+      when 'contacted_desc'
+        order_contacted_at_desc
+      when 'created_at_asc'
+        order_created_at_asc
       else
         order_created_at_desc
       end

db/migrate/20210601125410_add_runners_created_at_index.rb

+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddRunnersCreatedAtIndex < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :ci_runners, [:created_at, :id], order: { id: :desc }, name: 'index_ci_runners_on_created_at_and_id_desc'
+    add_concurrent_index :ci_runners, [:created_at, :id], order: { created_at: :desc, id: :desc }, name: 'index_ci_runners_on_created_at_desc_and_id_desc'
+  end
+
+  def down
+    remove_concurrent_index :ci_runners, [:created_at, :id], order: { id: :desc }, name: 'index_ci_runners_on_created_at_and_id_desc'
+    remove_concurrent_index :ci_runners, [:created_at, :id], order: { created_at: :desc, id: :desc }, name: 'index_ci_runners_on_created_at_desc_and_id_desc'
+  end
+end

db/migrate/20210601133459_replace_runners_contacted_at_index.rb

+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class ReplaceRunnersContactedAtIndex < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  OLD_INDEX_NAME = 'index_ci_runners_on_contacted_at'
+
+  def up
+    add_concurrent_index :ci_runners, [:contacted_at, :id], order: { id: :desc }, name: 'index_ci_runners_on_contacted_at_and_id_desc', using: 'btree'
+    add_concurrent_index :ci_runners, [:contacted_at, :id], order: { contacted_at: :desc, id: :desc }, name: 'index_ci_runners_on_contacted_at_desc_and_id_desc', using: 'btree'
+
+    remove_concurrent_index_by_name :ci_runners, OLD_INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :ci_runners, 'index_ci_runners_on_contacted_at_and_id_desc'
+    remove_concurrent_index_by_name :ci_runners, 'index_ci_runners_on_contacted_at_desc_and_id_desc'
+
+    add_concurrent_index :ci_runners, :contacted_at, name: OLD_INDEX_NAME, using: 'btree'
+  end
+end

db/schema_migrations/20210601125410

+fc500e4dd555a6baad91ad3c9fb8a2f8541e1613dd64afdbdd28b19447a28caf
\ No newline at end of file

db/schema_migrations/20210601133459

+4dcf6277439e8abe52534540100fa621fedcecb3eaf71ad5685ac0230cd2e5bb
\ No newline at end of file

db/structure.sql

@@ -22904,7 +22904,13 @@ CREATE INDEX index_ci_runner_projects_on_project_id ON ci_runner_projects USING
 
 CREATE INDEX index_ci_runner_projects_on_runner_id ON ci_runner_projects USING btree (runner_id);
 
-CREATE INDEX index_ci_runners_on_contacted_at ON ci_runners USING btree (contacted_at);
+CREATE INDEX index_ci_runners_on_contacted_at_and_id_desc ON ci_runners USING btree (contacted_at, id DESC);
+
+CREATE INDEX index_ci_runners_on_contacted_at_desc_and_id_desc ON ci_runners USING btree (contacted_at DESC, id DESC);
+
+CREATE INDEX index_ci_runners_on_created_at_and_id_desc ON ci_runners USING btree (created_at, id DESC);
+
+CREATE INDEX index_ci_runners_on_created_at_desc_and_id_desc ON ci_runners USING btree (created_at DESC, id DESC);
 
 CREATE INDEX index_ci_runners_on_description_trigram ON ci_runners USING gin (description gin_trgm_ops);
 

doc/api/graphql/reference/index.md

@@ -13843,7 +13843,9 @@ Values for sorting runners.
 | Value | Description |
 | ----- | ----------- |
 | <a id="cirunnersortcontacted_asc"></a>`CONTACTED_ASC` | Ordered by contacted_at in ascending order. |
-| <a id="cirunnersortcreated_desc"></a>`CREATED_DESC` | Ordered by created_date in descending order. |
+| <a id="cirunnersortcontacted_desc"></a>`CONTACTED_DESC` | Ordered by contacted_at in descending order. |
+| <a id="cirunnersortcreated_asc"></a>`CREATED_ASC` | Ordered by created_at in ascending order. |
+| <a id="cirunnersortcreated_desc"></a>`CREATED_DESC` | Ordered by created_at in descending order. |
 
 ### `CiRunnerStatus`
 

spec/finders/ci/runners_finder_spec.rb

@@ -51,23 +51,55 @@ RSpec.describe Ci::RunnersFinder do
       end
 
       context 'sort' do
+        let_it_be(:runner1) { create :ci_runner, created_at: '2018-07-12 07:00', contacted_at: 1.minute.ago }
+        let_it_be(:runner2) { create :ci_runner, created_at: '2018-07-12 08:00', contacted_at: 3.minutes.ago }
+        let_it_be(:runner3) { create :ci_runner, created_at: '2018-07-12 09:00', contacted_at: 2.minutes.ago }
+
+        subject do
+          described_class.new(current_user: admin, params: params).execute
+        end
+
+        shared_examples 'sorts by created_at descending' do
+          it 'sorts by created_at descending' do
+            is_expected.to eq [runner3, runner2, runner1]
+          end
+        end
+
         context 'without sort param' do
-          it 'sorts by created_at' do
-            runner1 = create :ci_runner, created_at: '2018-07-12 07:00'
-            runner2 = create :ci_runner, created_at: '2018-07-12 08:00'
-            runner3 = create :ci_runner, created_at: '2018-07-12 09:00'
+          let(:params) { {} }
+
+          it_behaves_like 'sorts by created_at descending'
+        end
+
+        %w(created_date created_at_desc).each do |sort|
+          context "with sort param equal to #{sort}" do
+            let(:params) { { sort: sort } }
+
+            it_behaves_like 'sorts by created_at descending'
+          end
+        end
+
+        context 'with sort param equal to created_at_asc' do
+          let(:params) { { sort: 'created_at_asc' } }
+
+          it 'sorts by created_at ascending' do
+            is_expected.to eq [runner1, runner2, runner3]
+          end
+        end
+
+        context 'with sort param equal to contacted_asc' do
+          let(:params) { { sort: 'contacted_asc' } }
 
-            expect(described_class.new(current_user: admin, params: {}).execute).to eq [runner3, runner2, runner1]
+          it 'sorts by contacted_at ascending' do
+            is_expected.to eq [runner2, runner3, runner1]
           end
         end
 
-        context 'with sort param' do
-          it 'sorts by specified attribute' do
-            runner1 = create :ci_runner, contacted_at: 1.minute.ago
-            runner2 = create :ci_runner, contacted_at: 3.minutes.ago
-            runner3 = create :ci_runner, contacted_at: 2.minutes.ago
+        context 'with sort param equal to contacted_desc' do
+          let(:params) { { sort: 'contacted_desc' } }
 
-            expect(described_class.new(current_user: admin, params: { sort: 'contacted_asc' }).execute).to eq [runner2, runner3, runner1]
+          it 'sorts by contacted_at descending' do
+            is_expected.to eq [runner1, runner3, runner2]
           end
         end
       end
@@ -246,8 +278,8 @@ RSpec.describe Ci::RunnersFinder do
       subject { described_class.new(current_user: user, group: group, params: params).sort_key }
 
       context 'no params' do
-        it 'returns created_date' do
-          expect(subject).to eq('created_date')
+        it 'returns created_at_desc' do
+          expect(subject).to eq('created_at_desc')
         end
       end
 

spec/graphql/resolvers/ci/runners_resolver_spec.rb

@@ -50,13 +50,29 @@ RSpec.describe Resolvers::Ci::RunnersResolver do
         it { is_expected.to eq([offline_project_runner, instance_runner, inactive_project_runner, group_runner]) }
       end
 
-      context "set to :created_date" do
+      context "set to :contacted_desc" do
         let(:args) do
-          { sort: :created_date }
+          { sort: :contacted_desc }
+        end
+
+        it { is_expected.to eq([offline_project_runner, instance_runner, inactive_project_runner, group_runner].reverse) }
+      end
+
+      context "set to :created_at_desc" do
+        let(:args) do
+          { sort: :created_at_desc }
         end
 
         it { is_expected.to eq([instance_runner, group_runner, offline_project_runner, inactive_project_runner]) }
       end
+
+      context "set to :created_at_asc" do
+        let(:args) do
+          { sort: :created_at_asc }
+        end
+
+        it { is_expected.to eq([instance_runner, group_runner, offline_project_runner, inactive_project_runner].reverse) }
+      end
     end
 
     context 'when type is filtered' do