From ff671366cb80a78c2846d5407d2380680a47a90d Mon Sep 17 00:00:00 2001 From: Nermin Vehabovic Date: Sat, 16 Feb 2019 11:03:42 +0100 Subject: [PATCH 1/7] Added: Include order by ID desc for tie breakers in pagination --- lib/api/helpers.rb | 10 +++++++++- lib/api/issues.rb | 3 +-- lib/api/merge_requests.rb | 2 +- lib/api/notes.rb | 2 +- lib/api/users.rb | 2 +- 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 2eb7b04711a..4d1b7714123 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -393,7 +393,15 @@ module API # rubocop: disable CodeReuse/ActiveRecord def reorder_projects(projects) - projects.reorder(params[:order_by] => params[:sort]) + projects.reorder(order_options_with_tie_breaker) + end + # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable CodeReuse/ActiveRecord + def order_options_with_tie_breaker + {params[:order_by] => params[:sort]}.tap do |order| + order['id'] ||= 'desc' + end end # rubocop: enable CodeReuse/ActiveRecord diff --git a/lib/api/issues.rb b/lib/api/issues.rb index b3636c98550..aa8010eb12d 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -29,8 +29,7 @@ module API issues = IssuesFinder.new(current_user, args).execute .preload(:assignees, :labels, :notes, :timelogs, :project, :author, :closed_by) - - issues.reorder(args[:order_by] => args[:sort]) + issues.reorder(order_options_with_tie_breaker) end # rubocop: enable CodeReuse/ActiveRecord diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index df46b4446ff..f8d2ba49d2f 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -38,7 +38,7 @@ module API args[:scope] = args[:scope].underscore if args[:scope] merge_requests = MergeRequestsFinder.new(current_user, args).execute - .reorder(args[:order_by] => args[:sort]) + .reorder(order_options_with_tie_breaker) merge_requests = paginate(merge_requests) .preload(:source_project, :target_project) diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 1bdf7aeb119..f7bd092ce50 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -39,7 +39,7 @@ module API # at the DB query level (which we cannot in that case), the current # page can have less elements than :per_page even if # there's more than one page. - raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort]) + raw_notes = noteable.notes.with_metadata.reorder(order_options_with_tie_breaker) notes = # paginate() only works with a relation. This could lead to a # mismatch between the pagination headers info and the actual notes diff --git a/lib/api/users.rb b/lib/api/users.rb index 8ce09a8881b..7d88880d412 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -26,7 +26,7 @@ module API # rubocop: disable CodeReuse/ActiveRecord def reorder_users(users) if params[:order_by] && params[:sort] - users.reorder(params[:order_by] => params[:sort]) + users.reorder(order_options_with_tie_breaker) else users end From f43cb6e79d36345a207aaaf00179c5acf9b5a3b3 Mon Sep 17 00:00:00 2001 From: Nermin Vehabovic Date: Sat, 16 Feb 2019 11:52:21 +0100 Subject: [PATCH 2/7] Fixed: Warnings from static code analysis --- lib/api/helpers.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 4d1b7714123..4656b27c5d1 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -397,13 +397,11 @@ module API end # rubocop: enable CodeReuse/ActiveRecord - # rubocop: disable CodeReuse/ActiveRecord def order_options_with_tie_breaker - {params[:order_by] => params[:sort]}.tap do |order| - order['id'] ||= 'desc' - end + order_options = { params[:order_by] => params[:sort] } + order_options['id'] ||= 'desc' + order_options end - # rubocop: enable CodeReuse/ActiveRecord def project_finder_params finder_params = { without_deleted: true } From b2a132ddce6e4b62561589c61c08ddea452118b9 Mon Sep 17 00:00:00 2001 From: Nermin Vehabovic Date: Sat, 16 Feb 2019 12:51:09 +0100 Subject: [PATCH 3/7] Added: Changelog details --- .../unreleased/54796-api-sort-tie-breaker-for-pagination.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/54796-api-sort-tie-breaker-for-pagination.yml diff --git a/changelogs/unreleased/54796-api-sort-tie-breaker-for-pagination.yml b/changelogs/unreleased/54796-api-sort-tie-breaker-for-pagination.yml new file mode 100644 index 00000000000..92b27f63f82 --- /dev/null +++ b/changelogs/unreleased/54796-api-sort-tie-breaker-for-pagination.yml @@ -0,0 +1,5 @@ +--- +title: 'API: Sort tie breaker with id DESC' +merge_request: 25311 +author: Nermin Vehabovic +type: changed From c8352c9fcce6ab482cf0707f4c271c8331b9827b Mon Sep 17 00:00:00 2001 From: Nermin Vehabovic Date: Sat, 16 Feb 2019 14:40:26 +0100 Subject: [PATCH 4/7] Added: Specs for issues index --- spec/requests/api/issues_spec.rb | 34 +++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 04908378a24..37e528a42d5 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -359,10 +359,38 @@ describe API::Issues do expect_paginated_array_response([]) end - it 'sorts by created_at descending by default' do - get api('/issues', user) + context 'without sort params' do + it 'sorts by created_at descending by default' do + get api('/issues', user) - expect_paginated_array_response([issue.id, closed_issue.id]) + expect_paginated_array_response([issue.id, closed_issue.id]) + end + + context 'with 2 issues with same created_at' do + let!(:closed_issue2) do + create :closed_issue, + author: user, + assignees: [user], + project: project, + milestone: milestone, + created_at: closed_issue.created_at, + updated_at: 1.hour.ago, + title: issue_title, + description: issue_description + end + + it 'page breaks first page correctly' do + get api('/issues?per_page=2', user) + + expect_paginated_array_response([issue.id, closed_issue2.id]) + end + + it 'page breaks second page correctly' do + get api('/issues?per_page=2&page=2', user) + + expect_paginated_array_response([closed_issue.id]) + end + end end it 'sorts ascending when requested' do From 44b2d759b276c565cfb5c2c6461ba11965dd0a17 Mon Sep 17 00:00:00 2001 From: Nermin Vehabovic Date: Sat, 16 Feb 2019 14:57:12 +0100 Subject: [PATCH 5/7] Added: Specs to cover other endpoints for the same reason --- spec/requests/api/issues_spec.rb | 68 +++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 6 deletions(-) diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 37e528a42d5..033cd1b2b38 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -641,10 +641,38 @@ describe API::Issues do expect_paginated_array_response(group_confidential_issue.id) end - it 'sorts by created_at descending by default' do - get api(base_url, user) + context 'without sort params' do + it 'sorts by created_at descending by default' do + get api(base_url, user) - expect_paginated_array_response([group_closed_issue.id, group_confidential_issue.id, group_issue.id]) + expect_paginated_array_response([group_closed_issue.id, group_confidential_issue.id, group_issue.id]) + end + + context 'with 2 issues with same created_at' do + let!(:group_issue2) do + create :issue, + author: user, + assignees: [user], + project: group_project, + milestone: group_milestone, + updated_at: 1.hour.ago, + title: issue_title, + description: issue_description, + created_at: group_issue.created_at + end + + it 'page breaks first page correctly' do + get api("#{base_url}?per_page=3", user) + + expect_paginated_array_response([group_closed_issue.id, group_confidential_issue.id, group_issue2.id]) + end + + it 'page breaks second page correctly' do + get api("#{base_url}?per_page=3&page=2", user) + + expect_paginated_array_response([group_issue.id]) + end + end end it 'sorts ascending when requested' do @@ -856,10 +884,38 @@ describe API::Issues do expect_paginated_array_response([issue.id, closed_issue.id]) end - it 'sorts by created_at descending by default' do - get api("#{base_url}/issues", user) + context 'without sort params' do + it 'sorts by created_at descending by default' do + get api("#{base_url}/issues", user) - expect_paginated_array_response([issue.id, confidential_issue.id, closed_issue.id]) + expect_paginated_array_response([issue.id, confidential_issue.id, closed_issue.id]) + end + + context 'with 2 issues with same created_at' do + let!(:closed_issue2) do + create :closed_issue, + author: user, + assignees: [user], + project: project, + milestone: milestone, + created_at: closed_issue.created_at, + updated_at: 1.hour.ago, + title: issue_title, + description: issue_description + end + + it 'page breaks first page correctly' do + get api("#{base_url}/issues?per_page=3", user) + + expect_paginated_array_response([issue.id, confidential_issue.id, closed_issue2.id]) + end + + it 'page breaks second page correctly' do + get api("#{base_url}/issues?per_page=3&page=2", user) + + expect_paginated_array_response([closed_issue.id]) + end + end end it 'sorts ascending when requested' do From eccfab4445e89dbd3c9b9c756d3daab10eef320f Mon Sep 17 00:00:00 2001 From: Nermin Vehabovic Date: Sat, 16 Feb 2019 16:11:31 +0100 Subject: [PATCH 6/7] Added: Specs for sort page breaks on notes --- lib/api/helpers.rb | 12 +++--- .../shared_examples/requests/api/notes.rb | 42 ++++++++++++++++--- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 4656b27c5d1..54cd4cd9cdb 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -299,6 +299,12 @@ module API items.search(text) end + def order_options_with_tie_breaker + order_options = { params[:order_by] => params[:sort] } + order_options['id'] ||= 'desc' + order_options + end + # error helpers def forbidden!(reason = nil) @@ -397,12 +403,6 @@ module API end # rubocop: enable CodeReuse/ActiveRecord - def order_options_with_tie_breaker - order_options = { params[:order_by] => params[:sort] } - order_options['id'] ||= 'desc' - order_options - end - def project_finder_params finder_params = { without_deleted: true } finder_params[:owned] = true if params[:owned].present? diff --git a/spec/support/shared_examples/requests/api/notes.rb b/spec/support/shared_examples/requests/api/notes.rb index 71499e85654..57eefd5ef01 100644 --- a/spec/support/shared_examples/requests/api/notes.rb +++ b/spec/support/shared_examples/requests/api/notes.rb @@ -8,13 +8,45 @@ shared_examples 'noteable API' do |parent_type, noteable_type, id_name| create_list(:note, 3, params) end - it 'sorts by created_at in descending order by default' do - get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user) + context 'without sort params' do + it 'sorts by created_at in descending order by default' do + get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user) - response_dates = json_response.map { |note| note['created_at'] } + response_dates = json_response.map { |note| note['created_at'] } - expect(json_response.length).to eq(4) - expect(response_dates).to eq(response_dates.sort.reverse) + expect(json_response.length).to eq(4) + expect(response_dates).to eq(response_dates.sort.reverse) + end + + context '2 notes with equal created_at' do + before do + @first_note = Note.first + + params = { noteable: noteable, author: user } + params[:project] = parent if parent.is_a?(Project) + params[:created_at] = @first_note.created_at + + @note2 = create(:note, params) + end + + it 'page breaks first page correctly' do + get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes?per_page=4", user) + + response_ids = json_response.map { |note| note['id'] } + + expect(response_ids).to include(@note2.id) + expect(response_ids).not_to include(@first_note.id) + end + + it 'page breaks second page correctly' do + get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes?per_page=4&page=2", user) + + response_ids = json_response.map { |note| note['id'] } + + expect(response_ids).not_to include(@note2.id) + expect(response_ids).to include(@first_note.id) + end + end end it 'sorts by ascending order when requested' do From b467dca48548e6c209afc8c265454e8fb84b0255 Mon Sep 17 00:00:00 2001 From: Nermin Vehabovic Date: Sat, 16 Feb 2019 16:57:14 +0100 Subject: [PATCH 7/7] Added: Specs for sort page break tie in the merge request list API --- .../requests/api/merge_requests_list.rb | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/spec/support/shared_examples/requests/api/merge_requests_list.rb b/spec/support/shared_examples/requests/api/merge_requests_list.rb index 453f42251c8..6713ec47ace 100644 --- a/spec/support/shared_examples/requests/api/merge_requests_list.rb +++ b/spec/support/shared_examples/requests/api/merge_requests_list.rb @@ -257,6 +257,38 @@ shared_examples 'merge requests list' do expect(response_dates).to eq(response_dates.sort.reverse) end + context '2 merge requests with equal created_at' do + let!(:closed_mr2) do + create :merge_request, + state: 'closed', + milestone: milestone1, + author: user, + assignee: user, + source_project: project, + target_project: project, + title: "Test", + created_at: @mr_earlier.created_at + end + + it 'page breaks first page correctly' do + get api("#{endpoint_path}?sort=desc&per_page=4", user) + + response_ids = json_response.map { |merge_request| merge_request['id'] } + + expect(response_ids).to include(closed_mr2.id) + expect(response_ids).not_to include(@mr_earlier.id) + end + + it 'page breaks second page correctly' do + get api("#{endpoint_path}?sort=desc&per_page=4&page=2", user) + + response_ids = json_response.map { |merge_request| merge_request['id'] } + + expect(response_ids).not_to include(closed_mr2.id) + expect(response_ids).to include(@mr_earlier.id) + end + end + it 'returns an array of merge_requests ordered by updated_at' do path = endpoint_path + '?order_by=updated_at'