From 8558b6f4c06528ec16c4d217eb44f8eced599a2a Mon Sep 17 00:00:00 2001 From: Sony Mathew <2040199+sony-mathew@users.noreply.github.com> Date: Tue, 16 Jun 2026 17:53:44 +0530 Subject: [PATCH] fix(storage): update migrated blob service names Default storage migrations to update active_storage_blobs.service_name after successful uploads while allowing copy-only runs with UPDATE_BLOB_SERVICE_NAME=false. Remove unused migrator helper methods and cover the migrator/rake flag behavior in specs. --- lib/active_storage/migrator.rb | 19 ++----- lib/tasks/storage_migrations.rake | 3 +- spec/lib/active_storage/migrator_spec.rb | 44 +++++++++++++++- .../rake/task_storage_migrations_spec.rb | 51 +++++++++++++------ 4 files changed, 85 insertions(+), 32 deletions(-) diff --git a/lib/active_storage/migrator.rb b/lib/active_storage/migrator.rb index e13530b4a..4dc086029 100644 --- a/lib/active_storage/migrator.rb +++ b/lib/active_storage/migrator.rb @@ -6,7 +6,7 @@ class ActiveStorage::Migrator Rails.logger = Logger.new($stdout) Rails.logger.level = Logger::DEBUG - def self.migrate(from_service_name, to_service_name) + def self.migrate(from_service_name, to_service_name, update_service_name: true) configs = load_storage_config # Check if services are configured correctly if configs[from_service_name.to_s].nil? || configs[to_service_name.to_s].nil? @@ -20,7 +20,7 @@ class ActiveStorage::Migrator Rails.logger.debug { "#{ActiveStorage::Blob.count} Blobs to migrate from #{from_service_name} to #{to_service_name}" } - migrate_blobs(from_service, to_service) + migrate_blobs(from_service, to_service, to_service_name, update_service_name: update_service_name) end def self.load_storage_config @@ -28,22 +28,11 @@ class ActiveStorage::Migrator YAML.load(yaml_with_env) end - def self.configure_services(from_service_name, to_service_name, configs) - from_service = ActiveStorage::Service.configure(from_service_name, { from_service_name.to_sym => configs[from_service_name.to_s] }) - to_service = ActiveStorage::Service.configure(to_service_name, { to_service_name.to_sym => configs[to_service_name.to_s] }) - [from_service, to_service] - end - - def self.configure_service(service_name, configs) - service_config = configs[service_name.to_s] - ActiveStorage::Service.configure(service_name, { service_name.to_sym => service_config }) - end - def self.configure_blob_service(service) ActiveStorage::Blob.service = service end - def self.migrate_blobs(_from_service, to_service) + def self.migrate_blobs(_from_service, to_service, to_service_name, update_service_name: true) # Configure the blob service for the source service ActiveStorage::Blob.find_each do |blob| next unless blob.image? @@ -54,6 +43,8 @@ class ActiveStorage::Migrator checksum = blob.checksum to_service.upload(blob.key, io, checksum: checksum) end + + blob.update!(service_name: to_service_name.to_s) if update_service_name end Rails.logger.debug { 'Successful migration' } end diff --git a/lib/tasks/storage_migrations.rake b/lib/tasks/storage_migrations.rake index 816290d2d..1e67cc916 100644 --- a/lib/tasks/storage_migrations.rake +++ b/lib/tasks/storage_migrations.rake @@ -3,9 +3,10 @@ namespace :storage do task migrate: :environment do from_service = ENV.fetch('FROM', nil) to_service = ENV.fetch('TO', nil) + update_service_name = ActiveModel::Type::Boolean.new.cast(ENV.fetch('UPDATE_BLOB_SERVICE_NAME', true)) raise 'Missing FROM or TO argument. Usage: FROM=service_name TO=service_name rake storage:migrate' if from_service.nil? || to_service.nil? - ActiveStorage::Migrator.migrate(from_service.to_sym, to_service.to_sym) + ActiveStorage::Migrator.migrate(from_service.to_sym, to_service.to_sym, update_service_name: update_service_name) end end diff --git a/spec/lib/active_storage/migrator_spec.rb b/spec/lib/active_storage/migrator_spec.rb index bdc62d22d..d031d3324 100644 --- a/spec/lib/active_storage/migrator_spec.rb +++ b/spec/lib/active_storage/migrator_spec.rb @@ -1,4 +1,5 @@ require 'rails_helper' +require 'stringio' RSpec.describe ActiveStorage::Migrator do describe '.migrate' do @@ -14,9 +15,15 @@ RSpec.describe ActiveStorage::Migrator do it 'migrates blobs from one service to another' do expect(ActiveStorage::Service).to receive(:configure).with('local', any_args) expect(ActiveStorage::Service).to receive(:configure).with('amazon', any_args) - expect(described_class).to receive(:migrate_blobs).with(from_service_stub, to_service_stub) + expect(described_class).to receive(:migrate_blobs).with(from_service_stub, to_service_stub, 'amazon', update_service_name: true) expect { described_class.migrate('local', 'amazon') }.not_to raise_error end + + it 'passes the service-name update flag to blob migration' do + expect(described_class).to receive(:migrate_blobs).with(from_service_stub, to_service_stub, 'amazon', update_service_name: false) + + described_class.migrate('local', 'amazon', update_service_name: false) + end end context 'when services are not configured correctly' do @@ -28,4 +35,39 @@ RSpec.describe ActiveStorage::Migrator do end end end + + describe '.migrate_blobs' do + let(:from_service_stub) { instance_double(ActiveStorage::Service) } + let(:to_service_stub) { instance_double(ActiveStorage::Service) } + let(:blob) { instance_double(ActiveStorage::Blob, image?: true, key: 'blob-key', checksum: 'checksum') } + let(:io) { StringIO.new('blob-content') } + + before do + allow(ActiveStorage::Blob).to receive(:find_each).and_yield(blob) + allow(blob).to receive(:open).and_yield(io) + end + + it 'updates the blob service after uploading to the target service' 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 + + described_class.migrate_blobs(from_service_stub, to_service_stub, :amazon, 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(from_service_stub, to_service_stub, :amazon, update_service_name: false) + end + + it 'does not update the blob service when upload fails' do + allow(to_service_stub).to receive(:upload).and_raise(ActiveStorage::IntegrityError) + + expect(blob).not_to receive(:update!) + expect do + described_class.migrate_blobs(from_service_stub, to_service_stub, :amazon, update_service_name: true) + end.to raise_error(ActiveStorage::IntegrityError) + 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 2fe1e5b7b..d31e80eda 100644 --- a/spec/lib/tasks/rake/task_storage_migrations_spec.rb +++ b/spec/lib/tasks/rake/task_storage_migrations_spec.rb @@ -4,30 +4,49 @@ require 'rails_helper' RSpec.describe Rake::Task do describe 'storage_migrations' do describe 'rake task' do - context 'when FROM argument is missing' do - before do - ENV['FROM'] = nil - end + subject(:task) { described_class['storage:migrate'] } + before do + task.reenable + end + + context 'when FROM argument is missing' do it 'raises an error' do - expect do - described_class['storage:migrate'].invoke - end.to raise_error(RuntimeError, - 'Missing FROM or TO argument. Usage: FROM=service_name TO=service_name rake storage:migrate') + with_modified_env FROM: nil, TO: 'amazon' do + expect do + task.invoke + end.to raise_error(RuntimeError, + 'Missing FROM or TO argument. Usage: FROM=service_name TO=service_name rake storage:migrate') + end end end context 'when TO argument is missing' do - before do - ENV['FROM'] = 'service_name' - ENV['TO'] = nil + it 'raises an error' do + with_modified_env FROM: 'service_name', TO: nil do + expect do + task.invoke + end.to raise_error(RuntimeError, + 'Missing FROM or TO argument. Usage: FROM=service_name TO=service_name rake storage:migrate') + end + end + end + + context 'when required arguments are present' do + it 'updates blob service names by default' do + expect(ActiveStorage::Migrator).to receive(:migrate).with(:local, :amazon, update_service_name: true) + + with_modified_env FROM: 'local', TO: 'amazon', UPDATE_BLOB_SERVICE_NAME: nil do + task.invoke + end end - it 'raises an error' do - expect do - described_class['storage:migrate'].invoke - end.to raise_error(RuntimeError, - 'Missing FROM or TO argument. Usage: FROM=service_name TO=service_name rake storage:migrate') + it 'skips updating blob service names when disabled' do + expect(ActiveStorage::Migrator).to receive(:migrate).with(:local, :amazon, update_service_name: false) + + with_modified_env FROM: 'local', TO: 'amazon', UPDATE_BLOB_SERVICE_NAME: 'false' do + task.invoke + end end end end