Commit f94c8665 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin

Use Gitaly API to sort tags

* Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/299529
* Gitaly MR:
https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3707

**Problem**

We use Ruby to sort tags received from Gitaly. It is slower than
Gitaly sorting.

**Solution**

Provide sort parameter to Gitaly API and receive already sorted tags.

Changelog: added
parent 19b4429c
...@@ -721,18 +721,9 @@ class Repository ...@@ -721,18 +721,9 @@ class Repository
end end
def tags_sorted_by(value) def tags_sorted_by(value)
case value return raw_repository.tags(sort_by: value) if Feature.enabled?(:gitaly_tags_finder, project, default_enabled: :yaml)
when 'name_asc'
VersionSorter.sort(tags) { |tag| tag.name } tags_ruby_sort(value)
when 'name_desc'
VersionSorter.rsort(tags) { |tag| tag.name }
when 'updated_desc'
tags_sorted_by_committed_date.reverse
when 'updated_asc'
tags_sorted_by_committed_date
else
tags
end
end end
# Params: # Params:
...@@ -1164,6 +1155,23 @@ class Repository ...@@ -1164,6 +1155,23 @@ class Repository
@request_store_cache ||= Gitlab::RepositoryCache.new(self, backend: Gitlab::SafeRequestStore) @request_store_cache ||= Gitlab::RepositoryCache.new(self, backend: Gitlab::SafeRequestStore)
end end
# Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/339741
def tags_ruby_sort(value)
case value
when 'name_asc'
VersionSorter.sort(tags) { |tag| tag.name }
when 'name_desc'
VersionSorter.rsort(tags) { |tag| tag.name }
when 'updated_desc'
tags_sorted_by_committed_date.reverse
when 'updated_asc'
tags_sorted_by_committed_date
else
tags
end
end
# Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/339741
def tags_sorted_by_committed_date def tags_sorted_by_committed_date
# Annotated tags can point to any object (e.g. a blob), but generally # Annotated tags can point to any object (e.g. a blob), but generally
# tags point to a commit. If we don't have a commit, then just default # tags point to a commit. If we don't have a commit, then just default
......
---
name: gitaly_tags_finder
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69101
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/339741
milestone: '14.3'
type: development
group: group::source code
default_enabled: false
...@@ -191,9 +191,9 @@ module Gitlab ...@@ -191,9 +191,9 @@ module Gitlab
# Returns an Array of Tags # Returns an Array of Tags
# #
def tags def tags(sort_by: nil)
wrapped_gitaly_errors do wrapped_gitaly_errors do
gitaly_ref_client.tags gitaly_ref_client.tags(sort_by: sort_by)
end end
end end
......
...@@ -5,6 +5,16 @@ module Gitlab ...@@ -5,6 +5,16 @@ module Gitlab
class RefService class RefService
include Gitlab::EncodingHelper include Gitlab::EncodingHelper
TAGS_SORT_KEY = {
'name' => Gitaly::FindAllTagsRequest::SortBy::Key::REFNAME,
'updated' => Gitaly::FindAllTagsRequest::SortBy::Key::CREATORDATE
}.freeze
TAGS_SORT_DIRECTION = {
'asc' => Gitaly::SortDirection::ASCENDING,
'desc' => Gitaly::SortDirection::DESCENDING
}.freeze
# 'repository' is a Gitlab::Git::Repository # 'repository' is a Gitlab::Git::Repository
def initialize(repository) def initialize(repository)
@repository = repository @repository = repository
...@@ -84,13 +94,15 @@ module Gitlab ...@@ -84,13 +94,15 @@ module Gitlab
def local_branches(sort_by: nil, pagination_params: nil) def local_branches(sort_by: nil, pagination_params: nil)
request = Gitaly::FindLocalBranchesRequest.new(repository: @gitaly_repo, pagination_params: pagination_params) request = Gitaly::FindLocalBranchesRequest.new(repository: @gitaly_repo, pagination_params: pagination_params)
request.sort_by = sort_by_param(sort_by) if sort_by request.sort_by = sort_local_branches_by_param(sort_by) if sort_by
response = GitalyClient.call(@storage, :ref_service, :find_local_branches, request, timeout: GitalyClient.fast_timeout) response = GitalyClient.call(@storage, :ref_service, :find_local_branches, request, timeout: GitalyClient.fast_timeout)
consume_find_local_branches_response(response) consume_find_local_branches_response(response)
end end
def tags def tags(sort_by: nil)
request = Gitaly::FindAllTagsRequest.new(repository: @gitaly_repo) request = Gitaly::FindAllTagsRequest.new(repository: @gitaly_repo)
request.sort_by = sort_tags_by_param(sort_by) if sort_by
response = GitalyClient.call(@storage, :ref_service, :find_all_tags, request, timeout: GitalyClient.medium_timeout) response = GitalyClient.call(@storage, :ref_service, :find_all_tags, request, timeout: GitalyClient.medium_timeout)
consume_tags_response(response) consume_tags_response(response)
end end
...@@ -201,7 +213,7 @@ module Gitlab ...@@ -201,7 +213,7 @@ module Gitlab
response.flat_map { |message| message.names.map { |name| yield(name) } } response.flat_map { |message| message.names.map { |name| yield(name) } }
end end
def sort_by_param(sort_by) def sort_local_branches_by_param(sort_by)
sort_by = 'name' if sort_by == 'name_asc' sort_by = 'name' if sort_by == 'name_asc'
enum_value = Gitaly::FindLocalBranchesRequest::SortBy.resolve(sort_by.upcase.to_sym) enum_value = Gitaly::FindLocalBranchesRequest::SortBy.resolve(sort_by.upcase.to_sym)
...@@ -210,6 +222,17 @@ module Gitlab ...@@ -210,6 +222,17 @@ module Gitlab
enum_value enum_value
end end
def sort_tags_by_param(sort_by)
match = sort_by.match(/^(?<key>name|updated)_(?<direction>asc|desc)$/)
return unless match
Gitaly::FindAllTagsRequest::SortBy.new(
key: TAGS_SORT_KEY[match[:key]],
direction: TAGS_SORT_DIRECTION[match[:direction]]
)
end
def consume_find_local_branches_response(response) def consume_find_local_branches_response(response)
response.flat_map do |message| response.flat_map do |message|
message.branches.map do |gitaly_branch| message.branches.map do |gitaly_branch|
......
...@@ -109,6 +109,32 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do ...@@ -109,6 +109,32 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :tag_names it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :tag_names
end end
describe '#tags' do
subject { repository.tags }
it 'gets tags from GitalyClient' do
expect_next_instance_of(Gitlab::GitalyClient::RefService) do |service|
expect(service).to receive(:tags)
end
subject
end
context 'with sorting option' do
subject { repository.tags(sort_by: 'name_asc') }
it 'gets tags from GitalyClient' do
expect_next_instance_of(Gitlab::GitalyClient::RefService) do |service|
expect(service).to receive(:tags).with(sort_by: 'name_asc')
end
subject
end
end
it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :tags
end
describe '#archive_metadata' do describe '#archive_metadata' do
let(:storage_path) { '/tmp' } let(:storage_path) { '/tmp' }
let(:cache_key) { File.join(repository.gl_repository, SeedRepo::LastCommit::ID) } let(:cache_key) { File.join(repository.gl_repository, SeedRepo::LastCommit::ID) }
......
...@@ -154,6 +154,22 @@ RSpec.describe Gitlab::GitalyClient::RefService do ...@@ -154,6 +154,22 @@ RSpec.describe Gitlab::GitalyClient::RefService do
client.tags client.tags
end end
context 'with sorting option' do
it 'sends a correct find_all_tags message' do
expected_sort_by = Gitaly::FindAllTagsRequest::SortBy.new(
key: :REFNAME,
direction: :ASCENDING
)
expect_any_instance_of(Gitaly::RefService::Stub)
.to receive(:find_all_tags)
.with(gitaly_request_with_params(sort_by: expected_sort_by), kind_of(Hash))
.and_return([])
client.tags(sort_by: 'name_asc')
end
end
end end
describe '#branch_names_contains_sha' do describe '#branch_names_contains_sha' do
......
...@@ -68,51 +68,69 @@ RSpec.describe Repository do ...@@ -68,51 +68,69 @@ RSpec.describe Repository do
describe 'tags_sorted_by' do describe 'tags_sorted_by' do
let(:tags_to_compare) { %w[v1.0.0 v1.1.0] } let(:tags_to_compare) { %w[v1.0.0 v1.1.0] }
let(:feature_flag) { true }
before do
stub_feature_flags(gitaly_tags_finder: feature_flag)
end
context 'name_desc' do context 'name_desc' do
subject { repository.tags_sorted_by('name_desc').map(&:name) & tags_to_compare } subject { repository.tags_sorted_by('name_desc').map(&:name) & tags_to_compare }
it { is_expected.to eq(['v1.1.0', 'v1.0.0']) } it { is_expected.to eq(['v1.1.0', 'v1.0.0']) }
context 'when feature flag is disabled' do
let(:feature_flag) { false }
it { is_expected.to eq(['v1.1.0', 'v1.0.0']) }
end
end end
context 'name_asc' do context 'name_asc' do
subject { repository.tags_sorted_by('name_asc').map(&:name) & tags_to_compare } subject { repository.tags_sorted_by('name_asc').map(&:name) & tags_to_compare }
it { is_expected.to eq(['v1.0.0', 'v1.1.0']) } it { is_expected.to eq(['v1.0.0', 'v1.1.0']) }
context 'when feature flag is disabled' do
let(:feature_flag) { false }
it { is_expected.to eq(['v1.0.0', 'v1.1.0']) }
end
end end
context 'updated' do context 'updated' do
let(:tag_a) { repository.find_tag('v1.0.0') } let(:latest_tag) { 'v0.0.0' }
let(:tag_b) { repository.find_tag('v1.1.0') }
before do
rugged_repo(repository).tags.create(latest_tag, repository.commit.id)
end
after do
rugged_repo(repository).tags.delete(latest_tag)
end
context 'desc' do context 'desc' do
subject { repository.tags_sorted_by('updated_desc').map(&:name) } subject { repository.tags_sorted_by('updated_desc').map(&:name) & (tags_to_compare + [latest_tag]) }
before do it { is_expected.to eq([latest_tag, 'v1.1.0', 'v1.0.0']) }
double_first = double(committed_date: Time.current)
double_last = double(committed_date: Time.current - 1.second)
allow(tag_a).to receive(:dereferenced_target).and_return(double_first) context 'when feature flag is disabled' do
allow(tag_b).to receive(:dereferenced_target).and_return(double_last) let(:feature_flag) { false }
allow(repository).to receive(:tags).and_return([tag_a, tag_b])
end
it { is_expected.to eq(['v1.0.0', 'v1.1.0']) } it { is_expected.to eq([latest_tag, 'v1.1.0', 'v1.0.0']) }
end
end end
context 'asc' do context 'asc' do
subject { repository.tags_sorted_by('updated_asc').map(&:name) } subject { repository.tags_sorted_by('updated_asc').map(&:name) & (tags_to_compare + [latest_tag]) }
before do it { is_expected.to eq(['v1.0.0', 'v1.1.0', latest_tag]) }
double_first = double(committed_date: Time.current - 1.second)
double_last = double(committed_date: Time.current)
allow(tag_a).to receive(:dereferenced_target).and_return(double_last) context 'when feature flag is disabled' do
allow(tag_b).to receive(:dereferenced_target).and_return(double_first) let(:feature_flag) { false }
allow(repository).to receive(:tags).and_return([tag_a, tag_b])
end
it { is_expected.to eq(['v1.1.0', 'v1.0.0']) } it { is_expected.to eq(['v1.0.0', 'v1.1.0', latest_tag]) }
end
end end
context 'annotated tag pointing to a blob' do context 'annotated tag pointing to a blob' do
...@@ -125,21 +143,33 @@ RSpec.describe Repository do ...@@ -125,21 +143,33 @@ RSpec.describe Repository do
tagger: { name: 'John Smith', email: 'john@gmail.com' } } tagger: { name: 'John Smith', email: 'john@gmail.com' } }
rugged_repo(repository).tags.create(annotated_tag_name, 'a48e4fc218069f68ef2e769dd8dfea3991362175', **options) rugged_repo(repository).tags.create(annotated_tag_name, 'a48e4fc218069f68ef2e769dd8dfea3991362175', **options)
end
double_first = double(committed_date: Time.current - 1.second) it { is_expected.to eq(['v1.0.0', 'v1.1.0', annotated_tag_name]) }
double_last = double(committed_date: Time.current)
allow(tag_a).to receive(:dereferenced_target).and_return(double_last) context 'when feature flag is disabled' do
allow(tag_b).to receive(:dereferenced_target).and_return(double_first) let(:feature_flag) { false }
end
it { is_expected.to eq(['v1.1.0', 'v1.0.0', annotated_tag_name]) } it { is_expected.to eq(['v1.0.0', 'v1.1.0', annotated_tag_name]) }
end
after do after do
rugged_repo(repository).tags.delete(annotated_tag_name) rugged_repo(repository).tags.delete(annotated_tag_name)
end end
end end
end end
context 'unknown option' do
subject { repository.tags_sorted_by('unknown_desc').map(&:name) & tags_to_compare }
it { is_expected.to eq(['v1.0.0', 'v1.1.0']) }
context 'when feature flag is disabled' do
let(:feature_flag) { false }
it { is_expected.to eq(['v1.0.0', 'v1.1.0']) }
end
end
end end
describe '#ref_exists?' do describe '#ref_exists?' do
......
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