Merge branch '340591_refactor_tags_finder' into 'master'

Restore TagsFinder class interface

See merge request gitlab-org/gitlab!74210

app/controllers/projects/tags_controller.rb

@@ -14,18 +14,24 @@ class Projects::TagsController < Projects::ApplicationController
 
   # rubocop: disable CodeReuse/ActiveRecord
   def index
-    params[:sort] = params[:sort].presence || sort_value_recently_updated
+    begin
+      params[:sort] = params[:sort].presence || sort_value_recently_updated
 
-    @sort = params[:sort]
+      @sort = params[:sort]
 
-    @tags, @tags_loading_error = TagsFinder.new(@repository, params).execute
+      @tags = TagsFinder.new(@repository, params).execute
 
-    @tags = Kaminari.paginate_array(@tags).page(params[:page])
-    tag_names = @tags.map(&:name)
-    @tags_pipelines = @project.ci_pipelines.latest_successful_for_refs(tag_names)
+      @tags = Kaminari.paginate_array(@tags).page(params[:page])
+      tag_names = @tags.map(&:name)
+      @tags_pipelines = @project.ci_pipelines.latest_successful_for_refs(tag_names)
 
-    @releases = project.releases.where(tag: tag_names)
-    @tag_pipeline_statuses = Ci::CommitStatusesFinder.new(@project, @repository, current_user, @tags).execute
+      @releases = project.releases.where(tag: tag_names)
+      @tag_pipeline_statuses = Ci::CommitStatusesFinder.new(@project, @repository, current_user, @tags).execute
+
+    rescue Gitlab::Git::CommandError => e
+      @tags = []
+      @tags_loading_error = e
+    end
 
     respond_to do |format|
       status = @tags_loading_error ? :service_unavailable : :ok

app/controllers/projects_controller.rb

@@ -293,7 +293,11 @@ class ProjectsController < Projects::ApplicationController
     end
 
     if find_tags && @repository.tag_count.nonzero?
-      tags, _ = TagsFinder.new(@repository, params).execute
+      tags = begin
+        TagsFinder.new(@repository, params).execute
+      rescue Gitlab::Git::CommandError
+        []
+      end
 
       options['Tags'] = tags.take(100).map(&:name)
     end

app/finders/tags_finder.rb

@@ -8,8 +8,6 @@ class TagsFinder < GitRefsFinder
   def execute
     tags = repository.tags_sorted_by(sort)
 
-    [by_search(tags), nil]
-  rescue Gitlab::Git::CommandError => e
-    [[], e]
+    by_search(tags)
   end
 end

lib/api/tags.rb

@@ -24,7 +24,7 @@ module API
         use :pagination
       end
       get ':id/repository/tags', feature_category: :source_code_management do
-        tags, _ = ::TagsFinder.new(user_project.repository,
+        tags = ::TagsFinder.new(user_project.repository,
                                 sort: "#{params[:order_by]}_#{params[:sort]}",
                                 search: params[:search]).execute
 
@@ -35,6 +35,9 @@ module API
         else
           present paginated_tags, with: Entities::Tag, project: user_project
         end
+
+      rescue Gitlab::Git::CommandError
+        service_unavailable!
       end
 
       desc 'Get a single repository tag' do

spec/controllers/projects/tags_controller_spec.rb

@@ -25,7 +25,7 @@ RSpec.describe Projects::TagsController do
       with_them do
         it 'returns 503 status code' do
           expect_next_instance_of(TagsFinder) do |finder|
-            expect(finder).to receive(:execute).and_return([[], Gitlab::Git::CommandError.new])
+            expect(finder).to receive(:execute).and_raise(Gitlab::Git::CommandError)
           end
 
           get :index, params: { namespace_id: project.namespace.to_param, project_id: project }, format: format

spec/controllers/projects_controller_spec.rb

@@ -1143,6 +1143,22 @@ RSpec.describe ProjectsController do
       expect(json_response["Commits"]).to include("123456")
     end
 
+    context 'when gitaly is unavailable' do
+      before do
+        expect_next_instance_of(TagsFinder) do |finder|
+          allow(finder).to receive(:execute).and_raise(Gitlab::Git::CommandError)
+        end
+      end
+
+      it 'gets an empty list of tags' do
+        get :refs, params: { namespace_id: project.namespace, id: project, ref: "123456" }
+
+        expect(json_response["Branches"]).to include("master")
+        expect(json_response["Tags"]).to eq([])
+        expect(json_response["Commits"]).to include("123456")
+      end
+    end
+
     context "when preferred language is Japanese" do
       before do
         user.update!(preferred_language: 'ja')

spec/finders/tags_finder_spec.rb

@@ -8,12 +8,7 @@ RSpec.describe TagsFinder do
   let_it_be(:repository) { project.repository }
 
   def load_tags(params)
-    tags_finder = described_class.new(repository, params)
-    tags, error = tags_finder.execute
-
-    expect(error).to eq(nil)
-
-    tags
+    described_class.new(repository, params).execute
   end
 
   describe '#execute' do
@@ -102,14 +97,12 @@ RSpec.describe TagsFinder do
     end
 
     context 'when Gitaly is unavailable' do
-      it 'returns empty list of tags' do
+      it 'raises an exception' do
         expect(Gitlab::GitalyClient).to receive(:call).and_raise(GRPC::Unavailable)
 
         tags_finder = described_class.new(repository, {})
-        tags, error = tags_finder.execute
 
-        expect(error).to be_a(Gitlab::Git::CommandError)
-        expect(tags).to eq([])
+        expect { tags_finder.execute }.to raise_error(Gitlab::Git::CommandError)
       end
     end
   end

spec/requests/api/tags_spec.rb

@@ -208,6 +208,20 @@ RSpec.describe API::Tags do
 
       it_behaves_like "get repository tags"
     end
+
+    context 'when gitaly is unavailable' do
+      let(:route) { "/projects/#{project_id}/repository/tags" }
+
+      before do
+        expect_next_instance_of(TagsFinder) do |finder|
+          allow(finder).to receive(:execute).and_raise(Gitlab::Git::CommandError)
+        end
+      end
+
+      it_behaves_like '503 response' do
+        let(:request) { get api(route, user) }
+      end
+    end
   end
 
   describe 'GET /projects/:id/repository/tags/:tag_name' do

spec/support/shared_examples/requests/api/status_shared_examples.rb

@@ -76,3 +76,14 @@ RSpec.shared_examples '412 response' do
     end
   end
 end
+
+RSpec.shared_examples '503 response' do
+  before do
+    # Fires the request
+    request
+  end
+
+  it 'returns 503' do
+    expect(response).to have_gitlab_http_status(:service_unavailable)
+  end
+end