diff --git a/app/workers/backup_job.rb b/app/workers/backup_job.rb index 9626d673585b..267550194918 100644 --- a/app/workers/backup_job.rb +++ b/app/workers/backup_job.rb @@ -161,16 +161,19 @@ def create_backup_archive!(file_name:, db_dump_file_name:, attachments: attachme zipfile.get_output_stream("openproject.sql") { |f| f.write File.read(db_dump_file_name) } end - # delete locally cached files that were downloaded just for the backup - paths_to_clean.each do |path| - FileUtils.rm_rf path - end + remove_paths! paths_to_clean # delete locally cached files that were downloaded just for the backup @archived = true file_name end + def remove_paths!(paths) + paths.each do |path| + FileUtils.rm_rf path + end + end + def get_cache_folder_path(attachment) # expecting paths like /tmp/op_uploaded_files/1639754082-3468-0002-0911/file.ext # just making extra sure so we don't delete anything wrong later on @@ -178,6 +181,7 @@ def get_cache_folder_path(attachment) raise "Unexpected cache path for attachment ##{attachment.id}: #{attachment.diskfile}" end + # returning parent as each cached file is in a separate folder which shall be removed too Pathname(attachment.diskfile.path).parent.to_s end diff --git a/spec/workers/backup_job_spec.rb b/spec/workers/backup_job_spec.rb index 1bb19fbc1013..69aaf29ce0e5 100644 --- a/spec/workers/backup_job_spec.rb +++ b/spec/workers/backup_job_spec.rb @@ -57,7 +57,7 @@ let(:db_dump_success) { false } - let(:arguments) { [{ backup: backup, user: user, **opts }] } + let(:arguments) { [{ backup: backup, user: user, **opts.except(:remote_storage) }] } let(:user) { FactoryBot.create :admin } @@ -105,7 +105,11 @@ def backed_up_attachment(attachment) "attachment/file/#{attachment.id}/#{attachment.filename}" end - before { perform } + before do + allow(job).to receive(:remove_paths!) + + perform + end it "destroys any previous backups" do expect(Backup.find_by(id: previous_backup.id)).to be_nil @@ -127,6 +131,16 @@ def backed_up_attachment(attachment) it "does not include pending direct uploads" do expect(backup_files).not_to include backed_up_attachment(pending_direct_upload) end + + if opts[:remote_storage] == true + it "cleans up locally cached files afterwards" do + expect(job).to have_received(:remove_paths!).with([Pathname(attachment.diskfile.path).parent.to_s]) + end + else + it "does not clean up files afterwards as none were cached" do + expect(job).to have_received(:remove_paths!).with([]) + end + end else it "does not include attachments in the backup" do expect(backup_files).not_to include backed_up_attachment(attachment) @@ -140,6 +154,35 @@ def backed_up_attachment(attachment) it_behaves_like "it creates a backup" end + context( + "with remote storage", + with_config: { + attachments_storage: :fog, + fog: { + directory: MockCarrierwave.bucket, + credentials: MockCarrierwave.credentials + } + } + ) do + let(:dummy_path) { "/tmp/op_uploaded_files/1639754082-3468-0002-0911/file.ext" } + + before do + FileUtils.mkdir_p Pathname(dummy_path).parent.to_s + File.open(dummy_path, "w") { |f| f.puts 'dummy' } + + allow_any_instance_of(LocalFileUploader).to receive(:cached?).and_return(true) + allow_any_instance_of(LocalFileUploader) + .to receive(:local_file) + .and_return(File.new(dummy_path)) + end + + after do + FileUtils.rm dummy_path if File.exist? dummy_path + end + + it_behaves_like "it creates a backup", remote_storage: true + end + context "with include_attachments: false" do it_behaves_like "it creates a backup", include_attachments: false end