Commit e9c3b012 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin

Add keyset pagination for tags API

Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/299529

Add Gitaly-based pagination support to TagsFinder. It should improve
the overall performance of the Tags endpoint.

Changelog: added
parent c9336b9c
...@@ -5,9 +5,36 @@ class TagsFinder < GitRefsFinder ...@@ -5,9 +5,36 @@ class TagsFinder < GitRefsFinder
super(repository, params) super(repository, params)
end end
def execute def execute(gitaly_pagination: false)
tags = repository.tags_sorted_by(sort) tags = if gitaly_pagination
repository.tags_sorted_by(sort, pagination_params)
else
repository.tags_sorted_by(sort)
end
by_search(tags) by_search(tags)
rescue ArgumentError => e
raise Gitlab::Git::InvalidPageToken, "Invalid page token: #{page_token}" if e.message.include?('page token')
raise
end
def total
repository.tag_count
end
private
def per_page
params[:per_page].presence
end
def page_token
"#{Gitlab::Git::TAG_REF_PREFIX}#{@params[:page_token]}" if params[:page_token]
end
def pagination_params
{ limit: per_page, page_token: page_token }
end end
end end
---
name: tag_list_keyset_pagination
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74239
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/345595
milestone: '14.5'
type: development
group: group::source code
default_enabled: false
...@@ -21,14 +21,17 @@ module API ...@@ -21,14 +21,17 @@ module API
optional :order_by, type: String, values: %w[name updated], default: 'updated', optional :order_by, type: String, values: %w[name updated], default: 'updated',
desc: 'Return tags ordered by `name` or `updated` fields.' desc: 'Return tags ordered by `name` or `updated` fields.'
optional :search, type: String, desc: 'Return list of tags matching the search criteria' optional :search, type: String, desc: 'Return list of tags matching the search criteria'
optional :page_token, type: String, desc: 'Name of tag to start the paginaition from'
use :pagination use :pagination
end end
get ':id/repository/tags', feature_category: :source_code_management, urgency: :low do get ':id/repository/tags', feature_category: :source_code_management, urgency: :low do
tags = ::TagsFinder.new(user_project.repository, tags_finder = ::TagsFinder.new(user_project.repository,
sort: "#{params[:order_by]}_#{params[:sort]}", sort: "#{params[:order_by]}_#{params[:sort]}",
search: params[:search]).execute search: params[:search],
page_token: params[:page_token],
per_page: params[:per_page])
paginated_tags = paginate(::Kaminari.paginate_array(tags)) paginated_tags = Gitlab::Pagination::GitalyKeysetPager.new(self, user_project).paginate(tags_finder)
if Feature.enabled?(:api_caching_tags, user_project, type: :development) if Feature.enabled?(:api_caching_tags, user_project, type: :development)
present_cached paginated_tags, with: Entities::Tag, project: user_project, cache_context: -> (_tag) { user_project.cache_key } present_cached paginated_tags, with: Entities::Tag, project: user_project, cache_context: -> (_tag) { user_project.cache_key }
...@@ -36,6 +39,8 @@ module API ...@@ -36,6 +39,8 @@ module API
present paginated_tags, with: Entities::Tag, project: user_project present paginated_tags, with: Entities::Tag, project: user_project
end end
rescue Gitlab::Git::InvalidPageToken => e
unprocessable_entity!(e.message)
rescue Gitlab::Git::CommandError rescue Gitlab::Git::CommandError
service_unavailable! service_unavailable!
end end
......
...@@ -17,6 +17,7 @@ module Gitlab ...@@ -17,6 +17,7 @@ module Gitlab
OSError = Class.new(BaseError) OSError = Class.new(BaseError)
UnknownRef = Class.new(BaseError) UnknownRef = Class.new(BaseError)
CommandTimedOut = Class.new(CommandError) CommandTimedOut = Class.new(CommandError)
InvalidPageToken = Class.new(BaseError)
class << self class << self
include Gitlab::EncodingHelper include Gitlab::EncodingHelper
......
...@@ -30,6 +30,8 @@ module Gitlab ...@@ -30,6 +30,8 @@ module Gitlab
if finder.is_a?(BranchesFinder) if finder.is_a?(BranchesFinder)
Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: :yaml) Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: :yaml)
elsif finder.is_a?(TagsFinder)
Feature.enabled?(:tag_list_keyset_pagination, project, default_enabled: :yaml)
elsif finder.is_a?(::Repositories::TreeFinder) elsif finder.is_a?(::Repositories::TreeFinder)
Feature.enabled?(:repository_tree_gitaly_pagination, project, default_enabled: :yaml) Feature.enabled?(:repository_tree_gitaly_pagination, project, default_enabled: :yaml)
else else
...@@ -42,6 +44,8 @@ module Gitlab ...@@ -42,6 +44,8 @@ module Gitlab
if finder.is_a?(BranchesFinder) if finder.is_a?(BranchesFinder)
Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: :yaml) Feature.enabled?(:branch_list_keyset_pagination, project, default_enabled: :yaml)
elsif finder.is_a?(TagsFinder)
Feature.enabled?(:tag_list_keyset_pagination, project, default_enabled: :yaml)
elsif finder.is_a?(::Repositories::TreeFinder) elsif finder.is_a?(::Repositories::TreeFinder)
Feature.enabled?(:repository_tree_gitaly_pagination, project, default_enabled: :yaml) Feature.enabled?(:repository_tree_gitaly_pagination, project, default_enabled: :yaml)
else else
......
...@@ -7,8 +7,8 @@ RSpec.describe TagsFinder do ...@@ -7,8 +7,8 @@ RSpec.describe TagsFinder do
let_it_be(:project) { create(:project, :repository) } let_it_be(:project) { create(:project, :repository) }
let_it_be(:repository) { project.repository } let_it_be(:repository) { project.repository }
def load_tags(params) def load_tags(params, gitaly_pagination: false)
described_class.new(repository, params).execute described_class.new(repository, params).execute(gitaly_pagination: gitaly_pagination)
end end
describe '#execute' do describe '#execute' do
...@@ -96,6 +96,72 @@ RSpec.describe TagsFinder do ...@@ -96,6 +96,72 @@ RSpec.describe TagsFinder do
end end
end end
context 'with Gitaly pagination' do
subject { load_tags(params, gitaly_pagination: true) }
context 'by page_token and per_page' do
let(:params) { { page_token: 'v1.0.0', per_page: 1 } }
it 'filters tags' do
result = subject
expect(result.map(&:name)).to eq(%w(v1.1.0))
end
end
context 'by next page_token and per_page' do
let(:params) { { page_token: 'v1.1.0', per_page: 2 } }
it 'filters branches' do
result = subject
expect(result.map(&:name)).to eq(%w(v1.1.1))
end
end
context 'by per_page only' do
let(:params) { { per_page: 2 } }
it 'filters branches' do
result = subject
expect(result.map(&:name)).to eq(%w(v1.0.0 v1.1.0))
end
end
context 'by page_token only' do
let(:params) { { page_token: 'feature' } }
it 'raises an error' do
expect do
subject
end.to raise_error(Gitlab::Git::InvalidPageToken, 'Invalid page token: refs/tags/feature')
end
end
context 'pagination and sort' do
context 'by per_page' do
let(:params) { { sort: 'updated_desc', per_page: 5 } }
it 'filters branches' do
result = subject
expect(result.map(&:name)).to eq(%w(v1.1.1 v1.1.0 v1.0.0))
end
end
context 'by page_token and per_page' do
let(:params) { { sort: 'updated_desc', page_token: 'v1.1.1', per_page: 2 } }
it 'filters branches' do
result = subject
expect(result.map(&:name)).to eq(%w(v1.1.0 v1.0.0))
end
end
end
end
context 'when Gitaly is unavailable' do context 'when Gitaly is unavailable' do
it 'raises an exception' do it 'raises an exception' do
expect(Gitlab::GitalyClient).to receive(:call).and_raise(GRPC::Unavailable) expect(Gitlab::GitalyClient).to receive(:call).and_raise(GRPC::Unavailable)
......
...@@ -17,6 +17,10 @@ RSpec.describe API::Tags do ...@@ -17,6 +17,10 @@ RSpec.describe API::Tags do
end end
describe 'GET /projects/:id/repository/tags' do describe 'GET /projects/:id/repository/tags' do
before do
stub_feature_flags(tag_list_keyset_pagination: false)
end
shared_examples "get repository tags" do shared_examples "get repository tags" do
let(:route) { "/projects/#{project_id}/repository/tags" } let(:route) { "/projects/#{project_id}/repository/tags" }
...@@ -143,6 +147,55 @@ RSpec.describe API::Tags do ...@@ -143,6 +147,55 @@ RSpec.describe API::Tags do
expect(expected_tag['release']['description']).to eq(description) expect(expected_tag['release']['description']).to eq(description)
end end
end end
context 'with keyset pagination on', :aggregate_errors do
before do
stub_feature_flags(tag_list_keyset_pagination: true)
end
context 'with keyset pagination option' do
let(:base_params) { { pagination: 'keyset' } }
context 'with gitaly pagination params' do
context 'with high limit' do
let(:params) { base_params.merge(per_page: 100) }
it 'returns all repository tags' do
get api(route, user), params: params
expect(response).to have_gitlab_http_status(:ok)
expect(response).to match_response_schema('public_api/v4/tags')
expect(response.headers).not_to include('Link')
tag_names = json_response.map { |x| x['name'] }
expect(tag_names).to match_array(project.repository.tag_names)
end
end
context 'with low limit' do
let(:params) { base_params.merge(per_page: 2) }
it 'returns limited repository tags' do
get api(route, user), params: params
expect(response).to have_gitlab_http_status(:ok)
expect(response).to match_response_schema('public_api/v4/tags')
expect(response.headers).to include('Link')
tag_names = json_response.map { |x| x['name'] }
expect(tag_names).to match_array(%w(v1.1.0 v1.1.1))
end
end
context 'with missing page token' do
let(:params) { base_params.merge(page_token: 'unknown') }
it_behaves_like '422 response' do
let(:request) { get api(route, user), params: params }
let(:message) { 'Invalid page token: refs/tags/unknown' }
end
end
end
end
end
end end
context ":api_caching_tags flag enabled", :use_clean_rails_memory_store_caching do context ":api_caching_tags flag enabled", :use_clean_rails_memory_store_caching do
......
...@@ -77,6 +77,24 @@ RSpec.shared_examples '412 response' do ...@@ -77,6 +77,24 @@ RSpec.shared_examples '412 response' do
end end
end end
RSpec.shared_examples '422 response' do
let(:message) { nil }
before do
# Fires the request
request
end
it 'returns 422' do
expect(response).to have_gitlab_http_status(:unprocessable_entity)
expect(json_response).to be_an Object
if message.present?
expect(json_response['message']).to eq(message)
end
end
end
RSpec.shared_examples '503 response' do RSpec.shared_examples '503 response' do
before do before do
# Fires the request # Fires the request
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment