Commit 455dfb68 authored by Douglas Barbosa Alexandre's avatar Douglas Barbosa Alexandre

Merge branch '216216-fix-mr-diffs-external-diffs-store-bug' into 'master'

Set MergeRequestDiff#external_diff_store in more cases

Closes #197317

See merge request gitlab-org/gitlab!31005
parents ac828d46 f7ca4bea
...@@ -141,7 +141,7 @@ class MergeRequestDiff < ApplicationRecord ...@@ -141,7 +141,7 @@ class MergeRequestDiff < ApplicationRecord
after_create :save_git_content, unless: :importing? after_create :save_git_content, unless: :importing?
after_create_commit :set_as_latest_diff, 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) 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) 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 ...@@ -401,8 +401,10 @@ class MergeRequestDiff < ApplicationRecord
end end
def update_external_diff_store def update_external_diff_store
update_column(:external_diff_store, external_diff.object_store) if return unless has_attribute?(:external_diff_store)
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 end
def saved_change_to_external_diff? def saved_change_to_external_diff?
......
---
title: Correctly track the store that external MR diffs are placed on
merge_request: 31005
author:
type: fixed
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
require 'spec_helper' require 'spec_helper'
describe MergeRequestDiff do describe MergeRequestDiff do
using RSpec::Parameterized::TableSyntax
include RepoHelpers include RepoHelpers
let(:diff_with_commits) { create(:merge_request).merge_request_diff } let(:diff_with_commits) { create(:merge_request).merge_request_diff }
...@@ -125,18 +127,71 @@ describe MergeRequestDiff do ...@@ -125,18 +127,71 @@ describe MergeRequestDiff do
end end
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 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 } 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 expect(diff).not_to be_stored_externally
stub_external_diffs_setting(enabled: true) 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! diff.migrate_files_to_external_storage!
expect(diff).to be_stored_externally expect(diff).to be_stored_externally
expect(diff.external_diff_store).to eq(remote_store)
end end
it 'does nothing with an external diff' do it 'does nothing with an external diff' do
......
...@@ -45,7 +45,7 @@ module StubObjectStorage ...@@ -45,7 +45,7 @@ module StubObjectStorage
def stub_external_diffs_object_storage(uploader = described_class, **params) def stub_external_diffs_object_storage(uploader = described_class, **params)
stub_object_storage_uploader(config: Gitlab.config.external_diffs.object_store, stub_object_storage_uploader(config: Gitlab.config.external_diffs.object_store,
uploader: uploader, uploader: uploader,
remote_directory: 'external_diffs', remote_directory: 'external-diffs',
**params) **params)
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment