Bag of small changes and improvements

Squashing quite a few smaller fixes from review

lib/api/helpers/pagination.rb

@@ -4,7 +4,8 @@ module API
   module Helpers
     module Pagination
       def paginate(relation)
-        if params[:pagination] == "keyset" && Feature.enabled?(:api_keyset_pagination)
+        return Gitlab::Pagination::OffsetPagination.new(self).paginate(relation) unless keyset_pagination_enabled?
+
         request_context = Gitlab::Pagination::Keyset::RequestContext.new(self)
 
         unless Gitlab::Pagination::Keyset.available?(request_context, relation)
@@ -12,9 +13,12 @@ module API
         end
 
         Gitlab::Pagination::Keyset.paginate(request_context, relation)
-        else
-          Gitlab::Pagination::OffsetPagination.new(self).paginate(relation)
       end
+
+      private
+
+      def keyset_pagination_enabled?
+        params[:pagination] == 'keyset' && Feature.enabled?(:api_keyset_pagination)
       end
     end
   end

lib/gitlab/pagination/keyset.rb

@@ -12,7 +12,7 @@ module Gitlab
 
         # This is only available for Project and order-by id (asc/desc)
         return false unless relation.klass == Project
-        return false unless order_by[:id]
+        return false unless order_by.size == 1 && order_by[:id]
 
         true
       end

lib/gitlab/pagination/keyset/page.rb

@@ -12,7 +12,7 @@ module Gitlab
         attr_reader :order_by
 
         def initialize(order_by: {}, lower_bounds: nil, per_page: DEFAULT_PAGE_SIZE, end_reached: false)
-          @order_by = order_by
+          @order_by = order_by.with_indifferent_access
           @lower_bounds = lower_bounds
           @per_page = per_page
           @end_reached = end_reached

lib/gitlab/pagination/keyset/pager.rb

@@ -12,7 +12,7 @@ module Gitlab
 
         def paginate(relation)
           # Validate assumption: The last two columns must match the page order_by
-          raise "Page's order_by doesnt match the relation's order: #{present_order} vs #{page.order_by}" unless correct_order?(relation)
+          validate_order!(relation)
 
           # This performs the database query and retrieves records
           # We retrieve one record more to check if we have data beyond this page
@@ -43,10 +43,12 @@ module Gitlab
           @page ||= request.page
         end
 
-        def correct_order?(rel)
+        def validate_order!(rel)
           present_order = rel.order_values.map { |val| [val.expr.name, val.direction] }.last(2).to_h
 
-          page.order_by.with_indifferent_access == present_order.with_indifferent_access
+          unless page.order_by.with_indifferent_access == present_order.with_indifferent_access
+            raise ArgumentError, "Page's order_by does not match the relation's order: #{present_order} vs #{page.order_by}"
+          end
         end
       end
     end

lib/gitlab/pagination/keyset/request_context.rb

@@ -6,8 +6,13 @@ module Gitlab
       class RequestContext
         attr_reader :request
 
-        DEFAULT_SORT_DIRECTION = :asc
-        TIE_BREAKER = { id: :desc }.freeze
+        DEFAULT_SORT_DIRECTION = :desc
+        PRIMARY_KEY = :id
+
+        # A tie breaker is added as an additional order-by column
+        # to establish a well-defined order. We use the primary key
+        # column here.
+        TIE_BREAKER = { PRIMARY_KEY => DEFAULT_SORT_DIRECTION }.freeze
 
         def initialize(request)
           @request = request
@@ -15,7 +20,7 @@ module Gitlab
 
         # extracts Paging information from request parameters
         def page
-          Page.new(order_by: order_by, per_page: params[:per_page])
+          @page ||= Page.new(order_by: order_by, per_page: params[:per_page])
         end
 
         def apply_headers(next_page)
@@ -27,14 +32,18 @@ module Gitlab
         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 = { 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]
+          order_by = order_by.merge(TIE_BREAKER) unless order_by[PRIMARY_KEY]
 
           order_by
         end
 
+        def params
+          @params ||= request.params
+        end
+
         def lower_bounds_params(page)
           page.lower_bounds.each_with_object({}) do |(column, value), params|
             filter = filter_with_comparator(page, column)
@@ -52,8 +61,10 @@ module Gitlab
           end
         end
 
-        def params
-          @params ||= request.params
+        def page_href(page)
+          base_request_uri.tap do |uri|
+            uri.query = query_params_for(page).to_query
+          end.to_s
         end
 
         def pagination_links(next_page)
@@ -72,12 +83,6 @@ module Gitlab
         def query_params_for(page)
           request.params.merge(lower_bounds_params(page))
         end
-
-        def page_href(page)
-          base_request_uri.tap do |uri|
-            uri.query = query_params_for(page).to_query
-          end.to_s
-        end
       end
     end
   end

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

@@ -21,6 +21,12 @@ describe Gitlab::Pagination::Keyset::Page do
 
       expect(per_page).to eq(described_class::DEFAULT_PAGE_SIZE)
     end
+
+    it 'uses the given value if it is within range' do
+      per_page = described_class.new(per_page: 10).per_page
+
+      expect(per_page).to eq(10)
+    end
   end
 
   describe '#next' do

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

@@ -5,11 +5,11 @@ require 'spec_helper'
 describe Gitlab::Pagination::Keyset::Pager do
   let(:relation) { Project.all.order(id: :asc) }
   let(:request) { double('request', page: page, apply_headers: nil) }
-  let(:page) { Gitlab::Pagination::Keyset::Page.new(order_by: { id: :asc }, per_page: 20) }
+  let(:page) { Gitlab::Pagination::Keyset::Page.new(order_by: { id: :asc }, per_page: 3) }
   let(:next_page) { double('next page') }
 
   before_all do
-    create_list(:project, 25)
+    create_list(:project, 7)
   end
 
   describe '#paginate' do
@@ -22,15 +22,27 @@ describe Gitlab::Pagination::Keyset::Pager do
     end
 
     it 'passes information about next page to request' do
-      lower_bounds = relation.limit(20).last.slice(:id)
+      lower_bounds = relation.limit(page.per_page).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
 
-    context 'while retrieving the last page' do
-      let(:relation) { Project.where('id >= ?', Project.maximum(:id) - 10).order(id: :asc) }
+    context 'when retrieving the last page' do
+      let(:relation) { Project.where('id > ?', Project.maximum(:id) - page.per_page).order(id: :asc) }
+
+      it 'indicates this is the last page' do
+        expect(request).to receive(:apply_headers) do |next_page|
+          expect(next_page.end_reached?).to be_truthy
+        end
+
+        subject
+      end
+    end
+
+    context 'when retrieving an empty page' do
+      let(:relation) { Project.where('id > ?', Project.maximum(:id) + 1).order(id: :asc) }
 
       it 'indicates this is the last page' do
         expect(request).to receive(:apply_headers) do |next_page|
@@ -42,7 +54,15 @@ describe Gitlab::Pagination::Keyset::Pager do
     end
 
     it 'returns an array with the loaded records' do
-      expect(subject).to eq(relation.limit(20).to_a)
+      expect(subject).to eq(relation.limit(page.per_page).to_a)
+    end
+
+    context 'validating the order clause' do
+      let(:page) { Gitlab::Pagination::Keyset::Page.new(order_by: { created_at: :asc }, per_page: 3) }
+
+      it 'raises an error if has a different order clause than the page' do
+        expect { subject }.to raise_error(ArgumentError, /order_by does not match/)
+      end
     end
   end
 end

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

@@ -8,15 +8,13 @@ describe Gitlab::Pagination::Keyset::RequestContext do
   describe '#page' do
     subject { described_class.new(request).page }
 
-    let(:params) { { order_by: :id } }
-
     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)
+        expect(page.order_by).to eq('id' => :desc)
       end
     end
 
@@ -26,7 +24,7 @@ describe Gitlab::Pagination::Keyset::RequestContext do
       it 'extracts order_by/sorting information and adds tie breaker' do
         page = subject
 
-        expect(page.order_by).to eq(created_at: :desc, id: :desc)
+        expect(page.order_by).to eq('created_at' => :desc, 'id' => :desc)
       end
     end
 
@@ -36,7 +34,7 @@ describe Gitlab::Pagination::Keyset::RequestContext do
       it 'defaults to tie breaker' do
         page = subject
 
-        expect(page.order_by).to eq({ id: :desc })
+        expect(page.order_by).to eq({ 'id' => :desc })
       end
     end
 

spec/lib/gitlab/pagination/keyset_spec.rb

@@ -48,7 +48,7 @@ describe Gitlab::Pagination::Keyset do
     end
 
     context 'with other order-by columns' do
-      let(:order_by) { { created_at: :desc } }
+      let(:order_by) { { created_at: :desc, id: :desc } }
       it 'returns false for Project' do
         expect(subject.available?(request_context, Project.all)).to be_falsey
       end

spec/requests/api/projects_spec.rb

@@ -570,6 +570,87 @@ describe API::Projects do
         let(:projects) { Project.all }
       end
     end
+
+    context 'with keyset pagination' do
+      let(:current_user) { user }
+      let(:projects) { [public_project, project, project2, project3] }
+
+      context 'headers and records' do
+        let(:params) { { pagination: 'keyset', order_by: :id, sort: :asc, per_page: 1 } }
+
+        it 'includes a pagination header with link to the next page' do
+          get api('/projects', current_user), params: params
+
+          expect(response.header).to include('Links')
+          expect(response.header['Links']).to include('pagination=keyset')
+          expect(response.header['Links']).to include("id_after=#{public_project.id}")
+        end
+
+        it 'contains only the first project with per_page = 1' do
+          get api('/projects', current_user), params: params
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response).to be_an Array
+          expect(json_response.map { |p| p['id'] }).to contain_exactly(public_project.id)
+        end
+
+        it 'does not include a link if the end has reached and there is no more data' do
+          get api('/projects', current_user), params: params.merge(id_after: project2.id)
+
+          expect(response.header).not_to include('Links')
+        end
+
+        it 'responds with 501 if order_by is different from id' do
+          get api('/projects', current_user), params: params.merge(order_by: :created_at)
+
+          expect(response).to have_gitlab_http_status(501)
+        end
+      end
+
+      context 'with descending sorting' do
+        let(:params) { { pagination: 'keyset', order_by: :id, sort: :desc, per_page: 1 } }
+
+        it 'includes a pagination header with link to the next page' do
+          get api('/projects', current_user), params: params
+
+          expect(response.header).to include('Links')
+          expect(response.header['Links']).to include('pagination=keyset')
+          expect(response.header['Links']).to include("id_before=#{project3.id}")
+        end
+
+        it 'contains only the last project with per_page = 1' do
+          get api('/projects', current_user), params: params
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response).to be_an Array
+          expect(json_response.map { |p| p['id'] }).to contain_exactly(project3.id)
+        end
+      end
+
+      context 'retrieving the full relation' do
+        let(:params) { { pagination: 'keyset', order_by: :id, sort: :desc, per_page: 2 } }
+
+        it 'returns all projects' do
+          url = '/projects'
+          requests = 0
+          ids = []
+
+          while url && requests <= 5 # circuit breaker
+            requests += 1
+            get api(url, current_user), params: params
+
+            links = response.header['Links']
+            url = links&.match(/<[^>]+(\/projects\?[^>]+)>; rel="next"/) do |match|
+              match[1]
+            end
+
+            ids += JSON.parse(response.body).map { |p| p['id'] }
+          end
+
+          expect(ids).to contain_exactly(*projects.map(&:id))
+        end
+      end
+    end
   end
 
   describe 'POST /projects' do