From de9c80562d6bc626a9bf9d56b73b8350b6782c1a Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 23 Sep 2020 17:52:56 +0200 Subject: [PATCH] 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. --- .../development/api_commits_without_count.yml | 7 +++ lib/api/commits.rb | 30 ++++++---- lib/api/helpers/pagination.rb | 4 +- lib/gitlab/pagination/offset_pagination.rb | 8 +-- .../pagination/offset_pagination_spec.rb | 30 +++++++++- spec/requests/api/commits_spec.rb | 59 +++++++++---------- 6 files changed, 91 insertions(+), 47 deletions(-) create mode 100644 config/feature_flags/development/api_commits_without_count.yml 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 00000000000000..bd5ff8d5eb319a --- /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 20877fb5c5fa41..3097bcc0ef10af 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 a6ae9a87f98a8b..227aec224e5469 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 8796dd4d7ec1f8..c59f4505d18957 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 be20f0194f7fe0..c9a23170137bbf 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 d34244771ad5ec..98c1e0228d43ff 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 -- GitLab