From 031caf212607282f06b9c080a7f55e3049b2c896 Mon Sep 17 00:00:00 2001 From: sujay patel Date: Thu, 13 Jun 2019 01:33:21 +0530 Subject: [PATCH 1/4] Adding order by to list runner jobs api. --- app/finders/runner_jobs_finder.rb | 19 +++++++- .../51794-add-ordering-to-runner-jobs-api.yml | 5 +++ doc/api/runners.md | 2 + lib/api/runners.rb | 2 + spec/finders/runner_jobs_finder_spec.rb | 22 ++++++++++ spec/requests/api/runners_spec.rb | 44 +++++++++++++++++++ 6 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml diff --git a/app/finders/runner_jobs_finder.rb b/app/finders/runner_jobs_finder.rb index 4fca4ec94f3..5e2c779cef8 100644 --- a/app/finders/runner_jobs_finder.rb +++ b/app/finders/runner_jobs_finder.rb @@ -3,6 +3,8 @@ class RunnerJobsFinder attr_reader :runner, :params + ALLOWED_INDEXED_COLUMNS = %w[id].freeze + def initialize(runner, params = {}) @runner = runner @params = params @@ -11,7 +13,7 @@ class RunnerJobsFinder def execute items = @runner.builds items = by_status(items) - items + sort_items(items) end private @@ -23,4 +25,19 @@ class RunnerJobsFinder items.where(status: params[:status]) end # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable CodeReuse/ActiveRecord + def sort_items(items) + return items unless ALLOWED_INDEXED_COLUMNS.include?(params[:order_by]) + + order_by = params[:order_by] + sort = if params[:sort].match?(/\A(ASC|DESC)\z/i) + params[:sort] + else + :desc + end + + items.order(order_by => sort) + end + # rubocop: enable CodeReuse/ActiveRecord end diff --git a/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml b/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml new file mode 100644 index 00000000000..908a132688c --- /dev/null +++ b/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml @@ -0,0 +1,5 @@ +--- +title: Add order_by and sort params to list runner jobs api +merge_request: 29629 +author: Sujay Patel +type: added diff --git a/doc/api/runners.md b/doc/api/runners.md index 2d91428d1c1..b5d25bf23a1 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -291,6 +291,8 @@ GET /runners/:id/jobs |-----------|---------|----------|---------------------| | `id` | integer | yes | The ID of a runner | | `status` | string | no | Status of the job; one of: `running`, `success`, `failed`, `canceled` | +| `order_by`| string | no | Order jobs by `id`. | +| `sort` | string | no | Sort jobs in `asc` or `desc` order (default: `desc`) | ``` curl --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/runners/1/jobs?status=running" diff --git a/lib/api/runners.rb b/lib/api/runners.rb index f3fea463e7f..c2d371b6867 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -115,6 +115,8 @@ module API params do requires :id, type: Integer, desc: 'The ID of the runner' optional :status, type: String, desc: 'Status of the job', values: Ci::Build::AVAILABLE_STATUSES + optional :order_by, type: String, desc: 'Order by `id` or not', values: RunnerJobsFinder::ALLOWED_INDEXED_COLUMNS + optional :sort, type: String, values: %w[asc desc], default: 'desc', desc: 'Sort by asc (ascending) or desc (descending)' use :pagination end get ':id/jobs' do diff --git a/spec/finders/runner_jobs_finder_spec.rb b/spec/finders/runner_jobs_finder_spec.rb index 97304170c4e..01f45a37ba8 100644 --- a/spec/finders/runner_jobs_finder_spec.rb +++ b/spec/finders/runner_jobs_finder_spec.rb @@ -35,5 +35,27 @@ describe RunnerJobsFinder do end end end + + context 'when order_by and sort are specified' do + context 'when order_by id and sort is asc' do + let(:params) { { order_by: 'id', sort: 'asc' } } + let!(:jobs) { create_list(:ci_build, 2, runner: runner, project: project, user: create(:user)) } + + it 'sorts as id: :asc' do + is_expected.to eq(jobs.sort_by(&:id)) + end + end + end + + context 'when order_by is specified and sort is not specified' do + context 'when order_by id and sort is not specified' do + let(:params) { { order_by: 'id' } } + let!(:jobs) { create_list(:ci_build, 2, runner: runner, project: project, user: create(:user)) } + + it 'sorts as id: :desc' do + is_expected.to eq(jobs.sort_by(&:id).reverse) + end + end + end end end diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 5548e3fd01a..f5ce3a3570e 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -584,6 +584,34 @@ describe API::Runners do end end + context 'when valid order_by is provided' do + context 'when sort order is not specified' do + it 'return jobs in descending order' do + get api("/runners/#{project_runner.id}/jobs?order_by=id", admin) + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + + expect(json_response).to be_an(Array) + expect(json_response.length).to eq(2) + expect(json_response.first).to include('id' => job_5.id) + end + end + + context 'when sort order is specified as asc' do + it 'return jobs sorted in ascending order' do + get api("/runners/#{project_runner.id}/jobs?order_by=id&sort=asc", admin) + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + + expect(json_response).to be_an(Array) + expect(json_response.length).to eq(2) + expect(json_response.first).to include('id' => job_4.id) + end + end + end + context 'when invalid status is provided' do it 'return 400' do get api("/runners/#{project_runner.id}/jobs?status=non-existing", admin) @@ -591,6 +619,22 @@ describe API::Runners do expect(response).to have_gitlab_http_status(400) end end + + context 'when invalid order_by is provided' do + it 'return 400' do + get api("/runners/#{project_runner.id}/jobs?order_by=non-existing", admin) + + expect(response).to have_gitlab_http_status(400) + end + end + + context 'when invalid sort is provided' do + it 'return 400' do + get api("/runners/#{project_runner.id}/jobs?sort=non-existing", admin) + + expect(response).to have_gitlab_http_status(400) + end + end end context "when runner doesn't exist" do From 7edeedc700cdb825aa885b53ceca8ebc145c1b23 Mon Sep 17 00:00:00 2001 From: sujay patel Date: Thu, 13 Jun 2019 01:33:21 +0530 Subject: [PATCH 2/4] Adding order by to list runner jobs api. --- app/finders/runner_jobs_finder.rb | 22 ++++++++++- .../51794-add-ordering-to-runner-jobs-api.yml | 5 +++ doc/api/runners.md | 2 + spec/finders/runner_jobs_finder_spec.rb | 37 +++++++++++++++++++ 4 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml diff --git a/app/finders/runner_jobs_finder.rb b/app/finders/runner_jobs_finder.rb index 4fca4ec94f3..f1ee1d38255 100644 --- a/app/finders/runner_jobs_finder.rb +++ b/app/finders/runner_jobs_finder.rb @@ -3,6 +3,8 @@ class RunnerJobsFinder attr_reader :runner, :params + ALLOWED_INDEXED_COLUMNS = %w[id created_at].freeze + def initialize(runner, params = {}) @runner = runner @params = params @@ -11,7 +13,7 @@ class RunnerJobsFinder def execute items = @runner.builds items = by_status(items) - items + sort_items(items) end private @@ -23,4 +25,22 @@ class RunnerJobsFinder items.where(status: params[:status]) end # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable CodeReuse/ActiveRecord + def sort_items(items) + order_by = if ALLOWED_INDEXED_COLUMNS.include?(params[:order_by]) + params[:order_by] + else + :id + end + + sort = if params[:sort] =~ /\A(ASC|DESC)\z/i + params[:sort] + else + :desc + end + + items.order(order_by => sort) + end + # rubocop: enable CodeReuse/ActiveRecord end diff --git a/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml b/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml new file mode 100644 index 00000000000..6af61d7b145 --- /dev/null +++ b/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml @@ -0,0 +1,5 @@ +--- +title: 51794-add-order-by-to-list-runner-jobs-api +merge_request: +author: Sujay Patel +type: added diff --git a/doc/api/runners.md b/doc/api/runners.md index 2d91428d1c1..b70b954031b 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -291,6 +291,8 @@ GET /runners/:id/jobs |-----------|---------|----------|---------------------| | `id` | integer | yes | The ID of a runner | | `status` | string | no | Status of the job; one of: `running`, `success`, `failed`, `canceled` | +| `order_by`| string | no | Order jobs by `id` or `created_at` (default: id) | +| `sort` | string | no | Sort jobs in `asc` or `desc` order (default: `desc`) | ``` curl --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/runners/1/jobs?status=running" diff --git a/spec/finders/runner_jobs_finder_spec.rb b/spec/finders/runner_jobs_finder_spec.rb index 97304170c4e..9ed6f50ddfb 100644 --- a/spec/finders/runner_jobs_finder_spec.rb +++ b/spec/finders/runner_jobs_finder_spec.rb @@ -35,5 +35,42 @@ describe RunnerJobsFinder do end end end + + context 'when order_by and sort are specified' do + context 'when order_by created_at' do + let(:params) { { order_by: 'created_at', sort: 'asc' } } + let!(:jobs) { Array.new(2) { create(:ci_build, runner: runner, project: project, user: create(:user)) } } + + it 'sorts as created_at: :asc' do + is_expected.to match_array(jobs) + end + + context 'when sort is invalid' do + let(:params) { { order_by: 'created_at', sort: 'invalid_sort' } } + + it 'sorts as created_at: :desc' do + is_expected.to eq(jobs.sort_by { |p| -p.user.id }) + end + end + end + + context 'when order_by is invalid' do + let(:params) { { order_by: 'invalid_column', sort: 'asc' } } + let!(:jobs) { Array.new(2) { create(:ci_build, runner: runner, project: project, user: create(:user)) } } + + it 'sorts as id: :asc' do + is_expected.to eq(jobs.sort_by { |p| p.id }) + end + end + + context 'when both are nil' do + let(:params) { { order_by: nil, sort: nil } } + let!(:jobs) { Array.new(2) { create(:ci_build, runner: runner, project: project, user: create(:user)) } } + + it 'sorts as id: :desc' do + is_expected.to eq(jobs.sort_by { |p| -p.id }) + end + end + end end end From b71250ca0f1b9df4f728bdb322502e3544058ca5 Mon Sep 17 00:00:00 2001 From: sujay patel Date: Thu, 13 Jun 2019 01:33:21 +0530 Subject: [PATCH 3/4] Adding order by to list runner jobs api. --- app/finders/runner_jobs_finder.rb | 22 ++++++++++- .../51794-add-ordering-to-runner-jobs-api.yml | 5 +++ doc/api/runners.md | 2 + spec/finders/runner_jobs_finder_spec.rb | 37 +++++++++++++++++++ 4 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml diff --git a/app/finders/runner_jobs_finder.rb b/app/finders/runner_jobs_finder.rb index 4fca4ec94f3..f1ee1d38255 100644 --- a/app/finders/runner_jobs_finder.rb +++ b/app/finders/runner_jobs_finder.rb @@ -3,6 +3,8 @@ class RunnerJobsFinder attr_reader :runner, :params + ALLOWED_INDEXED_COLUMNS = %w[id created_at].freeze + def initialize(runner, params = {}) @runner = runner @params = params @@ -11,7 +13,7 @@ class RunnerJobsFinder def execute items = @runner.builds items = by_status(items) - items + sort_items(items) end private @@ -23,4 +25,22 @@ class RunnerJobsFinder items.where(status: params[:status]) end # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable CodeReuse/ActiveRecord + def sort_items(items) + order_by = if ALLOWED_INDEXED_COLUMNS.include?(params[:order_by]) + params[:order_by] + else + :id + end + + sort = if params[:sort] =~ /\A(ASC|DESC)\z/i + params[:sort] + else + :desc + end + + items.order(order_by => sort) + end + # rubocop: enable CodeReuse/ActiveRecord end diff --git a/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml b/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml new file mode 100644 index 00000000000..6af61d7b145 --- /dev/null +++ b/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml @@ -0,0 +1,5 @@ +--- +title: 51794-add-order-by-to-list-runner-jobs-api +merge_request: +author: Sujay Patel +type: added diff --git a/doc/api/runners.md b/doc/api/runners.md index 90e9fbff247..425b9aeef1b 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -291,6 +291,8 @@ GET /runners/:id/jobs |-----------|---------|----------|---------------------| | `id` | integer | yes | The ID of a runner | | `status` | string | no | Status of the job; one of: `running`, `success`, `failed`, `canceled` | +| `order_by`| string | no | Order jobs by `id` or `created_at` (default: id) | +| `sort` | string | no | Sort jobs in `asc` or `desc` order (default: `desc`) | ``` curl --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/runners/1/jobs?status=running" diff --git a/spec/finders/runner_jobs_finder_spec.rb b/spec/finders/runner_jobs_finder_spec.rb index 97304170c4e..9ed6f50ddfb 100644 --- a/spec/finders/runner_jobs_finder_spec.rb +++ b/spec/finders/runner_jobs_finder_spec.rb @@ -35,5 +35,42 @@ describe RunnerJobsFinder do end end end + + context 'when order_by and sort are specified' do + context 'when order_by created_at' do + let(:params) { { order_by: 'created_at', sort: 'asc' } } + let!(:jobs) { Array.new(2) { create(:ci_build, runner: runner, project: project, user: create(:user)) } } + + it 'sorts as created_at: :asc' do + is_expected.to match_array(jobs) + end + + context 'when sort is invalid' do + let(:params) { { order_by: 'created_at', sort: 'invalid_sort' } } + + it 'sorts as created_at: :desc' do + is_expected.to eq(jobs.sort_by { |p| -p.user.id }) + end + end + end + + context 'when order_by is invalid' do + let(:params) { { order_by: 'invalid_column', sort: 'asc' } } + let!(:jobs) { Array.new(2) { create(:ci_build, runner: runner, project: project, user: create(:user)) } } + + it 'sorts as id: :asc' do + is_expected.to eq(jobs.sort_by { |p| p.id }) + end + end + + context 'when both are nil' do + let(:params) { { order_by: nil, sort: nil } } + let!(:jobs) { Array.new(2) { create(:ci_build, runner: runner, project: project, user: create(:user)) } } + + it 'sorts as id: :desc' do + is_expected.to eq(jobs.sort_by { |p| -p.id }) + end + end + end end end From e241c89977c32fabbbe5a49c1ba69564d5e09e31 Mon Sep 17 00:00:00 2001 From: sujay patel Date: Thu, 13 Jun 2019 01:33:21 +0530 Subject: [PATCH 4/4] Adding order by to list runner jobs api. --- app/finders/runner_jobs_finder.rb | 11 ++--- .../51794-add-ordering-to-runner-jobs-api.yml | 4 +- doc/api/runners.md | 2 +- lib/api/runners.rb | 2 + spec/finders/runner_jobs_finder_spec.rb | 35 +++++---------- spec/requests/api/runners_spec.rb | 44 +++++++++++++++++++ 6 files changed, 63 insertions(+), 35 deletions(-) diff --git a/app/finders/runner_jobs_finder.rb b/app/finders/runner_jobs_finder.rb index f1ee1d38255..ef90817416a 100644 --- a/app/finders/runner_jobs_finder.rb +++ b/app/finders/runner_jobs_finder.rb @@ -3,7 +3,7 @@ class RunnerJobsFinder attr_reader :runner, :params - ALLOWED_INDEXED_COLUMNS = %w[id created_at].freeze + ALLOWED_INDEXED_COLUMNS = %w[id].freeze def initialize(runner, params = {}) @runner = runner @@ -28,13 +28,10 @@ class RunnerJobsFinder # rubocop: disable CodeReuse/ActiveRecord def sort_items(items) - order_by = if ALLOWED_INDEXED_COLUMNS.include?(params[:order_by]) - params[:order_by] - else - :id - end + return items unless ALLOWED_INDEXED_COLUMNS.include?(params[:order_by]) - sort = if params[:sort] =~ /\A(ASC|DESC)\z/i + order_by = params[:order_by] + sort = if /\A(ASC|DESC)\z/i.match?(params[:sort]) params[:sort] else :desc diff --git a/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml b/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml index 6af61d7b145..908a132688c 100644 --- a/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml +++ b/changelogs/unreleased/51794-add-ordering-to-runner-jobs-api.yml @@ -1,5 +1,5 @@ --- -title: 51794-add-order-by-to-list-runner-jobs-api -merge_request: +title: Add order_by and sort params to list runner jobs api +merge_request: 29629 author: Sujay Patel type: added diff --git a/doc/api/runners.md b/doc/api/runners.md index 425b9aeef1b..1318b9ca828 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -291,7 +291,7 @@ GET /runners/:id/jobs |-----------|---------|----------|---------------------| | `id` | integer | yes | The ID of a runner | | `status` | string | no | Status of the job; one of: `running`, `success`, `failed`, `canceled` | -| `order_by`| string | no | Order jobs by `id` or `created_at` (default: id) | +| `order_by`| string | no | Order jobs by `id`. | | `sort` | string | no | Sort jobs in `asc` or `desc` order (default: `desc`) | ``` diff --git a/lib/api/runners.rb b/lib/api/runners.rb index f3fea463e7f..c2d371b6867 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -115,6 +115,8 @@ module API params do requires :id, type: Integer, desc: 'The ID of the runner' optional :status, type: String, desc: 'Status of the job', values: Ci::Build::AVAILABLE_STATUSES + optional :order_by, type: String, desc: 'Order by `id` or not', values: RunnerJobsFinder::ALLOWED_INDEXED_COLUMNS + optional :sort, type: String, values: %w[asc desc], default: 'desc', desc: 'Sort by asc (ascending) or desc (descending)' use :pagination end get ':id/jobs' do diff --git a/spec/finders/runner_jobs_finder_spec.rb b/spec/finders/runner_jobs_finder_spec.rb index 9ed6f50ddfb..01f45a37ba8 100644 --- a/spec/finders/runner_jobs_finder_spec.rb +++ b/spec/finders/runner_jobs_finder_spec.rb @@ -37,38 +37,23 @@ describe RunnerJobsFinder do end context 'when order_by and sort are specified' do - context 'when order_by created_at' do - let(:params) { { order_by: 'created_at', sort: 'asc' } } - let!(:jobs) { Array.new(2) { create(:ci_build, runner: runner, project: project, user: create(:user)) } } - - it 'sorts as created_at: :asc' do - is_expected.to match_array(jobs) - end - - context 'when sort is invalid' do - let(:params) { { order_by: 'created_at', sort: 'invalid_sort' } } - - it 'sorts as created_at: :desc' do - is_expected.to eq(jobs.sort_by { |p| -p.user.id }) - end - end - end - - context 'when order_by is invalid' do - let(:params) { { order_by: 'invalid_column', sort: 'asc' } } - let!(:jobs) { Array.new(2) { create(:ci_build, runner: runner, project: project, user: create(:user)) } } + context 'when order_by id and sort is asc' do + let(:params) { { order_by: 'id', sort: 'asc' } } + let!(:jobs) { create_list(:ci_build, 2, runner: runner, project: project, user: create(:user)) } it 'sorts as id: :asc' do - is_expected.to eq(jobs.sort_by { |p| p.id }) + is_expected.to eq(jobs.sort_by(&:id)) end end + end - context 'when both are nil' do - let(:params) { { order_by: nil, sort: nil } } - let!(:jobs) { Array.new(2) { create(:ci_build, runner: runner, project: project, user: create(:user)) } } + context 'when order_by is specified and sort is not specified' do + context 'when order_by id and sort is not specified' do + let(:params) { { order_by: 'id' } } + let!(:jobs) { create_list(:ci_build, 2, runner: runner, project: project, user: create(:user)) } it 'sorts as id: :desc' do - is_expected.to eq(jobs.sort_by { |p| -p.id }) + is_expected.to eq(jobs.sort_by(&:id).reverse) end end end diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 5548e3fd01a..f5ce3a3570e 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -584,6 +584,34 @@ describe API::Runners do end end + context 'when valid order_by is provided' do + context 'when sort order is not specified' do + it 'return jobs in descending order' do + get api("/runners/#{project_runner.id}/jobs?order_by=id", admin) + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + + expect(json_response).to be_an(Array) + expect(json_response.length).to eq(2) + expect(json_response.first).to include('id' => job_5.id) + end + end + + context 'when sort order is specified as asc' do + it 'return jobs sorted in ascending order' do + get api("/runners/#{project_runner.id}/jobs?order_by=id&sort=asc", admin) + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + + expect(json_response).to be_an(Array) + expect(json_response.length).to eq(2) + expect(json_response.first).to include('id' => job_4.id) + end + end + end + context 'when invalid status is provided' do it 'return 400' do get api("/runners/#{project_runner.id}/jobs?status=non-existing", admin) @@ -591,6 +619,22 @@ describe API::Runners do expect(response).to have_gitlab_http_status(400) end end + + context 'when invalid order_by is provided' do + it 'return 400' do + get api("/runners/#{project_runner.id}/jobs?order_by=non-existing", admin) + + expect(response).to have_gitlab_http_status(400) + end + end + + context 'when invalid sort is provided' do + it 'return 400' do + get api("/runners/#{project_runner.id}/jobs?sort=non-existing", admin) + + expect(response).to have_gitlab_http_status(400) + end + end end context "when runner doesn't exist" do