Merge branch '18915-pagination-with-priority-sort-repeats-results' into 'master'
Fix pagination on sorts with lots of ties ## What does this MR do? Fixes #18915. As we only order by the sorted column, we don't have any tie-breaker. Some orderings, like priority and weight, have lots of ties, so you can see duplicate results as you page through. (Timestamp columns are less susceptible to this.) ## Are there points in the code the reviewer needs to double check? I just picked `id DESC`, this could as easily be `id ASC`. ## Why was this MR needed? Postgres and MySQL don't guarantee that pagination with `LIMIT` and `OFFSET` will work as expected if the ordering isn't unique. From the Postgres docs: > When using `LIMIT`, it is important to use an `ORDER BY` clause that > constrains the result rows into a unique order. Otherwise you will get > an unpredictable subset of the query's rows Before: [1] pry(main)> issues = 1.upto(Issue.count).map { |i| Issue.sort('priority').page(i).per(1).map(&:id) }.flatten [2] pry(main)> issues.count => 81 [3] pry(main)> issues.uniq.count => 42 After: [1] pry(main)> issues = 1.upto(Issue.count).map { |i| Issue.sort('priority').page(i).per(1).map(&:id) }.flatten [2] pry(main)> issues.count => 81 [3] pry(main)> issues.uniq.count => 81 See merge request !4878
This commit is contained in:
commit
26b9577719
3 changed files with 27 additions and 9 deletions
|
@ -4,6 +4,7 @@ v 8.10.0 (unreleased)
|
||||||
- Wrap code blocks on Activies and Todos page. !4783 (winniehell)
|
- Wrap code blocks on Activies and Todos page. !4783 (winniehell)
|
||||||
- Add Sidekiq queue duration to transaction metrics.
|
- Add Sidekiq queue duration to transaction metrics.
|
||||||
- Fix MR-auto-close text added to description. !4836
|
- Fix MR-auto-close text added to description. !4836
|
||||||
|
- Fix pagination when sorting by columns with lots of ties (like priority)
|
||||||
- Implement Subresource Integrity for CSS and JavaScript assets. This prevents malicious assets from loading in the case of a CDN compromise.
|
- Implement Subresource Integrity for CSS and JavaScript assets. This prevents malicious assets from loading in the case of a CDN compromise.
|
||||||
|
|
||||||
v 8.9.1
|
v 8.9.1
|
||||||
|
|
|
@ -112,15 +112,18 @@ module Issuable
|
||||||
end
|
end
|
||||||
|
|
||||||
def sort(method, excluded_labels: [])
|
def sort(method, excluded_labels: [])
|
||||||
case method.to_s
|
sorted = case method.to_s
|
||||||
when 'milestone_due_asc' then order_milestone_due_asc
|
when 'milestone_due_asc' then order_milestone_due_asc
|
||||||
when 'milestone_due_desc' then order_milestone_due_desc
|
when 'milestone_due_desc' then order_milestone_due_desc
|
||||||
when 'downvotes_desc' then order_downvotes_desc
|
when 'downvotes_desc' then order_downvotes_desc
|
||||||
when 'upvotes_desc' then order_upvotes_desc
|
when 'upvotes_desc' then order_upvotes_desc
|
||||||
when 'priority' then order_labels_priority(excluded_labels: excluded_labels)
|
when 'priority' then order_labels_priority(excluded_labels: excluded_labels)
|
||||||
else
|
else
|
||||||
order_by(method)
|
order_by(method)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
# Break ties with the ID column for pagination
|
||||||
|
sorted.order(id: :desc)
|
||||||
end
|
end
|
||||||
|
|
||||||
def order_labels_priority(excluded_labels: [])
|
def order_labels_priority(excluded_labels: [])
|
||||||
|
|
|
@ -154,6 +154,20 @@ describe Issue, "Issuable" do
|
||||||
expect(issues).to match_array([issue1, issue2, issue, issue3])
|
expect(issues).to match_array([issue1, issue2, issue, issue3])
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context 'when all of the results are level on the sort key' do
|
||||||
|
let!(:issues) do
|
||||||
|
10.times { create(:issue, project: project) }
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'has no duplicates across pages' do
|
||||||
|
sorted_issue_ids = 1.upto(10).map do |i|
|
||||||
|
project.issues.sort('milestone_due_desc').page(i).per(1).first.id
|
||||||
|
end
|
||||||
|
|
||||||
|
expect(sorted_issue_ids).to eq(sorted_issue_ids.uniq)
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue