More flexible keyset pagination

Since we now support id_before/id_after and order_by id in asc/desc
order, we can just use that for keyset pagination.

lib/gitlab/pagination/keyset.rb

@@ -4,11 +4,7 @@ module Gitlab
   module Pagination
     module Keyset
       def self.paginate(request_context, relation)
-        paged_relation = Gitlab::Pagination::Keyset::Pager.new(request_context).paginate(relation)
-
-        request_context.apply_headers(paged_relation)
-
-        paged_relation.relation
+        Gitlab::Pagination::Keyset::Pager.new(request_context).paginate(relation)
       end
     end
   end

lib/gitlab/pagination/keyset/page.rb

@@ -3,30 +3,40 @@
 module Gitlab
   module Pagination
     module Keyset
+      # A Page models the pagination information for a particular page of the collection
       class Page
+        # Default and maximum size of records for a page
         DEFAULT_PAGE_SIZE = 20
 
-        attr_reader :last_value, :column
+        attr_accessor :lower_bounds, :end_reached
+        attr_reader :order_by
 
-        def initialize(last_value, column: :id, per_page: DEFAULT_PAGE_SIZE, is_first_page: false)
-          @last_value = last_value
-          @column = column
-          @per_page = per_page || DEFAULT_PAGE_SIZE
-          @is_first_page = is_first_page
+        def initialize(order_by: {}, lower_bounds: nil, per_page: DEFAULT_PAGE_SIZE, end_reached: false)
+          @order_by = order_by
+          @lower_bounds = lower_bounds
+          @per_page = per_page
+          @end_reached = end_reached
         end
 
+        # Number of records to return per page
         def per_page
           return DEFAULT_PAGE_SIZE if @per_page <= 0
 
           [@per_page, DEFAULT_PAGE_SIZE].min
         end
 
-        def empty?
-          last_value.nil? && !first_page?
+        # Determine whether this page indicates the end of the collection
+        def end_reached?
+          @end_reached
         end
 
-        def first_page?
-          @is_first_page
+        # Construct a Page for the next page
+        # Uses identical order_by/per_page information for the next page
+        def next(lower_bounds, end_reached)
+          dup.tap do |next_page|
+            next_page.lower_bounds = lower_bounds
+            next_page.end_reached = end_reached
+          end
         end
       end
     end

lib/gitlab/pagination/keyset/paged_relation.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Pagination
-    module Keyset
-      class PagedRelation
-        attr_reader :relation, :page
-
-        def initialize(relation, page)
-          @relation = relation
-          @page = page
-        end
-
-        # return Page information for next page
-        def next_page
-          last_record_in_page = relation.last
-          last_value = last_record_in_page&.read_attribute(page.column)
-
-          Page.new(last_value, column: page.column, per_page: page.per_page)
-        end
-      end
-    end
-  end
-end

lib/gitlab/pagination/keyset/pager.rb

@@ -11,21 +11,47 @@ module Gitlab
         end
 
         def paginate(relation)
-          paged_relation = relation.limit(page.per_page).reorder(page.column => :asc) # rubocop: disable CodeReuse/ActiveRecord
+          relation = relation.limit(page.per_page) # rubocop: disable CodeReuse/ActiveRecord
 
-          if val = page.last_value
-            # TODO: check page.column is valid
-            paged_relation = paged_relation.where("#{page.column} > ?", val) # rubocop: disable CodeReuse/ActiveRecord
-          end
+          # Validate an assumption we're making (TODO: subject to be removed)
+          check_order!(relation)
+
+          apply_headers(relation.last)
 
-          PagedRelation.new(paged_relation, page)
+          relation
         end
 
         private
 
+        def apply_headers(last_record_in_page)
+          lower_bounds = last_record_in_page&.slice(page.order_by.keys)
+          next_page = page.next(lower_bounds, last_record_in_page.nil?)
+
+          request.apply_headers(next_page)
+        end
+
         def page
           @page ||= request.page
         end
+
+        def order_by(rel)
+          rel.order_values.map { |val| [val.expr.name, val.direction] }
+        end
+
+        def check_order!(rel)
+          present_order = order_by(rel).last(2).to_h
+
+          if to_sym_vals(page.order_by) != to_sym_vals(present_order)
+            # The last two columns must match the page order_by
+            raise "Page order_by doesnt match the relation\'s order: #{present_order} vs #{page.order_by}"
+          end
+        end
+
+        def to_sym_vals(hash)
+          hash.each_with_object({}) do |(k, v), h|
+            h[k&.to_sym] = v&.to_sym
+          end
+        end
       end
     end
   end

lib/gitlab/pagination/keyset/request_context.rb

@@ -6,7 +6,8 @@ module Gitlab
       class RequestContext
         attr_reader :request
 
-        REQUEST_PARAM = :id_after
+        DEFAULT_SORT_DIRECTION = :asc
+        TIE_BREAKER = { id: :desc }.freeze
 
         def initialize(request)
           @request = request
@@ -14,27 +15,53 @@ module Gitlab
 
         # extracts Paging information from request parameters
         def page
-          last_value = request.params[REQUEST_PARAM]
+          Page.new(order_by: order_by, per_page: params[:per_page])
+        end
 
-          Page.new(last_value, per_page: request.params[:per_page], is_first_page: !last_value.nil?)
+        def apply_headers(next_page)
+          request.header('Links', pagination_links(next_page))
         end
 
-        def apply_headers(paged_relation)
-          next_page = paged_relation.next_page
-          links = pagination_links(next_page)
+        private
+
+        def order_by
+          return TIE_BREAKER.dup unless params[:order_by]
+
+          order_by = { params[:order_by]&.to_sym => params[:sort]&.to_sym || DEFAULT_SORT_DIRECTION }
+
+          # Order by an additional unique key, we use the primary key here
+          order_by = order_by.merge(TIE_BREAKER) unless order_by[:id]
 
-          request.header('Links', links.join(', '))
+          order_by
         end
 
-        private
+        def lower_bounds_params(page)
+          page.lower_bounds.each_with_object({}) do |(column, value), params|
+            filter = filter_with_comparator(page, column)
+            params[filter] = value
+          end
+        end
 
-        def pagination_links(next_page)
-          [].tap do |links|
-            links << %(<#{page_href}>; rel="first")
-            links << %(<#{page_href(next_page)}>; rel="next") unless next_page.empty?
+        def filter_with_comparator(page, column)
+          direction = page.order_by[column]
+
+          if direction&.to_sym == :desc
+            "#{column}_before"
+          else
+            "#{column}_after"
           end
         end
 
+        def params
+          @params ||= request.params
+        end
+
+        def pagination_links(next_page)
+          return if next_page.end_reached?
+
+          %(<#{page_href(next_page)}>; rel="next")
+        end
+
         def base_request_uri
           @base_request_uri ||= URI.parse(request.request.url).tap do |uri|
             uri.host = Gitlab.config.gitlab.host
@@ -43,14 +70,10 @@ module Gitlab
         end
 
         def query_params_for(page)
-          if page && !page.empty?
-            request.params.merge(REQUEST_PARAM => page.last_value)
-          else
-            request.params.except(REQUEST_PARAM)
-          end
+          request.params.merge(lower_bounds_params(page))
         end
 
-        def page_href(page = nil)
+        def page_href(page)
           base_request_uri.tap do |uri|
             uri.query = query_params_for(page).to_query
           end.to_s

spec/lib/gitlab/pagination/keyset/page_spec.rb

@@ -5,21 +5,56 @@ require 'spec_helper'
 describe Gitlab::Pagination::Keyset::Page do
   describe '#per_page' do
     it 'limits to a maximum of 20 records per page' do
-      per_page = described_class.new(double, per_page: 21).per_page
+      per_page = described_class.new(per_page: 21).per_page
 
       expect(per_page).to eq(described_class::DEFAULT_PAGE_SIZE)
     end
 
     it 'uses default value when given 0' do
-      per_page = described_class.new(double, per_page: 0).per_page
+      per_page = described_class.new(per_page: 0).per_page
 
       expect(per_page).to eq(described_class::DEFAULT_PAGE_SIZE)
     end
 
     it 'uses default value when given negative values' do
-      per_page = described_class.new(double, per_page: -1).per_page
+      per_page = described_class.new(per_page: -1).per_page
 
       expect(per_page).to eq(described_class::DEFAULT_PAGE_SIZE)
     end
   end
+
+  describe '#next' do
+    let(:page) { described_class.new(order_by: order_by, lower_bounds: lower_bounds, per_page: per_page, end_reached: end_reached) }
+    subject { page.next(new_lower_bounds, new_end_reached) }
+
+    let(:order_by) { {} }
+    let(:lower_bounds) { double }
+    let(:per_page) { 10 }
+    let(:end_reached) { false }
+
+    let(:new_lower_bounds) { double }
+    let(:new_end_reached) { true }
+
+    it 'copies over order_by' do
+      expect(subject.order_by).to eq(page.order_by)
+    end
+
+    it 'copies over per_page' do
+      expect(subject.per_page).to eq(page.per_page)
+    end
+
+    it 'dups the instance' do
+      expect(subject).not_to eq(page)
+    end
+
+    it 'sets lower_bounds only on new instance' do
+      expect(subject.lower_bounds).to eq(new_lower_bounds)
+      expect(page.lower_bounds).to eq(lower_bounds)
+    end
+
+    it 'sets end_reached only on new instance' do
+      expect(subject.end_reached?).to eq(new_end_reached)
+      expect(page.end_reached?).to eq(end_reached)
+    end
+  end
 end

spec/lib/gitlab/pagination/keyset/paged_relation_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe Gitlab::Pagination::Keyset::PagedRelation do
-  before_all do
-    create_list(:project, 10)
-  end
-
-  let(:relation) { Project.all.limit(page.per_page) }
-  let(:page) { double('page', column: :id, per_page: 5) }
-
-  describe '#next_page' do
-    subject { described_class.new(relation, page).next_page }
-
-    it 'retrieves the last record on the page to establish a last_value for the page' do
-      next_page = subject
-
-      expect(next_page.last_value).to eq(relation.last.id)
-      expect(next_page.column).to eq(page.column)
-      expect(next_page.per_page).to eq(page.per_page)
-    end
-
-    context 'when the page is empty' do
-      let(:relation) { Project.none }
-
-      it 'returns a Page indicating its emptiness' do
-        next_page = subject
-
-        expect(next_page.empty?).to be_truthy
-        expect(next_page.column).to eq(page.column)
-        expect(next_page.per_page).to eq(page.per_page)
-      end
-    end
-  end
-end

spec/lib/gitlab/pagination/keyset/pager_spec.rb

@@ -3,54 +3,40 @@
 require 'spec_helper'
 
 describe Gitlab::Pagination::Keyset::Pager do
-  let(:relation) { Project.all }
-  let(:request) { double('request', page: page) }
-  let(:page) { double('page', per_page: 20, column: :id, last_value: 10) }
+  let(:relation) { Project.all.order(id: :asc) }
+  let(:request) { double('request', page: page, apply_headers: nil) }
+  let(:page) { double('page', per_page: 20, order_by: { id: :asc }, lower_bounds: nil, next: nil) }
+  let(:next_page) { double('next page') }
+
+  before_all do
+    create_list(:project, 5)
+  end
 
   describe '#paginate' do
     subject { described_class.new(request).paginate(relation) }
 
     it 'applies a limit' do
-      allow(relation).to receive(:order).and_return(relation)
       expect(relation).to receive(:limit).with(page.per_page).and_call_original
 
       subject
     end
 
-    it 'sorts by pagination order' do
-      allow(relation).to receive(:limit).and_return(relation)
-      expect(relation).to receive(:reorder).with(page.column => :asc).and_call_original
-
-      subject
-    end
-
-    context 'without paging information' do
-      let(:page) { double('page', per_page: 20, column: :id, last_value: nil) }
-
-      it 'considers this the first page and does not apply any filter' do
-        allow(relation).to receive(:limit).and_return(relation)
-
-        expect(relation).not_to receive(:where)
-
+    it 'loads the result relation only once' do
+      expect do
         subject
-      end
+      end.not_to exceed_query_limit(1)
     end
 
-    it 'applies a filter based on the paging information' do
-      allow(relation).to receive(:limit).and_return(relation)
-      allow(relation).to receive(:order).and_return(relation)
-
-      expect(relation).to receive(:where).with('id > ?', 10).and_call_original
+    it 'passes information about next page to request' do
+      lower_bounds = relation.last.slice(:id)
+      expect(page).to receive(:next).with(lower_bounds, false).and_return(next_page)
+      expect(request).to receive(:apply_headers).with(next_page)
 
       subject
     end
 
-    it 'adds limit, order,where to the query' do
-      expect(subject.relation).to eq(Project.where('id > ?', page.last_value).limit(page.per_page).order(id: :asc))
-    end
-
-    it 'passes through the page information' do
-      expect(subject.page).to eq(page)
+    it 'returns the limited relation' do
+      expect(subject).to eq(relation.limit(20))
     end
   end
 end

spec/lib/gitlab/pagination/keyset/request_context_spec.rb

@@ -4,59 +4,73 @@ require 'spec_helper'
 
 describe Gitlab::Pagination::Keyset::RequestContext do
   let(:request) { double('request', params: params) }
-  let(:params) { { id_after: 5, per_page: 10 } }
 
   describe '#page' do
     subject { described_class.new(request).page }
 
-    it 'extracts last_value information' do
-      page = subject
+    let(:params) { { order_by: :id } }
 
-      expect(page.last_value).to eq(params[:id_after])
+    context 'with only order_by given' do
+      let(:params) { { order_by: :id } }
+
+      it 'extracts order_by/sorting information' do
+        page = subject
+
+        expect(page.order_by).to eq(id: :asc)
+      end
     end
 
-    it 'extracts per_page information' do
-      page = subject
+    context 'with order_by and sort given' do
+      let(:params) { { order_by: :created_at, sort: :desc } }
 
-      expect(page.per_page).to eq(params[:per_page])
+      it 'extracts order_by/sorting information and adds tie breaker' do
+        page = subject
+
+        expect(page.order_by).to eq(created_at: :desc, id: :desc)
+      end
+    end
+
+    context 'with no order_by information given' do
+      let(:params) { {} }
+
+      it 'defaults to tie breaker' do
+        page = subject
+
+        expect(page.order_by).to eq({ id: :desc })
+      end
     end
 
-    context 'with no id_after value present' do
-      let(:params) { { id_after: 5, per_page: 10 } }
+    context 'with per_page params given' do
+      let(:params) { { per_page: 10 } }
 
-      it 'indicates this is the first page' do
+      it 'extracts per_page information' do
         page = subject
 
-        expect(page.first_page?).to be_truthy
+        expect(page.per_page).to eq(params[:per_page])
       end
     end
   end
 
   describe '#apply_headers' do
-    let(:paged_relation) { double('paged relation', next_page: next_page) }
     let(:request) { double('request', url: "http://#{Gitlab.config.gitlab.host}/api/v4/projects?foo=bar") }
     let(:params) { { foo: 'bar' } }
     let(:request_context) { double('request context', params: params, request: request) }
-    let(:next_page) { double('next page', last_value: 42, empty?: false) }
+    let(:next_page) { double('next page', order_by: { id: :asc }, lower_bounds: { id: 42 }, end_reached?: false) }
 
-    subject { described_class.new(request_context).apply_headers(paged_relation) }
+    subject { described_class.new(request_context).apply_headers(next_page) }
 
-    it 'sets Links header with a link to the first page' do
+    it 'sets Links header with same host/path as the original request' do
       orig_uri = URI.parse(request_context.request.url)
 
       expect(request_context).to receive(:header) do |name, header|
         expect(name).to eq('Links')
 
-        first_link, _ = /<([^>]+)>; rel="first"/.match(header).captures
+        first_link, _ = /<([^>]+)>; rel="next"/.match(header).captures
 
-        URI.parse(first_link).tap do |uri|
-          expect(uri.host).to eq(orig_uri.host)
-          expect(uri.path).to eq(orig_uri.path)
+        uri = URI.parse(first_link)
 
-          query = CGI.parse(uri.query)
-          expect(query.except('id_after')).to eq(CGI.parse(orig_uri.query).except("id_after"))
-          expect(query['id_after']).to be_empty
-        end
+        expect(uri.host).to eq(orig_uri.host)
+        expect(uri.path).to eq(orig_uri.path)
       end
 
       subject
@@ -70,17 +84,34 @@ describe Gitlab::Pagination::Keyset::RequestContext do
 
         first_link, _ = /<([^>]+)>; rel="next"/.match(header).captures
 
-        URI.parse(first_link).tap do |uri|
-          expect(uri.host).to eq(orig_uri.host)
-          expect(uri.path).to eq(orig_uri.path)
+        query = CGI.parse(URI.parse(first_link).query)
 
-          query = CGI.parse(uri.query)
-          expect(query.except('id_after')).to eq(CGI.parse(orig_uri.query).except("id_after"))
-          expect(query['id_after']).to eq(["42"])
-        end
+        expect(query.except('id_after')).to eq(CGI.parse(orig_uri.query).except('id_after'))
+        expect(query['id_after']).to eq(['42'])
       end
 
       subject
     end
+
+    context 'with descending order' do
+      let(:next_page) { double('next page', order_by: { id: :desc }, lower_bounds: { id: 42 }, end_reached?: false) }
+
+      it 'sets Links header with a link to the next page' do
+        orig_uri = URI.parse(request_context.request.url)
+
+        expect(request_context).to receive(:header) do |name, header|
+          expect(name).to eq('Links')
+
+          first_link, _ = /<([^>]+)>; rel="next"/.match(header).captures
+
+          query = CGI.parse(URI.parse(first_link).query)
+
+          expect(query.except('id_before')).to eq(CGI.parse(orig_uri.query).except('id_before'))
+          expect(query['id_before']).to eq(['42'])
+        end
+
+        subject
+      end
+    end
   end
 end

spec/lib/gitlab/pagination/keyset_spec.rb

@@ -6,29 +6,16 @@ describe Gitlab::Pagination::Keyset do
   describe '.paginate' do
     subject { described_class.paginate(request_context, relation) }
 
-    let(:request_context) { instance_double(Gitlab::Pagination::Keyset::RequestContext, apply_headers: nil) }
-    let(:pager) { instance_double(Gitlab::Pagination::Keyset::Pager, paginate: paged_relation)}
-    let(:relation) { double('relation') }
-    let(:paged_relation) { double('paged relation', relation: double) }
+    let(:request_context) { double }
+    let(:relation) { double }
+    let(:pager) { double }
+    let(:result) { double }
 
-    before do
-      allow(Gitlab::Pagination::Keyset::Pager).to receive(:new).with(request_context).and_return(pager)
-    end
-
-    it 'applies headers' do
-      expect(request_context).to receive(:apply_headers).with(paged_relation)
-
-      subject
-    end
-
-    it 'returns the paginated relation' do
-      expect(subject).to eq(paged_relation.relation)
-    end
-
-    it 'paginates the relation' do
-      expect(pager).to receive(:paginate).with(relation).and_return(paged_relation)
+    it 'uses Pager to paginate the relation' do
+      expect(Gitlab::Pagination::Keyset::Pager).to receive(:new).with(request_context).and_return(pager)
+      expect(pager).to receive(:paginate).with(relation).and_return(result)
 
-      subject
+      expect(subject).to eq(result)
     end
   end
 end