Allow MR diffs to be placed into an object store

4 files changed, 204 insertions, 63 deletions

diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 33e984dc399..1849d3bac12 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -46,7 +46,7 @@ describe MergeRequestDiff do
     it { expect(first_diff.reload).not_to be_latest }
   end
 
-  describe '#diffs' do
+  shared_examples_for 'merge request diffs' do
     let(:merge_request) { create(:merge_request, :with_diffs) }
     let!(:diff) { merge_request.merge_request_diff.reload }
 
@@ -91,98 +91,110 @@ describe MergeRequestDiff do
         diff.diffs.diff_files
       end
     end
-  end
 
-  describe '#raw_diffs' do
-    context 'when the :ignore_whitespace_change option is set' do
-      it 'creates a new compare object instead of loading from the DB' do
-        expect(diff_with_commits).not_to receive(:load_diffs)
-        expect(diff_with_commits.compare).to receive(:diffs).and_call_original
+    describe '#raw_diffs' do
+      context 'when the :ignore_whitespace_change option is set' do
+        it 'creates a new compare object instead of using preprocessed data' do
+          expect(diff_with_commits).not_to receive(:load_diffs)
+          expect(diff_with_commits.compare).to receive(:diffs).and_call_original
 
-        diff_with_commits.raw_diffs(ignore_whitespace_change: true)
+          diff_with_commits.raw_diffs(ignore_whitespace_change: true)
+        end
       end
-    end
 
-    context 'when the raw diffs are empty' do
-      before do
-        MergeRequestDiffFile.where(merge_request_diff_id: diff_with_commits.id).delete_all
-      end
+      context 'when the raw diffs are empty' do
+        before do
+          MergeRequestDiffFile.where(merge_request_diff_id: diff_with_commits.id).delete_all
+        end
 
-      it 'returns an empty DiffCollection' do
-        expect(diff_with_commits.raw_diffs).to be_a(Gitlab::Git::DiffCollection)
-        expect(diff_with_commits.raw_diffs).to be_empty
+        it 'returns an empty DiffCollection' do
+          expect(diff_with_commits.raw_diffs).to be_a(Gitlab::Git::DiffCollection)
+          expect(diff_with_commits.raw_diffs).to be_empty
+        end
       end
-    end
 
-    context 'when the raw diffs exist' do
-      it 'returns the diffs' do
-        expect(diff_with_commits.raw_diffs).to be_a(Gitlab::Git::DiffCollection)
-        expect(diff_with_commits.raw_diffs).not_to be_empty
-      end
+      context 'when the raw diffs exist' do
+        it 'returns the diffs' do
+          expect(diff_with_commits.raw_diffs).to be_a(Gitlab::Git::DiffCollection)
+          expect(diff_with_commits.raw_diffs).not_to be_empty
+        end
 
-      context 'when the :paths option is set' do
-        let(:diffs) { diff_with_commits.raw_diffs(paths: ['files/ruby/popen.rb', 'files/ruby/popen.rb']) }
+        context 'when the :paths option is set' do
+          let(:diffs) { diff_with_commits.raw_diffs(paths: ['files/ruby/popen.rb', 'files/ruby/popen.rb']) }
 
-        it 'only returns diffs that match the (old path, new path) given' do
-          expect(diffs.map(&:new_path)).to contain_exactly('files/ruby/popen.rb')
-        end
+          it 'only returns diffs that match the (old path, new path) given' do
+            expect(diffs.map(&:new_path)).to contain_exactly('files/ruby/popen.rb')
+          end
 
-        it 'only serializes diff files found by query' do
-          expect(diff_with_commits.merge_request_diff_files.count).to be > 10
-          expect_any_instance_of(MergeRequestDiffFile).to receive(:to_hash).once
+          it 'only serializes diff files found by query' do
+            expect(diff_with_commits.merge_request_diff_files.count).to be > 10
+            expect_any_instance_of(MergeRequestDiffFile).to receive(:to_hash).once
 
-          diffs
-        end
+            diffs
+          end
 
-        it 'uses the diffs from the DB' do
-          expect(diff_with_commits).to receive(:load_diffs)
+          it 'uses the preprocessed diffs' do
+            expect(diff_with_commits).to receive(:load_diffs)
 
-          diffs
+            diffs
+          end
         end
       end
     end
-  end
 
-  describe '#save_diffs' do
-    it 'saves collected state' do
-      mr_diff = create(:merge_request).merge_request_diff
+    describe '#save_diffs' do
+      it 'saves collected state' do
+        mr_diff = create(:merge_request).merge_request_diff
 
-      expect(mr_diff.collected?).to be_truthy
-    end
+        expect(mr_diff.collected?).to be_truthy
+      end
 
-    it 'saves overflow state' do
-      allow(Commit).to receive(:max_diff_options)
-        .and_return(max_lines: 0, max_files: 0)
+      it 'saves overflow state' do
+        allow(Commit).to receive(:max_diff_options)
+          .and_return(max_lines: 0, max_files: 0)
 
-      mr_diff = create(:merge_request).merge_request_diff
+        mr_diff = create(:merge_request).merge_request_diff
 
-      expect(mr_diff.overflow?).to be_truthy
-    end
+        expect(mr_diff.overflow?).to be_truthy
+      end
 
-    it 'saves empty state' do
-      allow_any_instance_of(described_class).to receive_message_chain(:compare, :commits)
-        .and_return([])
+      it 'saves empty state' do
+        allow_any_instance_of(described_class).to receive_message_chain(:compare, :commits)
+          .and_return([])
 
-      mr_diff = create(:merge_request).merge_request_diff
+        mr_diff = create(:merge_request).merge_request_diff
 
-      expect(mr_diff.empty?).to be_truthy
-    end
+        expect(mr_diff.empty?).to be_truthy
+      end
 
-    it 'expands collapsed diffs before saving' do
-      mr_diff = create(:merge_request, source_branch: 'expand-collapse-lines', target_branch: 'master').merge_request_diff
-      diff_file = mr_diff.merge_request_diff_files.find_by(new_path: 'expand-collapse/file-5.txt')
+      it 'expands collapsed diffs before saving' do
+        mr_diff = create(:merge_request, source_branch: 'expand-collapse-lines', target_branch: 'master').merge_request_diff
+        diff_file = mr_diff.merge_request_diff_files.find_by(new_path: 'expand-collapse/file-5.txt')
 
-      expect(diff_file.diff).not_to be_empty
+        expect(diff_file.diff).not_to be_empty
+      end
+
+      it 'saves binary diffs correctly' do
+        path = 'files/images/icn-time-tracking.pdf'
+        mr_diff = create(:merge_request, source_branch: 'add-pdf-text-binary', target_branch: 'master').merge_request_diff
+        diff_file = mr_diff.merge_request_diff_files.find_by(new_path: path)
+
+        expect(diff_file).to be_binary
+        expect(diff_file.diff).to eq(mr_diff.compare.diffs(paths: [path]).to_a.first.diff)
+      end
     end
+  end
 
-    it 'saves binary diffs correctly' do
-      path = 'files/images/icn-time-tracking.pdf'
-      mr_diff = create(:merge_request, source_branch: 'add-pdf-text-binary', target_branch: 'master').merge_request_diff
-      diff_file = mr_diff.merge_request_diff_files.find_by(new_path: path)
+  describe 'internal diffs configured' do
+    include_examples 'merge request diffs'
+  end
 
-      expect(diff_file).to be_binary
-      expect(diff_file.diff).to eq(mr_diff.compare.diffs(paths: [path]).to_a.first.diff)
+  describe 'external diffs configured' do
+    before do
+      stub_external_diffs_setting(enabled: true)
     end
+
+    include_examples 'merge request diffs'
   end
 
   describe '#commit_shas' do
@@ -245,4 +257,55 @@ describe MergeRequestDiff do
       expect(subject.modified_paths).to eq(%w{foo bar baz})
     end
   end
+
+  describe '#opening_external_diff' do
+    subject(:diff) { diff_with_commits }
+
+    context 'external diffs disabled' do
+      it { expect(diff.external_diff).not_to be_exists }
+
+      it 'yields nil' do
+        expect { |b| diff.opening_external_diff(&b) }.to yield_with_args(nil)
+      end
+    end
+
+    context 'external diffs enabled' do
+      let(:test_dir) { 'tmp/tests/external-diffs' }
+
+      around do |example|
+        FileUtils.mkdir_p(test_dir)
+
+        begin
+          example.run
+        ensure
+          FileUtils.rm_rf(test_dir)
+        end
+      end
+
+      before do
+        stub_external_diffs_setting(enabled: true, storage_path: test_dir)
+      end
+
+      it { expect(diff.external_diff).to be_exists }
+
+      it 'yields an open file' do
+        expect { |b| diff.opening_external_diff(&b) }.to yield_with_args(File)
+      end
+
+      it 'is re-entrant' do
+        outer_file_a =
+          diff.opening_external_diff do |outer_file|
+            diff.opening_external_diff do |inner_file|
+              expect(outer_file).to eq(inner_file)
+            end
+
+            outer_file
+          end
+
+        diff.opening_external_diff do |outer_file_b|
+          expect(outer_file_a).not_to eq(outer_file_b)
+        end
+      end
+    end
+  end
 end
diff --git a/spec/support/helpers/stub_configuration.rb b/spec/support/helpers/stub_configuration.rb
index 2851cd9733c..ff21bbe28ca 100644
--- a/spec/support/helpers/stub_configuration.rb
+++ b/spec/support/helpers/stub_configuration.rb
@@ -56,6 +56,10 @@ module StubConfiguration
     allow(Gitlab.config.lfs).to receive_messages(to_settings(messages))
   end
 
+  def stub_external_diffs_setting(messages)
+    allow(Gitlab.config.external_diffs).to receive_messages(to_settings(messages))
+  end
+
   def stub_artifacts_setting(messages)
     allow(Gitlab.config.artifacts).to receive_messages(to_settings(messages))
   end
diff --git a/spec/support/helpers/stub_object_storage.rb b/spec/support/helpers/stub_object_storage.rb
index 58b5c6a6435..e0c50e533a6 100644
--- a/spec/support/helpers/stub_object_storage.rb
+++ b/spec/support/helpers/stub_object_storage.rb
@@ -42,6 +42,13 @@ module StubObjectStorage
                                  **params)
   end
 
+  def stub_external_diffs_object_storage(uploader = described_class, **params)
+    stub_object_storage_uploader(config: Gitlab.config.external_diffs.object_store,
+                                 uploader: uploader,
+                                 remote_directory: 'external_diffs',
+                                 **params)
+  end
+
   def stub_lfs_object_storage(**params)
     stub_object_storage_uploader(config: Gitlab.config.lfs.object_store,
                                  uploader: LfsObjectUploader,
diff --git a/spec/uploaders/external_diff_uploader_spec.rb b/spec/uploaders/external_diff_uploader_spec.rb
new file mode 100644
index 00000000000..1c959770dc4
--- /dev/null
+++ b/spec/uploaders/external_diff_uploader_spec.rb
@@ -0,0 +1,67 @@
+require 'spec_helper'
+
+describe ExternalDiffUploader do
+  let(:diff) { create(:merge_request).merge_request_diff }
+  let(:path) { Gitlab.config.external_diffs.storage_path }
+
+  subject(:uploader) { described_class.new(diff, :external_diff) }
+
+  it_behaves_like "builds correct paths",
+                  store_dir: %r[merge_request_diffs/mr-\d+],
+                  cache_dir: %r[/external-diffs/tmp/cache],
+                  work_dir: %r[/external-diffs/tmp/work]
+
+  context "object store is REMOTE" do
+    before do
+      stub_external_diffs_object_storage
+    end
+
+    include_context 'with storage', described_class::Store::REMOTE
+
+    it_behaves_like "builds correct paths",
+                    store_dir: %r[merge_request_diffs/mr-\d+]
+  end
+
+  describe 'migration to object storage' do
+    context 'with object storage disabled' do
+      it "is skipped" do
+        expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async)
+
+        diff
+      end
+    end
+
+    context 'with object storage enabled' do
+      before do
+        stub_external_diffs_setting(enabled: true)
+        stub_external_diffs_object_storage(background_upload: true)
+      end
+
+      it 'is scheduled to run after creation' do
+        expect(ObjectStorage::BackgroundMoveWorker).to receive(:perform_async).with(described_class.name, 'MergeRequestDiff', :external_diff, kind_of(Numeric))
+
+        diff
+      end
+    end
+  end
+
+  describe 'remote file' do
+    context 'with object storage enabled' do
+      before do
+        stub_external_diffs_setting(enabled: true)
+        stub_external_diffs_object_storage
+
+        diff.update!(external_diff_store: described_class::Store::REMOTE)
+      end
+
+      it 'can store file remotely' do
+        allow(ObjectStorage::BackgroundMoveWorker).to receive(:perform_async)
+
+        diff
+
+        expect(diff.external_diff_store).to eq(described_class::Store::REMOTE)
+        expect(diff.external_diff.path).not_to be_blank
+      end
+    end
+  end
+end