attachments migration should move only the base folder

2 files changed, 30 insertions, 49 deletions

diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb
index 58a47da2fcb..68b9a72661c 100644
--- a/app/services/projects/hashed_storage/migrate_attachments_service.rb
+++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb
@@ -3,51 +3,38 @@ module Projects
     class MigrateAttachmentsService < BaseService
       attr_reader :logger
 
-      BATCH_SIZE = 500
-
       def initialize(project, logger = nil)
         @project = project
         @logger = logger || Rails.logger
       end
 
       def execute
-        project_before_migration = project.dup
+        old_path = FileUploader.dynamic_path_segment(project)
         project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments]
+        new_path = FileUploader.dynamic_path_segment(project)
 
-        project.uploads.find_each(batch_size: BATCH_SIZE) do |upload|
-          old_path = attachments_path(project_before_migration, upload)
-          new_path = attachments_path(project, upload)
-          move_attachment(old_path, new_path)
-        end
-
+        move_folder!(old_path, new_path)
         project.save!
       end
 
       private
 
-      def attachments_path(project, upload)
-        File.join(
-          FileUploader.dynamic_path_segment(project),
-          upload.path
-        )
-      end
-
-      def move_attachment(old_path, new_path)
-        unless File.file?(old_path)
-          logger.error("Failed to migrate attachment from '#{old_path}' to '#{new_path}', source file doesn't exist (PROJECT_ID=#{project.id})")
+      def move_folder!(old_path, new_path)
+        unless File.exist?(old_path)
+          logger.info("Skipped attachments migration from '#{old_path}' to '#{new_path}', source path doesn't exist (PROJECT_ID=#{project.id})")
           return
         end
 
-        # Create attachments folder if doesn't exist yet
-        FileUtils.mkdir_p(File.dirname(new_path)) unless Dir.exist?(File.dirname(new_path))
-
-        if File.file?(new_path)
-          logger.info("Skipped attachment migration from '#{old_path}' to '#{new_path}', target file already exist (PROJECT_ID=#{project.id})")
+        if File.exist?(new_path)
+          logger.error("Cannot migrate attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})")
           return
         end
 
+        # Create hashed storage base path folder
+        FileUtils.mkdir_p(File.expand_path('..', new_path))
+
         FileUtils.mv(old_path, new_path)
-        logger.info("Migrated project attachment from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})")
+        logger.info("Migrated project attachments from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})")
       end
     end
   end
diff --git a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
index 81f05074261..ce43a7e4d54 100644
--- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
+++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
@@ -8,65 +8,59 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
 
   let!(:upload) { Upload.find_by(path: file_uploader.relative_path) }
   let(:file_uploader) { build(:file_uploader, project: project) }
-  let(:old_path) { attachments_path(legacy_storage, upload) }
-  let(:new_path) { attachments_path(hashed_storage, upload) }
-
-  let(:other_file_uploader) { build(:file_uploader, project: project) }
-  let(:other_old_path) { attachments_path(legacy_storage, other_upload) }
-  let(:other_new_path) { attachments_path(hashed_storage, other_upload) }
+  let(:old_path) { File.join(base_path(legacy_storage), upload.path) }
+  let(:new_path) { File.join(base_path(hashed_storage), upload.path) }
 
   context '#execute' do
     context 'when succeeds' do
       it 'moves attachments to hashed storage layout' do
         expect(File.file?(old_path)).to be_truthy
         expect(File.file?(new_path)).to be_falsey
+        expect(File.exist?(base_path(legacy_storage))).to be_truthy
+        expect(File.exist?(base_path(hashed_storage))).to be_falsey
+        expect(FileUtils).to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)).and_call_original
 
         service.execute
 
+        expect(File.exist?(base_path(hashed_storage))).to be_truthy
+        expect(File.exist?(base_path(legacy_storage))).to be_falsey
         expect(File.file?(old_path)).to be_falsey
         expect(File.file?(new_path)).to be_truthy
       end
     end
 
-    context 'when original file does not exist anymore' do
-      let!(:other_upload) { Upload.find_by(path: other_file_uploader.relative_path) }
-
+    context 'when original folder does not exist anymore' do
       before do
-        File.unlink(old_path)
+        FileUtils.rm_rf(base_path(legacy_storage))
       end
 
-      it 'skips moving the file and goes to next' do
-        expect(FileUtils).not_to receive(:mv).with(old_path, new_path)
-        expect(FileUtils).to receive(:mv).with(other_old_path, other_new_path).and_call_original
+      it 'skips moving folders and go to next' do
+        expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage))
 
         service.execute
 
+        expect(File.exist?(base_path(hashed_storage))).to be_falsey
         expect(File.file?(new_path)).to be_falsey
-        expect(File.file?(other_new_path)).to be_truthy
       end
     end
 
-    context 'when target file already exists' do
-      let!(:other_upload) { Upload.find_by(path: other_file_uploader.relative_path) }
-
+    context 'when target folder already exists' do
       before do
-        FileUtils.mkdir_p(File.dirname(new_path))
-        FileUtils.touch(new_path)
+        FileUtils.mkdir_p(base_path(hashed_storage))
       end
 
       it 'skips moving the file and goes to next' do
-        expect(FileUtils).not_to receive(:mv).with(old_path, new_path)
-        expect(FileUtils).to receive(:mv).with(other_old_path, other_new_path).and_call_original
-        expect(File.file?(new_path)).to be_truthy
+        expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage))
 
         service.execute
 
+        expect(File.exist?(base_path(legacy_storage))).to be_truthy
         expect(File.file?(old_path)).to be_truthy
       end
     end
   end
 
-  def attachments_path(storage, upload)
-    File.join(CarrierWave.root, FileUploader.base_dir, storage.disk_path, upload.path)
+  def base_path(storage)
+    File.join(CarrierWave.root, FileUploader.base_dir, storage.disk_path)
   end
 end