Refactored LfsImportService and ImportServicefj-refactor-lfs-import-code

In order to make `LfsImportService` more reusable, we
need to extract the logic inside `ImportService` and
encapsulate it into the service.

11 files changed, 324 insertions, 233 deletions

diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb
index 7214e9efaf6..50754d4cdac 100644
--- a/app/services/projects/import_service.rb
+++ b/app/services/projects/import_service.rb
@@ -94,16 +94,13 @@ module Projects
 
       return unless project.lfs_enabled?
 
-      lfs_objects_to_download = Projects::LfsPointers::LfsImportService.new(project).execute
+      result = Projects::LfsPointers::LfsImportService.new(project).execute
 
-      lfs_objects_to_download.each do |lfs_download_object|
-        Projects::LfsPointers::LfsDownloadService.new(project, lfs_download_object)
-                                                 .execute
+      if result[:status] == :error
+        # To avoid aborting the importing process, we silently fail
+        # if any exception raises.
+        Gitlab::AppLogger.error("The Lfs import process failed. #{result[:message]}")
       end
-    rescue => e
-      # Right now, to avoid aborting the importing process, we silently fail
-      # if any exception raises.
-      Rails.logger.error("The Lfs import process failed. #{e.message}")
     end
 
     def import_data
diff --git a/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb b/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb
index a9570176e81..954dd4a95a3 100644
--- a/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb
+++ b/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb
@@ -21,9 +21,9 @@ module Projects
       # This method accepts two parameters:
       # - oids: hash of oids to query. The structure is { lfs_file_oid => lfs_file_size }
       #
-      # Returns a hash with the structure { lfs_file_oids => download_link }
+      # Returns a array of LfsDownloadObject
       def execute(oids)
-        return {} unless project&.lfs_enabled? && remote_uri && oids.present?
+        return [] unless project&.lfs_enabled? && remote_uri && oids.present?
 
         get_download_links(oids)
       end
diff --git a/app/services/projects/lfs_pointers/lfs_import_service.rb b/app/services/projects/lfs_pointers/lfs_import_service.rb
index 9215fa0a7bf..d19a59b852b 100644
--- a/app/services/projects/lfs_pointers/lfs_import_service.rb
+++ b/app/services/projects/lfs_pointers/lfs_import_service.rb
@@ -1,95 +1,22 @@
 # frozen_string_literal: true
 
-# This service manages the whole worflow of discovering the Lfs files in a
-# repository, linking them to the project and downloading (and linking) the non
-# existent ones.
+# This service is responbible of managing the retrieval of the lfs objects
+# to download and perform the download.
 module Projects
   module LfsPointers
     class LfsImportService < BaseService
-      include Gitlab::Utils::StrongMemoize
-
-      HEAD_REV = 'HEAD'.freeze
-      LFS_ENDPOINT_PATTERN = /^\t?url\s*=\s*(.+)$/.freeze
-      LFS_BATCH_API_ENDPOINT = '/info/lfs/objects/batch'.freeze
-
-      LfsImportError = Class.new(StandardError)
-
       def execute
-        return {} unless project&.lfs_enabled?
+        return success unless project&.lfs_enabled?
 
-        if external_lfs_endpoint?
-          # If the endpoint host is different from the import_url it means
-          # that the repo is using a third party service for storing the LFS files.
-          # In this case, we have to disable lfs in the project
-          disable_lfs!
+        lfs_objects_to_download = LfsObjectDownloadListService.new(project).execute
 
-          return {}
+        lfs_objects_to_download.each do |lfs_download_object|
+          LfsDownloadService.new(project, lfs_download_object).execute
         end
 
-        get_download_links
-      rescue LfsDownloadLinkListService::DownloadLinksError => e
-        raise LfsImportError, "The LFS objects download list couldn't be imported. Error: #{e.message}"
-      end
-
-      private
-
-      def external_lfs_endpoint?
-        lfsconfig_endpoint_uri && lfsconfig_endpoint_uri.host != import_uri.host
-      end
-
-      def disable_lfs!
-        project.update(lfs_enabled: false)
-      end
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def get_download_links
-        existent_lfs = LfsListService.new(project).execute
-        linked_oids = LfsLinkService.new(project).execute(existent_lfs.keys)
-
-        # Retrieving those oids not linked and which we need to download
-        not_linked_lfs = existent_lfs.except(*linked_oids)
-
-        LfsDownloadLinkListService.new(project, remote_uri: current_endpoint_uri).execute(not_linked_lfs)
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-
-      def lfsconfig_endpoint_uri
-        strong_memoize(:lfsconfig_endpoint_uri) do
-          # Retrieveing the blob data from the .lfsconfig file
-          data = project.repository.lfsconfig_for(HEAD_REV)
-          # Parsing the data to retrieve the url
-          parsed_data = data&.match(LFS_ENDPOINT_PATTERN)
-
-          if parsed_data
-            URI.parse(parsed_data[1]).tap do |endpoint|
-              endpoint.user ||= import_uri.user
-              endpoint.password ||= import_uri.password
-            end
-          end
-        end
-      rescue URI::InvalidURIError
-        raise LfsImportError, 'Invalid URL in .lfsconfig file'
-      end
-
-      def import_uri
-        @import_uri ||= URI.parse(project.import_url)
-      rescue URI::InvalidURIError
-        raise LfsImportError, 'Invalid project import URL'
-      end
-
-      def current_endpoint_uri
-        (lfsconfig_endpoint_uri || default_endpoint_uri)
-      end
-
-      # The import url must end with '.git' here we ensure it is
-      def default_endpoint_uri
-        @default_endpoint_uri ||= begin
-          import_uri.dup.tap do |uri|
-            path = uri.path.gsub(%r(/$), '')
-            path += '.git' unless path.ends_with?('.git')
-            uri.path = path + LFS_BATCH_API_ENDPOINT
-          end
-        end
+        success
+      rescue => e
+        error(e.message)
       end
     end
   end
diff --git a/app/services/projects/lfs_pointers/lfs_link_service.rb b/app/services/projects/lfs_pointers/lfs_link_service.rb
index 8401f3d1d89..e3c956250f0 100644
--- a/app/services/projects/lfs_pointers/lfs_link_service.rb
+++ b/app/services/projects/lfs_pointers/lfs_link_service.rb
@@ -6,9 +6,9 @@ module Projects
     class LfsLinkService < BaseService
       # Accept an array of oids to link
       #
-      # Returns a hash with the same structure with oids linked
+      # Returns an array with the oid of the existent lfs objects
       def execute(oids)
-        return {} unless project&.lfs_enabled?
+        return [] unless project&.lfs_enabled?
 
         # Search and link existing LFS Object
         link_existing_lfs_objects(oids)
diff --git a/app/services/projects/lfs_pointers/lfs_object_download_list_service.rb b/app/services/projects/lfs_pointers/lfs_object_download_list_service.rb
new file mode 100644
index 00000000000..d2416f3553a
--- /dev/null
+++ b/app/services/projects/lfs_pointers/lfs_object_download_list_service.rb
@@ -0,0 +1,98 @@
+# frozen_string_literal: true
+
+# This service manages the whole worflow of discovering the Lfs files in a
+# repository, linking them to the project and downloading (and linking) the non
+# existent ones.
+module Projects
+  module LfsPointers
+    class LfsObjectDownloadListService < BaseService
+      include Gitlab::Utils::StrongMemoize
+
+      HEAD_REV = 'HEAD'.freeze
+      LFS_ENDPOINT_PATTERN = /^\t?url\s*=\s*(.+)$/.freeze
+      LFS_BATCH_API_ENDPOINT = '/info/lfs/objects/batch'.freeze
+
+      LfsObjectDownloadListError = Class.new(StandardError)
+
+      def execute
+        return [] unless project&.lfs_enabled?
+
+        if external_lfs_endpoint?
+          # If the endpoint host is different from the import_url it means
+          # that the repo is using a third party service for storing the LFS files.
+          # In this case, we have to disable lfs in the project
+          disable_lfs!
+
+          return []
+        end
+
+        get_download_links
+      rescue LfsDownloadLinkListService::DownloadLinksError => e
+        raise LfsObjectDownloadListError, "The LFS objects download list couldn't be imported. Error: #{e.message}"
+      end
+
+      private
+
+      def external_lfs_endpoint?
+        lfsconfig_endpoint_uri && lfsconfig_endpoint_uri.host != import_uri.host
+      end
+
+      def disable_lfs!
+        unless project.update(lfs_enabled: false)
+          raise LfsDownloadLinkListService::DownloadLinksError, "Invalid project state"
+        end
+      end
+
+      def get_download_links
+        LfsDownloadLinkListService.new(project, remote_uri: current_endpoint_uri).execute(oids_to_download)
+      end
+
+      def oids_to_download
+        existent_lfs = LfsListService.new(project).execute
+        linked_oids = LfsLinkService.new(project).execute(existent_lfs.keys)
+
+        # Retrieving those oids not linked and which we need to download
+        existent_lfs.except(*linked_oids) # rubocop: disable CodeReuse/ActiveRecord
+      end
+
+      def lfsconfig_endpoint_uri
+        strong_memoize(:lfsconfig_endpoint_uri) do
+          # Retrieveing the blob data from the .lfsconfig file
+          data = project.repository.lfsconfig_for(HEAD_REV)
+          # Parsing the data to retrieve the url
+          parsed_data = data&.match(LFS_ENDPOINT_PATTERN)
+
+          if parsed_data
+            URI.parse(parsed_data[1]).tap do |endpoint|
+              endpoint.user ||= import_uri.user
+              endpoint.password ||= import_uri.password
+            end
+          end
+        end
+      rescue URI::InvalidURIError
+        raise LfsObjectDownloadListError, 'Invalid URL in .lfsconfig file'
+      end
+
+      def import_uri
+        @import_uri ||= URI.parse(project.import_url)
+      rescue URI::InvalidURIError
+        raise LfsObjectDownloadListError, 'Invalid project import URL'
+      end
+
+      def current_endpoint_uri
+        (lfsconfig_endpoint_uri || default_endpoint_uri)
+      end
+
+      # The import url must end with '.git' here we ensure it is
+      def default_endpoint_uri
+        @default_endpoint_uri ||= begin
+          import_uri.dup.tap do |uri|
+            path = uri.path.gsub(%r(/$), '')
+            path += '.git' unless path.ends_with?('.git')
+            uri.path = path + LFS_BATCH_API_ENDPOINT
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb
index 7f233a52f50..d485aba6a69 100644
--- a/spec/services/projects/import_service_spec.rb
+++ b/spec/services/projects/import_service_spec.rb
@@ -5,15 +5,11 @@ require 'spec_helper'
 describe Projects::ImportService do
   let!(:project) { create(:project) }
   let(:user) { project.creator }
-  let(:import_url) { 'http://www.gitlab.com/demo/repo.git' }
-  let(:oid_download_links) { { 'oid1' => "#{import_url}/gitlab-lfs/objects/oid1", 'oid2' => "#{import_url}/gitlab-lfs/objects/oid2" } }
 
   subject { described_class.new(project, user) }
 
   before do
     allow(project).to receive(:lfs_enabled?).and_return(true)
-    allow_any_instance_of(Projects::LfsPointers::LfsDownloadService).to receive(:execute)
-    allow_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(oid_download_links)
   end
 
   describe '#async?' do
@@ -77,7 +73,6 @@ describe Projects::ImportService do
       context 'when repository creation succeeds' do
         it 'does not download lfs files' do
           expect_any_instance_of(Projects::LfsPointers::LfsImportService).not_to receive(:execute)
-          expect_any_instance_of(Projects::LfsPointers::LfsDownloadService).not_to receive(:execute)
 
           subject.execute
         end
@@ -114,7 +109,6 @@ describe Projects::ImportService do
         context 'when repository import scheduled' do
           it 'does not download lfs objects' do
             expect_any_instance_of(Projects::LfsPointers::LfsImportService).not_to receive(:execute)
-            expect_any_instance_of(Projects::LfsPointers::LfsDownloadService).not_to receive(:execute)
 
             subject.execute
           end
@@ -130,7 +124,7 @@ describe Projects::ImportService do
         it 'succeeds if repository import is successful' do
           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)
-          expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return({})
+          expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(status: :success)
 
           result = subject.execute
 
@@ -146,6 +140,18 @@ describe Projects::ImportService do
           expect(result[:message]).to eq "Error importing repository #{project.safe_import_url} into #{project.full_path} - Failed to import the repository [FILTERED]"
         end
 
+        context 'when lfs import fails' do
+          it 'logs the error' do
+            error_message = 'error message'
+            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)
+            expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(status: :error, message: error_message)
+            expect(Gitlab::AppLogger).to receive(:error).with("The Lfs import process failed. #{error_message}")
+
+            subject.execute
+          end
+        end
+
         context 'when repository import scheduled' do
           before do
             allow_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_return(true)
@@ -155,10 +161,7 @@ describe Projects::ImportService do
           it 'downloads lfs objects if lfs_enabled is enabled for project' do
             allow(project).to receive(:lfs_enabled?).and_return(true)
 
-            service = double
-            expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(oid_download_links)
-            expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).and_return(service).twice
-            expect(service).to receive(:execute).twice
+            expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute)
 
             subject.execute
           end
@@ -166,7 +169,6 @@ describe Projects::ImportService do
           it 'does not download lfs objects if lfs_enabled is not enabled for project' do
             allow(project).to receive(:lfs_enabled?).and_return(false)
             expect_any_instance_of(Projects::LfsPointers::LfsImportService).not_to receive(:execute)
-            expect_any_instance_of(Projects::LfsPointers::LfsDownloadService).not_to receive(:execute)
 
             subject.execute
           end
@@ -208,7 +210,6 @@ describe Projects::ImportService do
           allow(Gitlab::GithubImport::ParallelImporter).to receive(:imports_repository?).and_return(true)
 
           expect_any_instance_of(Projects::LfsPointers::LfsImportService).not_to receive(:execute)
-          expect_any_instance_of(Projects::LfsPointers::LfsDownloadService).not_to receive(:execute)
 
           subject.execute
         end
@@ -216,13 +217,21 @@ describe Projects::ImportService do
         it 'does not have a custom repository importer downloads lfs objects' do
           allow(Gitlab::GithubImport::ParallelImporter).to receive(:imports_repository?).and_return(false)
 
-          service = double
-          expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(oid_download_links)
-          expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).and_return(service).twice
-          expect(service).to receive(:execute).twice
+          expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute)
 
           subject.execute
         end
+
+        context 'when lfs import fails' do
+          it 'logs the error' do
+            error_message = 'error message'
+            allow(Gitlab::GithubImport::ParallelImporter).to receive(:imports_repository?).and_return(false)
+            expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(status: :error, message: error_message)
+            expect(Gitlab::AppLogger).to receive(:error).with("The Lfs import process failed. #{error_message}")
+
+            subject.execute
+          end
+        end
       end
     end
 
diff --git a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb
index f1c0f5b9576..ea254b368ae 100644
--- a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb
+++ b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb
@@ -1,5 +1,4 @@
 # frozen_string_literal: true
-
 require 'spec_helper'
 
 describe Projects::LfsPointers::LfsDownloadLinkListService do
diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb
index cde3f2d6155..f4470b50753 100644
--- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb
+++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb
@@ -1,5 +1,4 @@
 # frozen_string_literal: true
-
 require 'spec_helper'
 
 describe Projects::LfsPointers::LfsDownloadService do
diff --git a/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb
index 5c9ca99df7c..7ca20a6d751 100644
--- a/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb
+++ b/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb
@@ -1,148 +1,63 @@
 # frozen_string_literal: true
-
 require 'spec_helper'
 
 describe Projects::LfsPointers::LfsImportService do
+  let(:project) { create(:project) }
+  let(:user) { project.creator }
   let(:import_url) { 'http://www.gitlab.com/demo/repo.git' }
-  let(:default_endpoint) { "#{import_url}/info/lfs/objects/batch"}
-  let(:group) { create(:group, lfs_enabled: true)}
-  let!(:project) { create(:project, namespace: group, import_url: import_url, lfs_enabled: true) }
-  let!(:lfs_objects_project) { create_list(:lfs_objects_project, 2, project: project) }
-  let!(:existing_lfs_objects) { LfsObject.pluck(:oid, :size).to_h }
-  let(:oids) { { 'oid1' => 123, 'oid2' => 125 } }
   let(:oid_download_links) { { 'oid1' => "#{import_url}/gitlab-lfs/objects/oid1", 'oid2' => "#{import_url}/gitlab-lfs/objects/oid2" } }
-  let(:all_oids) { existing_lfs_objects.merge(oids) }
-  let(:remote_uri) { URI.parse(lfs_endpoint) }
-
-  subject { described_class.new(project) }
-
-  before do
-    allow(project.repository).to receive(:lfsconfig_for).and_return(nil)
-    allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
-    allow_any_instance_of(Projects::LfsPointers::LfsListService).to receive(:execute).and_return(all_oids)
-  end
-
-  describe '#execute' do
-    context 'when no lfs pointer is linked' do
-      before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return([])
-        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
-        expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)).and_call_original
-      end
-
-      it 'retrieves all lfs pointers in the project repository' do
-        expect_any_instance_of(Projects::LfsPointers::LfsListService).to receive(:execute)
-
-        subject.execute
-      end
-
-      it 'links existent lfs objects to the project' do
-        expect_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute)
-
-        subject.execute
-      end
 
-      it 'retrieves the download links of non existent objects' do
-        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids)
+  subject { described_class.new(project, user) }
 
-        subject.execute
-      end
+  context 'when lfs is enabled for the project' do
+    before do
+      allow(project).to receive(:lfs_enabled?).and_return(true)
     end
 
-    context 'when some lfs objects are linked' do
-      before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(existing_lfs_objects.keys)
-        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
-      end
+    it 'downloads lfs objects' do
+      service = double
+      expect_any_instance_of(Projects::LfsPointers::LfsObjectDownloadListService).to receive(:execute).and_return(oid_download_links)
+      expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).and_return(service).twice
+      expect(service).to receive(:execute).twice
 
-      it 'retrieves the download links of non existent objects' do
-        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(oids)
+      result = subject.execute
 
-        subject.execute
-      end
+      expect(result[:status]).to eq :success
     end
 
-    context 'when all lfs objects are linked' do
-      before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(all_oids.keys)
-        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute)
-      end
+    context 'when no downloadable lfs object links' do
+      it 'does not call LfsDownloadService' do
+        expect_any_instance_of(Projects::LfsPointers::LfsObjectDownloadListService).to receive(:execute).and_return({})
+        expect(Projects::LfsPointers::LfsDownloadService).not_to receive(:new)
 
-      it 'retrieves no download links' do
-        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with({}).and_call_original
+        result = subject.execute
 
-        expect(subject.execute).to be_empty
+        expect(result[:status]).to eq :success
       end
     end
 
-    context 'when lfsconfig file exists' do
-      before do
-        allow(project.repository).to receive(:lfsconfig_for).and_return("[lfs]\n\turl = #{lfs_endpoint}\n")
-      end
-
-      context 'when url points to the same import url host' do
-        let(:lfs_endpoint) { "#{import_url}/different_endpoint" }
-        let(:service) { double }
-
-        before do
-          allow(service).to receive(:execute)
-        end
-        it 'downloads lfs object using the new endpoint' do
-          expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: remote_uri).and_return(service)
-
-          subject.execute
-        end
-
-        context 'when import url has credentials' do
-          let(:import_url) { 'http://user:password@www.gitlab.com/demo/repo.git'}
-
-          it 'adds the credentials to the new endpoint' do
-            expect(Projects::LfsPointers::LfsDownloadLinkListService)
-              .to receive(:new).with(project, remote_uri: URI.parse("http://user:password@www.gitlab.com/demo/repo.git/different_endpoint"))
-              .and_return(service)
-
-            subject.execute
-          end
-
-          context 'when url has its own credentials' do
-            let(:lfs_endpoint) { "http://user1:password1@www.gitlab.com/demo/repo.git/different_endpoint" }
+    context 'when an exception is raised' do
+      it 'returns error' do
+        error_message = "error message"
+        expect_any_instance_of(Projects::LfsPointers::LfsObjectDownloadListService).to receive(:execute).and_raise(StandardError, error_message)
 
-            it 'does not add the import url credentials' do
-              expect(Projects::LfsPointers::LfsDownloadLinkListService)
-                .to receive(:new).with(project, remote_uri: remote_uri)
-                .and_return(service)
+        result = subject.execute
 
-              subject.execute
-            end
-          end
-        end
-      end
-
-      context 'when url points to a third party service' do
-        let(:lfs_endpoint) { 'http://third_party_service.com/info/lfs/objects/' }
-
-        it 'disables lfs from the project' do
-          expect(project.lfs_enabled?).to be_truthy
-
-          subject.execute
-
-          expect(project.lfs_enabled?).to be_falsey
-        end
-
-        it 'does not download anything' do
-          expect_any_instance_of(Projects::LfsPointers::LfsListService).not_to receive(:execute)
-
-          subject.execute
-        end
+        expect(result[:status]).to eq :error
+        expect(result[:message]).to eq error_message
       end
     end
   end
 
-  describe '#default_endpoint_uri' do
-    let(:import_url) { 'http://www.gitlab.com/demo/repo' }
+  context 'when lfs is not enabled for the project' do
+    it 'does not download lfs objects' do
+      allow(project).to receive(:lfs_enabled?).and_return(false)
+      expect(Projects::LfsPointers::LfsObjectDownloadListService).not_to receive(:new)
+      expect(Projects::LfsPointers::LfsDownloadService).not_to receive(:new)
+
+      result = subject.execute
 
-    it 'adds suffix .git if the url does not have it' do
-      expect(subject.send(:default_endpoint_uri).path).to match(/repo.git/)
+      expect(result[:status]).to eq :success
     end
   end
 end
diff --git a/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb
index 5caa9de732e..849601c4a63 100644
--- a/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb
+++ b/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb
@@ -1,5 +1,4 @@
 # frozen_string_literal: true
-
 require 'spec_helper'
 
 describe Projects::LfsPointers::LfsLinkService do
diff --git a/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb
new file mode 100644
index 00000000000..9dac29765a2
--- /dev/null
+++ b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb
@@ -0,0 +1,148 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Projects::LfsPointers::LfsObjectDownloadListService do
+  let(:import_url) { 'http://www.gitlab.com/demo/repo.git' }
+  let(:default_endpoint) { "#{import_url}/info/lfs/objects/batch"}
+  let(:group) { create(:group, lfs_enabled: true)}
+  let!(:project) { create(:project, namespace: group, import_url: import_url, lfs_enabled: true) }
+  let!(:lfs_objects_project) { create_list(:lfs_objects_project, 2, project: project) }
+  let!(:existing_lfs_objects) { LfsObject.pluck(:oid, :size).to_h }
+  let(:oids) { { 'oid1' => 123, 'oid2' => 125 } }
+  let(:oid_download_links) { { 'oid1' => "#{import_url}/gitlab-lfs/objects/oid1", 'oid2' => "#{import_url}/gitlab-lfs/objects/oid2" } }
+  let(:all_oids) { existing_lfs_objects.merge(oids) }
+  let(:remote_uri) { URI.parse(lfs_endpoint) }
+
+  subject { described_class.new(project) }
+
+  before do
+    allow(project.repository).to receive(:lfsconfig_for).and_return(nil)
+    allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+    allow_any_instance_of(Projects::LfsPointers::LfsListService).to receive(:execute).and_return(all_oids)
+  end
+
+  describe '#execute' do
+    context 'when no lfs pointer is linked' do
+      before do
+        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return([])
+        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
+        expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)).and_call_original
+      end
+
+      it 'retrieves all lfs pointers in the project repository' do
+        expect_any_instance_of(Projects::LfsPointers::LfsListService).to receive(:execute)
+
+        subject.execute
+      end
+
+      it 'links existent lfs objects to the project' do
+        expect_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute)
+
+        subject.execute
+      end
+
+      it 'retrieves the download links of non existent objects' do
+        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids)
+
+        subject.execute
+      end
+    end
+
+    context 'when some lfs objects are linked' do
+      before do
+        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(existing_lfs_objects.keys)
+        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
+      end
+
+      it 'retrieves the download links of non existent objects' do
+        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(oids)
+
+        subject.execute
+      end
+    end
+
+    context 'when all lfs objects are linked' do
+      before do
+        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(all_oids.keys)
+        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute)
+      end
+
+      it 'retrieves no download links' do
+        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with({}).and_call_original
+
+        expect(subject.execute).to be_empty
+      end
+    end
+
+    context 'when lfsconfig file exists' do
+      before do
+        allow(project.repository).to receive(:lfsconfig_for).and_return("[lfs]\n\turl = #{lfs_endpoint}\n")
+      end
+
+      context 'when url points to the same import url host' do
+        let(:lfs_endpoint) { "#{import_url}/different_endpoint" }
+        let(:service) { double }
+
+        before do
+          allow(service).to receive(:execute)
+        end
+
+        it 'downloads lfs object using the new endpoint' do
+          expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: remote_uri).and_return(service)
+
+          subject.execute
+        end
+
+        context 'when import url has credentials' do
+          let(:import_url) { 'http://user:password@www.gitlab.com/demo/repo.git'}
+
+          it 'adds the credentials to the new endpoint' do
+            expect(Projects::LfsPointers::LfsDownloadLinkListService)
+              .to receive(:new).with(project, remote_uri: URI.parse("http://user:password@www.gitlab.com/demo/repo.git/different_endpoint"))
+              .and_return(service)
+
+            subject.execute
+          end
+
+          context 'when url has its own credentials' do
+            let(:lfs_endpoint) { "http://user1:password1@www.gitlab.com/demo/repo.git/different_endpoint" }
+
+            it 'does not add the import url credentials' do
+              expect(Projects::LfsPointers::LfsDownloadLinkListService)
+                .to receive(:new).with(project, remote_uri: remote_uri)
+                .and_return(service)
+
+              subject.execute
+            end
+          end
+        end
+      end
+
+      context 'when url points to a third party service' do
+        let(:lfs_endpoint) { 'http://third_party_service.com/info/lfs/objects/' }
+
+        it 'disables lfs from the project' do
+          expect(project.lfs_enabled?).to be_truthy
+
+          subject.execute
+
+          expect(project.lfs_enabled?).to be_falsey
+        end
+
+        it 'does not download anything' do
+          expect_any_instance_of(Projects::LfsPointers::LfsListService).not_to receive(:execute)
+
+          subject.execute
+        end
+      end
+    end
+  end
+
+  describe '#default_endpoint_uri' do
+    let(:import_url) { 'http://www.gitlab.com/demo/repo' }
+
+    it 'adds suffix .git if the url does not have it' do
+      expect(subject.send(:default_endpoint_uri).path).to match(/repo.git/)
+    end
+  end
+end