From b2a8e03f62f77460d863937b567a70d2b3e617bf Mon Sep 17 00:00:00 2001 From: Sony Mathew <2040199+sony-mathew@users.noreply.github.com> Date: Tue, 16 Jun 2026 18:25:42 +0530 Subject: [PATCH] test(storage): cover blob migration contract Replace mock-only migrator specs with disk-backed ActiveStorage blob coverage for non-image uploads, service-name updates, missing files, upload failures, and idempotent reruns. Add a rake task regression for re-enabled repeated invocation. --- spec/lib/active_storage/migrator_spec.rb | 121 +++++++++++++----- .../rake/task_storage_migrations_spec.rb | 10 ++ 2 files changed, 101 insertions(+), 30 deletions(-) diff --git a/spec/lib/active_storage/migrator_spec.rb b/spec/lib/active_storage/migrator_spec.rb index 2af2aba2c..11bf8a120 100644 --- a/spec/lib/active_storage/migrator_spec.rb +++ b/spec/lib/active_storage/migrator_spec.rb @@ -1,5 +1,7 @@ require 'rails_helper' +require 'fileutils' require 'stringio' +require 'tmpdir' RSpec.describe ActiveStorage::Migrator do describe '.migrate' do @@ -44,61 +46,120 @@ RSpec.describe ActiveStorage::Migrator do end describe '.migrate_blobs' do - let(:to_service_stub) { instance_double(ActiveStorage::Service) } - let(:source_blobs) { instance_double(ActiveRecord::Relation, count: 1) } - let(:blob) { instance_double(ActiveStorage::Blob, id: 1, key: 'blob-key', checksum: 'checksum') } - let(:io) { StringIO.new('blob-content') } + let(:from_root) { Dir.mktmpdir('active-storage-local') } + let(:to_root) { Dir.mktmpdir('active-storage-amazon') } + let(:other_root) { Dir.mktmpdir('active-storage-other') } + let(:from_service) { build_disk_service('local', from_root) } + let(:to_service) { build_disk_service('amazon', to_root) } + let(:other_service) { build_disk_service('other', other_root) } before do - allow(ActiveStorage::Service).to receive(:configure).with(:amazon, any_args).and_return(to_service_stub) - allow(ActiveStorage::Blob).to receive(:where).with(service_name: 'local').and_return(source_blobs) - allow(source_blobs).to receive(:find_each).and_yield(blob) - allow(blob).to receive(:open).and_yield(io) - allow(blob).to receive(:update!) - allow(to_service_stub).to receive(:upload) + allow(described_class).to receive(:configure_service).with(:amazon).and_return(to_service) end - it 'migrates blobs regardless of content type' do - expect(to_service_stub).to receive(:upload).with('blob-key', io, checksum: 'checksum').ordered - expect(blob).to receive(:update!).with(service_name: 'amazon').ordered + around do |example| + original_services = ActiveStorage::Blob.services + original_service = ActiveStorage::Blob.service + + ActiveStorage::Blob.services = { + 'local' => from_service, + 'amazon' => to_service, + 'other' => other_service + } + ActiveStorage::Blob.service = from_service + + example.run + ensure + ActiveStorage::Blob.services = original_services + ActiveStorage::Blob.service = original_service + FileUtils.rm_rf([from_root, to_root, other_root]) + end + + it 'uploads non-image blobs and updates their service name' do + blob = create_blob(service_name: 'local', filename: 'invoice.pdf', content_type: 'application/pdf', content: 'pdf-content') described_class.migrate_blobs(:local, :amazon, should_update_service_name: true) + + expect(blob).not_to be_image + expect(blob.reload.service_name).to eq('amazon') + expect(to_service.download(blob.key)).to eq('pdf-content') end it 'only migrates blobs from the source service' do - expect(ActiveStorage::Blob).to receive(:where).with(service_name: 'local').and_return(source_blobs) - expect(source_blobs).to receive(:find_each).and_yield(blob) + local_blob = create_blob(service_name: 'local', filename: 'audio.mp3', content_type: 'audio/mpeg', content: 'audio-content') + other_blob = create_blob(service_name: 'other', filename: 'other.pdf', content_type: 'application/pdf', content: 'other-content') described_class.migrate_blobs(:local, :amazon, should_update_service_name: true) - end - it 'skips the blob service update when the flag is disabled' do - expect(to_service_stub).to receive(:upload).with('blob-key', io, checksum: 'checksum') - expect(blob).not_to receive(:update!) - - described_class.migrate_blobs(:local, :amazon, should_update_service_name: false) + expect(local_blob.reload.service_name).to eq('amazon') + expect(to_service.download(local_blob.key)).to eq('audio-content') + expect(other_blob.reload.service_name).to eq('other') + expect(to_service.exist?(other_blob.key)).to be(false) + expect(other_service.download(other_blob.key)).to eq('other-content') end it 'skips missing source files and continues migration' do - missing_blob = instance_double(ActiveStorage::Blob, id: 2, key: 'missing-key') + missing_blob = create_blob(service_name: 'local', filename: 'missing.pdf', content_type: 'application/pdf', content: 'missing-content') + existing_blob = create_blob(service_name: 'local', filename: 'existing.pdf', content_type: 'application/pdf', content: 'existing-content') - allow(source_blobs).to receive(:find_each).and_yield(missing_blob).and_yield(blob) - allow(missing_blob).to receive(:open).and_raise(ActiveStorage::FileNotFoundError) + from_service.delete(missing_blob.key) - expect(missing_blob).not_to receive(:update!) - expect(to_service_stub).to receive(:upload).with('blob-key', io, checksum: 'checksum') - expect(blob).to receive(:update!).with(service_name: 'amazon') + expect do + described_class.migrate_blobs(:local, :amazon, should_update_service_name: true) + end.not_to raise_error - described_class.migrate_blobs(:local, :amazon, should_update_service_name: true) + expect(missing_blob.reload.service_name).to eq('local') + expect(to_service.exist?(missing_blob.key)).to be(false) + expect(existing_blob.reload.service_name).to eq('amazon') + expect(to_service.download(existing_blob.key)).to eq('existing-content') end it 'does not update the blob service when upload fails' do - allow(to_service_stub).to receive(:upload).and_raise(ActiveStorage::IntegrityError) + blob = create_blob(service_name: 'local', filename: 'report.pdf', content_type: 'application/pdf', content: 'report-content') + + allow(to_service).to receive(:upload).and_raise(ActiveStorage::IntegrityError) - expect(blob).not_to receive(:update!) expect do described_class.migrate_blobs(:local, :amazon, should_update_service_name: true) end.to raise_error(ActiveStorage::IntegrityError) + + expect(blob.reload.service_name).to eq('local') + expect(to_service.exist?(blob.key)).to be(false) + end + + it 'skips the blob service update when the flag is disabled' do + blob = create_blob(service_name: 'local', filename: 'manual.pdf', content_type: 'application/pdf', content: 'manual-content') + + described_class.migrate_blobs(:local, :amazon, should_update_service_name: false) + + expect(blob.reload.service_name).to eq('local') + expect(to_service.download(blob.key)).to eq('manual-content') + end + + it 'does not rewrite blobs that were already migrated' do + blob = create_blob(service_name: 'local', filename: 'archive.pdf', content_type: 'application/pdf', content: 'archive-content') + + allow(to_service).to receive(:upload).and_call_original + + 2.times { described_class.migrate_blobs(:local, :amazon, should_update_service_name: true) } + + expect(to_service).to have_received(:upload).with(blob.key, an_instance_of(Tempfile), checksum: blob.checksum).once + expect(blob.reload.service_name).to eq('amazon') + expect(ActiveStorage::Blob.where(id: blob.id, service_name: 'local')).to be_empty + end + + def build_disk_service(service_name, root) + ActiveStorage::Service.configure(service_name, { service_name => { service: 'Disk', root: root } }) + end + + def create_blob(service_name:, filename:, content_type:, content:) + ActiveStorage::Blob.create_and_upload!( + io: StringIO.new(content), + filename: filename, + content_type: content_type, + service_name: service_name, + identify: false + ) end end end diff --git a/spec/lib/tasks/rake/task_storage_migrations_spec.rb b/spec/lib/tasks/rake/task_storage_migrations_spec.rb index f250f3472..552c814b3 100644 --- a/spec/lib/tasks/rake/task_storage_migrations_spec.rb +++ b/spec/lib/tasks/rake/task_storage_migrations_spec.rb @@ -48,6 +48,16 @@ RSpec.describe Rake::Task do task.invoke end end + + it 'can be invoked again after the task is re-enabled' do + expect(ActiveStorage::Migrator).to receive(:migrate).twice.with(:local, :amazon, should_update_service_name: true) + + with_modified_env FROM: 'local', TO: 'amazon', UPDATE_BLOB_SERVICE_NAME: nil do + task.invoke + task.reenable + task.invoke + end + end end end end