diff options
author | Adam Mulvany <amulvany@gitlab.com> | 2019-02-15 11:16:58 +1100 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2019-02-21 18:29:00 +0100 |
commit | 38bbc097fabfc90344443003df030d97aee63673 (patch) | |
tree | dc4f51378a51cdafc39bab3f39be60ab529800ff | |
parent | cf8c2eeba4fdf0f107aad312c09ab26eac04a29c (diff) | |
download | gitlab-ce-38bbc097fabfc90344443003df030d97aee63673.tar.gz |
Properly implement API pagination headers and add specs
Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r-- | lib/api/helpers/pagination.rb | 75 | ||||
-rw-r--r-- | spec/lib/api/helpers/pagination_spec.rb | 155 | ||||
-rw-r--r-- | spec/lib/gitlab/serializer/pagination_spec.rb | 10 | ||||
-rw-r--r-- | spec/serializers/environment_serializer_spec.rb | 9 | ||||
-rw-r--r-- | spec/serializers/pipeline_serializer_spec.rb | 14 |
5 files changed, 109 insertions, 154 deletions
diff --git a/lib/api/helpers/pagination.rb b/lib/api/helpers/pagination.rb index de59c915d66..d00e61678b5 100644 --- a/lib/api/helpers/pagination.rb +++ b/lib/api/helpers/pagination.rb @@ -13,6 +13,33 @@ module API strategy.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 = nil + 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 KeysetPaginationInfo attr_reader :relation, :request_context @@ -85,7 +112,7 @@ module API end end - class KeysetPaginationStrategy + class KeysetPaginationStrategy < Base attr_reader :request_context delegate :params, :header, :request, to: :request_context @@ -141,10 +168,6 @@ module API ] end - def per_page - params[:per_page] - end - def add_default_pagination_headers header 'X-Per-Page', per_page.to_s end @@ -154,22 +177,12 @@ module API header 'Link', link_for('next', next_page_params) end - def page_href(next_page_params) - request_url = request.url.split('?').first - request_params = params.dup - request_params[:per_page] = per_page - - request_params.merge!(next_page_params) if next_page_params - - "#{request_url}?#{request_params.to_query}" - end - def link_for(rel, next_page_params) %(<#{page_href(next_page_params)}>; rel="#{rel}") end end - class DefaultPaginationStrategy + class DefaultPaginationStrategy < Base attr_reader :request_context delegate :params, :header, :request, to: :request_context @@ -198,15 +211,13 @@ module API end end - # rubocop: disable CodeReuse/ActiveRecord def add_default_order(relation) if relation.is_a?(ActiveRecord::Relation) && relation.order_values.empty? - relation = relation.order(:id) + relation = relation.order(:id) # rubocop: disable CodeReuse/ActiveRecord end relation end - # rubocop: enable CodeReuse/ActiveRecord def add_pagination_headers(paginated_data) header 'X-Per-Page', paginated_data.limit_value.to_s @@ -222,27 +233,13 @@ module API end def pagination_links(paginated_data) - request_url = request.url.split('?').first - request_params = params.clone - request_params[:per_page] = paginated_data.limit_value - - links = [] - - request_params[:page] = paginated_data.prev_page - links << %(<#{request_url}?#{request_params.to_query}>; rel="prev") if request_params[:page] - - request_params[:page] = paginated_data.next_page - links << %(<#{request_url}?#{request_params.to_query}>; rel="next") if request_params[:page] - - request_params[:page] = 1 - links << %(<#{request_url}?#{request_params.to_query}>; rel="first") - - unless data_without_counts?(paginated_data) - request_params[:page] = total_pages(paginated_data) - links << %(<#{request_url}?#{request_params.to_query}>; rel="last") - end + [].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.join(', ') + links << %(<#{page_href(page: total_pages(paginated_data))}>; rel="last") unless data_without_counts?(paginated_data) + end.join(', ') end def total_pages(paginated_data) diff --git a/spec/lib/api/helpers/pagination_spec.rb b/spec/lib/api/helpers/pagination_spec.rb index 2890aa4ae38..6e215ea1561 100644 --- a/spec/lib/api/helpers/pagination_spec.rb +++ b/spec/lib/api/helpers/pagination_spec.rb @@ -2,6 +2,8 @@ require 'spec_helper' describe API::Helpers::Pagination do let(:resource) { Project.all } + let(:incoming_api_projects_url) { "#{Gitlab.config.gitlab.url}:8080/api/v4/projects" } + let(:canonical_api_projects_url) { "#{Gitlab.config.gitlab.url}/api/v4/projects" } subject do Class.new.include(described_class).new @@ -9,13 +11,19 @@ describe API::Helpers::Pagination do describe '#paginate (keyset pagination)' do let(:value) { spy('return value') } + let(:base_query) do + { + pagination: 'keyset', + foo: 'bar', + bar: 'baz' + } + end + let(:query) { base_query } before do - allow(value).to receive(:to_query).and_return(value) - allow(subject).to receive(:header).and_return(value) - allow(subject).to receive(:params).and_return(value) - allow(subject).to receive(:request).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}")) end context 'when resource can be paginated' do @@ -28,10 +36,7 @@ describe API::Helpers::Pagination do end describe 'first page' do - before do - allow(subject).to receive(:params) - .and_return({ pagination: 'keyset', per_page: 2 }) - end + let(:query) { base_query.merge(per_page: 2) } it 'returns appropriate amount of resources' do expect(subject.paginate(resource).count).to eq 2 @@ -43,7 +48,7 @@ describe API::Helpers::Pagination do it 'adds appropriate headers' do expect_header('X-Per-Page', '2') - expect_header('X-Next-Page', "#{value}?ks_prev_id=#{projects[1].id}&pagination=keyset&per_page=2") + expect_header('X-Next-Page', "#{canonical_api_projects_url}?#{query.merge(ks_prev_id: projects[1].id).to_query}") expect_header('Link', anything) do |_key, val| expect(val).to include('rel="next"') @@ -54,10 +59,7 @@ describe API::Helpers::Pagination do end describe 'second page' do - before do - allow(subject).to receive(:params) - .and_return({ pagination: 'keyset', per_page: 2, ks_prev_id: projects[1].id }) - end + let(:query) { base_query.merge(per_page: 2, ks_prev_id: projects[1].id) } it 'returns appropriate amount of resources' do expect(subject.paginate(resource).count).to eq 1 @@ -69,7 +71,7 @@ describe API::Helpers::Pagination do it 'adds appropriate headers' do expect_header('X-Per-Page', '2') - expect_header('X-Next-Page', "#{value}?ks_prev_id=#{projects[2].id}&pagination=keyset&per_page=2") + expect_header('X-Next-Page', "#{canonical_api_projects_url}?#{query.merge(ks_prev_id: projects[2].id).to_query}") expect_header('Link', anything) do |_key, val| expect(val).to include('rel="next"') @@ -80,10 +82,7 @@ describe API::Helpers::Pagination do end describe 'third page' do - before do - allow(subject).to receive(:params) - .and_return({ pagination: 'keyset', per_page: 2, ks_prev_id: projects[2].id }) - end + let(:query) { base_query.merge(per_page: 2, ks_prev_id: projects[2].id) } it 'returns appropriate amount of resources' do expect(subject.paginate(resource).count).to eq 0 @@ -91,6 +90,7 @@ describe API::Helpers::Pagination do it 'adds appropriate headers' do expect_header('X-Per-Page', '2') + expect_no_header('X-Next-Page') expect(subject).not_to receive(:header).with('Link') subject.paginate(resource) @@ -99,10 +99,7 @@ describe API::Helpers::Pagination do context 'if order' do context 'is not present' do - before do - allow(subject).to receive(:params) - .and_return({ pagination: 'keyset', per_page: 2 }) - end + let(:query) { base_query.merge(per_page: 2) } it 'is not present it adds default order(:id) desc' do resource.order_values = [] @@ -144,9 +141,7 @@ describe API::Helpers::Pagination do # (key is the id) end - it 'it also orders by primary key' do - allow(subject).to receive(:params) - .and_return({ pagination: 'keyset', per_page: 2 }) + it 'also orders by primary key' do paginated_relation = subject.paginate(resource) expect(paginated_relation.order_values).to be_present @@ -157,46 +152,45 @@ describe API::Helpers::Pagination do expect(paginated_relation.order_values.second.expr.name).to eq :id end - it 'it returns the right records (first page)' do - allow(subject).to receive(:params) - .and_return({ pagination: 'keyset', per_page: 2 }) + it 'returns the right records (first page)' do result = subject.paginate(resource) expect(result.first).to eq(projects[1]) expect(result.second).to eq(projects[3]) end - it 'it returns the right records (second page)' do - allow(subject).to receive(:params) - .and_return({ pagination: 'keyset', ks_prev_id: projects[3].id, ks_prev_name: projects[3].name, per_page: 2 }) - result = subject.paginate(resource) + describe 'second page' do + let(:query) { base_query.merge(ks_prev_id: projects[3].id, ks_prev_name: projects[3].name, per_page: 2) } - expect(result.first).to eq(projects[2]) - expect(result.second).to eq(projects[6]) - end + it 'returns the right records (second page)' do + result = subject.paginate(resource) - it 'it returns the right records (third page), note increased per_page' do - allow(subject).to receive(:params) - .and_return({ pagination: 'keyset', ks_prev_id: projects[6].id, ks_prev_name: projects[6].name, per_page: 5 }) - result = subject.paginate(resource) + expect(result.first).to eq(projects[2]) + expect(result.second).to eq(projects[6]) + end - expect(result.size).to eq(3) - expect(result.first).to eq(projects[0]) - expect(result.second).to eq(projects[4]) - expect(result.last).to eq(projects[5]) + it 'returns the right link to the next page' do + expect_header('X-Per-Page', '2') + expect_header('X-Next-Page', "#{canonical_api_projects_url}?#{query.merge(ks_prev_id: projects[6].id, ks_prev_name: projects[6].name).to_query}") + expect_header('Link', anything) do |_key, val| + expect(val).to include('rel="next"') + end + + subject.paginate(resource) + end end - it 'it returns the right link to the next page' do - allow(subject).to receive(:params) - .and_return({ pagination: 'keyset', ks_prev_id: projects[3].id, ks_prev_name: projects[3].name, per_page: 2 }) + describe 'third page' do + let(:query) { base_query.merge(ks_prev_id: projects[6].id, ks_prev_name: projects[6].name, per_page: 5) } - expect_header('X-Per-Page', '2') - expect_header('X-Next-Page', "#{value}?ks_prev_id=#{projects[6].id}&ks_prev_name=#{projects[6].name}&pagination=keyset&per_page=2") - expect_header('Link', anything) do |_key, val| - expect(val).to include('rel="next"') - end + it 'returns the right records (third page), note increased per_page' do + result = subject.paginate(resource) - subject.paginate(resource) + expect(result.size).to eq(3) + expect(result.first).to eq(projects[0]) + expect(result.second).to eq(projects[4]) + expect(result.last).to eq(projects[5]) + end end end end @@ -205,25 +199,13 @@ describe API::Helpers::Pagination do describe '#paginate (default offset-based pagination)' do let(:value) { spy('return value') } + let(:base_query) { { foo: 'bar', bar: 'baz' } } + let(:query) { base_query } before do - allow(value).to receive(:to_query).and_return(value) - allow(subject).to receive(:header).and_return(value) - allow(subject).to receive(:params).and_return(value) - allow(subject).to receive(:request).and_return(value) - end - - describe 'required instance methods' do - let(:return_spy) { spy } - - it 'requires some instance methods' do - expect_message(:header) - expect_message(:params) - expect_message(:request) - - subject.paginate(resource) - end + allow(subject).to receive(:params).and_return(query) + allow(subject).to receive(:request).and_return(double(url: "#{incoming_api_projects_url}?#{query.to_query}")) end context 'when resource can be paginated' do @@ -232,11 +214,6 @@ describe API::Helpers::Pagination do end describe 'first page' do - before do - allow(subject).to receive(:params) - .and_return({ page: 1, per_page: 2 }) - end - shared_examples 'response with pagination headers' do it 'adds appropriate headers' do expect_header('X-Total', '3') @@ -247,9 +224,9 @@ describe API::Helpers::Pagination do expect_header('X-Prev-Page', '') expect_header('Link', anything) do |_key, val| - expect(val).to include('rel="first"') - expect(val).to include('rel="last"') - expect(val).to include('rel="next"') + expect(val).to include(%Q(<#{canonical_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) + expect(val).to include(%Q(<#{canonical_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="last")) + expect(val).to include(%Q(<#{canonical_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="next")) expect(val).not_to include('rel="prev"') end @@ -267,6 +244,8 @@ describe API::Helpers::Pagination do 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' @@ -311,9 +290,9 @@ describe API::Helpers::Pagination do expect_header('X-Prev-Page', '') expect_header('Link', anything) do |_key, val| - expect(val).to include('rel="first"') + expect(val).to include(%Q(<#{canonical_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) + expect(val).to include(%Q(<#{canonical_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="next")) expect(val).not_to include('rel="last"') - expect(val).to include('rel="next"') expect(val).not_to include('rel="prev"') end @@ -324,10 +303,7 @@ describe API::Helpers::Pagination do end describe 'second page' do - before do - allow(subject).to receive(:params) - .and_return({ page: 2, per_page: 2 }) - end + 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 @@ -342,9 +318,9 @@ describe API::Helpers::Pagination do expect_header('X-Prev-Page', '1') expect_header('Link', anything) do |_key, val| - expect(val).to include('rel="first"') - expect(val).to include('rel="last"') - expect(val).to include('rel="prev"') + expect(val).to include(%Q(<#{canonical_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) + expect(val).to include(%Q(<#{canonical_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="last")) + expect(val).to include(%Q(<#{canonical_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="prev")) expect(val).not_to include('rel="next"') end @@ -376,10 +352,7 @@ describe API::Helpers::Pagination do context 'when resource empty' do describe 'first page' do - before do - allow(subject).to receive(:params) - .and_return({ page: 1, per_page: 2 }) - end + 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 @@ -394,8 +367,8 @@ describe API::Helpers::Pagination do expect_header('X-Prev-Page', '') expect_header('Link', anything) do |_key, val| - expect(val).to include('rel="first"') - expect(val).to include('rel="last"') + expect(val).to include(%Q(<#{canonical_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) + expect(val).to include(%Q(<#{canonical_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') diff --git a/spec/lib/gitlab/serializer/pagination_spec.rb b/spec/lib/gitlab/serializer/pagination_spec.rb index 1bc6536439e..c54be78f050 100644 --- a/spec/lib/gitlab/serializer/pagination_spec.rb +++ b/spec/lib/gitlab/serializer/pagination_spec.rb @@ -1,16 +1,12 @@ require 'spec_helper' describe Gitlab::Serializer::Pagination do - let(:request) { spy('request') } + let(:request) { double(url: "#{Gitlab.config.gitlab.url}:8080/api/v4/projects?#{query.to_query}", query_parameters: query) } let(:response) { spy('response') } let(:headers) { spy('headers') } before do - allow(request).to receive(:query_parameters) - .and_return(params) - - allow(response).to receive(:headers) - .and_return(headers) + allow(response).to receive(:headers).and_return(headers) end let(:pagination) { described_class.new(request, response) } @@ -19,7 +15,7 @@ describe Gitlab::Serializer::Pagination do subject { pagination.paginate(resource) } let(:resource) { User.all } - let(:params) { { page: 1, per_page: 2 } } + let(:query) { { page: 1, per_page: 2 } } context 'when a multiple resources are present in relation' do before do diff --git a/spec/serializers/environment_serializer_spec.rb b/spec/serializers/environment_serializer_spec.rb index 3541bd5f12e..375a28a8c72 100644 --- a/spec/serializers/environment_serializer_spec.rb +++ b/spec/serializers/environment_serializer_spec.rb @@ -124,10 +124,10 @@ describe EnvironmentSerializer do end context 'when used with pagination' do - let(:request) { spy('request') } + let(:request) { double(url: "#{Gitlab.config.gitlab.url}:8080/api/v4/projects?#{query.to_query}", query_parameters: query) } let(:response) { spy('response') } let(:resource) { Environment.all } - let(:pagination) { { page: 1, per_page: 2 } } + let(:query) { { page: 1, per_page: 2 } } let(:serializer) do described_class @@ -135,11 +135,6 @@ describe EnvironmentSerializer do .with_pagination(request, response) end - before do - allow(request).to receive(:query_parameters) - .and_return(pagination) - end - subject { serializer.represent(resource) } it 'creates a paginated serializer' do diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 79aa32b29bb..2bdcb2a45f6 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -38,15 +38,9 @@ describe PipelineSerializer do end context 'when used with pagination' do - let(:request) { spy('request') } + let(:request) { double(url: "#{Gitlab.config.gitlab.url}:8080/api/v4/projects?#{query.to_query}", query_parameters: query) } let(:response) { spy('response') } - let(:pagination) { {} } - - before do - allow(request) - .to receive(:query_parameters) - .and_return(pagination) - end + let(:query) { {} } let(:serializer) do described_class.new(current_user: user) @@ -60,7 +54,7 @@ describe PipelineSerializer do context 'when resource is not paginatable' do context 'when a single pipeline object is being serialized' do let(:resource) { create(:ci_empty_pipeline) } - let(:pagination) { { page: 1, per_page: 1 } } + let(:query) { { page: 1, per_page: 1 } } it 'raises error' do expect { subject }.to raise_error( @@ -71,7 +65,7 @@ describe PipelineSerializer do context 'when resource is paginatable relation' do let(:resource) { Ci::Pipeline.all } - let(:pagination) { { page: 1, per_page: 2 } } + let(:query) { { page: 1, per_page: 2 } } context 'when a single pipeline object is present in relation' do before do |