Merge branch 'am-fix-pagination-relative-links' into 'master'

Properly implement API pagination headers and add specs

Closes #57684

See merge request gitlab-org/gitlab-ce!25267

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)

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,27 +152,38 @@ 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 })
+          describe 'second page' do
+            let(:query) { base_query.merge(ks_prev_id: projects[3].id, ks_prev_name: projects[3].name, per_page: 2) }
+
+            it 'returns the right records (second page)' do
               result = subject.paginate(resource)
 
               expect(result.first).to eq(projects[2])
               expect(result.second).to eq(projects[6])
             end
 
-          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 })
+            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
+
+          describe 'third page' do
+            let(:query) { base_query.merge(ks_prev_id: projects[6].id, ks_prev_name: projects[6].name, per_page: 5) }
+
+            it 'returns the right records (third page), note increased per_page' do
               result = subject.paginate(resource)
 
               expect(result.size).to eq(3)
@@ -185,18 +191,6 @@ describe API::Helpers::Pagination do
               expect(result.second).to eq(projects[4])
               expect(result.last).to eq(projects[5])
             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 })
-
-            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
-
-            subject.paginate(resource)
           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')

spec/lib/gitlab/serializer/pagination_spec.rb

 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

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

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