Merge branch '34565-support-limits-for-offset-pagination-and-define-default' into 'master'

Support limits for offset pagination and define default

See merge request gitlab-org/gitlab!28460

app/models/namespace.rb

@@ -349,7 +349,7 @@ class Namespace < ApplicationRecord
     # We default to PlanLimits.new otherwise a lot of specs would fail
     # On production each plan should already have associated limits record
     # https://gitlab.com/gitlab-org/gitlab/issues/36037
-    actual_plan.limits || PlanLimits.new
+    actual_plan.actual_limits
   end
 
   def actual_plan_name

app/models/plan.rb

@@ -26,6 +26,10 @@ class Plan < ApplicationRecord
     DEFAULT_PLANS
   end
 
+  def actual_limits
+    self.limits || PlanLimits.new
+  end
+
   def default?
     self.class.default_plans.include?(name)
   end

changelogs/unreleased/38358-update-migration-helpers-to-use-check-constraints-instead-of-change.yml

+---
+title: Support limits for offset based pagination
+merge_request: 28460
+author:
+type: changed

db/migrate/20200415203024_add_offset_pagination_plan_limit.rb

+# frozen_string_literal: true
+
+class AddOffsetPaginationPlanLimit < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :plan_limits, :offset_pagination_limit, :integer, default: 50000, null: false
+  end
+end

db/structure.sql

@@ -4793,7 +4793,8 @@ CREATE TABLE public.plan_limits (
     project_hooks integer DEFAULT 100 NOT NULL,
     group_hooks integer DEFAULT 50 NOT NULL,
     ci_project_subscriptions integer DEFAULT 2 NOT NULL,
-    ci_pipeline_schedules integer DEFAULT 10 NOT NULL
+    ci_pipeline_schedules integer DEFAULT 10 NOT NULL,
+    offset_pagination_limit integer DEFAULT 50000 NOT NULL
 );
 
 CREATE SEQUENCE public.plan_limits_id_seq
@@ -13603,6 +13604,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200415161021
 20200415161206
 20200415192656
+20200415203024
 20200416005331
 20200416111111
 20200416120128

doc/administration/instance_limits.md

@@ -99,6 +99,29 @@ header. Such emails don't create comments on issues or merge requests.
 Sentry payloads sent to GitLab have a 1 MB maximum limit, both for security reasons
 and to limit memory consumption.
 
+## Max offset allowed via REST API for offset-based pagination
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/34565) in GitLab 13.0.
+
+When using offset-based pagination in the REST API, there is a limit to the maximum
+requested offset into the set of results. This limit is only applied to endpoints that
+support keyset-based pagination. More information about pagination options can be
+found in the [API docs section on pagination](../api/README.md#pagination).
+
+To set this limit on a self-managed installation, run the following in the
+[GitLab Rails console](troubleshooting/debug.md#starting-a-rails-console-session):
+
+```ruby
+# If limits don't exist for the default plan, you can create one with:
+# Plan.default.create_limits!
+
+Plan.default.limits.update!(offset_pagination_limit: 10000)
+```
+
+- **Default offset pagination limit:** 50000
+
+NOTE: **Note:** Set the limit to `0` to disable it.
+
 ## CI/CD limits
 
 ### Number of jobs in active pipelines

lib/api/helpers/pagination_strategies.rb

@@ -3,19 +3,24 @@
 module API
   module Helpers
     module PaginationStrategies
-      def paginate_with_strategies(relation)
-        paginator = paginator(relation)
+      def paginate_with_strategies(relation, request_scope)
+        paginator = paginator(relation, request_scope)
 
         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?
+      def paginator(relation, request_scope = nil)
+        return keyset_paginator(relation) if keyset_pagination_enabled?
 
-        request_context = Gitlab::Pagination::Keyset::RequestContext.new(self)
+        offset_paginator(relation, request_scope)
+      end
+
+      private
 
+      def keyset_paginator(relation)
+        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
@@ -23,11 +28,28 @@ module API
         Gitlab::Pagination::Keyset::Pager.new(request_context)
       end
 
-      private
+      def offset_paginator(relation, request_scope)
+        offset_limit = limit_for_scope(request_scope)
+        if Gitlab::Pagination::Keyset.available_for_type?(relation) && offset_limit_exceeded?(offset_limit)
+          return error!("Offset pagination has a maximum allowed offset of #{offset_limit} " \
+            "for requests that return objects of type #{relation.klass}. " \
+            "Remaining records can be retrieved using keyset pagination.", 405)
+        end
+
+        Gitlab::Pagination::OffsetPagination.new(self)
+      end
 
       def keyset_pagination_enabled?
         params[:pagination] == 'keyset'
       end
+
+      def limit_for_scope(scope)
+        (scope || Plan.default).actual_limits.offset_pagination_limit
+      end
+
+      def offset_limit_exceeded?(offset_limit)
+        offset_limit.positive? && params[:page] * params[:per_page] > offset_limit
+      end
     end
   end
 end

lib/api/projects.rb

@@ -95,7 +95,7 @@ module API
         projects = reorder_projects(projects)
         projects = apply_filters(projects)
 
-        records, options = paginate_with_strategies(projects) do |projects|
+        records, options = paginate_with_strategies(projects, options[:request_scope]) do |projects|
           projects, options = with_custom_attributes(projects, options)
 
           options = options.reverse_merge(
@@ -313,7 +313,7 @@ module API
       get ':id/forks' do
         forks = ForkProjectsFinder.new(user_project, params: project_finder_params, current_user: current_user).execute
 
-        present_projects forks
+        present_projects forks, request_scope: user_project
       end
 
       desc 'Check pages access of this project'

lib/gitlab/pagination/keyset.rb

@@ -3,11 +3,18 @@
 module Gitlab
   module Pagination
     module Keyset
+      SUPPORTED_TYPES = [
+        Project
+      ].freeze
+
+      def self.available_for_type?(relation)
+        SUPPORTED_TYPES.include?(relation.klass)
+      end
+
       def self.available?(request_context, relation)
         order_by = request_context.page.order_by
 
-        # This is only available for Project and order-by id (asc/desc)
-        return false unless relation.klass == Project
+        return false unless available_for_type?(relation)
         return false unless order_by.size == 1 && order_by[:id]
 
         true

spec/lib/api/helpers/pagination_strategies_spec.rb

@@ -6,7 +6,7 @@ describe API::Helpers::PaginationStrategies do
   subject { Class.new.include(described_class).new }
 
   let(:expected_result) { double("result") }
-  let(:relation) { double("relation") }
+  let(:relation) { double("relation", klass: "SomeClass") }
   let(:params) { {} }
 
   before do
@@ -17,18 +17,18 @@ describe API::Helpers::PaginationStrategies do
     let(:paginator) { double("paginator", paginate: expected_result, finalize: nil) }
 
     before do
-      allow(subject).to receive(:paginator).with(relation).and_return(paginator)
+      allow(subject).to receive(:paginator).with(relation, nil).and_return(paginator)
     end
 
     it 'yields paginated relation' do
-      expect { |b| subject.paginate_with_strategies(relation, &b) }.to yield_with_args(expected_result)
+      expect { |b| subject.paginate_with_strategies(relation, nil, &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|
+      subject.paginate_with_strategies(relation, nil) do |records|
         some_options = {}
         [return_value, some_options]
       end
@@ -37,7 +37,7 @@ describe API::Helpers::PaginationStrategies do
     it 'returns whatever the block returns' do
       return_value = [double, double]
 
-      result = subject.paginate_with_strategies(relation) do |records|
+      result = subject.paginate_with_strategies(relation, nil) do |records|
         return_value
       end
 
@@ -47,18 +47,79 @@ describe API::Helpers::PaginationStrategies do
 
   describe '#paginator' do
     context 'offset pagination' do
+      let(:plan_limits) { Plan.default.actual_limits }
+      let(:offset_limit) { plan_limits.offset_pagination_limit }
       let(:paginator) { double("paginator") }
 
       before do
         allow(subject).to receive(:keyset_pagination_enabled?).and_return(false)
       end
 
+      context 'when keyset pagination is available for the relation' do
+        before do
+          allow(Gitlab::Pagination::Keyset).to receive(:available_for_type?).and_return(true)
+        end
+
+        context 'when a request scope is given' do
+          let(:params) { { per_page: 100, page: offset_limit / 100 + 1 } }
+          let(:request_scope) { double("scope", actual_limits: plan_limits) }
+
+          context 'when the scope limit is exceeded' do
+            it 'renders a 405 error' do
+              expect(subject).to receive(:error!).with(/maximum allowed offset/, 405)
+
+              subject.paginator(relation, request_scope)
+            end
+          end
+
+          context 'when the scope limit is not exceeded' do
+            let(:params) { { per_page: 100, page: offset_limit / 100 } }
+
+            it 'delegates to OffsetPagination' do
+              expect(Gitlab::Pagination::OffsetPagination).to receive(:new).with(subject).and_return(paginator)
+
+              expect(subject.paginator(relation, request_scope)).to eq(paginator)
+            end
+          end
+        end
+
+        context 'when a request scope is not given' do
+          context 'when the default limits are exceeded' do
+            let(:params) { { per_page: 100, page: offset_limit / 100 + 1 } }
+
+            it 'renders a 405 error' do
+              expect(subject).to receive(:error!).with(/maximum allowed offset/, 405)
+
+              subject.paginator(relation)
+            end
+          end
+
+          context 'when the default limits are not exceeded' do
+            let(:params) { { per_page: 100, page: offset_limit / 100 } }
+
+            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
+        end
+      end
+
+      context 'when keyset pagination is not available for the relation' do
+        let(:params) { { per_page: 100, page: offset_limit / 100 + 1 } }
+
+        before do
+          allow(Gitlab::Pagination::Keyset).to receive(:available_for_type?).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
+    end
 
     context 'for keyset pagination' do
       let(:params) { { pagination: 'keyset' } }

spec/lib/gitlab/pagination/keyset_spec.rb

@@ -3,6 +3,18 @@
 require 'spec_helper'
 
 describe Gitlab::Pagination::Keyset do
+  describe '.available_for_type?' do
+    subject { described_class }
+
+    it 'returns true for Project' do
+      expect(subject.available_for_type?(Project.all)).to be_truthy
+    end
+
+    it 'return false for other types of relations' do
+      expect(subject.available_for_type?(User.all)).to be_falsey
+    end
+  end
+
   describe '.available?' do
     subject { described_class }