Merge branch 'bvl-commits-api-exclude-count' into 'master'

Exclude the total headers from the commits API

See merge request gitlab-org/gitlab!43159

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,6 +62,17 @@ module API
                                                   first_parent: first_parent,
                                                   order: order)
 
+        serializer = with_stats ? Entities::CommitWithStats : Entities::Commit
+
+        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)
+
+          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)
@@ -72,10 +83,9 @@ module API
 
           paginated_commits = Kaminari.paginate_array(commits, total_count: commit_count)
 
-        serializer = with_stats ? Entities::CommitWithStats : Entities::Commit
-
           present paginate(paginated_commits), with: serializer
         end
+      end
 
       desc 'Commit multiple file changes as one commit' do
         success Entities::CommitDetail

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", user)
+          get api("/projects/#{project_id}/repository/commits?all=true&per_page=50", user)
 
-          expect(response).to include_pagination_headers
-          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(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