1 files changed, 112 insertions, 3 deletions

diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 0839dde696a..d153ccedf8c 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe MergeRequestDiff do
+RSpec.describe MergeRequestDiff do
   using RSpec::Parameterized::TableSyntax
 
   include RepoHelpers
@@ -162,7 +162,8 @@ describe MergeRequestDiff do
     let(:uploader) { ExternalDiffUploader }
     let(:file_store) { uploader::Store::LOCAL }
     let(:remote_store) { uploader::Store::REMOTE }
-    let(:diff) { create(:merge_request).merge_request_diff }
+    let(:merge_request) { create(:merge_request) }
+    let(:diff) { merge_request.merge_request_diff }
 
     it 'converts from in-database to external file storage' do
       expect(diff).not_to be_stored_externally
@@ -177,6 +178,30 @@ describe MergeRequestDiff do
       expect(diff.external_diff_store).to eq(file_store)
     end
 
+    it 'safely handles a transaction error when migrating to external storage' do
+      expect(diff).not_to be_stored_externally
+      expect(diff.external_diff).not_to be_exists
+
+      stub_external_diffs_setting(enabled: true)
+
+      expect(diff).not_to receive(:save!)
+      expect(Gitlab::Database)
+        .to receive(:bulk_insert)
+        .with('merge_request_diff_files', anything)
+        .and_raise(ActiveRecord::Rollback)
+
+      expect { diff.migrate_files_to_external_storage! }.not_to change(diff, :merge_request_diff_files)
+
+      diff.reload
+
+      expect(diff).not_to be_stored_externally
+
+      # The diff is written outside of the transaction, which is desirable to
+      # avoid long transaction times when migrating, but it does mean we can
+      # leave the file dangling on failure
+      expect(diff.external_diff).to be_exists
+    end
+
     it 'converts from in-database to external object storage' do
       expect(diff).not_to be_stored_externally
 
@@ -209,6 +234,33 @@ describe MergeRequestDiff do
 
       diff.migrate_files_to_external_storage!
     end
+
+    context 'diff adds an empty file' do
+      let(:project) { create(:project, :test_repo) }
+      let(:merge_request) do
+        create(
+          :merge_request,
+          source_project: project,
+          target_project: project,
+          source_branch: 'empty-file',
+          target_branch: 'master'
+        )
+      end
+
+      it 'migrates the diff to object storage' do
+        create_file_in_repo(project, 'master', 'empty-file', 'empty-file', '')
+
+        expect(diff).not_to be_stored_externally
+
+        stub_external_diffs_setting(enabled: true)
+        stub_external_diffs_object_storage(uploader, direct_upload: true)
+
+        diff.migrate_files_to_external_storage!
+
+        expect(diff).to be_stored_externally
+        expect(diff.external_diff_store).to eq(remote_store)
+      end
+    end
   end
 
   describe '#migrate_files_to_database!' do
@@ -476,7 +528,7 @@ describe MergeRequestDiff do
     include_examples 'merge request diffs'
   end
 
-  describe 'external diffs always enabled' do
+  describe 'external diffs on disk always enabled' do
     before do
       stub_external_diffs_setting(enabled: true, when: 'always')
     end
@@ -484,6 +536,63 @@ describe MergeRequestDiff do
     include_examples 'merge request diffs'
   end
 
+  describe 'external diffs in object storage always enabled' do
+    let(:uploader) { ExternalDiffUploader }
+    let(:remote_store) { uploader::Store::REMOTE }
+
+    subject(:diff) { merge_request.merge_request_diff }
+
+    before do
+      stub_external_diffs_setting(enabled: true, when: 'always')
+      stub_external_diffs_object_storage(uploader, direct_upload: true)
+    end
+
+    # We can't use the full merge request diffs shared examples here because
+    # reading from the fake object store isn't implemented yet
+
+    context 'empty diff' do
+      let(:merge_request) { create(:merge_request, :without_diffs) }
+
+      it 'creates an empty diff' do
+        expect(diff.state).to eq('empty')
+        expect(diff).not_to be_stored_externally
+      end
+    end
+
+    context 'normal diff' do
+      let(:merge_request) { create(:merge_request) }
+
+      it 'creates a diff in object storage' do
+        expect(diff).to be_stored_externally
+        expect(diff.state).to eq('collected')
+        expect(diff.external_diff_store).to eq(remote_store)
+      end
+    end
+
+    context 'diff adding an empty file' do
+      let(:project) { create(:project, :test_repo) }
+      let(:merge_request) do
+        create(
+          :merge_request,
+          source_project: project,
+          target_project: project,
+          source_branch: 'empty-file',
+          target_branch: 'master'
+        )
+      end
+
+      it 'creates a diff in object storage' do
+        create_file_in_repo(project, 'master', 'empty-file', 'empty-file', '')
+
+        diff.reload
+
+        expect(diff).to be_stored_externally
+        expect(diff.state).to eq('collected')
+        expect(diff.external_diff_store).to eq(remote_store)
+      end
+    end
+  end
+
   describe 'exernal diffs enabled for outdated diffs' do
     before do
       stub_external_diffs_setting(enabled: true, when: 'outdated')