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 diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 2eb7b04711a..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) @@ -393,7 +399,7 @@ 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 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 diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 04908378a24..033cd1b2b38 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 @@ -613,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 @@ -828,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 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' 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