diff --git a/config/feature_flags/development/api_commits_without_count.yml b/config/feature_flags/development/api_commits_without_count.yml new file mode 100644 index 0000000000000000000000000000000000000000..bd5ff8d5eb319a7165413a6b3f7bf8497a6f69fe --- /dev/null +++ b/config/feature_flags/development/api_commits_without_count.yml @@ -0,0 +1,7 @@ +--- +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 diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 20877fb5c5fa41b6e1fc918416ade9f45ecf4f84..3097bcc0ef10afeaa6848704ba1a0a8a22771086 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -62,19 +62,29 @@ def authorize_push_to_branch!(branch) 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 diff --git a/lib/api/helpers/pagination.rb b/lib/api/helpers/pagination.rb index a6ae9a87f98a8b8dcaaed57dc3b744449c63b242..227aec224e54698b49ad0db730d90479f2e48d5f 100644 --- a/lib/api/helpers/pagination.rb +++ b/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 diff --git a/lib/gitlab/pagination/offset_pagination.rb b/lib/gitlab/pagination/offset_pagination.rb index 8796dd4d7ec1f8f7442c8c873137d28c35a45195..c59f4505d189575a85e7686c364c387c243e2acd 100644 --- a/lib/gitlab/pagination/offset_pagination.rb +++ b/lib/gitlab/pagination/offset_pagination.rb @@ -10,9 +10,9 @@ def initialize(request_context) @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 @@ def add_default_order(relation) 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 diff --git a/spec/lib/gitlab/pagination/offset_pagination_spec.rb b/spec/lib/gitlab/pagination/offset_pagination_spec.rb index be20f0194f7fe006fd404771298624453033c7a4..c9a23170137bbf9ad6d14955c08395e2b0cf449a 100644 --- a/spec/lib/gitlab/pagination/offset_pagination_spec.rb +++ b/spec/lib/gitlab/pagination/offset_pagination_spec.rb @@ -13,7 +13,7 @@ let(:request_context) { double("request_context") } - subject do + subject(:paginator) do described_class.new(request_context) end @@ -119,6 +119,34 @@ 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 diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index d34244771ad5ec15f494c4c4da8d73948ed1a18c..98c1e0228d43fff39abbf45f1b6df4bb1ad05711 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -36,6 +36,13 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ def push_params(branch_name) 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 @@ def push_params(branch_name) 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 @@ def push_params(branch_name) 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 @@ def push_params(branch_name) 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