diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 7b15d21c0956ea47c227562d489c1993fb130602..ea3e00d9cd3d0a33b9d1d2b66cc14a4f42796c49 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -141,7 +141,7 @@ def self.ids_for_external_storage_migration(limit:) 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 @@ def write_uploader(column, identifier) 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? diff --git a/changelogs/unreleased/216216-fix-mr-diffs-external-diffs-store-bug.yml b/changelogs/unreleased/216216-fix-mr-diffs-external-diffs-store-bug.yml new file mode 100644 index 0000000000000000000000000000000000000000..6051b3f70c620d625c19f9866a89baae65e0626b --- /dev/null +++ b/changelogs/unreleased/216216-fix-mr-diffs-external-diffs-store-bug.yml @@ -0,0 +1,5 @@ +--- +title: Correctly track the store that external MR diffs are placed on +merge_request: 31005 +author: +type: fixed diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 016af4f269b8e2e0596bbdfff1bd3d9175b62d59..0839dde696a76e6c7c3bb44e8a4447f0f67884d4 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/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 @@ 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 diff --git a/spec/support/helpers/stub_object_storage.rb b/spec/support/helpers/stub_object_storage.rb index d4ac286e9596610f91a8aa3aec4cad8089dcf52c..b473cdaefc116ad9cede6d725360c265db94b62d 100644 --- a/spec/support/helpers/stub_object_storage.rb +++ b/spec/support/helpers/stub_object_storage.rb @@ -45,7 +45,7 @@ def stub_artifacts_object_storage(**params) 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