diff options
author | Sean McGivern <sean@gitlab.com> | 2016-06-23 14:15:46 +0100 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2016-06-23 14:33:20 +0100 |
commit | aa973f2eecce822f70f78f0086b0550bc35d05d9 (patch) | |
tree | 45d1dfd081ea017a1c3cd94784aae6c6bb481cb5 | |
parent | 2de9d66fe44fd98e6ba1264058c642d05a33f2e8 (diff) | |
download | gitlab-ce-18915-pagination-with-priority-sort-repeats-results.tar.gz |
Fix pagination on sorts with lots of ties18915-pagination-with-priority-sort-repeats-results
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
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/models/concerns/issuable.rb | 21 |
2 files changed, 13 insertions, 9 deletions
diff --git a/CHANGELOG b/CHANGELOG index 8a37274c023..3841dcbef7b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,7 @@ v 8.10.0 (unreleased) - Wrap code blocks on Activies and Todos page. !4783 (winniehell) - Add Sidekiq queue duration to transaction metrics. - 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. v 8.9.0 diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 0ccd3474b81..38da0dafad2 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -112,15 +112,18 @@ module Issuable end def sort(method, excluded_labels: []) - case method.to_s - when 'milestone_due_asc' then order_milestone_due_asc - when 'milestone_due_desc' then order_milestone_due_desc - when 'downvotes_desc' then order_downvotes_desc - when 'upvotes_desc' then order_upvotes_desc - when 'priority' then order_labels_priority(excluded_labels: excluded_labels) - else - order_by(method) - end + sorted = case method.to_s + when 'milestone_due_asc' then order_milestone_due_asc + when 'milestone_due_desc' then order_milestone_due_desc + when 'downvotes_desc' then order_downvotes_desc + when 'upvotes_desc' then order_upvotes_desc + when 'priority' then order_labels_priority(excluded_labels: excluded_labels) + else + order_by(method) + end + + # Break ties with the ID column for pagination + sorted.order(id: :desc) end def order_labels_priority(excluded_labels: []) |