Merge branch 'ab/keyset-two-step' into 'master'

Fix and re-enable keyset pagination

See merge request gitlab-org/gitlab!22424

doc/api/README.md

@@ -426,7 +426,7 @@ Status: 200 OK
 The link to the next page contains an additional filter `id_after=42` which excludes records we have retrieved already.
 Note the type of filter depends on the `order_by` option used and we may have more than one additional filter.
 
-The `Link` header is absent when the end of the collection has been reached and there are no additional records to retrieve.
+When the end of the collection has been reached and there are no additional records to retrieve, the `Link` header is absent and the resulting array is empty.
 
 We recommend using only the given link to retrieve the next page instead of building your own URL. Apart from the headers shown,
 we don't expose additional pagination headers.

lib/api/helpers.rb

@@ -4,6 +4,7 @@ module API
   module Helpers
     include Gitlab::Utils
     include Helpers::Pagination
+    include Helpers::PaginationStrategies
 
     SUDO_HEADER = "HTTP_SUDO"
     GITLAB_SHARED_SECRET_HEADER = "Gitlab-Shared-Secret"

lib/api/helpers/pagination.rb

@@ -3,34 +3,9 @@
 module API
   module Helpers
     module Pagination
-      # This returns an ActiveRecord relation
       def paginate(relation)
         Gitlab::Pagination::OffsetPagination.new(self).paginate(relation)
       end
-
-      # This applies pagination and executes the query
-      # It always returns an array instead of an ActiveRecord relation
-      def paginate_and_retrieve!(relation)
-        offset_or_keyset_pagination(relation).to_a
-      end
-
-      private
-
-      def offset_or_keyset_pagination(relation)
-        return paginate(relation) unless keyset_pagination_enabled?
-
-        request_context = Gitlab::Pagination::Keyset::RequestContext.new(self)
-
-        unless Gitlab::Pagination::Keyset.available?(request_context, relation)
-          return error!('Keyset pagination is not yet available for this type of request', 405)
-        end
-
-        Gitlab::Pagination::Keyset.paginate(request_context, relation)
-      end
-
-      def keyset_pagination_enabled?
-        params[:pagination] == 'keyset' && Feature.enabled?(:api_keyset_pagination, default_enabled: true)
-      end
     end
   end
 end

lib/api/helpers/pagination_strategies.rb

+# frozen_string_literal: true
+
+module API
+  module Helpers
+    module PaginationStrategies
+      def paginate_with_strategies(relation)
+        paginator = paginator(relation)
+
+        yield(paginator.paginate(relation)).tap do |records, _|
+          paginator.finalize(records)
+        end
+      end
+
+      def paginator(relation)
+        return Gitlab::Pagination::OffsetPagination.new(self) unless keyset_pagination_enabled?
+
+        request_context = Gitlab::Pagination::Keyset::RequestContext.new(self)
+
+        unless Gitlab::Pagination::Keyset.available?(request_context, relation)
+          return error!('Keyset pagination is not yet available for this type of request', 405)
+        end
+
+        Gitlab::Pagination::Keyset::Pager.new(request_context)
+      end
+
+      private
+
+      def keyset_pagination_enabled?
+        params[:pagination] == 'keyset' && Feature.enabled?(:api_keyset_pagination, default_enabled: true)
+      end
+    end
+  end
+end

lib/api/projects.rb

@@ -90,18 +90,22 @@ module API
       def present_projects(projects, options = {})
         projects = reorder_projects(projects)
         projects = apply_filters(projects)
-        projects = paginate(projects)
-        projects, options = with_custom_attributes(projects, options)
 
-        options = options.reverse_merge(
-          with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails,
-          statistics: params[:statistics],
-          current_user: current_user,
-          license: false
-        )
-        options[:with] = Entities::BasicProjectDetails if params[:simple]
+        records, options = paginate_with_strategies(projects) do |projects|
+          projects, options = with_custom_attributes(projects, options)
+
+          options = options.reverse_merge(
+            with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails,
+            statistics: params[:statistics],
+            current_user: current_user,
+            license: false
+          )
+          options[:with] = Entities::BasicProjectDetails if params[:simple]
+
+          [options[:with].prepare_relation(projects, options), options]
+        end
 
-        present options[:with].prepare_relation(projects, options), options
+        present records, options
       end
 
       def translate_params_for_compatibility(params)

lib/gitlab/pagination/base.rb

@@ -3,6 +3,14 @@
 module Gitlab
   module Pagination
     class Base
+      def paginate(relation)
+        raise NotImplementedError
+      end
+
+      def finalize(records)
+        # Optional: Called with the actual set of records
+      end
+
       private
 
       def per_page

lib/gitlab/pagination/keyset.rb

@@ -3,10 +3,6 @@
 module Gitlab
   module Pagination
     module Keyset
-      def self.paginate(request_context, relation)
-        Gitlab::Pagination::Keyset::Pager.new(request_context).paginate(relation)
-      end
-
       def self.available?(request_context, relation)
         order_by = request_context.page.order_by
 

lib/gitlab/pagination/keyset/page.rb

@@ -11,14 +11,13 @@ module Gitlab
         # Maximum number of records for a page
         MAXIMUM_PAGE_SIZE = 100
 
-        attr_accessor :lower_bounds, :end_reached
+        attr_accessor :lower_bounds
         attr_reader :order_by
 
-        def initialize(order_by: {}, lower_bounds: nil, per_page: DEFAULT_PAGE_SIZE, end_reached: false)
+        def initialize(order_by: {}, lower_bounds: nil, per_page: DEFAULT_PAGE_SIZE)
           @order_by = order_by.symbolize_keys
           @lower_bounds = lower_bounds&.symbolize_keys
           @per_page = per_page
-          @end_reached = end_reached
         end
 
         # Number of records to return per page
@@ -28,17 +27,11 @@ module Gitlab
           [@per_page, MAXIMUM_PAGE_SIZE].min
         end
 
-        # Determine whether this page indicates the end of the collection
-        def end_reached?
-          @end_reached
-        end
-
         # Construct a Page for the next page
         # Uses identical order_by/per_page information for the next page
-        def next(lower_bounds, end_reached)
+        def next(lower_bounds)
           dup.tap do |next_page|
             next_page.lower_bounds = lower_bounds&.symbolize_keys
-            next_page.end_reached = end_reached
           end
         end
       end

lib/gitlab/pagination/keyset/pager.rb

@@ -14,27 +14,20 @@ module Gitlab
           # Validate assumption: The last two columns must match the page order_by
           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
-          all_records = relation.limit(page.per_page + 1).to_a # rubocop: disable CodeReuse/ActiveRecord
-
-          records_for_page = all_records.first(page.per_page)
-
-          # If we retrieved more records than belong on this page,
-          # we know there's a next page
-          there_is_more = all_records.size > records_for_page.size
-          apply_headers(records_for_page.last, there_is_more)
+          relation.limit(page.per_page) # rubocop: disable CodeReuse/ActiveRecord
+        end
 
-          records_for_page
+        def finalize(records)
+          apply_headers(records.last)
         end
 
         private
 
-        def apply_headers(last_record_in_page, there_is_more)
-          end_reached = last_record_in_page.nil? || !there_is_more
-          lower_bounds = last_record_in_page&.slice(page.order_by.keys)
+        def apply_headers(last_record_in_page)
+          return unless last_record_in_page
 
-          next_page = page.next(lower_bounds, end_reached)
+          lower_bounds = last_record_in_page&.slice(page.order_by.keys)
+          next_page = page.next(lower_bounds)
 
           request.apply_headers(next_page)
         end

lib/gitlab/pagination/keyset/request_context.rb

@@ -68,8 +68,6 @@ module Gitlab
         end
 
         def pagination_links(next_page)
-          return if next_page.end_reached?
-
           %(<#{page_href(next_page)}>; rel="next")
         end
 

spec/lib/api/helpers/pagination_spec.rb

@@ -5,70 +5,14 @@ require 'spec_helper'
 describe API::Helpers::Pagination do
   subject { Class.new.include(described_class).new }
 
-  let(:expected_result) { double("result", to_a: double) }
-  let(:relation) { double("relation") }
-  let(:params) { {} }
+  let(:paginator) { double('paginator') }
+  let(:relation) { double('relation') }
+  let(:expected_result) { double('expected result') }
 
-  before do
-    allow(subject).to receive(:params).and_return(params)
-  end
-
-  describe '#paginate' do
-    let(:offset_pagination) { double("offset pagination") }
-
-    it 'delegates to OffsetPagination' do
-      expect(::Gitlab::Pagination::OffsetPagination).to receive(:new).with(subject).and_return(offset_pagination)
-      expect(offset_pagination).to receive(:paginate).with(relation).and_return(expected_result)
-
-      result = subject.paginate(relation)
-
-      expect(result).to eq(expected_result)
-    end
-  end
-
-  describe '#paginate_and_retrieve!' do
-    context 'for offset pagination' do
-      before do
-        allow(Gitlab::Pagination::Keyset).to receive(:available?).and_return(false)
-      end
-
-      it 'delegates to paginate' do
-        expect(subject).to receive(:paginate).with(relation).and_return(expected_result)
-
-        result = subject.paginate_and_retrieve!(relation)
-
-        expect(result).to eq(expected_result.to_a)
-      end
-    end
-
-    context 'for keyset pagination' do
-      let(:params) { { pagination: 'keyset' } }
-      let(:request_context) { double('request context') }
-
-      before do
-        allow(Gitlab::Pagination::Keyset::RequestContext).to receive(:new).with(subject).and_return(request_context)
-      end
-
-      context 'when keyset pagination is available' do
-        it 'delegates to KeysetPagination' do
-          expect(Gitlab::Pagination::Keyset).to receive(:available?).and_return(true)
-          expect(Gitlab::Pagination::Keyset).to receive(:paginate).with(request_context, relation).and_return(expected_result)
-
-          result = subject.paginate_and_retrieve!(relation)
-
-          expect(result).to eq(expected_result.to_a)
-        end
-      end
-
-      context 'when keyset pagination is not available' do
-        it 'renders a 501 error if keyset pagination isnt available yet' do
-          expect(Gitlab::Pagination::Keyset).to receive(:available?).with(request_context, relation).and_return(false)
-          expect(Gitlab::Pagination::Keyset).not_to receive(:paginate)
-          expect(subject).to receive(:error!).with(/not yet available/, 405)
+  it 'delegates to OffsetPagination' do
+    expect(Gitlab::Pagination::OffsetPagination).to receive(:new).with(subject).and_return(paginator)
+    expect(paginator).to receive(:paginate).with(relation).and_return(expected_result)
 
-          subject.paginate_and_retrieve!(relation)
-        end
-      end
-    end
+    expect(subject.paginate(relation)).to eq(expected_result)
   end
 end

spec/lib/api/helpers/pagination_strategies_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Helpers::PaginationStrategies do
+  subject { Class.new.include(described_class).new }
+
+  let(:expected_result) { double("result") }
+  let(:relation) { double("relation") }
+  let(:params) { {} }
+
+  before do
+    allow(subject).to receive(:params).and_return(params)
+  end
+
+  describe '#paginate_with_strategies' do
+    let(:paginator) { double("paginator", paginate: expected_result, finalize: nil) }
+
+    before do
+      allow(subject).to receive(:paginator).with(relation).and_return(paginator)
+    end
+
+    it 'yields paginated relation' do
+      expect { |b| subject.paginate_with_strategies(relation, &b) }.to yield_with_args(expected_result)
+    end
+
+    it 'calls #finalize with first value returned from block' do
+      return_value = double
+      expect(paginator).to receive(:finalize).with(return_value)
+
+      subject.paginate_with_strategies(relation) do |records|
+        some_options = {}
+        [return_value, some_options]
+      end
+    end
+
+    it 'returns whatever the block returns' do
+      return_value = [double, double]
+
+      result = subject.paginate_with_strategies(relation) do |records|
+        return_value
+      end
+
+      expect(result).to eq(return_value)
+    end
+  end
+
+  describe '#paginator' do
+    context 'offset pagination' do
+      let(:paginator) { double("paginator") }
+
+      before do
+        allow(subject).to receive(:keyset_pagination_enabled?).and_return(false)
+      end
+
+      it 'delegates to OffsetPagination' do
+        expect(Gitlab::Pagination::OffsetPagination).to receive(:new).with(subject).and_return(paginator)
+
+        expect(subject.paginator(relation)).to eq(paginator)
+      end
+    end
+
+    context 'for keyset pagination' do
+      let(:params) { { pagination: 'keyset' } }
+      let(:request_context) { double('request context') }
+      let(:pager) { double('pager') }
+
+      before do
+        allow(subject).to receive(:keyset_pagination_enabled?).and_return(true)
+        allow(Gitlab::Pagination::Keyset::RequestContext).to receive(:new).with(subject).and_return(request_context)
+      end
+
+      context 'when keyset pagination is available' do
+        before do
+          allow(Gitlab::Pagination::Keyset).to receive(:available?).and_return(true)
+          allow(Gitlab::Pagination::Keyset::Pager).to receive(:new).with(request_context).and_return(pager)
+        end
+
+        it 'delegates to Pager' do
+          expect(subject.paginator(relation)).to eq(pager)
+        end
+      end
+
+      context 'when keyset pagination is not available' do
+        before do
+          allow(Gitlab::Pagination::Keyset).to receive(:available?).with(request_context, relation).and_return(false)
+        end
+
+        it 'renders a 501 error' do
+          expect(subject).to receive(:error!).with(/not yet available/, 405)
+
+          subject.paginator(relation)
+        end
+      end
+    end
+  end
+end

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

@@ -30,16 +30,14 @@ describe Gitlab::Pagination::Keyset::Page do
   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(:page) { described_class.new(order_by: order_by, lower_bounds: lower_bounds, per_page: per_page) }
+    subject { page.next(new_lower_bounds) }
 
     let(:order_by) { { id: :desc } }
     let(:lower_bounds) { { id: 42 } }
     let(:per_page) { 10 }
-    let(:end_reached) { false }
 
     let(:new_lower_bounds) { { id: 21 } }
-    let(:new_end_reached) { true }
 
     it 'copies over order_by' do
       expect(subject.order_by).to eq(page.order_by)
@@ -57,10 +55,5 @@ describe Gitlab::Pagination::Keyset::Page 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/pager_spec.rb

@@ -15,15 +15,37 @@ describe Gitlab::Pagination::Keyset::Pager do
   describe '#paginate' do
     subject { described_class.new(request).paginate(relation) }
 
-    it 'loads the result relation only once' do
+    it 'does not execute a query' do
       expect do
         subject
-      end.not_to exceed_query_limit(1)
+      end.not_to exceed_query_limit(0)
     end
 
+    it 'applies a LIMIT' do
+      expect(subject.limit_value).to eq(page.per_page)
+    end
+
+    it 'returns the limited relation' do
+      expect(subject).to eq(relation.limit(page.per_page))
+    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
+
+  describe '#finalize' do
+    let(:records) { relation.limit(page.per_page).load }
+
+    subject { described_class.new(request).finalize(records) }
+
     it 'passes information about next page to request' do
-      lower_bounds = relation.limit(page.per_page).last.slice(:id)
-      expect(page).to receive(:next).with(lower_bounds, false).and_return(next_page)
+      lower_bounds = records.last.slice(:id)
+      expect(page).to receive(:next).with(lower_bounds).and_return(next_page)
       expect(request).to receive(:apply_headers).with(next_page)
 
       subject
@@ -32,10 +54,10 @@ describe Gitlab::Pagination::Keyset::Pager do
     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
+      it 'indicates there is another (likely empty) page' do
+        lower_bounds = records.last.slice(:id)
+        expect(page).to receive(:next).with(lower_bounds).and_return(next_page)
+        expect(request).to receive(:apply_headers).with(next_page)
 
         subject
       end
@@ -45,24 +67,10 @@ describe Gitlab::Pagination::Keyset::Pager 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|
-          expect(next_page.end_reached?).to be_truthy
-        end
+        expect(request).not_to receive(:apply_headers)
 
         subject
       end
     end
-
-    it 'returns an array with the loaded records' do
-      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

@@ -53,7 +53,7 @@ describe Gitlab::Pagination::Keyset::RequestContext do
     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', order_by: { id: :asc }, lower_bounds: { id: 42 }, end_reached?: false) }
+    let(:next_page) { double('next page', order_by: { id: :asc }, lower_bounds: { id: 42 }) }
 
     subject { described_class.new(request_context).apply_headers(next_page) }
 
@@ -92,7 +92,7 @@ describe Gitlab::Pagination::Keyset::RequestContext do
     end
 
     context 'with descending order' do
-      let(:next_page) { double('next page', order_by: { id: :desc }, lower_bounds: { id: 42 }, end_reached?: false) }
+      let(:next_page) { double('next page', order_by: { id: :desc }, lower_bounds: { id: 42 }) }
 
       it 'sets Links header with a link to the next page' do
         orig_uri = URI.parse(request_context.request.url)

spec/lib/gitlab/pagination/keyset_spec.rb

@@ -3,22 +3,6 @@
 require 'spec_helper'
 
 describe Gitlab::Pagination::Keyset do
-  describe '.paginate' do
-    subject { described_class.paginate(request_context, relation) }
-
-    let(:request_context) { double }
-    let(:relation) { double }
-    let(:pager) { double }
-    let(:result) { double }
-
-    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)
-
-      expect(subject).to eq(result)
-    end
-  end
-
   describe '.available?' do
     subject { described_class }
 

spec/requests/api/projects_spec.rb

@@ -570,6 +570,102 @@ 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 'still includes a link if the end has reached and there is no more data after this page' do
+          get api('/projects', current_user), params: params.merge(id_after: project2.id)
+
+          expect(response.header).to include('Links')
+          expect(response.header['Links']).to include('pagination=keyset')
+          expect(response.header['Links']).to include("id_after=#{project3.id}")
+        end
+
+        it 'does not include a next link when the page does not have any records' do
+          get api('/projects', current_user), params: params.merge(id_after: Project.maximum(:id))
+
+          expect(response.header).not_to include('Links')
+        end
+
+        it 'returns an empty array when the page does not have any records' do
+          get api('/projects', current_user), params: params.merge(id_after: Project.maximum(:id))
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response).to eq([])
+        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(405)
+        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