Commit d0df47b8 authored by Sean Arnold's avatar Sean Arnold Committed by Markus Koller

Limit max pagination count for relations to 1000

This is behind a feature flag: lower_relation_max_count_limit

Changelog: performance
parent 83c1861b
---
name: lower_relation_max_count_limit
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69620
rollout_issue_url:
milestone: '14.3'
type: ops
group: group::verify
default_enabled: false
......@@ -4,6 +4,7 @@ module Kaminari
# Active Record specific page scope methods implementations
module ActiveRecordRelationMethodsWithLimit
MAX_COUNT_LIMIT = 10_000
MAX_COUNT_NEW_LOWER_LIMIT = 1_000
# This is a modified version of
# https://github.com/kaminari/kaminari/blob/c5186f5d9b7f23299d115408e62047447fd3189d/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb#L17-L41
......@@ -21,7 +22,8 @@ module Kaminari
return @total_count = (current_page - 1) * limit_value + @records.length if @records.any? && (@records.length < limit_value)
end
limit = options.fetch(:limit, MAX_COUNT_LIMIT).to_i
max_limit = Feature.enabled?(:lower_relation_max_count_limit, type: :ops) ? MAX_COUNT_NEW_LOWER_LIMIT : MAX_COUNT_LIMIT
limit = options.fetch(:limit, max_limit).to_i
# #count overrides the #select which could include generated columns referenced in #order, so skip #order here, where it's irrelevant to the result anyway
c = except(:offset, :limit, :order)
# Remove includes only if they are irrelevant
......
......@@ -29,7 +29,7 @@ module Gitlab
return pagination_data unless Feature.enabled?(:api_kaminari_count_with_limit, type: :ops)
limited_total_count = pagination_data.total_count_with_limit
if limited_total_count > Kaminari::ActiveRecordRelationMethods::MAX_COUNT_LIMIT
if limited_total_count > max_limit
# The call to `total_count_with_limit` memoizes `@arel` because of a call to `references_eager_loaded_tables?`
# We need to call `reset` because `without_count` relies on `@arel` being unmemoized
pagination_data.reset.without_count
......@@ -38,6 +38,14 @@ module Gitlab
end
end
def max_limit
if Feature.enabled?(:lower_relation_max_count_limit, type: :ops)
Kaminari::ActiveRecordRelationMethods::MAX_COUNT_NEW_LOWER_LIMIT
else
Kaminari::ActiveRecordRelationMethods::MAX_COUNT_LIMIT
end
end
def needs_pagination?(relation)
return true unless relation.respond_to?(:current_page)
return true if params[:page].present? && relation.current_page != params[:page].to_i
......
......@@ -82,7 +82,7 @@ RSpec.describe Gitlab::Pagination::OffsetPagination do
context 'when the api_kaminari_count_with_limit feature flag is enabled' do
before do
stub_feature_flags(api_kaminari_count_with_limit: true)
stub_feature_flags(api_kaminari_count_with_limit: true, lower_relation_max_count_limit: false)
end
context 'when resources count is less than MAX_COUNT_LIMIT' do
......@@ -120,6 +120,41 @@ RSpec.describe Gitlab::Pagination::OffsetPagination do
end
end
context 'when lower_relation_max_count_limit FF is enabled' do
before do
stub_feature_flags(lower_relation_max_count_limit: true)
end
it_behaves_like 'paginated response'
it_behaves_like 'response with pagination headers'
context 'when limit is met' do
before do
stub_const("::Kaminari::ActiveRecordRelationMethods::MAX_COUNT_NEW_LOWER_LIMIT", 2)
end
it_behaves_like 'paginated response'
it 'does not return the X-Total and X-Total-Pages headers' do
expect_no_header('X-Total')
expect_no_header('X-Total-Pages')
expect_header('X-Per-Page', '2')
expect_header('X-Page', '1')
expect_header('X-Next-Page', '2')
expect_header('X-Prev-Page', '')
expect_header('Link', anything) do |_key, val|
expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first"))
expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="next"))
expect(val).not_to include('rel="last"')
expect(val).not_to include('rel="prev"')
end
subject.paginate(resource)
end
end
end
it 'does not return the total headers when excluding them' do
expect_no_header('X-Total')
expect_no_header('X-Total-Pages')
......
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