Support object storage at FileMover class
This commit is contained in:
parent
44e1915d4f
commit
114dd97642
|
@ -1,6 +1,8 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
class FileMover
|
||||
include Gitlab::Utils::StrongMemoize
|
||||
|
||||
attr_reader :secret, :file_name, :from_model, :to_model, :update_field
|
||||
|
||||
def initialize(file_path, update_field = :description, from_model:, to_model:)
|
||||
|
@ -12,8 +14,12 @@ class FileMover
|
|||
end
|
||||
|
||||
def execute
|
||||
temp_file_uploader.retrieve_from_store!(file_name)
|
||||
|
||||
return unless valid?
|
||||
|
||||
uploader.retrieve_from_store!(file_name)
|
||||
|
||||
move
|
||||
|
||||
if update_markdown
|
||||
|
@ -25,14 +31,23 @@ class FileMover
|
|||
private
|
||||
|
||||
def valid?
|
||||
Pathname.new(temp_file_path).realpath.to_path.start_with?(
|
||||
(Pathname(temp_file_uploader.root) + temp_file_uploader.base_dir).to_path
|
||||
)
|
||||
if temp_file_uploader.file_storage?
|
||||
Pathname.new(temp_file_path).realpath.to_path.start_with?(
|
||||
(Pathname(temp_file_uploader.root) + temp_file_uploader.base_dir).to_path
|
||||
)
|
||||
else
|
||||
temp_file_uploader.exists?
|
||||
end
|
||||
end
|
||||
|
||||
def move
|
||||
FileUtils.mkdir_p(File.dirname(file_path))
|
||||
FileUtils.move(temp_file_path, file_path)
|
||||
if temp_file_uploader.file_storage?
|
||||
FileUtils.mkdir_p(File.dirname(file_path))
|
||||
FileUtils.move(temp_file_path, file_path)
|
||||
else
|
||||
uploader.copy_file(temp_file_uploader.file)
|
||||
temp_file_uploader.upload.destroy!
|
||||
end
|
||||
end
|
||||
|
||||
def update_markdown
|
||||
|
@ -46,28 +61,36 @@ class FileMover
|
|||
|
||||
def update_upload_model
|
||||
return unless upload = temp_file_uploader.upload
|
||||
return if upload.destroyed?
|
||||
|
||||
upload.update!(model_id: to_model.id, model_type: to_model.type)
|
||||
upload.update!(model: to_model)
|
||||
end
|
||||
|
||||
def temp_file_path
|
||||
return @temp_file_path if @temp_file_path
|
||||
|
||||
temp_file_uploader.retrieve_from_store!(file_name)
|
||||
|
||||
@temp_file_path = temp_file_uploader.file.path
|
||||
strong_memoize(:temp_file_path) do
|
||||
temp_file_uploader.file.path
|
||||
end
|
||||
end
|
||||
|
||||
def file_path
|
||||
return @file_path if @file_path
|
||||
|
||||
uploader.retrieve_from_store!(file_name)
|
||||
|
||||
@file_path = uploader.file.path
|
||||
strong_memoize(:file_path) do
|
||||
uploader.file.path
|
||||
end
|
||||
end
|
||||
|
||||
def uploader
|
||||
@uploader ||= PersonalFileUploader.new(to_model, secret: secret)
|
||||
@uploader ||=
|
||||
begin
|
||||
uploader = PersonalFileUploader.new(to_model, secret: secret)
|
||||
|
||||
# Enforcing a REMOTE object storage given FileUploader#retrieve_from_store! won't do it
|
||||
# (there's no upload at the target yet).
|
||||
if uploader.class.object_store_enabled?
|
||||
uploader.object_store = ::ObjectStorage::Store::REMOTE
|
||||
end
|
||||
|
||||
uploader
|
||||
end
|
||||
end
|
||||
|
||||
def temp_file_uploader
|
||||
|
@ -77,6 +100,8 @@ class FileMover
|
|||
def revert
|
||||
Rails.logger.warn("Markdown not updated, file move reverted for #{to_model}")
|
||||
|
||||
FileUtils.move(file_path, temp_file_path)
|
||||
if temp_file_uploader.file_storage?
|
||||
FileUtils.move(file_path, temp_file_path)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -23,63 +23,110 @@ describe FileMover do
|
|||
subject { described_class.new(temp_file_path, from_model: user, to_model: snippet).execute }
|
||||
|
||||
describe '#execute' do
|
||||
before do
|
||||
tmp_uploader.store!(file)
|
||||
|
||||
expect(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path)))
|
||||
expect(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path))
|
||||
allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true)
|
||||
allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10)
|
||||
|
||||
stub_file_mover(temp_file_path)
|
||||
end
|
||||
|
||||
let(:tmp_upload) { tmp_uploader.upload }
|
||||
|
||||
context 'when move and field update successful' do
|
||||
it 'updates the description correctly' do
|
||||
subject
|
||||
before do
|
||||
tmp_uploader.store!(file)
|
||||
end
|
||||
|
||||
expect(snippet.reload.description)
|
||||
.to eq(
|
||||
"test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
|
||||
"same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "
|
||||
)
|
||||
context 'local storage' do
|
||||
before do
|
||||
allow(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path)))
|
||||
allow(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path))
|
||||
allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true)
|
||||
allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10)
|
||||
|
||||
stub_file_mover(temp_file_path)
|
||||
end
|
||||
|
||||
it 'updates existing upload record' do
|
||||
expect { subject }
|
||||
.to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') }
|
||||
.from([user.id, 'User']).to([snippet.id, 'PersonalSnippet'])
|
||||
context 'when move and field update successful' do
|
||||
it 'updates the description correctly' do
|
||||
subject
|
||||
|
||||
expect(snippet.reload.description)
|
||||
.to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
|
||||
"same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ")
|
||||
end
|
||||
|
||||
it 'updates existing upload record' do
|
||||
expect { subject }
|
||||
.to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') }
|
||||
.from([user.id, 'User']).to([snippet.id, 'Snippet'])
|
||||
end
|
||||
|
||||
it 'schedules a background migration' do
|
||||
expect_any_instance_of(PersonalFileUploader).to receive(:schedule_background_upload).once
|
||||
|
||||
subject
|
||||
end
|
||||
end
|
||||
|
||||
it 'schedules a background migration' do
|
||||
expect_any_instance_of(PersonalFileUploader).to receive(:schedule_background_upload).once
|
||||
context 'when update_markdown fails' do
|
||||
before do
|
||||
expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path))
|
||||
end
|
||||
|
||||
subject
|
||||
subject { described_class.new(file_path, :non_existing_field, from_model: user, to_model: snippet).execute }
|
||||
|
||||
it 'does not update the description' do
|
||||
subject
|
||||
|
||||
expect(snippet.reload.description)
|
||||
.to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
|
||||
"same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ")
|
||||
end
|
||||
|
||||
it 'does not change the upload record' do
|
||||
expect { subject }
|
||||
.not_to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') }
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when update_markdown fails' do
|
||||
context 'when tmp uploader is not local storage' do
|
||||
before do
|
||||
expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path))
|
||||
allow(PersonalFileUploader).to receive(:object_store_enabled?) { true }
|
||||
tmp_uploader.object_store = ObjectStorage::Store::REMOTE
|
||||
allow_any_instance_of(PersonalFileUploader).to receive(:file_storage?) { false }
|
||||
end
|
||||
|
||||
subject { described_class.new(file_path, :non_existing_field, from_model: user, to_model: snippet).execute }
|
||||
|
||||
it 'does not update the description' do
|
||||
subject
|
||||
|
||||
expect(snippet.reload.description)
|
||||
.to eq(
|
||||
"test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
|
||||
"same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "
|
||||
)
|
||||
after do
|
||||
FileUtils.rm_f(File.join('personal_snippet', snippet.id.to_s, secret, filename))
|
||||
end
|
||||
|
||||
it 'does not change the upload record' do
|
||||
expect { subject }
|
||||
.not_to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') }
|
||||
context 'when move and field update successful' do
|
||||
it 'updates the description correctly' do
|
||||
subject
|
||||
|
||||
expect(snippet.reload.description)
|
||||
.to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
|
||||
"same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ")
|
||||
end
|
||||
|
||||
it 'creates new target upload record an delete the old upload' do
|
||||
expect { subject }
|
||||
.to change { Upload.last.attributes.values_at('model_id', 'model_type') }
|
||||
.from([user.id, 'User']).to([snippet.id, 'Snippet'])
|
||||
|
||||
expect(Upload.count).to eq(1)
|
||||
end
|
||||
end
|
||||
|
||||
context 'when update_markdown fails' do
|
||||
subject { described_class.new(file_path, :non_existing_field, from_model: user, to_model: snippet).execute }
|
||||
|
||||
it 'does not update the description' do
|
||||
subject
|
||||
|
||||
expect(snippet.reload.description)
|
||||
.to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
|
||||
"same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ")
|
||||
end
|
||||
|
||||
it 'does not change the upload record' do
|
||||
expect { subject }
|
||||
.to change { Upload.last.attributes.values_at('model_id', 'model_type') }.from([user.id, 'User'])
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
Loading…
Reference in New Issue