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

3 files changed, 27 insertions, 9 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 400414b1c26..4e405478825 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.1
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: [])
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