Properly implement API pagination headers and add specs

Signed-off-by: Rémy Coutable <remy@rymai.me>

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/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