diff options
author | Rémy Coutable <remy@rymai.me> | 2016-06-23 15:02:10 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-06-23 15:02:10 +0000 |
commit | 26b957771974aee4457af5edf389509eb1ac80e7 (patch) | |
tree | 5d260ede73d63ba4ea462ae45f7d482c2e64e1de /spec | |
parent | 110b2759c0c74d7bc85785ab481ff902cd7014d0 (diff) | |
parent | d7a5a28c53c3af710ceb8ef4867ea61af7462903 (diff) | |
download | gitlab-ce-26b957771974aee4457af5edf389509eb1ac80e7.tar.gz |
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
Diffstat (limited to 'spec')
-rw-r--r-- | spec/models/concerns/issuable_spec.rb | 14 |
1 files changed, 14 insertions, 0 deletions
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index efbcbf72f76..89730ab8eb8 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -154,6 +154,20 @@ describe Issue, "Issuable" do expect(issues).to match_array([issue1, issue2, issue, issue3]) 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 |