Refactoring Projects::ImportService

app/models/repository.rb

@@ -402,10 +402,6 @@ class Repository
     expire_tags_cache
   end
 
-  def before_import
-    expire_content_cache
-  end
-
   # Runs code after the HEAD of a repository is changed.
   def after_change_head
     expire_method_caches(METHOD_CACHES_FOR_FILE_TYPES.keys)

app/services/projects/import_service.rb

@@ -11,7 +11,7 @@ module Projects
 
       success
     rescue => e
-      error(e.message)
+      error("Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}")
     end
 
     private
@@ -21,13 +21,9 @@ module Projects
         # In this case, we only want to import issues, not a repository.
         create_repository
       elsif !project.repository_exists?
-        if project.github_import? || project.gitea_import?
-          fetch_repository
-        else
         import_repository
       end
     end
-    end
 
     def create_repository
       unless project.create_repository
@@ -36,42 +32,40 @@ module Projects
     end
 
     def import_repository
+      raise Error, 'Blocked import URL.' if Gitlab::UrlBlocker.blocked_url?(project.import_url)
+
       begin
-        raise Error, "Blocked import URL." if Gitlab::UrlBlocker.blocked_url?(project.import_url)
-        gitlab_shell.import_repository(project.repository_storage_path, project.path_with_namespace, project.import_url)
-      rescue => e
+        if project.github_import? || project.gitea_import?
+          fetch_repository
+        else
+          clone_repository
+        end
+      rescue Gitlab::Shell::Error => e
         # Expire cache to prevent scenarios such as:
         # 1. First import failed, but the repo was imported successfully, so +exists?+ returns true
         # 2. Retried import, repo is broken or not imported but +exists?+ still returns true
-        project.repository.before_import if project.repository_exists?
+        project.repository.expire_content_cache if project.repository_exists?
 
-        raise Error, "Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}"
+        raise Error, e.message
       end
     end
 
-    def fetch_repository
-      begin
-        raise Error, 'Blocked import URL.' if Gitlab::UrlBlocker.blocked_url?(project.import_url)
+    def clone_repository
+      gitlab_shell.import_repository(project.repository_storage_path, project.path_with_namespace, project.import_url)
+    end
 
+    def fetch_repository
       project.create_repository
       project.repository.add_remote(project.import_type, project.import_url)
       project.repository.set_remote_as_mirror (project.import_type)
       project.repository.fetch_remote(project.import_type, forced: true)
       project.repository.remove_remote(project.import_type)
-      rescue => e
-        # Expire cache to prevent scenarios such as:
-        # 1. First import failed, but the repo was imported successfully, so +exists?+ returns true
-        # 2. Retried import, repo is broken or not imported but +exists?+ still returns true
-        project.repository.before_import if project.repository_exists?
-
-        raise Error, "Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}"
-      end
     end
 
     def import_data
       return unless has_importer?
 
-      project.repository.before_import unless project.gitlab_project_import?
+      project.repository.expire_content_cache unless project.gitlab_project_import?
 
       unless importer.execute
         raise Error, 'The remote data could not be imported.'

spec/models/repository_spec.rb

@@ -1293,14 +1293,6 @@ describe Repository, models: true do
     end
   end
 
-  describe '#before_import' do
-    it 'flushes the repository caches' do
-      expect(repository).to receive(:expire_content_cache)
-
-      repository.before_import
-    end
-  end
-
   describe '#after_import' do
     it 'flushes and builds the cache' do
       expect(repository).to receive(:expire_content_cache)

spec/services/projects/import_service_spec.rb

@@ -26,17 +26,45 @@ describe Projects::ImportService, services: true do
         result = subject.execute
 
         expect(result[:status]).to eq :error
-        expect(result[:message]).to eq 'The repository could not be created.'
+        expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - The repository could not be created."
       end
     end
 
     context 'with known url' do
       before do
         project.import_url = 'https://github.com/vim/vim.git'
+        project.import_type = 'github'
+      end
+
+      context 'with a Github repository' do
+        it 'succeeds if repository import is successfully' do
+          expect_any_instance_of(Repository).to receive(:fetch_remote).and_return(true)
+          expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true)
+
+          result = subject.execute
+
+          expect(result[:status]).to eq :success
+        end
+
+        it 'fails if repository import fails' do
+          expect_any_instance_of(Repository).to receive(:fetch_remote).and_raise(Gitlab::Shell::Error.new('Failed to import the repository'))
+
+          result = subject.execute
+
+          expect(result[:status]).to eq :error
+          expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Failed to import the repository"
+        end
+      end
+
+      context 'with a non Github repository' do
+        before do
+          project.import_url = 'https://bitbucket.org/vim/vim.git'
+          project.import_type = 'bitbucket'
         end
 
         it 'succeeds if repository import is successfully' do
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true)
+          expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_return(true)
+          expect_any_instance_of(Gitlab::BitbucketImport::Importer).to receive(:execute).and_return(true)
 
           result = subject.execute
 
@@ -44,7 +72,7 @@ describe Projects::ImportService, services: true do
         end
 
         it 'fails if repository import fails' do
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_raise(Gitlab::Shell::Error.new('Failed to import the repository'))
+          expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error.new('Failed to import the repository'))
 
           result = subject.execute
 
@@ -52,6 +80,7 @@ describe Projects::ImportService, services: true do
           expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Failed to import the repository"
         end
       end
+    end
 
     context 'with valid importer' do
       before do
@@ -64,8 +93,8 @@ describe Projects::ImportService, services: true do
       end
 
       it 'succeeds if importer succeeds' do
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true)
-        expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true)
+        allow_any_instance_of(Repository).to receive(:fetch_remote).and_return(true)
+        allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true)
 
         result = subject.execute
 
@@ -73,48 +102,42 @@ describe Projects::ImportService, services: true do
       end
 
       it 'flushes various caches' do
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).
-          with(project.repository_storage_path, project.path_with_namespace, project.import_url).
+        allow_any_instance_of(Repository).to receive(:fetch_remote).
           and_return(true)
 
-        expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).
+        allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).
           and_return(true)
 
-        expect_any_instance_of(Repository).to receive(:expire_emptiness_caches).
-          and_call_original
-
-        expect_any_instance_of(Repository).to receive(:expire_exists_cache).
-          and_call_original
+        expect_any_instance_of(Repository).to receive(:expire_content_cache)
 
         subject.execute
       end
 
       it 'fails if importer fails' do
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true)
-        expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(false)
+        allow_any_instance_of(Repository).to receive(:fetch_remote).and_return(true)
+        allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(false)
 
         result = subject.execute
 
         expect(result[:status]).to eq :error
-        expect(result[:message]).to eq 'The remote data could not be imported.'
+        expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - The remote data could not be imported."
       end
 
       it 'fails if importer raise an error' do
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true)
-        expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_raise(Projects::ImportService::Error.new('Github: failed to connect API'))
+        allow_any_instance_of(Gitlab::Shell).to receive(:fetch_remote).and_return(true)
+        allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_raise(Projects::ImportService::Error.new('Github: failed to connect API'))
 
         result = subject.execute
 
         expect(result[:status]).to eq :error
-        expect(result[:message]).to eq 'Github: failed to connect API'
+        expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Github: failed to connect API"
       end
 
-      it 'expires existence cache after error' do
+      it 'expires content cache after error' do
         allow_any_instance_of(Project).to receive(:repository_exists?).and_return(false, true)
 
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_raise(Gitlab::Shell::Error.new('Failed to import the repository'))
-        expect_any_instance_of(Repository).to receive(:expire_emptiness_caches).and_call_original
-        expect_any_instance_of(Repository).to receive(:expire_exists_cache).and_call_original
+        expect_any_instance_of(Gitlab::Shell).to receive(:fetch_remote).and_raise(Gitlab::Shell::Error.new('Failed to import the repository'))
+        expect_any_instance_of(Repository).to receive(:expire_content_cache)
 
         subject.execute
       end