Skip to content

G
gitlab-ce
Commit 957d97d0 authored by Andreas Brandl's avatar Andreas Brandl
Browse files
Options

Refactor pagination helper, extract classes 

This refactors the pagination helper and extracts the logic into
Gitlab::Pagination. The goal here is to change
Gitlab::Serializer::Pagination (which is in app/) to *not* use
Api::Helpers::Pagination anymore. Instead, it calls the extracted
classes directly.

This is a preparation to add more pagination strategies to the API only
(like keyset pagination).
parent ff60273a
Hide whitespace changes
Inline Side-by-side
Showing with 337 additions and 304 deletions
lib/api/helpers/pagination.rb
View file @ 957d97d0
......@@ -4,106 +4,7 @@ module API
module Helpers
module Pagination
def paginate(relation)
DefaultPaginationStrategy.new(self).paginate(relation)
end
class Base
private
def per_page
@per_page ||= params[:per_page]
end
def base_request_uri
@base_request_uri ||= URI.parse(request.url).tap do |uri|
uri.host = Gitlab.config.gitlab.host
uri.port = Gitlab.config.gitlab.port
end
end
def build_page_url(query_params:)
base_request_uri.tap do |uri|
uri.query = query_params
end.to_s
end
def page_href(next_page_params = {})
query_params = params.merge(**next_page_params, per_page: per_page).to_query
build_page_url(query_params: query_params)
end
end
class DefaultPaginationStrategy < Base
attr_reader :request_context
delegate :params, :header, :request, to: :request_context
def initialize(request_context)
@request_context = request_context
end
def paginate(relation)
paginate_with_limit_optimization(add_default_order(relation)).tap do |data|
add_pagination_headers(data)
end
end
private
def paginate_with_limit_optimization(relation)
pagination_data = relation.page(params[:page]).per(params[:per_page])
return pagination_data unless pagination_data.is_a?(ActiveRecord::Relation)
return pagination_data unless Feature.enabled?(:api_kaminari_count_with_limit)
limited_total_count = pagination_data.total_count_with_limit
if limited_total_count > Kaminari::ActiveRecordRelationMethods::MAX_COUNT_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
else
pagination_data
end
end
def add_default_order(relation)
if relation.is_a?(ActiveRecord::Relation) && relation.order_values.empty?
relation = relation.order(:id) # rubocop: disable CodeReuse/ActiveRecord
end
relation
end
def add_pagination_headers(paginated_data)
header 'X-Per-Page', paginated_data.limit_value.to_s
header 'X-Page', paginated_data.current_page.to_s
header 'X-Next-Page', paginated_data.next_page.to_s
header 'X-Prev-Page', paginated_data.prev_page.to_s
header 'Link', pagination_links(paginated_data)
return if data_without_counts?(paginated_data)
header 'X-Total', paginated_data.total_count.to_s
header 'X-Total-Pages', total_pages(paginated_data).to_s
end
def pagination_links(paginated_data)
[].tap do |links|
links << %(<#{page_href(page: paginated_data.prev_page)}>; rel="prev") if paginated_data.prev_page
links << %(<#{page_href(page: paginated_data.next_page)}>; rel="next") if paginated_data.next_page
links << %(<#{page_href(page: 1)}>; rel="first")
links << %(<#{page_href(page: total_pages(paginated_data))}>; rel="last") unless data_without_counts?(paginated_data)
end.join(', ')
end
def total_pages(paginated_data)
# Ensure there is in total at least 1 page
[paginated_data.total_pages, 1].max
end
def data_without_counts?(paginated_data)
paginated_data.is_a?(Kaminari::PaginatableWithoutCount)
end
::Gitlab::Pagination::OffsetPagination.new(self).paginate(relation)
end
end
end
......
lib/gitlab/pagination/base.rb 0 → 100644
View file @ 957d97d0
# frozen_string_literal: true
module Gitlab
module Pagination
class Base
private
def per_page
@per_page ||= params[:per_page]
end
def base_request_uri
@base_request_uri ||= URI.parse(request.url).tap do |uri|
uri.host = Gitlab.config.gitlab.host
uri.port = Gitlab.config.gitlab.port
end
end
def build_page_url(query_params:)
base_request_uri.tap do |uri|
uri.query = query_params
end.to_s
end
def page_href(next_page_params = {})
query_params = params.merge(**next_page_params, per_page: per_page).to_query
build_page_url(query_params: query_params)
end
end
end
end
lib/gitlab/pagination/offset_pagination.rb 0 → 100644
View file @ 957d97d0
# frozen_string_literal: true
module Gitlab
module Pagination
class OffsetPagination < Base
attr_reader :request_context
delegate :params, :header, :request, to: :request_context
def initialize(request_context)
@request_context = request_context
end
def paginate(relation)
paginate_with_limit_optimization(add_default_order(relation)).tap do |data|
add_pagination_headers(data)
end
end
private
def paginate_with_limit_optimization(relation)
pagination_data = relation.page(params[:page]).per(params[:per_page])
return pagination_data unless pagination_data.is_a?(ActiveRecord::Relation)
return pagination_data unless Feature.enabled?(:api_kaminari_count_with_limit)
limited_total_count = pagination_data.total_count_with_limit
if limited_total_count > Kaminari::ActiveRecordRelationMethods::MAX_COUNT_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
else
pagination_data
end
end
def add_default_order(relation)
if relation.is_a?(ActiveRecord::Relation) && relation.order_values.empty?
relation = relation.order(:id) # rubocop: disable CodeReuse/ActiveRecord
end
relation
end
def add_pagination_headers(paginated_data)
header 'X-Per-Page', paginated_data.limit_value.to_s
header 'X-Page', paginated_data.current_page.to_s
header 'X-Next-Page', paginated_data.next_page.to_s
header 'X-Prev-Page', paginated_data.prev_page.to_s
header 'Link', pagination_links(paginated_data)
return if data_without_counts?(paginated_data)
header 'X-Total', paginated_data.total_count.to_s
header 'X-Total-Pages', total_pages(paginated_data).to_s
end
def pagination_links(paginated_data)
[].tap do |links|
links << %(<#{page_href(page: paginated_data.prev_page)}>; rel="prev") if paginated_data.prev_page
links << %(<#{page_href(page: paginated_data.next_page)}>; rel="next") if paginated_data.next_page
links << %(<#{page_href(page: 1)}>; rel="first")
links << %(<#{page_href(page: total_pages(paginated_data))}>; rel="last") unless data_without_counts?(paginated_data)
end.join(', ')
end
def total_pages(paginated_data)
# Ensure there is in total at least 1 page
[paginated_data.total_pages, 1].max
end
def data_without_counts?(paginated_data)
paginated_data.is_a?(Kaminari::PaginatableWithoutCount)
end
end
end
end
lib/gitlab/serializer/pagination.rb
View file @ 957d97d0
......@@ -4,7 +4,6 @@ module Gitlab
module Serializer
class Pagination
InvalidResourceError = Class.new(StandardError)
include ::API::Helpers::Pagination
def initialize(request, response)
@request = request
......@@ -13,13 +12,13 @@ module Gitlab
def paginate(resource)
if resource.respond_to?(:page)
super(resource)
::Gitlab::Pagination::OffsetPagination.new(self).paginate(resource)
else
raise InvalidResourceError
end
end
# Methods needed by `API::Helpers::Pagination`
# Methods needed by `Gitlab::Pagination::OffsetPagination`
#
attr_reader :request
......
spec/lib/api/helpers/pagination_spec.rb
View file @ 957d97d0
......@@ -3,211 +3,20 @@
require 'spec_helper'
describe API::Helpers::Pagination do
let(:resource) { Project.all }
let(:custom_port) { 8080 }
let(:incoming_api_projects_url) { "#{Gitlab.config.gitlab.url}:#{custom_port}/api/v4/projects" }
subject { Class.new.include(described_class).new }
before do
stub_config_setting(port: custom_port)
end
describe '#paginate' do
let(:relation) { double("relation") }
let(:offset_pagination) { double("offset pagination") }
let(:expected_result) { double("result") }
subject do
Class.new.include(described_class).new
end
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)
describe '#paginate (default offset-based pagination)' do
let(:value) { spy('return value') }
let(:base_query) { { foo: 'bar', bar: 'baz' } }
let(:query) { base_query }
result = subject.paginate(relation)
before do
allow(subject).to receive(:header).and_return(value)
allow(subject).to receive(:params).and_return(query)
allow(subject).to receive(:request).and_return(double(url: "#{incoming_api_projects_url}?#{query.to_query}"))
expect(result).to eq(expected_result)
end
context 'when resource can be paginated' do
before do
create_list(:project, 3)
end
describe 'first page' do
shared_examples 'response with pagination headers' do
it 'adds appropriate headers' do
expect_header('X-Total', '3')
expect_header('X-Total-Pages', '2')
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="last"))
expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="next"))
expect(val).not_to include('rel="prev"')
end
subject.paginate(resource)
end
end
shared_examples 'paginated response' do
it 'returns appropriate amount of resources' do
expect(subject.paginate(resource).count).to eq 2
end
it 'executes only one SELECT COUNT query' do
expect { subject.paginate(resource) }.to make_queries_matching(/SELECT COUNT/, 1)
end
end
let(:query) { base_query.merge(page: 1, per_page: 2) }
context 'when the api_kaminari_count_with_limit feature flag is unset' do
it_behaves_like 'paginated response'
it_behaves_like 'response with pagination headers'
end
context 'when the api_kaminari_count_with_limit feature flag is disabled' do
before do
stub_feature_flags(api_kaminari_count_with_limit: false)
end
it_behaves_like 'paginated response'
it_behaves_like 'response with pagination headers'
end
context 'when the api_kaminari_count_with_limit feature flag is enabled' do
before do
stub_feature_flags(api_kaminari_count_with_limit: true)
end
context 'when resources count is less than MAX_COUNT_LIMIT' do
before do
stub_const("::Kaminari::ActiveRecordRelationMethods::MAX_COUNT_LIMIT", 4)
end
it_behaves_like 'paginated response'
it_behaves_like 'response with pagination headers'
end
context 'when resources count is more than MAX_COUNT_LIMIT' do
before do
stub_const("::Kaminari::ActiveRecordRelationMethods::MAX_COUNT_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
end
describe 'second page' do
let(:query) { base_query.merge(page: 2, per_page: 2) }
it 'returns appropriate amount of resources' do
expect(subject.paginate(resource).count).to eq 1
end
it 'adds appropriate headers' do
expect_header('X-Total', '3')
expect_header('X-Total-Pages', '2')
expect_header('X-Per-Page', '2')
expect_header('X-Page', '2')
expect_header('X-Next-Page', '')
expect_header('X-Prev-Page', '1')
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="last"))
expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="prev"))
expect(val).not_to include('rel="next"')
end
subject.paginate(resource)
end
end
context 'if order' do
it 'is not present it adds default order(:id) if no order is present' do
resource.order_values = []
paginated_relation = subject.paginate(resource)
expect(resource.order_values).to be_empty
expect(paginated_relation.order_values).to be_present
expect(paginated_relation.order_values.first).to be_ascending
expect(paginated_relation.order_values.first.expr.name).to eq 'id'
end
it 'is present it does not add anything' do
paginated_relation = subject.paginate(resource.order(created_at: :desc))
expect(paginated_relation.order_values).to be_present
expect(paginated_relation.order_values.first).to be_descending
expect(paginated_relation.order_values.first.expr.name).to eq 'created_at'
end
end
end
context 'when resource empty' do
describe 'first page' do
let(:query) { base_query.merge(page: 1, per_page: 2) }
it 'returns appropriate amount of resources' do
expect(subject.paginate(resource).count).to eq 0
end
it 'adds appropriate headers' do
expect_header('X-Total', '0')
expect_header('X-Total-Pages', '1')
expect_header('X-Per-Page', '2')
expect_header('X-Page', '1')
expect_header('X-Next-Page', '')
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: 1).to_query}>; rel="last"))
expect(val).not_to include('rel="prev"')
expect(val).not_to include('rel="next"')
expect(val).not_to include('page=0')
end
subject.paginate(resource)
end
end
end
end
def expect_header(*args, &block)
expect(subject).to receive(:header).with(*args, &block)
end
def expect_no_header(*args, &block)
expect(subject).not_to receive(:header).with(*args)
end
def expect_message(method)
expect(subject).to receive(method)
.at_least(:once).and_return(value)
end
end
spec/lib/gitlab/pagination/offset_pagination_spec.rb 0 → 100644
View file @ 957d97d0
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Pagination::OffsetPagination do
let(:resource) { Project.all }
let(:custom_port) { 8080 }
let(:incoming_api_projects_url) { "#{Gitlab.config.gitlab.url}:#{custom_port}/api/v4/projects" }
before do
stub_config_setting(port: custom_port)
end
let(:request_context) { double("request_context") }
subject do
described_class.new(request_context)
end
describe '#paginate' do
let(:value) { spy('return value') }
let(:base_query) { { foo: 'bar', bar: 'baz' } }
let(:query) { base_query }
before do
allow(request_context).to receive(:header).and_return(value)
allow(request_context).to receive(:params).and_return(query)
allow(request_context).to receive(:request).and_return(double(url: "#{incoming_api_projects_url}?#{query.to_query}"))
end
context 'when resource can be paginated' do
before do
create_list(:project, 3)
end
describe 'first page' do
shared_examples 'response with pagination headers' do
it 'adds appropriate headers' do
expect_header('X-Total', '3')
expect_header('X-Total-Pages', '2')
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="last"))
expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="next"))
expect(val).not_to include('rel="prev"')
end
subject.paginate(resource)
end
end
shared_examples 'paginated response' do
it 'returns appropriate amount of resources' do
expect(subject.paginate(resource).count).to eq 2
end
it 'executes only one SELECT COUNT query' do
expect { subject.paginate(resource) }.to make_queries_matching(/SELECT COUNT/, 1)
end
end
let(:query) { base_query.merge(page: 1, per_page: 2) }
context 'when the api_kaminari_count_with_limit feature flag is unset' do
it_behaves_like 'paginated response'
it_behaves_like 'response with pagination headers'
end
context 'when the api_kaminari_count_with_limit feature flag is disabled' do
before do
stub_feature_flags(api_kaminari_count_with_limit: false)
end
it_behaves_like 'paginated response'
it_behaves_like 'response with pagination headers'
end
context 'when the api_kaminari_count_with_limit feature flag is enabled' do
before do
stub_feature_flags(api_kaminari_count_with_limit: true)
end
context 'when resources count is less than MAX_COUNT_LIMIT' do
before do
stub_const("::Kaminari::ActiveRecordRelationMethods::MAX_COUNT_LIMIT", 4)
end
it_behaves_like 'paginated response'
it_behaves_like 'response with pagination headers'
end
context 'when resources count is more than MAX_COUNT_LIMIT' do
before do
stub_const("::Kaminari::ActiveRecordRelationMethods::MAX_COUNT_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
end
describe 'second page' do
let(:query) { base_query.merge(page: 2, per_page: 2) }
it 'returns appropriate amount of resources' do
expect(subject.paginate(resource).count).to eq 1
end
it 'adds appropriate headers' do
expect_header('X-Total', '3')
expect_header('X-Total-Pages', '2')
expect_header('X-Per-Page', '2')
expect_header('X-Page', '2')
expect_header('X-Next-Page', '')
expect_header('X-Prev-Page', '1')
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="last"))
expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="prev"))
expect(val).not_to include('rel="next"')
end
subject.paginate(resource)
end
end
context 'if order' do
it 'is not present it adds default order(:id) if no order is present' do
resource.order_values = []
paginated_relation = subject.paginate(resource)
expect(resource.order_values).to be_empty
expect(paginated_relation.order_values).to be_present
expect(paginated_relation.order_values.first).to be_ascending
expect(paginated_relation.order_values.first.expr.name).to eq 'id'
end
it 'is present it does not add anything' do
paginated_relation = subject.paginate(resource.order(created_at: :desc))
expect(paginated_relation.order_values).to be_present
expect(paginated_relation.order_values.first).to be_descending
expect(paginated_relation.order_values.first.expr.name).to eq 'created_at'
end
end
end
context 'when resource empty' do
describe 'first page' do
let(:query) { base_query.merge(page: 1, per_page: 2) }
it 'returns appropriate amount of resources' do
expect(subject.paginate(resource).count).to eq 0
end
it 'adds appropriate headers' do
expect_header('X-Total', '0')
expect_header('X-Total-Pages', '1')
expect_header('X-Per-Page', '2')
expect_header('X-Page', '1')
expect_header('X-Next-Page', '')
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: 1).to_query}>; rel="last"))
expect(val).not_to include('rel="prev"')
expect(val).not_to include('rel="next"')
expect(val).not_to include('page=0')
end
subject.paginate(resource)
end
end
end
end
def expect_header(*args, &block)
expect(subject).to receive(:header).with(*args, &block)
end
def expect_no_header(*args, &block)
expect(subject).not_to receive(:header).with(*args)
end
def expect_message(method)
expect(subject).to receive(method)
.at_least(:once).and_return(value)
end
end
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7