Merge branch 'pks-user-squash-structured-errors' into 'master'

gitaly: Adapt to structured errors returned by UserSquash

See merge request gitlab-org/gitlab!81905

lib/gitlab/gitaly_client/operation_service.rb

@@ -164,16 +164,20 @@ module Gitlab
         Gitlab::Git::OperationService::BranchUpdate.from_gitaly(branch_update)
 
       rescue GRPC::BadStatus => e
-        decoded_error = decode_detailed_error(e)
+        detailed_error = decode_detailed_error(e)
 
-        raise unless decoded_error.present?
-
-        # We simply ignore any reference update errors which are typically an
-        # indicator of multiple RPC calls trying to update the same reference
-        # at the same point in time.
-        return if decoded_error.is_a?(Gitlab::Git::ReferenceUpdateError)
-
-        raise decoded_error
+        case detailed_error&.error
+        when :access_check
+          access_check_error = detailed_error.access_check
+          # These messages were returned from internal/allowed API calls
+          raise Gitlab::Git::PreReceiveError.new(fallback_message: access_check_error.error_message)
+        when :reference_update
+          # We simply ignore any reference update errors which are typically an
+          # indicator of multiple RPC calls trying to update the same reference
+          # at the same point in time.
+        else
+          raise
+        end
       ensure
         request_enum.close
       end
@@ -295,6 +299,26 @@ module Gitlab
         end
 
         response.squash_sha
+      rescue GRPC::BadStatus => e
+        detailed_error = decode_detailed_error(e)
+
+        case detailed_error&.error
+        when :resolve_revision, :rebase_conflict
+          # Theoretically, we could now raise specific errors based on the type
+          # of the detailed error. Most importantly, we get error details when
+          # Gitaly was not able to resolve the `start_sha` or `end_sha` via a
+          # ResolveRevisionError, and we get information about which files are
+          # conflicting via a MergeConflictError.
+          #
+          # We don't do this now though such that we can maintain backwards
+          # compatibility with the minimum required set of changes during the
+          # transitory period where we're migrating UserSquash to use
+          # structured errors. We thus continue to just return a GitError, like
+          # we previously did.
+          raise Gitlab::Git::Repository::GitError, e.details
+        else
+          raise
+        end
       end
 
       def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:)
@@ -492,23 +516,7 @@ module Gitlab
         prefix = %r{type\.googleapis\.com\/gitaly\.(?<error_type>.+)}
         error_type = prefix.match(detailed_error.type_url)[:error_type]
 
-        detailed_error = Gitaly.const_get(error_type, false).decode(detailed_error.value)
-
-        case detailed_error.error
-        when :access_check
-          access_check_error = detailed_error.access_check
-          # These messages were returned from internal/allowed API calls
-          Gitlab::Git::PreReceiveError.new(fallback_message: access_check_error.error_message)
-        when :reference_update
-          reference_update_error = detailed_error.reference_update
-          Gitlab::Git::ReferenceUpdateError.new(err.details,
-                                                reference_update_error.reference_name,
-                                                reference_update_error.old_oid,
-                                                reference_update_error.new_oid)
-        else
-          # We're handling access_check only for now, but we'll add more detailed error types
-          nil
-        end
+        Gitaly.const_get(error_type, false).decode(detailed_error.value)
       rescue NameError, NoMethodError
         # Error Class might not be known to ruby yet
         nil

spec/lib/gitlab/gitaly_client/operation_service_spec.rb

@@ -437,41 +437,93 @@ RSpec.describe Gitlab::GitalyClient::OperationService do
       end
     end
 
-    describe '#user_commit_files' do
-      subject do
-        client.user_commit_files(
-          gitaly_user, 'my-branch', 'Commit files message', [], 'janedoe@example.com', 'Jane Doe',
-          'master', repository)
+    shared_examples '#user_squash with an error' do
+      it 'raises a GitError exception' do
+        expect_any_instance_of(Gitaly::OperationService::Stub)
+          .to receive(:user_squash).with(request, kind_of(Hash))
+          .and_raise(raised_error)
+
+        expect { subject }.to raise_error(expected_error)
       end
+    end
 
-      before do
-        expect_any_instance_of(Gitaly::OperationService::Stub)
-          .to receive(:user_commit_files).with(kind_of(Enumerator), kind_of(Hash))
-          .and_return(response)
+    context 'when ResolveRevisionError is raised' do
+      let(:raised_error) do
+        new_detailed_error(
+          GRPC::Core::StatusCodes::INVALID_ARGUMENT,
+          'something failed',
+          Gitaly::UserSquashError.new(
+            resolve_revision: Gitaly::ResolveRevisionError.new(
+              revision: start_sha
+            )))
       end
 
-      context 'when a pre_receive_error is present' do
-        let(:response) { Gitaly::UserCommitFilesResponse.new(pre_receive_error: "GitLab: something failed") }
+      let(:expected_error) { Gitlab::Git::Repository::GitError }
 
-        it 'raises a PreReceiveError' do
-          expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
-        end
+      it_behaves_like '#user_squash with an error'
+    end
+
+    context 'when RebaseConflictError is raised' do
+      let(:raised_error) do
+        new_detailed_error(
+          GRPC::Core::StatusCodes::INTERNAL,
+          'something failed',
+          Gitaly::UserSquashError.new(
+            rebase_conflict: Gitaly::MergeConflictError.new(
+              conflicting_files: ['conflicting-file']
+            )))
       end
 
-      context 'when an index_error is present' do
-        let(:response) { Gitaly::UserCommitFilesResponse.new(index_error: "something failed") }
+      let(:expected_error) { Gitlab::Git::Repository::GitError }
 
-        it 'raises a PreReceiveError' do
-          expect { subject }.to raise_error(Gitlab::Git::Index::IndexError, "something failed")
-        end
+      it_behaves_like '#user_squash with an error'
+    end
+
+    context 'when non-detailed gRPC error is raised' do
+      let(:raised_error) do
+        GRPC::Internal.new('non-detailed error')
       end
 
-      context 'when branch_update is nil' do
-        let(:response) { Gitaly::UserCommitFilesResponse.new }
+      let(:expected_error) { GRPC::Internal }
 
-        it { expect(subject).to be_nil }
+      it_behaves_like '#user_squash with an error'
+    end
+  end
+
+  describe '#user_commit_files' do
+    subject do
+      client.user_commit_files(
+        gitaly_user, 'my-branch', 'Commit files message', [], 'janedoe@example.com', 'Jane Doe',
+        'master', repository)
+    end
+
+    before do
+      expect_any_instance_of(Gitaly::OperationService::Stub)
+        .to receive(:user_commit_files).with(kind_of(Enumerator), kind_of(Hash))
+        .and_return(response)
+    end
+
+    context 'when a pre_receive_error is present' do
+      let(:response) { Gitaly::UserCommitFilesResponse.new(pre_receive_error: "GitLab: something failed") }
+
+      it 'raises a PreReceiveError' do
+        expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
       end
     end
+
+    context 'when an index_error is present' do
+      let(:response) { Gitaly::UserCommitFilesResponse.new(index_error: "something failed") }
+
+      it 'raises a PreReceiveError' do
+        expect { subject }.to raise_error(Gitlab::Git::Index::IndexError, "something failed")
+      end
+    end
+
+    context 'when branch_update is nil' do
+      let(:response) { Gitaly::UserCommitFilesResponse.new }
+
+      it { expect(subject).to be_nil }
+    end
   end
 
   describe '#user_commit_patches' do