From da3320140bc8005ad9f5c2b03023cc6ca8fab4b6 Mon Sep 17 00:00:00 2001 From: Mason Ballengee Date: Wed, 8 Jan 2025 14:29:10 -0500 Subject: [PATCH] Create 'FileMover.move' convenience method This commit also includes some tightening up of the test suite and removing an unneeded `File#rewind` call from `FileLocator#local_file`. --- app/jobs/master_file_management_jobs.rb | 3 +- app/models/master_file.rb | 3 +- app/services/file_locator.rb | 1 - app/services/file_mover.rb | 60 ++++++++++++++------- spec/services/file_mover_spec.rb | 72 ++++++++++++------------- 5 files changed, 78 insertions(+), 61 deletions(-) diff --git a/app/jobs/master_file_management_jobs.rb b/app/jobs/master_file_management_jobs.rb index 44ab6e70d9..f01ddb76ab 100644 --- a/app/jobs/master_file_management_jobs.rb +++ b/app/jobs/master_file_management_jobs.rb @@ -29,8 +29,7 @@ def perform(id, newpath) Rails.logger.info "Masterfile #{newpath} already moved" elsif old_locator.exists? new_locator = FileLocator.new(newpath) - copy_method = "#{old_locator.uri.scheme}_to_#{new_locator.uri.scheme}".to_sym - FileMover.send(copy_method, old_locator, new_locator) + FileMover.move(old_locator, new_locator) masterfile.file_location = newpath masterfile.save Rails.logger.info "#{oldpath} has been moved to #{newpath}" diff --git a/app/models/master_file.rb b/app/models/master_file.rb index 9c54fd811f..3951ca10ee 100644 --- a/app/models/master_file.rb +++ b/app/models/master_file.rb @@ -696,8 +696,7 @@ def saveOriginal(file, original_name = nil, dropbox_dir = media_object.collectio end old_locator = FileLocator.new(realpath) new_locator = FileLocator.new(path) - copy_method = "#{old_locator.uri.scheme}_to_#{new_locator.uri.scheme}".to_sym - FileMover.send(copy_method, old_locator, new_locator) + FileMover.move(old_locator, new_locator) realpath = path end diff --git a/app/services/file_locator.rb b/app/services/file_locator.rb index 081e4d2af1..24e6045b12 100644 --- a/app/services/file_locator.rb +++ b/app/services/file_locator.rb @@ -107,7 +107,6 @@ def local_location def local_file @local_file ||= Tempfile.new(filename) File.binwrite(@local_file, reader.read) - @local_file.rewind @local_file ensure @local_file.close diff --git a/app/services/file_mover.rb b/app/services/file_mover.rb index ea7efe8984..b5320cccd4 100644 --- a/app/services/file_mover.rb +++ b/app/services/file_mover.rb @@ -14,32 +14,52 @@ class FileMover class << self - def s3_to_s3(source, dest) - source_object = FileLocator::S3File.new(source.source).object - dest_object = FileLocator::S3File.new(dest.source).object - if dest_object.copy_from(source_object, multipart_copy: source_object.size > 15.megabytes) - source_object.delete if FileLocator.new(dest.source).exists? - end + def move(source, dest, method: nil) + new(source, dest, method: method).move end + end + + def initialize(source, dest, method: nil) + @source = source + @dest = dest + @method = method + end + + def move + send(method, @source, @dest) + end + + def method + @method || "#{@source.uri.scheme}_to_#{@dest.uri.scheme}".to_sym + end + + private - def s3_to_file(source, dest) - source_object = FileLocator::S3File.new(source.source).object - FileUtils.mkdir_p File.dirname(dest.uri.path) unless File.exist? File.dirname(dest.uri.path) - if source_object.download_file(dest.uri.path) - source_object.delete - end + def s3_to_s3(source, dest) + source_object = FileLocator::S3File.new(source.source).object + dest_object = FileLocator::S3File.new(dest.source).object + if dest_object.copy_from(source_object, multipart_copy: source_object.size > 15.megabytes) + source_object.delete if FileLocator.new(dest.source).exists? end + end - def file_to_s3(source, dest) - dest_object = FileLocator::S3File.new(dest.source).object - if dest_object.upload_file(source.uri.path) - FileUtils.rm(source.uri.path) - end + def s3_to_file(source, dest) + source_object = FileLocator::S3File.new(source.source).object + FileUtils.mkdir_p File.dirname(dest.uri.path) unless File.exist? File.dirname(dest.uri.path) + if source_object.download_file(dest.uri.path) + source_object.delete end + end - def file_to_file(source, dest) - FileUtils.mkdir_p File.dirname(dest.location) unless File.exist? File.dirname(dest.location) - FileUtils.mv source.location, dest.location + def file_to_s3(source, dest) + dest_object = FileLocator::S3File.new(dest.source).object + if dest_object.upload_file(source.uri.path) + FileUtils.rm(source.uri.path) end end + + def file_to_file(source, dest) + FileUtils.mkdir_p File.dirname(dest.location) unless File.exist? File.dirname(dest.location) + FileUtils.mv source.location, dest.location + end end \ No newline at end of file diff --git a/spec/services/file_mover_spec.rb b/spec/services/file_mover_spec.rb index c3be7f5e22..6b453e01f5 100644 --- a/spec/services/file_mover_spec.rb +++ b/spec/services/file_mover_spec.rb @@ -16,78 +16,78 @@ require 'fakefs/safe' describe FileMover, type: :service do - let(:fs_location) { '/path/to/source/test.mp4' } - let(:fs_file) { FileLocator.new("file://#{fs_location}") } - let(:fs_dest) { FileLocator.new('file:///path/to/dest/test.mp4') } - - context 's3 methods' do + describe '.move' do + let(:fs_location) { '/path/to/source/test.mp4' } + let(:fs_file) { FileLocator.new("file://#{fs_location}") } + let(:fs_dest) { FileLocator.new('file:///path/to/dest/test.mp4') } let(:s3_file) { FileLocator.new('s3://source_bucket/test.mp4') } let(:s3_dest) { FileLocator.new('s3://dest_bucket/test.mp4') } - before :each do - @source_object = double(key: 'test.mp4', bucket_name: 'source_bucket') + before(:each, s3: true) do + @source_object = double(key: 'test.mp4', bucket_name: 'source_bucket', size: 1) + @dest_object = double(key: 'test.mp4', bucket_name: 'dest_bucket', exists?: true) allow(Aws::S3::Object).to receive(:new).and_call_original allow(Aws::S3::Object).to receive(:new).with(key: 'test.mp4', bucket_name: 'source_bucket').and_return(@source_object) - allow(@source_object).to receive(:size).and_return(1) + allow(Aws::S3::Object).to receive(:new).with(key: 'test.mp4', bucket_name: 'dest_bucket').and_return(@dest_object) allow(@source_object).to receive(:delete).and_return(true) allow(@source_object).to receive(:download_file).and_return(true) + allow(@dest_object).to receive(:copy_from).and_return(true) end - describe '.s3_to_s3' do + describe 's3 source to s3 dest', s3: true do it 'copies file from source to dest' do - expect_any_instance_of(Aws::S3::Object).to receive(:copy_from).with(Aws::S3::Object.new(key: 'test.mp4', bucket_name: 'source_bucket'), multipart_copy: false) - described_class.s3_to_s3(s3_file, s3_dest) + expect(@dest_object).to receive(:copy_from).with(Aws::S3::Object.new(key: 'test.mp4', bucket_name: 'source_bucket'), multipart_copy: false) + described_class.move(s3_file, s3_dest) end it 'deletes file after copying' do - allow_any_instance_of(Aws::S3::Object).to receive(:copy_from).and_return(true) expect(@source_object).to receive(:delete) - described_class.s3_to_s3(s3_file, s3_dest) + described_class.move(s3_file, s3_dest) end end - describe '.s3_to_file' do + describe 's3 source to filesystem dest', s3: true do it 'copies file from source to dest' do expect(@source_object).to receive(:download_file).with(fs_dest.uri.path) - described_class.s3_to_file(s3_file, fs_dest) + described_class.move(s3_file, fs_dest) end it 'deletes file after copying' do expect(@source_object).to receive(:delete) - described_class.s3_to_file(s3_file, fs_dest) + described_class.move(s3_file, fs_dest) end end - describe '.file_to_s3' do + describe 'filesystem source to s3 dest', s3: true do it 'copies file from source to dest' do - expect_any_instance_of(Aws::S3::Object).to receive(:upload_file).with(fs_file.uri.path) - described_class.file_to_s3(fs_file, s3_dest) + expect(@dest_object).to receive(:upload_file).with(fs_file.uri.path) + described_class.move(fs_file, s3_dest) end it 'deletes file after copying' do - allow_any_instance_of(Aws::S3::Object).to receive(:upload_file).and_return(true) + allow(@dest_object).to receive(:upload_file).and_return(true) expect(FileUtils).to receive(:rm).with(fs_file.uri.path) - described_class.file_to_s3(fs_file, s3_dest) + described_class.move(fs_file, s3_dest) end end - end - describe '.file_to_file' do - before :each do - FakeFS.activate! - FileUtils.mkdir_p File.dirname(fs_location) - File.open(fs_location, 'w'){} - end + describe 'file system source to filesystem dest' do + before :each do + FakeFS.activate! + FileUtils.mkdir_p File.dirname(fs_location) + File.open(fs_location, 'w'){} + end - after :each do - FakeFS.deactivate! - end + after :each do + FakeFS.deactivate! + end - it 'moves file from source to dest' do - expect(File.exist? fs_location).to be true - described_class.file_to_file(fs_file, fs_dest) - expect(File.exist? fs_location).to be false - expect(File.exist? '/path/to/dest/test.mp4').to be true + it 'moves file from source to dest' do + expect(File.exist? fs_location).to be true + described_class.move(fs_file, fs_dest) + expect(File.exist? fs_location).to be false + expect(File.exist? '/path/to/dest/test.mp4').to be true + end end end end \ No newline at end of file