Commit dbb70c6c authored by pbair's avatar pbair

Add plan limit for offset pagination

Use PlanLimits to control the maximum value allowed to be retreived
through offset pagination for API endpoints that support keyset
pagination.
parent d717a07a
...@@ -349,7 +349,7 @@ class Namespace < ApplicationRecord ...@@ -349,7 +349,7 @@ class Namespace < ApplicationRecord
# We default to PlanLimits.new otherwise a lot of specs would fail # We default to PlanLimits.new otherwise a lot of specs would fail
# On production each plan should already have associated limits record # On production each plan should already have associated limits record
# https://gitlab.com/gitlab-org/gitlab/issues/36037 # https://gitlab.com/gitlab-org/gitlab/issues/36037
actual_plan.limits || PlanLimits.new actual_plan.actual_limits
end end
def actual_plan_name def actual_plan_name
......
...@@ -26,6 +26,10 @@ class Plan < ApplicationRecord ...@@ -26,6 +26,10 @@ class Plan < ApplicationRecord
DEFAULT_PLANS DEFAULT_PLANS
end end
def actual_limits
self.limits || PlanLimits.new
end
def default? def default?
self.class.default_plans.include?(name) self.class.default_plans.include?(name)
end end
......
# 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
...@@ -4793,7 +4793,8 @@ CREATE TABLE public.plan_limits ( ...@@ -4793,7 +4793,8 @@ CREATE TABLE public.plan_limits (
project_hooks integer DEFAULT 100 NOT NULL, project_hooks integer DEFAULT 100 NOT NULL,
group_hooks integer DEFAULT 50 NOT NULL, group_hooks integer DEFAULT 50 NOT NULL,
ci_project_subscriptions integer DEFAULT 2 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 CREATE SEQUENCE public.plan_limits_id_seq
...@@ -13603,6 +13604,7 @@ COPY "schema_migrations" (version) FROM STDIN; ...@@ -13603,6 +13604,7 @@ COPY "schema_migrations" (version) FROM STDIN;
20200415161021 20200415161021
20200415161206 20200415161206
20200415192656 20200415192656
20200415203024
20200416005331 20200416005331
20200416111111 20200416111111
20200416120128 20200416120128
......
...@@ -3,19 +3,24 @@ ...@@ -3,19 +3,24 @@
module API module API
module Helpers module Helpers
module PaginationStrategies module PaginationStrategies
def paginate_with_strategies(relation) def paginate_with_strategies(relation, request_scope)
paginator = paginator(relation) paginator = paginator(relation, request_scope)
yield(paginator.paginate(relation)).tap do |records, _| yield(paginator.paginate(relation)).tap do |records, _|
paginator.finalize(records) paginator.finalize(records)
end end
end end
def paginator(relation) def paginator(relation, request_scope)
return Gitlab::Pagination::OffsetPagination.new(self) unless keyset_pagination_enabled? 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) unless Gitlab::Pagination::Keyset.available?(request_context, relation)
return error!('Keyset pagination is not yet available for this type of request', 405) return error!('Keyset pagination is not yet available for this type of request', 405)
end end
...@@ -23,11 +28,27 @@ module API ...@@ -23,11 +28,27 @@ module API
Gitlab::Pagination::Keyset::Pager.new(request_context) Gitlab::Pagination::Keyset::Pager.new(request_context)
end 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}, " \
"remaining records can be retrieved using keyset pagination.", 405)
end
Gitlab::Pagination::OffsetPagination.new(self)
end
def keyset_pagination_enabled? def keyset_pagination_enabled?
params[:pagination] == 'keyset' params[:pagination] == 'keyset'
end end
def limit_for_scope(scope)
(scope&.actual_limits || 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 end
end end
...@@ -95,7 +95,7 @@ module API ...@@ -95,7 +95,7 @@ module API
projects = reorder_projects(projects) projects = reorder_projects(projects)
projects = apply_filters(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) projects, options = with_custom_attributes(projects, options)
options = options.reverse_merge( options = options.reverse_merge(
...@@ -313,7 +313,7 @@ module API ...@@ -313,7 +313,7 @@ module API
get ':id/forks' do get ':id/forks' do
forks = ForkProjectsFinder.new(user_project, params: project_finder_params, current_user: current_user).execute 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 end
desc 'Check pages access of this project' desc 'Check pages access of this project'
......
...@@ -3,11 +3,14 @@ ...@@ -3,11 +3,14 @@
module Gitlab module Gitlab
module Pagination module Pagination
module Keyset module Keyset
def self.available_for_type?(relation)
relation.klass == Project
end
def self.available?(request_context, relation) def self.available?(request_context, relation)
order_by = request_context.page.order_by order_by = request_context.page.order_by
# This is only available for Project and order-by id (asc/desc) return false unless available_for_type?(relation)
return false unless relation.klass == Project
return false unless order_by.size == 1 && order_by[:id] return false unless order_by.size == 1 && order_by[:id]
true true
......
...@@ -17,18 +17,18 @@ describe API::Helpers::PaginationStrategies do ...@@ -17,18 +17,18 @@ describe API::Helpers::PaginationStrategies do
let(:paginator) { double("paginator", paginate: expected_result, finalize: nil) } let(:paginator) { double("paginator", paginate: expected_result, finalize: nil) }
before do before do
allow(subject).to receive(:paginator).with(relation).and_return(paginator) allow(subject).to receive(:paginator).with(relation, nil).and_return(paginator)
end end
it 'yields paginated relation' do 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 end
it 'calls #finalize with first value returned from block' do it 'calls #finalize with first value returned from block' do
return_value = double return_value = double
expect(paginator).to receive(:finalize).with(return_value) 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 = {} some_options = {}
[return_value, some_options] [return_value, some_options]
end end
...@@ -37,7 +37,7 @@ describe API::Helpers::PaginationStrategies do ...@@ -37,7 +37,7 @@ describe API::Helpers::PaginationStrategies do
it 'returns whatever the block returns' do it 'returns whatever the block returns' do
return_value = [double, double] return_value = [double, double]
result = subject.paginate_with_strategies(relation) do |records| result = subject.paginate_with_strategies(relation, nil) do |records|
return_value return_value
end end
...@@ -47,16 +47,77 @@ describe API::Helpers::PaginationStrategies do ...@@ -47,16 +47,77 @@ describe API::Helpers::PaginationStrategies do
describe '#paginator' do describe '#paginator' do
context 'offset pagination' do context 'offset pagination' do
let(:plan_limits) { Plan.default.actual_limits }
let(:offset_limit) { plan_limits.offset_pagination_limit }
let(:paginator) { double("paginator") } let(:paginator) { double("paginator") }
before do before do
allow(subject).to receive(:keyset_pagination_enabled?).and_return(false) allow(subject).to receive(:keyset_pagination_enabled?).and_return(false)
end end
it 'delegates to OffsetPagination' do context 'when keyset pagination is available for the relation' do
expect(Gitlab::Pagination::OffsetPagination).to receive(:new).with(subject).and_return(paginator) 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, nil)
end
end
expect(subject.paginator(relation)).to eq(paginator) 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, nil)).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, nil)).to eq(paginator)
end
end end
end end
...@@ -77,7 +138,7 @@ describe API::Helpers::PaginationStrategies do ...@@ -77,7 +138,7 @@ describe API::Helpers::PaginationStrategies do
end end
it 'delegates to Pager' do it 'delegates to Pager' do
expect(subject.paginator(relation)).to eq(pager) expect(subject.paginator(relation, nil)).to eq(pager)
end end
end end
...@@ -89,7 +150,7 @@ describe API::Helpers::PaginationStrategies do ...@@ -89,7 +150,7 @@ describe API::Helpers::PaginationStrategies do
it 'renders a 501 error' do it 'renders a 501 error' do
expect(subject).to receive(:error!).with(/not yet available/, 405) expect(subject).to receive(:error!).with(/not yet available/, 405)
subject.paginator(relation) subject.paginator(relation, nil)
end end
end end
end end
......
...@@ -3,6 +3,18 @@ ...@@ -3,6 +3,18 @@
require 'spec_helper' require 'spec_helper'
describe Gitlab::Pagination::Keyset do 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 describe '.available?' do
subject { described_class } subject { described_class }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment