Commit d7a5a28c authored by Sean McGivern's avatar Sean McGivern

Fix pagination on sorts with lots of ties

Postgres and MySQL don't guarantee that pagination with `LIMIT` and
`OFFSET` will work 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
parent 2de9d66f
...@@ -4,6 +4,7 @@ v 8.10.0 (unreleased) ...@@ -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.0 v 8.9.0
......
...@@ -112,15 +112,18 @@ module Issuable ...@@ -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 ...@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment