Merge branch 'sh-geo-refactor-lfs-downloads' into 'master'

Refactor Geo LFS downloader to use a Downloader subclass

See merge request !1424

app/services/geo/file_download_service.rb

@@ -11,17 +11,24 @@ module Geo
 
     def execute
       try_obtain_lease do |lease|
-        case object_type
-        when :lfs
-          download_lfs_object
-        else
-          log("unknown file type: #{object_type}")
-        end
+        bytes_downloaded = downloader.execute
+        success = bytes_downloaded && bytes_downloaded >= 0
+        update_registry(bytes_downloaded) if success
       end
     end
 
     private
 
+    def downloader
+      begin
+        klass = Gitlab::Geo.const_get("#{object_type.capitalize}Downloader")
+        klass.new(object_db_id)
+      rescue NameError
+        log("unknown file type: #{object_type}")
+        raise
+      end
+    end
+
     def try_obtain_lease
       uuid = Gitlab::ExclusiveLease.new(lease_key, timeout: LEASE_TIMEOUT).try_obtain
 
@@ -34,20 +41,6 @@ module Geo
       end
     end
 
-    def download_lfs_object
-      lfs_object = LfsObject.find_by_id(object_db_id)
-
-      return unless lfs_object.present?
-
-      transfer = ::Gitlab::Geo::LfsTransfer.new(lfs_object)
-      bytes_downloaded = transfer.download_from_primary
-
-      success = bytes_downloaded && bytes_downloaded >= 0
-      update_registry(bytes_downloaded) if success
-
-      success
-    end
-
     def log(message)
       Rails.logger.info "#{self.class.name}: #{message}"
     end

lib/gitlab/geo/file_downloader.rb

+module Gitlab
+  module Geo
+    class FileDownloader
+      attr_reader :object_db_id
+
+      def initialize(object_db_id)
+        @object_db_id = object_db_id
+      end
+
+      # Executes the actual file download
+      #
+      # Subclasses should return the number of bytes downloaded,
+      # or nil or -1 if a failure occurred.
+      def execute
+        raise NotImplementedError
+      end
+    end
+  end
+end

lib/gitlab/geo/lfs_downloader.rb

+module Gitlab
+  module Geo
+    class LfsDownloader < FileDownloader
+      def execute
+        lfs_object = LfsObject.find_by_id(object_db_id)
+
+        return unless lfs_object.present?
+
+        transfer = ::Gitlab::Geo::LfsTransfer.new(lfs_object)
+        transfer.download_from_primary
+      end
+    end
+  end
+end

spec/lib/gitlab/geo/lfs_downloader_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Geo::LfsDownloader do
+  let(:lfs_object) { create(:lfs_object) }
+
+  subject do
+    described_class.new(lfs_object.id)
+  end
+
+  context '#download_from_primary' do
+    it 'with LFS object' do
+      allow_any_instance_of(Gitlab::Geo::LfsTransfer)
+        .to receive(:download_from_primary).and_return(100)
+
+      expect(subject.execute).to eq(100)
+    end
+
+    it 'with unknown LFS object' do
+      expect(described_class.new(10000)).not_to receive(:download_from_primary)
+
+      expect(subject.execute).to be_nil
+    end
+  end
+end

spec/services/geo/file_download_service_spec.rb

@@ -20,5 +20,9 @@ describe Geo::FileDownloadService, services: true do
 
       expect{ subject.execute }.to change { Geo::FileRegistry.count }.by(1)
     end
+
+    it 'bad object type' do
+      expect{ described_class.new(:bad, lfs_object.id).execute }.to raise_error(NameError)
+    end
   end
 end