Skip to content

Commit

Permalink
Revert "Manage transaction rollback"
Browse files Browse the repository at this point in the history
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 665f225.

Fixes: carrierwaveuploader#2544
  • Loading branch information
fsateler committed Mar 17, 2021
1 parent 3083c12 commit 88e3717
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 49 deletions.
5 changes: 3 additions & 2 deletions lib/carrierwave/orm/activerecord.rb
Expand Up @@ -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
Expand Down
56 changes: 9 additions & 47 deletions spec/orm/activerecord_spec.rb
Expand Up @@ -672,7 +672,7 @@ def filename
end

after do
FileUtils.rm_rf(public_path("uploads"))
FileUtils.rm_rf(file_path("uploads"))
end

describe 'normally' do
Expand Down Expand Up @@ -757,7 +757,7 @@ def filename
end

after do
FileUtils.rm_rf(public_path("uploads"))
FileUtils.rm_rf(file_path("uploads"))
end

it "should remove old file if old file had a different path" do
Expand All @@ -780,52 +780,14 @@ 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
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)
Expand All @@ -843,7 +805,7 @@ def filename
end

after do
FileUtils.rm_rf(public_path("uploads"))
FileUtils.rm_rf(file_path("uploads"))
end

it "should remove old file1 and file2 if old file1 and file2 had a different paths" do
Expand Down Expand Up @@ -886,7 +848,7 @@ def filename
end

after do
FileUtils.rm_rf(public_path("uploads"))
FileUtils.rm_rf(file_path("uploads"))
end

it "should remove old file if old file had a different path" do
Expand Down Expand Up @@ -1434,7 +1396,7 @@ def filename
end

after do
FileUtils.rm_rf(public_path("uploads"))
FileUtils.rm_rf(file_path("uploads"))
end

describe 'normally' do
Expand Down Expand Up @@ -1511,7 +1473,7 @@ def filename
end

after do
FileUtils.rm_rf(public_path("uploads"))
FileUtils.rm_rf(file_path("uploads"))
end

it "should remove old file if old file had a different path" do
Expand Down Expand Up @@ -1548,7 +1510,7 @@ def filename
end

after do
FileUtils.rm_rf(public_path("uploads"))
FileUtils.rm_rf(file_path("uploads"))
end

it "should remove old file1 and file2 if old file1 and file2 had a different paths" do
Expand Down Expand Up @@ -1590,7 +1552,7 @@ def filename
end

after do
FileUtils.rm_rf(public_path("uploads"))
FileUtils.rm_rf(file_path("uploads"))
end

it "should remove old file if old file had a different path" do
Expand Down

0 comments on commit 88e3717

Please sign in to comment.