add specs around the migration code

app/uploaders/avatar_uploader.rb

@@ -10,17 +10,6 @@ class AvatarUploader < GitlabUploader
     model.avatar.file && model.avatar.file.present?
   end
 
-  # We set move_to_store and move_to_cache to 'false' to prevent stealing
-  # the avatar file from a project when forking it.
-  # https://gitlab.com/gitlab-org/gitlab-ce/issues/26158
-  def move_to_store
-    false
-  end
-
-  def move_to_cache
-    false
-  end
-
   private
 
   def dynamic_segment

app/uploaders/gitlab_uploader.rb

@@ -32,12 +32,12 @@ class GitlabUploader < CarrierWave::Uploader::Base
 
   # Reduce disk IO
   def move_to_cache
-    true
+    super || true
   end
 
   # Reduce disk IO
   def move_to_store
-    true
+    super || true
   end
 
   def exists?

config/initializers/1_settings.rb

@@ -343,8 +343,8 @@ Settings['artifacts'] ||= Settingslogic.new({})
 Settings.artifacts['enabled']      = true if Settings.artifacts['enabled'].nil?
 Settings.artifacts['storage_path'] = Settings.absolute(Settings.artifacts.values_at('path', 'storage_path').compact.first || File.join(Settings.shared['path'], "artifacts"))
 # Settings.artifact['path'] is deprecated, use `storage_path` instead
-Settings.artifacts['path'] = Settings.artifacts['storage_path']
-Settings.artifacts['max_size']   ||= 100 # in megabytes
+Settings.artifacts['path']         = Settings.artifacts['storage_path']
+Settings.artifacts['max_size'] ||= 100 # in megabytes
 
 Settings.artifacts['object_store'] ||= Settingslogic.new({})
 Settings.artifacts['object_store']['enabled']           ||= false

ee/app/uploaders/object_storage.rb

@@ -68,6 +68,7 @@ module ObjectStorage
       base.include(ObjectStorage)
 
       before :store, :verify_license!
+      after :migrate, :delete_migrated_file
     end
 
     class_methods do
@@ -155,26 +156,29 @@ module ObjectStorage
       return unless object_store != new_store
       return unless file
 
+      new_file = nil
       file_to_delete = file
+      from_object_store = object_store
       self.object_store = new_store # changes the storage and file
 
       cache_stored_file! if file_storage?
 
-      with_callbacks(:store, file_to_delete) do # for #store_versions!
-        storage.store!(file).tap do |new_file|
-          begin
-            @file = new_file
-            persist_object_store!
-            file_to_delete.delete if new_file.exists?
-          rescue => e
-            # in case of failure delete new file
-            new_file.delete
-            raise e
-          end
+      with_callbacks(:migrate, file_to_delete) do
+        with_callbacks(:store, file_to_delete) do # for #store_versions!
+          new_file = storage.store!(file)
+          persist_object_store!
+          self.file = new_file
         end
       end
 
       file
+    rescue => e
+      # in case of failure delete new file
+      new_file.delete unless new_file.nil?
+      # revert back to the old file
+      self.object_store = from_object_store
+      self.file = file_to_delete
+      raise e
     end
 
     def schedule_migration_to_object_storage(*args)
@@ -199,15 +203,15 @@ module ObjectStorage
     end
 
     def move_to_store
-      return true if Store::LOCAL
-
-      file.try(:storage) == storage
+      false
     end
 
     def move_to_cache
-      return true if object_store == Store::LOCAL
+      false
+    end
 
-      file.try(:storage) == cache_storage
+    def delete_migrated_file(migrated_file)
+      migrated_file.delete if exists?
     end
 
     def verify_license!(_file)
@@ -245,6 +249,12 @@ module ObjectStorage
 
     private
 
+    # this is a hack around CarrierWave. The #migrate method needs to be
+    # able to force the current file to the migrated file upon success.
+    def file=(file)
+      @file = file
+    end
+
     def serialization_column
       model.class.uploader_options.dig(mounted_as, :mount_on) || mounted_as
     end

spec/support/shared_examples/uploaders/object_storage_shared_examples.rb

@@ -4,6 +4,78 @@ shared_context 'with storage' do |store, **stub_params|
   end
 end
 
+shared_examples "migrates" do | to_store: , from_store: nil |
+  let(:to) { to_store }
+  let(:from) { from_store || subject.object_store }
+
+  def migrate(to)
+    subject.migrate!(to)
+  end
+
+  def checksum
+    Digest::SHA256.hexdigest(subject.read)
+  end
+
+  before do
+    migrate(from)
+  end
+
+  it 'does nothing when migrating to the current store' do
+    expect { migrate(from) }.not_to change { subject.object_store }.from(from)
+  end
+
+  it 'migrate to the specified store' do
+    from_checksum = checksum
+
+    expect { migrate(to) }.to change { subject.object_store }.from(from).to(to)
+    expect(checksum).to eq(from_checksum)
+  end
+
+  it 'removes the original file after the migration' do
+    original_file = subject.file.path
+    migrate(to)
+
+    expect(File.exist?(original_file)).to be_falsey
+  end
+
+  context 'migration is unsuccessful' do
+    shared_examples "handles gracefully" do |error:|
+      it 'does not update the object_store' do
+        expect { migrate(to) }.to raise_error(error)
+        expect(subject.object_store).to eq(from)
+      end
+
+      it 'does not delete the original file' do
+        expect { migrate(to) }.to raise_error(error)
+        expect(subject.exists?).to be_truthy
+      end
+    end
+
+    context 'when the store is not supported' do
+      let(:to) { -1 } # not a valid store
+
+      include_examples "handles gracefully", error: ObjectStorage::UnknownStoreError
+    end
+
+    context 'upon a fog failure' do
+      before do
+        storage_class = subject.send(:storage_for, to).class
+        expect_any_instance_of(storage_class).to receive(:store!).and_raise("Store failure.")
+      end
+
+      include_examples "handles gracefully", error: "Store failure."
+    end
+
+    context 'upon a database failure' do
+      before do
+        expect(uploader).to receive(:persist_object_store!).and_raise("ActiveRecord failure.")
+      end
+
+      include_examples "handles gracefully", error: "ActiveRecord failure."
+    end
+  end
+end
+
 shared_examples "matches the method pattern" do |method|
   let(:target) { subject }
   let(:args) { nil }

spec/uploaders/attachment_uploader_spec.rb

 require 'spec_helper'
 
 describe AttachmentUploader do
-  let(:uploader) { described_class.new(build_stubbed(:user), :attachment) }
+  let(:note) { create(:note, :with_attachment) }
+  let(:uploader) { note.attachment }
   let(:upload) { create(:upload, :attachment_upload, model: uploader.model) }
 
   subject { uploader }
 
   it_behaves_like 'builds correct paths',
-                  store_dir: %r[uploads/-/system/user/attachment/],
-                  upload_path: %r[uploads/-/system/user/attachment/],
-                  absolute_path: %r[#{CarrierWave.root}/uploads/-/system/user/attachment/]
-
-  describe '#move_to_cache' do
-    it 'is true' do
-      expect(uploader.move_to_cache).to eq(true)
-    end
-  end
-
-  describe '#move_to_store' do
-    it 'is true' do
-      expect(uploader.move_to_store).to eq(true)
-    end
-  end
+                  store_dir: %r[uploads/-/system/note/attachment/],
+                  upload_path: %r[uploads/-/system/note/attachment/],
+                  absolute_path: %r[#{CarrierWave.root}/uploads/-/system/note/attachment/]
 
   # EE-specific
   context "object_store is REMOTE" do
@@ -32,7 +21,17 @@ describe AttachmentUploader do
     include_context 'with storage', described_class::Store::REMOTE
 
     it_behaves_like 'builds correct paths',
-                    store_dir: %r[user/attachment/],
-                    upload_path: %r[user/attachment/]
+                    store_dir: %r[note/attachment/],
+                    upload_path: %r[note/attachment/]
+  end
+
+  describe "#migrate!" do
+    before do
+      uploader.store!(fixture_file_upload(Rails.root.join('spec/fixtures/doc_sample.txt')))
+      stub_uploads_object_storage
+    end
+
+    it_behaves_like "migrates", to_store: described_class::Store::REMOTE
+    it_behaves_like "migrates", from_store: described_class::Store::REMOTE, to_store: described_class::Store::LOCAL
   end
 end

spec/uploaders/avatar_uploader_spec.rb

@@ -12,18 +12,6 @@ describe AvatarUploader do
                   upload_path: %r[uploads/-/system/user/avatar/],
                   absolute_path: %r[#{CarrierWave.root}/uploads/-/system/user/avatar/]
 
-  describe '#move_to_cache' do
-    it 'is false' do
-      expect(uploader.move_to_cache).to eq(false)
-    end
-  end
-
-  describe '#move_to_store' do
-    it 'is false' do
-      expect(uploader.move_to_store).to eq(false)
-    end
-  end
-
   # EE-specific
   context "object_store is REMOTE" do
     before do
@@ -36,4 +24,17 @@ describe AvatarUploader do
                     store_dir: %r[user/avatar/],
                     upload_path: %r[user/avatar/]
   end
+
+  context "with a file" do
+    let(:project) { create(:project, :with_avatar) }
+    let(:uploader) { project.avatar }
+    let(:upload) { uploader.upload }
+
+    before do
+      stub_uploads_object_storage
+    end
+
+    it_behaves_like "migrates", to_store: described_class::Store::REMOTE
+    it_behaves_like "migrates", from_store: described_class::Store::REMOTE, to_store: described_class::Store::LOCAL
+  end
 end

spec/uploaders/file_uploader_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe FileUploader do
   let(:group) { create(:group, name: 'awesome') }
-  let(:project) { build_stubbed(:project, namespace: group, name: 'project') }
+  let(:project) { create(:project, namespace: group, name: 'project') }
   let(:uploader) { described_class.new(project) }
   let(:upload)  { double(model: project, path: 'secret/foo.jpg') }
 
@@ -17,7 +17,7 @@ describe FileUploader do
   shared_examples 'uses hashed storage' do
     context 'when rolled out attachments' do
       before do
-        expect(project).to receive(:disk_path).and_return('ca/fe/fe/ed')
+        allow(project).to receive(:disk_path).and_return('ca/fe/fe/ed')
       end
 
       let(:project) { build_stubbed(:project, :hashed, namespace: group, name: 'project') }
@@ -66,15 +66,13 @@ describe FileUploader do
     end
   end
 
-  describe '#move_to_cache' do
-    it 'is true' do
-      expect(uploader.move_to_cache).to eq(true)
+  describe "#migrate!" do
+    before do
+      uploader.store!(fixture_file_upload(Rails.root.join('spec/fixtures/dk.png')))
+      stub_uploads_object_storage
     end
-  end
 
-  describe '#move_to_store' do
-    it 'is true' do
-      expect(uploader.move_to_store).to eq(true)
-    end
+    it_behaves_like "migrates", to_store: described_class::Store::REMOTE
+    it_behaves_like "migrates", from_store: described_class::Store::REMOTE, to_store: described_class::Store::LOCAL
   end
 end

spec/uploaders/lfs_object_uploader_spec.rb

@@ -23,18 +23,6 @@ describe LfsObjectUploader do
                     store_dir: %r[\h{2}/\h{2}]
   end
 
-  describe '#move_to_cache' do
-    it 'is true' do
-      expect(uploader.move_to_cache).to eq(true)
-    end
-  end
-
-  describe '#move_to_store' do
-    it 'is true' do
-      expect(uploader.move_to_store).to eq(true)
-    end
-  end
-
   describe 'migration to object storage' do
     context 'with object storage disabled' do
       it "is skipped" do

spec/uploaders/namespace_file_uploader_spec.rb

@@ -26,4 +26,14 @@ describe NamespaceFileUploader do
                     store_dir: %r[namespace/\d+/\h+],
                     upload_path: IDENTIFIER
   end
+
+  describe "#migrate!" do
+    before do
+      uploader.store!(fixture_file_upload(Rails.root.join('spec/fixtures/doc_sample.txt')))
+      stub_uploads_object_storage
+    end
+
+    it_behaves_like "migrates", to_store: described_class::Store::REMOTE
+    it_behaves_like "migrates", from_store: described_class::Store::REMOTE, to_store: described_class::Store::LOCAL
+  end
 end

spec/uploaders/object_storage_spec.rb

@@ -83,7 +83,7 @@ describe ObjectStorage do
         expect(object).to receive(:file_store).and_return(described_class::Store::REMOTE)
       end
 
-      it "returns given value" do
+      it "returns the given value" do
         expect(uploader.object_store).to eq(described_class::Store::REMOTE)
       end
     end
@@ -143,11 +143,6 @@ describe ObjectStorage do
         it "uploader include described_class::Concern" do
           expect(uploader).to be_a(described_class::Concern)
         end
-
-        it 'moves files locally' do
-          expect(uploader.move_to_store).to be(true)
-          expect(uploader.move_to_cache).to be(true)
-        end
       end
 
       describe '#use_file' do

spec/uploaders/personal_file_uploader_spec.rb

@@ -43,4 +43,14 @@ describe PersonalFileUploader do
       )
     end
   end
+
+  describe "#migrate!" do
+    before do
+      uploader.store!(fixture_file_upload(Rails.root.join('spec/fixtures/doc_sample.txt')))
+      stub_uploads_object_storage
+    end
+
+    it_behaves_like "migrates", to_store: described_class::Store::REMOTE
+    it_behaves_like "migrates", from_store: described_class::Store::REMOTE, to_store: described_class::Store::LOCAL
+  end
 end

spec/uploaders/records_uploads_spec.rb

@@ -59,13 +59,13 @@ describe RecordsUploads do
     it "does not create an Upload record when the file doesn't exist" do
       allow(uploader).to receive(:file).and_return(double(exists?: false))
 
-      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.to_not change { Upload.count }
+      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count }
     end
 
     it 'does not create an Upload record if model is missing' do
       allow_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil)
 
-      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.to_not change { Upload.count }
+      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count }
     end
 
     it 'it destroys Upload records at the same path before recording' do