Exclude the total headers from the commits API

Excluding these headers allows us to avoid the extra Gitaly call
needed to count the commits for the specific call that is being made
to the API.

config/feature_flags/development/api_commits_without_count.yml

+---
+name: api_commits_without_count
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/43159
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/254994
+type: development
+group: team::Scalability
+default_enabled: false

lib/api/commits.rb

@@ -62,19 +62,29 @@ module API
                                                   first_parent: first_parent,
                                                   order: order)
 
-        commit_count =
-          if all || path || before || after || first_parent
-            user_project.repository.count_commits(ref: ref, path: path, before: before, after: after, all: all, first_parent: first_parent)
-          else
-            # Cacheable commit count.
-            user_project.repository.commit_count_for_ref(ref)
-          end
+        serializer = with_stats ? Entities::CommitWithStats : Entities::Commit
 
-        paginated_commits = Kaminari.paginate_array(commits, total_count: commit_count)
+        if Feature.enabled?(:api_commits_without_count, user_project)
+          # This tells kaminari that there is 1 more commit after the one we've
+          # loaded, meaning there will be a next page, if the currently loaded set
+          # of commits is equal to the requested page size.
+          commit_count = offset + commits.size + 1
+          paginated_commits = Kaminari.paginate_array(commits, total_count: commit_count)
 
-        serializer = with_stats ? Entities::CommitWithStats : Entities::Commit
+          present paginate(paginated_commits, exclude_total_headers: true), with: serializer
+        else
+          commit_count =
+            if all || path || before || after || first_parent
+              user_project.repository.count_commits(ref: ref, path: path, before: before, after: after, all: all, first_parent: first_parent)
+            else
+              # Cacheable commit count.
+              user_project.repository.commit_count_for_ref(ref)
+            end
 
-        present paginate(paginated_commits), with: serializer
+          paginated_commits = Kaminari.paginate_array(commits, total_count: commit_count)
+
+          present paginate(paginated_commits), with: serializer
+        end
       end
 
       desc 'Commit multiple file changes as one commit' do

lib/api/helpers/pagination.rb

@@ -3,8 +3,8 @@
 module API
   module Helpers
     module Pagination
-      def paginate(relation)
-        Gitlab::Pagination::OffsetPagination.new(self).paginate(relation)
+      def paginate(*args)
+        Gitlab::Pagination::OffsetPagination.new(self).paginate(*args)
       end
     end
   end

lib/gitlab/pagination/offset_pagination.rb

@@ -10,9 +10,9 @@ module Gitlab
         @request_context = request_context
       end
 
-      def paginate(relation)
+      def paginate(relation, exclude_total_headers: false)
         paginate_with_limit_optimization(add_default_order(relation)).tap do |data|
-          add_pagination_headers(data)
+          add_pagination_headers(data, exclude_total_headers)
         end
       end
 
@@ -47,14 +47,14 @@ module Gitlab
         relation
       end
 
-      def add_pagination_headers(paginated_data)
+      def add_pagination_headers(paginated_data, exclude_total_headers)
         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)
+        return if exclude_total_headers || data_without_counts?(paginated_data)
 
         header 'X-Total',       paginated_data.total_count.to_s
         header 'X-Total-Pages', total_pages(paginated_data).to_s

spec/lib/gitlab/pagination/offset_pagination_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe Gitlab::Pagination::OffsetPagination do
 
   let(:request_context) { double("request_context") }
 
-  subject do
+  subject(:paginator) do
     described_class.new(request_context)
   end
 
@@ -119,6 +119,34 @@ RSpec.describe Gitlab::Pagination::OffsetPagination do
               subject.paginate(resource)
             end
           end
+
+          it 'does not return the total headers when excluding them' do
+            expect_no_header('X-Total')
+            expect_no_header('X-Total-Pages')
+            expect_header('X-Per-Page', '2')
+            expect_header('X-Page', '1')
+
+            paginator.paginate(resource, exclude_total_headers: true)
+          end
+        end
+
+        context 'when resource is a paginatable array' do
+          let(:resource) { Kaminari.paginate_array(Project.all.to_a) }
+
+          it_behaves_like 'response with pagination headers'
+
+          it 'only returns the requested resources' do
+            expect(paginator.paginate(resource).count).to eq(2)
+          end
+
+          it 'does not return total headers when excluding them' do
+            expect_no_header('X-Total')
+            expect_no_header('X-Total-Pages')
+            expect_header('X-Per-Page', '2')
+            expect_header('X-Page', '1')
+
+            paginator.paginate(resource, exclude_total_headers: true)
+          end
         end
       end
 

spec/requests/api/commits_spec.rb

@@ -36,6 +36,13 @@ RSpec.describe API::Commits do
       end
 
       it 'include correct pagination headers' do
+        get api(route, current_user)
+
+        expect(response).to include_limited_pagination_headers
+      end
+
+      it 'includes the total headers when the count is not disabled' do
+        stub_feature_flags(api_commits_without_count: false)
         commit_count = project.repository.count_commits(ref: 'master').to_s
 
         get api(route, current_user)
@@ -79,12 +86,10 @@ RSpec.describe API::Commits do
         it 'include correct pagination headers' do
           commits = project.repository.commits("master", limit: 2)
           after = commits.second.created_at
-          commit_count = project.repository.count_commits(ref: 'master', after: after).to_s
 
           get api("/projects/#{project_id}/repository/commits?since=#{after.utc.iso8601}", user)
 
-          expect(response).to include_pagination_headers
-          expect(response.headers['X-Total']).to eq(commit_count)
+          expect(response).to include_limited_pagination_headers
           expect(response.headers['X-Page']).to eql('1')
         end
       end
@@ -109,12 +114,10 @@ RSpec.describe API::Commits do
         it 'include correct pagination headers' do
           commits = project.repository.commits("master", limit: 2)
           before = commits.second.created_at
-          commit_count = project.repository.count_commits(ref: 'master', before: before).to_s
 
           get api("/projects/#{project_id}/repository/commits?until=#{before.utc.iso8601}", user)
 
-          expect(response).to include_pagination_headers
-          expect(response.headers['X-Total']).to eq(commit_count)
+          expect(response).to include_limited_pagination_headers
           expect(response.headers['X-Page']).to eql('1')
         end
       end
@@ -137,49 +140,49 @@ RSpec.describe API::Commits do
       context "path optional parameter" do
         it "returns project commits matching provided path parameter" do
           path = 'files/ruby/popen.rb'
-          commit_count = project.repository.count_commits(ref: 'master', path: path).to_s
 
           get api("/projects/#{project_id}/repository/commits?path=#{path}", user)
 
           expect(json_response.size).to eq(3)
           expect(json_response.first["id"]).to eq("570e7b2abdd848b95f2f578043fc23bd6f6fd24d")
-          expect(response).to include_pagination_headers
-          expect(response.headers['X-Total']).to eq(commit_count)
+          expect(response).to include_limited_pagination_headers
         end
 
         it 'include correct pagination headers' do
           path = 'files/ruby/popen.rb'
-          commit_count = project.repository.count_commits(ref: 'master', path: path).to_s
 
           get api("/projects/#{project_id}/repository/commits?path=#{path}", user)
 
-          expect(response).to include_pagination_headers
-          expect(response.headers['X-Total']).to eq(commit_count)
+          expect(response).to include_limited_pagination_headers
           expect(response.headers['X-Page']).to eql('1')
         end
       end
 
       context 'all optional parameter' do
         it 'returns all project commits' do
-          commit_count = project.repository.count_commits(all: true)
+          expected_commit_ids = project.repository.commits(nil, all: true, limit: 50).map(&:id)
+
+          get api("/projects/#{project_id}/repository/commits?all=true&per_page=50", user)
 
-          get api("/projects/#{project_id}/repository/commits?all=true", user)
+          commit_ids = json_response.map { |c| c['id'] }
 
-          expect(response).to include_pagination_headers
-          expect(response.headers['X-Total']).to eq(commit_count.to_s)
+          expect(response).to include_limited_pagination_headers
+          expect(commit_ids).to eq(expected_commit_ids)
           expect(response.headers['X-Page']).to eql('1')
         end
       end
 
       context 'first_parent optional parameter' do
         it 'returns all first_parent commits' do
-          commit_count = project.repository.count_commits(ref: SeedRepo::Commit::ID, first_parent: true)
+          expected_commit_ids = project.repository.commits(SeedRepo::Commit::ID, limit: 50, first_parent: true).map(&:id)
 
-          get api("/projects/#{project_id}/repository/commits", user), params: { ref_name: SeedRepo::Commit::ID, first_parent: 'true' }
+          get api("/projects/#{project_id}/repository/commits?per_page=50", user), params: { ref_name: SeedRepo::Commit::ID, first_parent: 'true' }
 
-          expect(response).to include_pagination_headers
-          expect(commit_count).to eq(12)
-          expect(response.headers['X-Total']).to eq(commit_count.to_s)
+          commit_ids = json_response.map { |c| c['id'] }
+
+          expect(response).to include_limited_pagination_headers
+          expect(expected_commit_ids.size).to eq(12)
+          expect(commit_ids).to eq(expected_commit_ids)
         end
       end
 
@@ -209,11 +212,7 @@ RSpec.describe API::Commits do
         end
 
         it 'returns correct headers' do
-          commit_count = project.repository.count_commits(ref: ref_name).to_s
-
-          expect(response).to include_pagination_headers
-          expect(response.headers['X-Total']).to eq(commit_count)
-          expect(response.headers['X-Page']).to eq('1')
+          expect(response).to include_limited_pagination_headers
           expect(response.headers['Link']).to match(/page=1&per_page=5/)
           expect(response.headers['Link']).to match(/page=2&per_page=5/)
         end
@@ -972,7 +971,7 @@ RSpec.describe API::Commits do
         refs.concat(project.repository.tag_names_contains(commit_id).map {|name| ['tag', name]})
 
         expect(response).to have_gitlab_http_status(:ok)
-        expect(response).to include_pagination_headers
+        expect(response).to include_limited_pagination_headers
         expect(json_response).to be_an Array
         expect(json_response.map { |r| [r['type'], r['name']] }.compact).to eq(refs)
       end
@@ -1262,7 +1261,7 @@ RSpec.describe API::Commits do
         get api(route, current_user)
 
         expect(response).to have_gitlab_http_status(:ok)
-        expect(response).to include_pagination_headers
+        expect(response).to include_limited_pagination_headers
         expect(json_response.size).to be >= 1
         expect(json_response.first.keys).to include 'diff'
       end
@@ -1276,7 +1275,7 @@ RSpec.describe API::Commits do
           get api(route, current_user)
 
           expect(response).to have_gitlab_http_status(:ok)
-          expect(response).to include_pagination_headers
+          expect(response).to include_limited_pagination_headers
           expect(json_response.size).to be <= 1
         end
       end
@@ -1914,7 +1913,7 @@ RSpec.describe API::Commits do
       get api("/projects/#{project.id}/repository/commits/#{commit.id}/merge_requests", user)
 
       expect(response).to have_gitlab_http_status(:ok)
-      expect(response).to include_pagination_headers
+      expect(response).to include_limited_pagination_headers
       expect(json_response.length).to eq(1)
       expect(json_response[0]['id']).to eq(merged_mr.id)
     end