Fix Bitbucket Server importer error handling

The importer would display a 500 error page if you attempted to import
using a non-existent DNS entry. This commit rescues known network issues
and consolidates them into
BitbucketServer::Connection::ConnectionError`.  The previous error
handling in the paginator doesn't work because it returns a lazy
collection.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/56154

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

changelogs/unreleased/sh-fix-bitbucket-server-error-handling.yml

+---
+title: Fix Bitbucket Server importer error handling
+merge_request: 24343
+author:
+type: fixed

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

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

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
 

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

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