diff options
| -rw-r--r-- | app/uploaders/file_mover.rb | 61 | ||||
| -rw-r--r-- | spec/uploaders/file_mover_spec.rb | 119 |
2 files changed, 126 insertions, 54 deletions
diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb index dcf1e8792ad..12be1e2bb22 100644 --- a/app/uploaders/file_mover.rb +++ b/app/uploaders/file_mover.rb @@ -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 diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb index d5e8a90cecd..322c37b6050 100644 --- a/spec/uploaders/file_mover_spec.rb +++ b/spec/uploaders/file_mover_spec.rb @@ -23,63 +23,110 @@ describe FileMover do subject { described_class.new(temp_file_path, from_model: user, to_model: snippet).execute } describe '#execute' do + let(:tmp_upload) { tmp_uploader.upload } + before do tmp_uploader.store!(file) + end - 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) + 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 + 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 - context 'when move and field update successful' do - it 'updates the description correctly' do - subject + expect(snippet.reload.description) + .to eq("test  "\ + "same  ") + end - expect(snippet.reload.description) - .to eq( - "test  "\ - "same  " - ) - 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 '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']) + 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  "\ + "same  ") + 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 } + after do + FileUtils.rm_f(File.join('personal_snippet', snippet.id.to_s, secret, filename)) + end - it 'does not update the description' do - subject + context 'when move and field update successful' do + it 'updates the description correctly' do + subject - expect(snippet.reload.description) - .to eq( - "test  "\ - "same  " - ) + expect(snippet.reload.description) + .to eq("test  "\ + "same  ") + 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 - 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 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  "\ + "same  ") + 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 |