Merge branch 'fix/sm/atomic-migration' into 'master'

Fix migrate! method (Minimal fix with ExclusiveLock to prevent race conditions)

Closes #4928 and #4980

See merge request gitlab-org/gitlab-ee!4624

2 files changed, 37 insertions, 0 deletions

diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb
index 0bcf28f2c1c..714b2498538 100644
--- a/spec/uploaders/job_artifact_uploader_spec.rb
+++ b/spec/uploaders/job_artifact_uploader_spec.rb
@@ -67,4 +67,14 @@ describe JobArtifactUploader do
     it { is_expected.to include("/#{job_artifact.job_id}/#{job_artifact.id}/") }
     it { is_expected.to end_with("ci_build_artifacts.zip") }
   end
+
+  describe "#migrate!" do
+    before do
+      uploader.store!(fixture_file_upload(Rails.root.join('spec/fixtures/trace/sample_trace')))
+      stub_artifacts_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
diff --git a/spec/uploaders/object_storage_spec.rb b/spec/uploaders/object_storage_spec.rb
index e01ad9af1dc..64b59acb286 100644
--- a/spec/uploaders/object_storage_spec.rb
+++ b/spec/uploaders/object_storage_spec.rb
@@ -128,6 +128,33 @@ describe ObjectStorage do
         expect(uploader.object_store).to eq(uploader.upload.store)
       end
     end
+
+    describe '#migrate!' do
+      let(:new_store) { ObjectStorage::Store::REMOTE }
+
+      before do
+        stub_uploads_object_storage(uploader: AvatarUploader)
+      end
+
+      subject { uploader.migrate!(new_store) }
+
+      it 'persist @object_store to the recorded upload' do
+        subject
+
+        expect(uploader.upload.store).to eq(new_store)
+      end
+
+      describe 'fails' do
+        it 'is handled gracefully' do
+          store = uploader.object_store
+          expect_any_instance_of(Upload).to receive(:save!).and_raise("An error")
+
+          expect { subject }.to raise_error("An error")
+          expect(uploader.exists?).to be_truthy
+          expect(uploader.upload.store).to eq(store)
+        end
+      end
+    end
   end
 
   # this means the model holds an <mounted_as>_store attribute directly