From aac25c10af4218d6e1e70f90154b847b54ce0334 Mon Sep 17 00:00:00 2001 From: Mitsuhiro Shibuya Date: Mon, 1 May 2023 19:25:35 +0900 Subject: [PATCH] Stop relying on ActiveModel::Dirty change tracking for removal of unnecessary files --- lib/carrierwave/mount.rb | 55 ++++++-------------------- lib/carrierwave/mounter.rb | 61 +++++++++++++++-------------- lib/carrierwave/orm/activerecord.rb | 1 - spec/mount_multiple_spec.rb | 2 +- spec/orm/activerecord_spec.rb | 60 ++++++++++++++++++++++++++++ 5 files changed, 104 insertions(+), 75 deletions(-) diff --git a/lib/carrierwave/mount.rb b/lib/carrierwave/mount.rb index 722851b5c..59ab7ca6d 100644 --- a/lib/carrierwave/mount.rb +++ b/lib/carrierwave/mount.rb @@ -185,27 +185,6 @@ def #{column}_processing_error def #{column}_download_error #{column}_download_errors.last end - - def store_previous_changes_for_#{column} - @_previous_changes_for_#{column} = saved_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 @@ -340,28 +319,6 @@ def remote_#{column}_request_headers=(headers) def #{column}_identifiers _mounter(:#{column}).read_identifiers end - - def store_previous_changes_for_#{column} - @_previous_changes_for_#{column} = saved_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 @@ -430,6 +387,18 @@ def write_#{column}_identifier def mark_remove_#{column}_false _mounter(:#{column}).remove = false end + + def reset_previous_changes_for_#{column} + _mounter(:#{column}).reset_changes! + end + + def remove_previously_stored_#{column} + _mounter(:#{column}).remove_previous + end + + def remove_rolled_back_#{column} + _mounter(:#{column}).remove_added + end RUBY end diff --git a/lib/carrierwave/mounter.rb b/lib/carrierwave/mounter.rb index 7ada35d62..95683aab2 100644 --- a/lib/carrierwave/mounter.rb +++ b/lib/carrierwave/mounter.rb @@ -42,6 +42,9 @@ def initialize(record, column) @download_errors = [] @processing_errors = [] @integrity_errors = [] + + @removed_uploaders = [] + @added_uploaders = [] end def uploader_class @@ -63,7 +66,7 @@ def read_identifiers def uploaders @uploaders ||= read_identifiers.map do |identifier| uploader = blank_uploader - uploader.retrieve_from_store!(identifier) if identifier.present? + uploader.retrieve_from_store!(identifier) uploader end end @@ -92,7 +95,8 @@ def cache(new_files) uploader end end - end.compact + end.reject(&:blank?) + @removed_uploaders += (old_uploaders - @uploaders) write_temporary_identifier end @@ -135,7 +139,8 @@ def remote_urls=(urls) end def store! - uploaders.reject(&:blank?).each(&:store!) + uploaders.each(&:store!) + @added_uploaders += uploaders.reject(&:staged) end def write_identifier @@ -163,45 +168,39 @@ def remove? end def remove! - uploaders.reject(&:blank?).each(&:remove!) + uploaders.each(&:remove!) clear! end def clear! + @removed_uploaders += uploaders @remove = nil @uploaders = [] end + def reset_changes! + @removed_uploaders = [] + @added_uploaders = [] + end + def serialization_column option(:mount_on) || column end - def remove_previous(before=nil, after=nil) - after ||= [] - return unless before + def remove_previous + current_paths = uploaders.map(&:path) + @removed_uploaders + .reject {|uploader| current_paths.include?(uploader.path) } + .each { |uploader| uploader.remove! if uploader.remove_previously_stored_files_after_update } + reset_changes! + end - # both 'before' and 'after' can be string when 'mount_on' option is set - before = before.reject(&:blank?).map do |value| - if value.is_a?(String) - uploader = blank_uploader - uploader.retrieve_from_store!(value) - uploader - else - value - end - end - after_paths = after.reject(&:blank?).map do |value| - if value.is_a?(String) - uploader = blank_uploader - uploader.retrieve_from_store!(value) - uploader - else - value - end.path - end - before.each do |uploader| - uploader.remove! if uploader.remove_previously_stored_files_after_update && !after_paths.include?(uploader.path) - end + def remove_added + current_paths = (@removed_uploaders + @uploaders.select(&:staged)).map(&:path) + @added_uploaders + .reject {|uploader| current_paths.include?(uploader.path) } + .each { |uploader| uploader.remove! } + reset_changes! end private @@ -213,7 +212,9 @@ def option(name) def clear_unstaged @uploaders ||= [] - @uploaders.keep_if(&:staged) + staged, unstaged = @uploaders.partition(&:staged) + @uploaders = staged + @removed_uploaders += unstaged end def handle_error diff --git a/lib/carrierwave/orm/activerecord.rb b/lib/carrierwave/orm/activerecord.rb index c271bea8e..434235aeb 100644 --- a/lib/carrierwave/orm/activerecord.rb +++ b/lib/carrierwave/orm/activerecord.rb @@ -27,7 +27,6 @@ def mount_base(column, uploader=nil, options={}, &block) after_commit :"remove_#{column}!", :on => :destroy 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}" diff --git a/spec/mount_multiple_spec.rb b/spec/mount_multiple_spec.rb index 585fe55eb..170270b9b 100644 --- a/spec/mount_multiple_spec.rb +++ b/spec/mount_multiple_spec.rb @@ -319,7 +319,7 @@ def monkey @image_paths = instance.images.map(&:current_path) instance.images = [identifiers[0]] instance.store_images! - instance.send(:_mounter, :images).remove_previous(identifiers, identifiers[0..0]) + instance.send(:_mounter, :images).remove_previous expect(instance.images.map(&:identifier)).to eq ['bork.txt'] expect(File.exist?(@image_paths[0])).to be_truthy expect(File.exist?(@image_paths[1])).to be_falsey diff --git a/spec/orm/activerecord_spec.rb b/spec/orm/activerecord_spec.rb index 07f75e5b2..9e4358346 100644 --- a/spec/orm/activerecord_spec.rb +++ b/spec/orm/activerecord_spec.rb @@ -939,6 +939,29 @@ def filename expect(File.exist?(public_path('uploads/new.jpeg'))).to be_falsey expect(File.exist?(public_path('uploads/old.jpeg'))).to be_truthy end + + it "should clear @added_uploaders on commit" do + # Simulate the commit behavior, since we're using the transactional fixture + @event.run_callbacks(:commit) do + @event.image = stub_file('test.jpeg') + @event.save! + end + expect(@event.send(:_mounter, :image).instance_variable_get(:@added_uploaders)).to be_blank + end + + it "should remove all the previous images when saved multiple times" do + # Simulate the commit behavior, since we're using the transactional fixture + @event.run_callbacks(:commit) do + @event.image = stub_file('test.jpeg') + @event.save! + @event.image = stub_file('bork.txt') + @event.save! + @event.image = stub_file('new.jpeg') + @event.save! + end + expect(File.exist?(public_path('uploads/test.jpeg'))).to be false + expect(File.exist?(public_path('uploads/bork.txt'))).to be false + end end describe '#mount_uploader removing old files with multiple uploaders' do @@ -1707,6 +1730,43 @@ def filename end end + describe "#mount_uploaders into transaction" do + before do + @uploader.version :thumb + reset_class("Event") + Event.mount_uploaders(:images, @uploader) + @event = Event.new + end + + after do + FileUtils.rm_rf(public_path("uploads")) + end + + it "should clear @added_uploaders on commit" do + # Simulate the commit behavior, since we're using the transactional fixture + @event.run_callbacks(:commit) do + @event.images = [stub_file('test.jpeg'), stub_file('bork.txt')] + @event.save! + end + expect(@event.send(:_mounter, :images).instance_variable_get(:@added_uploaders)).to be_blank + end + + it "should remove all the images when trying to remove them one by one" do + # Simulate the commit behavior, since we're using the transactional fixture + @event.run_callbacks(:commit) do + @event.images = [stub_file('test.jpeg'), stub_file('bork.txt')] + @event.save! + @event.images = ['test.jpeg'] + @event.save! + @event.images = [] + @event.save! + end + expect(@event.images).to be_empty + expect(File.exist?(public_path('uploads/test.jpeg'))).to be false + expect(File.exist?(public_path('uploads/bork.txt'))).to be false + end + end + describe '#mount_uploaders removing old files with multiple uploaders' do before do @uploader = Class.new(CarrierWave::Uploader::Base)