Merge branch 'bvl-move-wrapped-gitaly-errors' into 'master'

Move Repository#wrapped_gitaly_errors into concern

See merge request gitlab-org/gitlab-ce!22691

10 files changed, 76 insertions, 35 deletions

diff --git a/lib/gitlab/git/blob.rb b/lib/gitlab/git/blob.rb
index 13b0bb930f4..0bd1d3420a2 100644
--- a/lib/gitlab/git/blob.rb
+++ b/lib/gitlab/git/blob.rb
@@ -5,6 +5,7 @@ module Gitlab
     class Blob
       include Gitlab::BlobHelper
       include Gitlab::EncodingHelper
+      extend Gitlab::Git::WrapsGitalyErrors
 
       # This number is the maximum amount of data that we want to display to
       # the user. We load as much as we can for encoding detection and LFS
@@ -75,7 +76,7 @@ module Gitlab
         # Returns array of Gitlab::Git::Blob
         # Does not guarantee blob data will be set
         def batch_lfs_pointers(repository, blob_ids)
-          repository.wrapped_gitaly_errors do
+          wrapped_gitaly_errors do
             repository.gitaly_blob_client.batch_lfs_pointers(blob_ids.to_a)
           end
         end
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index 74cdabfed9d..2820491b65d 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -3,6 +3,7 @@ module Gitlab
   module Git
     class Commit
       include Gitlab::EncodingHelper
+      extend Gitlab::Git::WrapsGitalyErrors
 
       attr_accessor :raw_commit, :head
 
@@ -59,7 +60,7 @@ module Gitlab
           # This saves us an RPC round trip.
           return nil if commit_id.include?(':')
 
-          commit = repo.wrapped_gitaly_errors do
+          commit = wrapped_gitaly_errors do
             repo.gitaly_commit_client.find_commit(commit_id)
           end
 
@@ -100,7 +101,7 @@ module Gitlab
         #   Commit.between(repo, '29eda46b', 'master')
         #
         def between(repo, base, head)
-          repo.wrapped_gitaly_errors do
+          wrapped_gitaly_errors do
             repo.gitaly_commit_client.between(base, head)
           end
         end
@@ -125,7 +126,7 @@ module Gitlab
         #        are documented here:
         #        http://www.rubydoc.info/github/libgit2/rugged/Rugged#SORT_NONE-constant)
         def find_all(repo, options = {})
-          repo.wrapped_gitaly_errors do
+          wrapped_gitaly_errors do
             Gitlab::GitalyClient::CommitService.new(repo).find_all_commits(options)
           end
         end
@@ -142,7 +143,7 @@ module Gitlab
         # relation to each other. The last 10 commits for a branch for example,
         # should go through .where
         def batch_by_oid(repo, oids)
-          repo.wrapped_gitaly_errors do
+          wrapped_gitaly_errors do
             repo.gitaly_commit_client.list_commits_by_oid(oids)
           end
         end
diff --git a/lib/gitlab/git/commit_stats.rb b/lib/gitlab/git/commit_stats.rb
index ae6f554bc06..83a9fd5f81a 100644
--- a/lib/gitlab/git/commit_stats.rb
+++ b/lib/gitlab/git/commit_stats.rb
@@ -3,6 +3,8 @@
 module Gitlab
   module Git
     class CommitStats
+      include Gitlab::Git::WrapsGitalyErrors
+
       attr_reader :id, :additions, :deletions, :total
 
       # Instantiate a CommitStats object
@@ -14,7 +16,7 @@ module Gitlab
         @deletions = 0
         @total = 0
 
-        repo.wrapped_gitaly_errors do
+        wrapped_gitaly_errors do
           gitaly_stats(repo, commit)
         end
       end
diff --git a/lib/gitlab/git/conflict/resolver.rb b/lib/gitlab/git/conflict/resolver.rb
index 6dc792c16b8..307f1b8cb66 100644
--- a/lib/gitlab/git/conflict/resolver.rb
+++ b/lib/gitlab/git/conflict/resolver.rb
@@ -2,6 +2,8 @@ module Gitlab
   module Git
     module Conflict
       class Resolver
+        include Gitlab::Git::WrapsGitalyErrors
+
         ConflictSideMissing = Class.new(StandardError)
         ResolutionError = Class.new(StandardError)
 
@@ -12,7 +14,7 @@ module Gitlab
         end
 
         def conflicts
-          @conflicts ||= @target_repository.wrapped_gitaly_errors do
+          @conflicts ||= wrapped_gitaly_errors do
             gitaly_conflicts_client(@target_repository).list_conflict_files.to_a
           end
         rescue GRPC::FailedPrecondition => e
@@ -22,7 +24,7 @@ module Gitlab
         end
 
         def resolve_conflicts(source_repository, resolution, source_branch:, target_branch:)
-          source_repository.wrapped_gitaly_errors do
+          wrapped_gitaly_errors do
             gitaly_conflicts_client(source_repository).resolve_conflicts(@target_repository, resolution, source_branch, target_branch)
           end
         end
diff --git a/lib/gitlab/git/remote_mirror.rb b/lib/gitlab/git/remote_mirror.rb
index e4743b4db0a..7f9520de5ce 100644
--- a/lib/gitlab/git/remote_mirror.rb
+++ b/lib/gitlab/git/remote_mirror.rb
@@ -1,13 +1,15 @@
 module Gitlab
   module Git
     class RemoteMirror
+      include Gitlab::Git::WrapsGitalyErrors
+
       def initialize(repository, ref_name)
         @repository = repository
         @ref_name = ref_name
       end
 
       def update(only_branches_matching: [])
-        @repository.wrapped_gitaly_errors do
+        wrapped_gitaly_errors do
           @repository.gitaly_remote_client.update_remote_mirror(@ref_name, only_branches_matching)
         end
       end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 9df04372cc2..fcc92341c40 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -6,6 +6,7 @@ module Gitlab
   module Git
     class Repository
       include Gitlab::Git::RepositoryMirroring
+      include Gitlab::Git::WrapsGitalyErrors
       include Gitlab::EncodingHelper
       include Gitlab::Utils::StrongMemoize
 
@@ -845,23 +846,9 @@ module Gitlab
       end
 
       def gitaly_migrate(method, status: Gitlab::GitalyClient::MigrationStatus::OPT_IN, &block)
-        Gitlab::GitalyClient.migrate(method, status: status, &block)
-      rescue GRPC::NotFound => e
-        raise NoRepository.new(e)
-      rescue GRPC::InvalidArgument => e
-        raise ArgumentError.new(e)
-      rescue GRPC::BadStatus => e
-        raise CommandError.new(e)
-      end
-
-      def wrapped_gitaly_errors(&block)
-        yield block
-      rescue GRPC::NotFound => e
-        raise NoRepository.new(e)
-      rescue GRPC::InvalidArgument => e
-        raise ArgumentError.new(e)
-      rescue GRPC::BadStatus => e
-        raise CommandError.new(e)
+        wrapped_gitaly_errors do
+          Gitlab::GitalyClient.migrate(method, status: status, &block)
+        end
       end
 
       def clean_stale_repository_files
diff --git a/lib/gitlab/git/tree.rb b/lib/gitlab/git/tree.rb
index e0867aeb5a7..b5b701699f0 100644
--- a/lib/gitlab/git/tree.rb
+++ b/lib/gitlab/git/tree.rb
@@ -2,6 +2,7 @@ module Gitlab
   module Git
     class Tree
       include Gitlab::EncodingHelper
+      extend Gitlab::Git::WrapsGitalyErrors
 
       attr_accessor :id, :root_id, :name, :path, :flat_path, :type,
         :mode, :commit_id, :submodule_url
@@ -15,7 +16,7 @@ module Gitlab
         def where(repository, sha, path = nil, recursive = false)
           path = nil if path == '' || path == '/'
 
-          repository.wrapped_gitaly_errors do
+          wrapped_gitaly_errors do
             repository.gitaly_commit_client.tree_entries(repository, sha, path, recursive)
           end
         end
diff --git a/lib/gitlab/git/wiki.rb b/lib/gitlab/git/wiki.rb
index 7fe56979d5c..02c643d0da0 100644
--- a/lib/gitlab/git/wiki.rb
+++ b/lib/gitlab/git/wiki.rb
@@ -1,6 +1,8 @@
 module Gitlab
   module Git
     class Wiki
+      include Gitlab::Git::WrapsGitalyErrors
+
       DuplicatePageError = Class.new(StandardError)
       OperationError = Class.new(StandardError)
 
@@ -65,37 +67,37 @@ module Gitlab
       end
 
       def write_page(name, format, content, commit_details)
-        @repository.wrapped_gitaly_errors do
+        wrapped_gitaly_errors do
           gitaly_write_page(name, format, content, commit_details)
         end
       end
 
       def delete_page(page_path, commit_details)
-        @repository.wrapped_gitaly_errors do
+        wrapped_gitaly_errors do
           gitaly_delete_page(page_path, commit_details)
         end
       end
 
       def update_page(page_path, title, format, content, commit_details)
-        @repository.wrapped_gitaly_errors do
+        wrapped_gitaly_errors do
           gitaly_update_page(page_path, title, format, content, commit_details)
         end
       end
 
       def pages(limit: 0)
-        @repository.wrapped_gitaly_errors do
+        wrapped_gitaly_errors do
           gitaly_get_all_pages(limit: limit)
         end
       end
 
       def page(title:, version: nil, dir: nil)
-        @repository.wrapped_gitaly_errors do
+        wrapped_gitaly_errors do
           gitaly_find_page(title: title, version: version, dir: dir)
         end
       end
 
       def file(name, version)
-        @repository.wrapped_gitaly_errors do
+        wrapped_gitaly_errors do
           gitaly_find_file(name, version)
         end
       end
@@ -105,7 +107,7 @@ module Gitlab
       #  :per_page - The number of items per page.
       #  :limit    - Total number of items to return.
       def page_versions(page_path, options = {})
-        versions = @repository.wrapped_gitaly_errors do
+        versions = wrapped_gitaly_errors do
           gitaly_wiki_client.page_versions(page_path, options)
         end
 
@@ -127,7 +129,7 @@ module Gitlab
       def page_formatted_data(title:, dir: nil, version: nil)
         version = version&.id
 
-        @repository.wrapped_gitaly_errors do
+        wrapped_gitaly_errors do
           gitaly_wiki_client.get_formatted_data(title: title, dir: dir, version: version)
         end
       end
diff --git a/lib/gitlab/git/wraps_gitaly_errors.rb b/lib/gitlab/git/wraps_gitaly_errors.rb
new file mode 100644
index 00000000000..4b161f7e6ce
--- /dev/null
+++ b/lib/gitlab/git/wraps_gitaly_errors.rb
@@ -0,0 +1,15 @@
+module Gitlab
+  module Git
+    module WrapsGitalyErrors
+      def wrapped_gitaly_errors(&block)
+        yield block
+      rescue GRPC::NotFound => e
+        raise Gitlab::Git::Repository::NoRepository.new(e)
+      rescue GRPC::InvalidArgument => e
+        raise ArgumentError.new(e)
+      rescue GRPC::BadStatus => e
+        raise Gitlab::Git::CommandError.new(e)
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/git/wraps_gitaly_errors_spec.rb b/spec/lib/gitlab/git/wraps_gitaly_errors_spec.rb
new file mode 100644
index 00000000000..bcf4814edb6
--- /dev/null
+++ b/spec/lib/gitlab/git/wraps_gitaly_errors_spec.rb
@@ -0,0 +1,28 @@
+require 'spec_helper'
+
+describe Gitlab::Git::WrapsGitalyErrors do
+  subject(:wrapper) do
+    klazz = Class.new { include Gitlab::Git::WrapsGitalyErrors }
+    klazz.new
+  end
+
+  describe "#wrapped_gitaly_errors" do
+    mapping = {
+      GRPC::NotFound => Gitlab::Git::Repository::NoRepository,
+      GRPC::InvalidArgument => ArgumentError,
+      GRPC::BadStatus => Gitlab::Git::CommandError
+    }
+
+    mapping.each do |grpc_error, error|
+      it "wraps #{grpc_error} in a #{error}" do
+        expect { wrapper.wrapped_gitaly_errors { raise grpc_error.new('wrapped') } }
+          .to raise_error(error)
+      end
+    end
+
+    it 'does not swallow other errors' do
+      expect { wrapper.wrapped_gitaly_errors { raise 'raised' } }
+        .to raise_error(RuntimeError)
+    end
+  end
+end