From b0d32ae97e81bca5f2c99f54d2159f394039660d Mon Sep 17 00:00:00 2001 From: Felipe Sateler Date: Wed, 17 Mar 2021 11:13:12 -0300 Subject: [PATCH 1/3] Revert "Manage transaction rollback" This commit attempts to fix leftover files in failed transactions, but in the process introduces three problems: - Uploader state is inconsistent between `save` and `commit`. For example, reading `my_object.file_url.url` will return a bogus url if I read before commit. - A file might not be uploaded after a successful commit. For example, if there is a network failure. This causes the transaction to not be rolled back and invalid records being recorded. - Even if the upload eventually completes, it can take an arbitrary amount of time, during which the record is invalid. This reverts commit 665f2254a2c70385b67942b13a4710dc6826c42d. Fixes: #2544 --- lib/carrierwave/orm/activerecord.rb | 5 ++-- spec/orm/activerecord_spec.rb | 40 +---------------------------- 2 files changed, 4 insertions(+), 41 deletions(-) diff --git a/lib/carrierwave/orm/activerecord.rb b/lib/carrierwave/orm/activerecord.rb index 59320c13c..99bfa8d56 100644 --- a/lib/carrierwave/orm/activerecord.rb +++ b/lib/carrierwave/orm/activerecord.rb @@ -56,12 +56,13 @@ def mount_base(column, uploader=nil, options={}, &block) validates_processing_of column if uploader_option(column.to_sym, :validate_processing) validates_download_of column if uploader_option(column.to_sym, :validate_download) + after_save :"store_#{column}!" before_save :"write_#{column}_identifier" - after_save :"store_previous_changes_for_#{column}" after_commit :"remove_#{column}!", :on => :destroy after_commit :"mark_remove_#{column}_false", :on => :update + + after_save :"store_previous_changes_for_#{column}" after_commit :"remove_previously_stored_#{column}", :on => :update - after_commit :"store_#{column}!", :on => [:create, :update] mod = Module.new prepend mod diff --git a/spec/orm/activerecord_spec.rb b/spec/orm/activerecord_spec.rb index 2b2d7834c..a9be24050 100644 --- a/spec/orm/activerecord_spec.rb +++ b/spec/orm/activerecord_spec.rb @@ -780,6 +780,7 @@ def filename Event.transaction do @event.image = stub_file('new.jpeg') @event.save + expect(File.exist?(public_path('uploads/new.jpeg'))).to be_truthy expect(File.exist?(public_path('uploads/old.jpeg'))).to be_truthy raise ActiveRecord::Rollback end @@ -787,45 +788,6 @@ def filename end end - describe "#mount_uploader into transaction" do - before do - @uploader.version :thumb - reset_class("Event") - Event.mount_uploader(:image, @uploader) - @event = Event.new - end - - after do - FileUtils.rm_rf(public_path("uploads")) - end - - it "should not store file during rollback" do - Event.transaction do - @event.image = stub_file('new.jpeg') - @event.save - - raise ActiveRecord::Rollback - end - - expect(File.exist?(public_path('uploads/new.jpeg'))).to be_falsey - end - - it "should not change file during rollback" do - @event.image = stub_file('old.jpeg') - @event.save - - Event.transaction do - @event.image = stub_file('new.jpeg') - @event.save - - raise ActiveRecord::Rollback - end - - expect(File.exist?(public_path('uploads/new.jpeg'))).to be_falsey - expect(File.exist?(public_path('uploads/old.jpeg'))).to be_truthy - end - end - describe '#mount_uploader removing old files with multiple uploaders' do before do @uploader = Class.new(CarrierWave::Uploader::Base) From 761adf89a9714cf110c1b598b79f10840bef63c7 Mon Sep 17 00:00:00 2001 From: Felipe Sateler Date: Thu, 18 Mar 2021 13:00:28 -0300 Subject: [PATCH 2/3] fix: remove uploaded files if transaction is rolled back This prevents accumulating stale files when transaction is aborted. --- lib/carrierwave/mount.rb | 26 +++++++++++++++++++ lib/carrierwave/orm/activerecord.rb | 2 ++ spec/orm/activerecord_spec.rb | 40 +++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/lib/carrierwave/mount.rb b/lib/carrierwave/mount.rb index 16ccb3dfe..2a691af20 100644 --- a/lib/carrierwave/mount.rb +++ b/lib/carrierwave/mount.rb @@ -199,10 +199,22 @@ def store_previous_changes_for_#{column} @_previous_changes_for_#{column} = attribute_changes[_mounter(:#{column}).serialization_column] end + def reset_previous_changes_for_#{column} + # We use this variable to pass information from save time to commit time. + # Make sure this doesn't persist across multiple transactions + @_previous_changes_for_#{column} = nil + end + def remove_previously_stored_#{column} before, after = @_previous_changes_for_#{column} _mounter(:#{column}).remove_previous([before], [after]) end + + def remove_rolled_back_#{column} + before, after = @_previous_changes_for_#{column} + _mounter(:#{column}).remove_previous([after], [before]) + @_previous_changes_for_#{column} = nil + end RUBY end @@ -351,9 +363,23 @@ def store_previous_changes_for_#{column} @_previous_changes_for_#{column} = attribute_changes[_mounter(:#{column}).serialization_column] end + def reset_previous_changes_for_#{column} + # We use this variable to pass information from save time to commit time. + # Make sure this doesn't persist across multiple transactions + @_previous_changes_for_#{column} = nil + end + def remove_previously_stored_#{column} + return unless @_previous_changes_for_#{column} _mounter(:#{column}).remove_previous(*@_previous_changes_for_#{column}) end + + def remove_rolled_back_#{column} + return unless @_previous_changes_for_#{column} + before, after = @_previous_changes_for_#{column} + _mounter(:#{column}).remove_previous(after, before) + @_previous_changes_for_#{column} = nil + end RUBY end diff --git a/lib/carrierwave/orm/activerecord.rb b/lib/carrierwave/orm/activerecord.rb index 99bfa8d56..d8b32a18e 100644 --- a/lib/carrierwave/orm/activerecord.rb +++ b/lib/carrierwave/orm/activerecord.rb @@ -62,7 +62,9 @@ def mount_base(column, uploader=nil, options={}, &block) after_commit :"mark_remove_#{column}_false", :on => :update after_save :"store_previous_changes_for_#{column}" + after_commit :"reset_previous_changes_for_#{column}" after_commit :"remove_previously_stored_#{column}", :on => :update + after_rollback :"remove_rolled_back_#{column}" mod = Module.new prepend mod diff --git a/spec/orm/activerecord_spec.rb b/spec/orm/activerecord_spec.rb index a9be24050..849aad671 100644 --- a/spec/orm/activerecord_spec.rb +++ b/spec/orm/activerecord_spec.rb @@ -785,6 +785,46 @@ def filename raise ActiveRecord::Rollback end expect(File.exist?(public_path('uploads/old.jpeg'))).to be_truthy + expect(File.exist?(public_path('uploads/new.jpeg'))).to be_falsey + end + end + + describe "#mount_uploader into transaction" do + before do + @uploader.version :thumb + reset_class("Event") + Event.mount_uploader(:image, @uploader) + @event = Event.new + end + + after do + FileUtils.rm_rf(public_path("uploads")) + end + + it "should not store file during rollback" do + Event.transaction do + @event.image = stub_file('new.jpeg') + @event.save + + raise ActiveRecord::Rollback + end + + expect(File.exist?(public_path('uploads/new.jpeg'))).to be_falsey + end + + it "should not change file during rollback" do + @event.image = stub_file('old.jpeg') + @event.save + + Event.transaction do + @event.image = stub_file('new.jpeg') + @event.save + + raise ActiveRecord::Rollback + end + + expect(File.exist?(public_path('uploads/new.jpeg'))).to be_falsey + expect(File.exist?(public_path('uploads/old.jpeg'))).to be_truthy end end From 8d98ecd19c043b32c97f7745790def0051de94f8 Mon Sep 17 00:00:00 2001 From: Felipe Sateler Date: Tue, 23 Mar 2021 11:12:09 -0300 Subject: [PATCH 3/3] add tests for behavior during transaction --- spec/orm/activerecord_spec.rb | 39 ++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/spec/orm/activerecord_spec.rb b/spec/orm/activerecord_spec.rb index 849aad671..9b9d70222 100644 --- a/spec/orm/activerecord_spec.rb +++ b/spec/orm/activerecord_spec.rb @@ -780,11 +780,48 @@ def filename Event.transaction do @event.image = stub_file('new.jpeg') @event.save - expect(File.exist?(public_path('uploads/new.jpeg'))).to be_truthy expect(File.exist?(public_path('uploads/old.jpeg'))).to be_truthy raise ActiveRecord::Rollback end expect(File.exist?(public_path('uploads/old.jpeg'))).to be_truthy + end + + it 'should remove new file if transaction is rollback' do + Event.transaction do + @event.image = stub_file('new.jpeg') + @event.save + expect(File.exist?(public_path('uploads/new.jpeg'))).to be_truthy + raise ActiveRecord::Rollback + end + expect(File.exist?(public_path('uploads/new.jpeg'))).to be_falsey + end + + it 'should give correct url during transaction' do + Event.transaction do + @event.image = stub_file('new.jpeg') + @event.save + expect(@event.image_url).to eq '/uploads/new.jpeg' + raise ActiveRecord::Rollback + end + end + + it 'should raise error at save if storage cannot be done, preserving old' do + allow(@event).to receive(:store_image!).and_raise(CarrierWave::UploadError) + Event.transaction do + @event.image = stub_file('new.jpeg') + expect{ @event.save }.to raise_error(CarrierWave::UploadError) + raise ActiveRecord::Rollback + end + expect(File.exist?(public_path('uploads/old.jpeg'))).to be_truthy + end + + it 'should remove new file if transaction is rollback' do + Event.transaction do + @event.image = stub_file('new.jpeg') + @event.save + expect(File.exist?(public_path('uploads/new.jpeg'))).to be_truthy + raise ActiveRecord::Rollback + end expect(File.exist?(public_path('uploads/new.jpeg'))).to be_falsey end end