Merge branch '299529_use_gitaly_sort_for_tags' into 'master'

Use Gitaly API to sort tags

See merge request gitlab-org/gitlab!69101

app/models/repository.rb

@@ -721,18 +721,9 @@ class Repository
   end
 
   def tags_sorted_by(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
+    return raw_repository.tags(sort_by: value) if Feature.enabled?(:gitaly_tags_finder, project, default_enabled: :yaml)
+
+    tags_ruby_sort(value)
   end
 
   # Params:
@@ -1164,6 +1155,23 @@ class Repository
     @request_store_cache ||= Gitlab::RepositoryCache.new(self, backend: Gitlab::SafeRequestStore)
   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
     # 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

config/feature_flags/development/gitaly_tags_finder.yml

+---
+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

lib/gitlab/git/repository.rb

@@ -191,9 +191,9 @@ module Gitlab
 
       # Returns an Array of Tags
       #
-      def tags
+      def tags(sort_by: nil)
         wrapped_gitaly_errors do
-          gitaly_ref_client.tags
+          gitaly_ref_client.tags(sort_by: sort_by)
         end
       end
 

lib/gitlab/gitaly_client/ref_service.rb

@@ -5,6 +5,16 @@ module Gitlab
     class RefService
       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
       def initialize(repository)
         @repository = repository
@@ -84,13 +94,15 @@ module Gitlab
 
       def local_branches(sort_by: nil, pagination_params: nil)
         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)
         consume_find_local_branches_response(response)
       end
 
-      def tags
+      def tags(sort_by: nil)
         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)
         consume_tags_response(response)
       end
@@ -201,7 +213,7 @@ module Gitlab
         response.flat_map { |message| message.names.map { |name| yield(name) } }
       end
 
-      def sort_by_param(sort_by)
+      def sort_local_branches_by_param(sort_by)
         sort_by = 'name' if sort_by == 'name_asc'
 
         enum_value = Gitaly::FindLocalBranchesRequest::SortBy.resolve(sort_by.upcase.to_sym)
@@ -210,6 +222,17 @@ module Gitlab
         enum_value
       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)
         response.flat_map do |message|
           message.branches.map do |gitaly_branch|

spec/lib/gitlab/git/repository_spec.rb

@@ -109,6 +109,32 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
     it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :tag_names
   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
     let(:storage_path) { '/tmp' }
     let(:cache_key) { File.join(repository.gl_repository, SeedRepo::LastCommit::ID) }

spec/lib/gitlab/gitaly_client/ref_service_spec.rb

@@ -154,6 +154,22 @@ RSpec.describe Gitlab::GitalyClient::RefService do
 
       client.tags
     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
 
   describe '#branch_names_contains_sha' do

spec/models/repository_spec.rb

@@ -68,51 +68,69 @@ RSpec.describe Repository do
 
   describe 'tags_sorted_by' do
     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
       subject { repository.tags_sorted_by('name_desc').map(&:name) & tags_to_compare }
 
       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
 
     context 'name_asc' do
       subject { repository.tags_sorted_by('name_asc').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
 
     context 'updated' do
-      let(:tag_a) { repository.find_tag('v1.0.0') }
-      let(:tag_b) { repository.find_tag('v1.1.0') }
+      let(:latest_tag) { 'v0.0.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
-        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
-          double_first = double(committed_date: Time.current)
-          double_last = double(committed_date: Time.current - 1.second)
+        it { is_expected.to eq([latest_tag, 'v1.1.0', 'v1.0.0']) }
 
-          allow(tag_a).to receive(:dereferenced_target).and_return(double_first)
-          allow(tag_b).to receive(:dereferenced_target).and_return(double_last)
-          allow(repository).to receive(:tags).and_return([tag_a, tag_b])
-        end
+        context 'when feature flag is disabled' do
+          let(:feature_flag) { false }
 
-        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
 
       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
-          double_first = double(committed_date: Time.current - 1.second)
-          double_last = double(committed_date: Time.current)
+        it { is_expected.to eq(['v1.0.0', 'v1.1.0', latest_tag]) }
 
-          allow(tag_a).to receive(:dereferenced_target).and_return(double_last)
-          allow(tag_b).to receive(:dereferenced_target).and_return(double_first)
-          allow(repository).to receive(:tags).and_return([tag_a, tag_b])
-        end
+        context 'when feature flag is disabled' do
+          let(:feature_flag) { false }
 
-        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
 
       context 'annotated tag pointing to a blob' do
@@ -125,21 +143,33 @@ RSpec.describe Repository do
                       tagger: { name: 'John Smith', email: 'john@gmail.com' } }
 
           rugged_repo(repository).tags.create(annotated_tag_name, 'a48e4fc218069f68ef2e769dd8dfea3991362175', **options)
+        end
 
-          double_first = double(committed_date: Time.current - 1.second)
-          double_last = double(committed_date: Time.current)
+        it { is_expected.to eq(['v1.0.0', 'v1.1.0', annotated_tag_name]) }
 
-          allow(tag_a).to receive(:dereferenced_target).and_return(double_last)
-          allow(tag_b).to receive(:dereferenced_target).and_return(double_first)
-        end
+        context 'when feature flag is disabled' do
+          let(:feature_flag) { false }
 
-        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
           rugged_repo(repository).tags.delete(annotated_tag_name)
         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
 
   describe '#ref_exists?' do