Set MergeRequestDiff#external_diff_store in more cases

Previously, we skipped setting this value on import, and only made a
change if the external_diff column had changed for a row. However, this
turned out to be counter-productive.

On import, MR diff files are not created through this mechanism anyway,
and sometimes we can update an MR diff while not changing the value of
the external_diff column (e.g. on migration). Just setting this should
be safe in both cases.

app/models/merge_request_diff.rb

@@ -141,7 +141,7 @@ class MergeRequestDiff < ApplicationRecord
   after_create :save_git_content, unless: :importing?
   after_create_commit :set_as_latest_diff, unless: :importing?
 
-  after_save :update_external_diff_store, if: -> { !importing? && saved_change_to_external_diff? }
+  after_save :update_external_diff_store
 
   def self.find_by_diff_refs(diff_refs)
     find_by(start_commit_sha: diff_refs.start_sha, head_commit_sha: diff_refs.head_sha, base_commit_sha: diff_refs.base_sha)
@@ -401,8 +401,10 @@ class MergeRequestDiff < ApplicationRecord
   end
 
   def update_external_diff_store
-    update_column(:external_diff_store, external_diff.object_store) if
-      has_attribute?(:external_diff_store)
+    return unless has_attribute?(:external_diff_store)
+    return unless saved_change_to_external_diff? || saved_change_to_stored_externally?
+
+    update_column(:external_diff_store, external_diff.object_store)
   end
 
   def saved_change_to_external_diff?

changelogs/unreleased/216216-fix-mr-diffs-external-diffs-store-bug.yml

+---
+title: Correctly track the store that external MR diffs are placed on
+merge_request: 31005
+author:
+type: fixed

spec/models/merge_request_diff_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe MergeRequestDiff do
+  using RSpec::Parameterized::TableSyntax
+
   include RepoHelpers
 
   let(:diff_with_commits) { create(:merge_request).merge_request_diff }
@@ -125,18 +127,71 @@ describe MergeRequestDiff do
     end
   end
 
+  describe '#update_external_diff_store' do
+    let_it_be(:merge_request) { create(:merge_request) }
+
+    let(:diff) { merge_request.merge_request_diff }
+    let(:store) { diff.external_diff.object_store }
+
+    where(:change_stored_externally, :change_external_diff) do
+      false | false
+      false | true
+      true  | false
+      true  | true
+    end
+
+    with_them do
+      it do
+        diff.stored_externally = true if change_stored_externally
+        diff.external_diff = "new-filename" if change_external_diff
+
+        update_store = receive(:update_column).with(:external_diff_store, store)
+
+        if change_stored_externally || change_external_diff
+          expect(diff).to update_store
+        else
+          expect(diff).not_to update_store
+        end
+
+        diff.save!
+      end
+    end
+  end
+
   describe '#migrate_files_to_external_storage!' do
+    let(:uploader) { ExternalDiffUploader }
+    let(:file_store) { uploader::Store::LOCAL }
+    let(:remote_store) { uploader::Store::REMOTE }
     let(:diff) { create(:merge_request).merge_request_diff }
 
-    it 'converts from in-database to external storage' do
+    it 'converts from in-database to external file storage' do
       expect(diff).not_to be_stored_externally
 
       stub_external_diffs_setting(enabled: true)
-      expect(diff).to receive(:save!)
+
+      expect(diff).to receive(:save!).and_call_original
+
+      diff.migrate_files_to_external_storage!
+
+      expect(diff).to be_stored_externally
+      expect(diff.external_diff_store).to eq(file_store)
+    end
+
+    it 'converts from in-database to external object storage' do
+      expect(diff).not_to be_stored_externally
+
+      stub_external_diffs_setting(enabled: true)
+
+      # Without direct_upload: true, the files would be saved to disk, and a
+      # background job would be enqueued to move the file to object storage
+      stub_external_diffs_object_storage(uploader, direct_upload: true)
+
+      expect(diff).to receive(:save!).and_call_original
 
       diff.migrate_files_to_external_storage!
 
       expect(diff).to be_stored_externally
+      expect(diff.external_diff_store).to eq(remote_store)
     end
 
     it 'does nothing with an external diff' do

spec/support/helpers/stub_object_storage.rb

@@ -45,7 +45,7 @@ module StubObjectStorage
   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',
+                                 remote_directory: 'external-diffs',
                                  **params)
   end