Merge branch 'sh-fix-bitbucket-server-error-handling' into 'master'

Fix Bitbucket Server importer error handling

Closes #56154

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

7 files changed, 43 insertions, 23 deletions

diff --git a/app/controllers/import/bitbucket_server_controller.rb b/app/controllers/import/bitbucket_server_controller.rb
index 575c40d5f6f..87338488eba 100644
--- a/app/controllers/import/bitbucket_server_controller.rb
+++ b/app/controllers/import/bitbucket_server_controller.rb
@@ -40,7 +40,7 @@ class Import::BitbucketServerController < Import::BaseController
     else
       render json: { errors: 'This namespace has already been taken! Please choose another one.' }, status: :unprocessable_entity
     end
-  rescue BitbucketServer::Client::ServerError => e
+  rescue BitbucketServer::Connection::ConnectionError => e
     render json: { errors: "Unable to connect to server: #{e}" }, status: :unprocessable_entity
   end
 
@@ -62,7 +62,7 @@ class Import::BitbucketServerController < Import::BaseController
     already_added_projects_names = @already_added_projects.pluck(:import_source)
 
     @repos.reject! { |repo| already_added_projects_names.include?(repo.browse_url) }
-  rescue BitbucketServer::Connection::ConnectionError, BitbucketServer::Client::ServerError => e
+  rescue BitbucketServer::Connection::ConnectionError => e
     flash[:alert] = "Unable to connect to server: #{e}"
     clear_session_data
     redirect_to new_import_bitbucket_server_path
diff --git a/changelogs/unreleased/sh-fix-bitbucket-server-error-handling.yml b/changelogs/unreleased/sh-fix-bitbucket-server-error-handling.yml
new file mode 100644
index 00000000000..87405fa0a78
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-bitbucket-server-error-handling.yml
@@ -0,0 +1,5 @@
+---
+title: Fix Bitbucket Server importer error handling
+merge_request: 24343
+author:
+type: fixed
diff --git a/lib/bitbucket_server/client.rb b/lib/bitbucket_server/client.rb
index 83e8808db07..6a608058813 100644
--- a/lib/bitbucket_server/client.rb
+++ b/lib/bitbucket_server/client.rb
@@ -4,18 +4,6 @@ module BitbucketServer
   class Client
     attr_reader :connection
 
-    ServerError = Class.new(StandardError)
-
-    SERVER_ERRORS = [SocketError,
-                     OpenSSL::SSL::SSLError,
-                     Errno::ECONNRESET,
-                     Errno::ECONNREFUSED,
-                     Errno::EHOSTUNREACH,
-                     Net::OpenTimeout,
-                     Net::ReadTimeout,
-                     Gitlab::HTTP::BlockedUrlError,
-                     BitbucketServer::Connection::ConnectionError].freeze
-
     def initialize(options = {})
       @connection = Connection.new(options)
     end
@@ -64,8 +52,6 @@ module BitbucketServer
     def get_collection(path, type, page_offset: 0, limit: nil)
       paginator = BitbucketServer::Paginator.new(connection, Addressable::URI.escape(path), type, page_offset: page_offset, limit: limit)
       BitbucketServer::Collection.new(paginator)
-    rescue *SERVER_ERRORS => e
-      raise ServerError, e
     end
   end
 end
diff --git a/lib/bitbucket_server/connection.rb b/lib/bitbucket_server/connection.rb
index 7efcdcf8619..9c14b26c65a 100644
--- a/lib/bitbucket_server/connection.rb
+++ b/lib/bitbucket_server/connection.rb
@@ -7,6 +7,17 @@ module BitbucketServer
     DEFAULT_API_VERSION = '1.0'
     SEPARATOR = '/'
 
+    NETWORK_ERRORS = [
+      SocketError,
+      OpenSSL::SSL::SSLError,
+      Errno::ECONNRESET,
+      Errno::ECONNREFUSED,
+      Errno::EHOSTUNREACH,
+      Net::OpenTimeout,
+      Net::ReadTimeout,
+      Gitlab::HTTP::BlockedUrlError
+    ].freeze
+
     attr_reader :api_version, :base_uri, :username, :token
 
     ConnectionError = Class.new(StandardError)
@@ -27,6 +38,8 @@ module BitbucketServer
       check_errors!(response)
 
       response.parsed_response
+    rescue *NETWORK_ERRORS => e
+      raise ConnectionError, e
     end
 
     def post(path, body)
@@ -38,6 +51,8 @@ module BitbucketServer
       check_errors!(response)
 
       response.parsed_response
+    rescue *NETWORK_ERRORS => e
+      raise ConnectionError, e
     end
 
     # We need to support two different APIs for deletion:
@@ -55,6 +70,8 @@ module BitbucketServer
       check_errors!(response)
 
       response.parsed_response
+    rescue *NETWORK_ERRORS => e
+      raise ConnectionError, e
     end
 
     private
diff --git a/spec/controllers/import/bitbucket_server_controller_spec.rb b/spec/controllers/import/bitbucket_server_controller_spec.rb
index 73195463a50..bb282db5a41 100644
--- a/spec/controllers/import/bitbucket_server_controller_spec.rb
+++ b/spec/controllers/import/bitbucket_server_controller_spec.rb
@@ -78,7 +78,7 @@ describe Import::BitbucketServerController do
     end
 
     it "returns an error when the server can't be contacted" do
-      expect(client).to receive(:repo).with(project_key, repo_slug).and_raise(BitbucketServer::Client::ServerError)
+      expect(client).to receive(:repo).with(project_key, repo_slug).and_raise(::BitbucketServer::Connection::ConnectionError)
 
       post :create, params: { project: project_key, repository: repo_slug }, format: :json
 
diff --git a/spec/lib/bitbucket_server/client_spec.rb b/spec/lib/bitbucket_server/client_spec.rb
index 5de0a9a65b5..b021e69800a 100644
--- a/spec/lib/bitbucket_server/client_spec.rb
+++ b/spec/lib/bitbucket_server/client_spec.rb
@@ -17,12 +17,6 @@ describe BitbucketServer::Client do
 
       subject.pull_requests(project, repo_slug)
     end
-
-    it 'throws an exception when connection fails' do
-      allow(BitbucketServer::Collection).to receive(:new).and_raise(OpenSSL::SSL::SSLError)
-
-      expect { subject.pull_requests(project, repo_slug) }.to raise_error(described_class::ServerError)
-    end
   end
 
   describe '#activities' do
diff --git a/spec/lib/bitbucket_server/connection_spec.rb b/spec/lib/bitbucket_server/connection_spec.rb
index b5da4cb1a49..ab8a5b89608 100644
--- a/spec/lib/bitbucket_server/connection_spec.rb
+++ b/spec/lib/bitbucket_server/connection_spec.rb
@@ -26,6 +26,12 @@ describe BitbucketServer::Connection do
 
       expect { subject.get(url) }.to raise_error(described_class::ConnectionError)
     end
+
+    it 'throws an exception upon a network error' do
+      WebMock.stub_request(:get, url).with(headers: { 'Accept' => 'application/json' }).to_raise(OpenSSL::SSL::SSLError)
+
+      expect { subject.get(url) }.to raise_error(described_class::ConnectionError)
+    end
   end
 
   describe '#post' do
@@ -42,6 +48,12 @@ describe BitbucketServer::Connection do
 
       expect { subject.post(url, payload) }.to raise_error(described_class::ConnectionError)
     end
+
+    it 'throws an exception upon a network error' do
+      WebMock.stub_request(:post, url).with(headers: { 'Accept' => 'application/json' }).to_raise(OpenSSL::SSL::SSLError)
+
+      expect { subject.post(url, payload) }.to raise_error(described_class::ConnectionError)
+    end
   end
 
   describe '#delete' do
@@ -63,6 +75,12 @@ describe BitbucketServer::Connection do
 
         expect { subject.delete(:branches, branch_path, payload) }.to raise_error(described_class::ConnectionError)
       end
+
+      it 'throws an exception upon a network error' do
+        WebMock.stub_request(:delete, branch_url).with(headers: headers).to_raise(OpenSSL::SSL::SSLError)
+
+        expect { subject.delete(:branches, branch_path, payload) }.to raise_error(described_class::ConnectionError)
+      end
     end
   end
 end