Stop relying on ActiveModel::Dirty change tracking for removal of unn…

…ecessary files

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
 

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

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}"

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

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)