Merge branch '37995-allow-external-diffs-to-be-removed' into 'master'

Allow external diff files to be removed.

Closes #37995

See merge request gitlab-org/gitlab!27602

app/models/merge_request_diff.rb

@@ -434,8 +434,50 @@ class MergeRequestDiff < ApplicationRecord
     merge_request_diff_files.reset
   end
 
+  # Transactionally migrate the current merge_request_diff_files entries from
+  # external storage, back to the database. This is the rollback operation for
+  # +migrate_files_to_external_storage!+
+  #
+  # If this diff isn't in external storage, the method is a no-op.
+  def migrate_files_to_database!
+    return unless stored_externally?
+    return if merge_request_diff_files.count == 0
+
+    rows = convert_external_diffs_to_database
+
+    transaction do
+      MergeRequestDiffFile.where(merge_request_diff_id: id).delete_all
+      Gitlab::Database.bulk_insert('merge_request_diff_files', rows)
+      update!(stored_externally: false)
+    end
+
+    # Only delete the external diff file after the contents have been saved to
+    # the database
+    remove_external_diff!
+    merge_request_diff_files.reset
+  end
+
   private
 
+  def convert_external_diffs_to_database
+    opening_external_diff do |external_file|
+      merge_request_diff_files.map do |diff_file|
+        row = diff_file.attributes.except('diff')
+
+        raise "Diff file lacks external diff offset or size: #{row.inspect}" unless
+          row['external_diff_offset'] && row['external_diff_size']
+
+        # The diff in the external file is already base64-encoded if necessary,
+        # matching the 'binary' attribute of the row. Reading it directly allows
+        # a cycle of decode-encode to be skipped
+        external_file.seek(row.delete('external_diff_offset'))
+        row['diff'] = external_file.read(row.delete('external_diff_size'))
+
+        row
+      end
+    end
+  end
+
   def diffs_in_batch_collection(batch_page, batch_size, diff_options:)
     Gitlab::Diff::FileCollection::MergeRequestDiffBatch.new(self,
                                                             batch_page,

changelogs/unreleased/37995-allow-external-diffs-to-be-removed.yml

+---
+title: Allow external diff files to be removed
+merge_request: 27602
+author:
+type: changed

spec/models/merge_request_diff_spec.rb

@@ -156,6 +156,41 @@ describe MergeRequestDiff do
     end
   end
 
+  describe '#migrate_files_to_database!' do
+    let(:diff) { create(:merge_request).merge_request_diff }
+
+    it 'converts from external to in-database storage' do
+      stub_external_diffs_setting(enabled: true)
+
+      expect(diff).to be_stored_externally
+      expect(diff).to receive(:update!).and_call_original
+
+      file = diff.external_diff
+      file_data = file.read
+      diff.migrate_files_to_database!
+
+      expect(diff).not_to be_stored_externally
+      expect(file).not_to exist
+      expect(diff.merge_request_diff_files.map(&:diff).join('')).to eq(file_data)
+    end
+
+    it 'does nothing with an in-database diff' do
+      expect(diff).not_to be_stored_externally
+      expect(diff).not_to receive(:update!)
+
+      diff.migrate_files_to_database!
+    end
+
+    it 'does nothing with an empty diff' do
+      stub_external_diffs_setting(enabled: true)
+      MergeRequestDiffFile.where(merge_request_diff_id: diff.id).delete_all
+
+      expect(diff).not_to receive(:update!)
+
+      diff.migrate_files_to_database!
+    end
+  end
+
   describe '#latest?' do
     let!(:mr) { create(:merge_request, :with_diffs) }
     let!(:first_diff) { mr.merge_request_diff }