Fix failing to remove existing files on updating by #remove_#{column}=, ##{column}_cache=, and #remote_#{column}_url=

Fixes #2778, Fixes #2779

Author: mshibuya

lib/carrierwave/mounter.rb

@@ -168,6 +168,7 @@ def blank?
     end
 
     def remove=(value)
+      uploaders # Ensure that uploaders are initialized based on the current value
       @remove = value
       write_temporary_identifier
     end
@@ -220,7 +221,7 @@ def option(name)
     end
 
     def clear_unstaged
-      @uploaders ||= []
+      uploaders # Ensure that uploaders are initialized based on the current value
       staged, unstaged = @uploaders.partition(&:staged)
       @uploaders = staged
       @removed_uploaders += unstaged

spec/mount_multiple_spec.rb

@@ -541,6 +541,15 @@ def monkey
           expect(instance.images[0].current_path).to match(/test.jpg$/)
         end
       end
+
+      context "when a file is already stored" do
+        before { allow(instance).to receive(:read_uploader).and_return(['bork.txt']) }
+
+        it "marks the previously uploaded file as removed" do
+          instance.images_cache = ['1369894322-123-0123-1234/test.jpg'].to_json
+          expect(instance.send(:_mounter, :images).instance_variable_get(:@removed_uploaders).map(&:identifier)).to eq ['bork.txt']
+        end
+      end
     end
 
     describe "#remote_images_urls" do
@@ -565,25 +574,24 @@ def monkey
       before do
         stub_request(:get, "http://www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub))
         stub_request(:get, "http://www.example.com/test.txt").to_return(status: 404)
-        instance.remote_images_urls = remote_images_url
       end
 
       context "does nothing when nil is assigned" do
-        let(:remote_images_url) { nil }
+        before { instance.remote_images_urls = nil }
 
         it { is_expected.to be_empty }
       end
 
       context "does nothing when an empty string is assigned" do
-        let(:remote_images_url) { '' }
+        before { instance.remote_images_urls = '' }
 
         it { is_expected.to be_empty }
       end
 
       context "retrieves from cache when a cache name is assigned" do
         subject { images[0].current_path }
 
-        let(:remote_images_url) { ["http://www.example.com/test.jpg"] }
+        before { instance.remote_images_urls = ["http://www.example.com/test.jpg"] }
 
         it { is_expected.to match(/test.jpg$/) }
 
@@ -595,12 +603,10 @@ def monkey
       context "writes over a previously stored file" do
         subject { images[0].current_path }
 
-        let(:remote_images_url) { ["http://www.example.com/test.jpg"] }
-
         before do
           instance.images = [stub_file("portrait.jpg")]
           instance.store_images!
-          instance.remote_images_urls = remote_images_url
+          instance.remote_images_urls = ["http://www.example.com/test.jpg"]
         end
 
         it { is_expected.to match(/test.jpg$/) }
@@ -609,11 +615,9 @@ def monkey
       context "does not write over a previously assigned file" do
         subject { images[0].current_path }
 
-        let(:remote_images_url) { ["http://www.example.com/test.jpg"] }
-
         before do
           instance.images = [stub_file("portrait.jpg")]
-          instance.remote_images_urls = remote_images_url
+          instance.remote_images_urls = ["http://www.example.com/test.jpg"]
         end
 
         it { is_expected.to match(/portrait.jpg$/) }
@@ -622,12 +626,10 @@ def monkey
       context "when an empty string is assigned" do
         subject { images[0].current_path }
 
-        let(:remote_images_url) { [""] }
-
         before do
           instance.images = [stub_file("portrait.jpg")]
           instance.store_images!
-          instance.remote_images_urls = remote_images_url
+          instance.remote_images_urls = [""]
         end
 
         it "does not write over a previously stored file" do
@@ -636,7 +638,7 @@ def monkey
       end
 
       context "if a file fails to be downloaded" do
-        let(:remote_images_url) { ["http://www.example.com/test.txt", "http://www.example.com/test.jpg"] }
+        before { instance.remote_images_urls = ["http://www.example.com/test.txt", "http://www.example.com/test.jpg"] }
 
         it "keeps files which was downloaded successfully" do
           expect(instance.images.map(&:identifier)).to eq ['test.jpg']
@@ -645,14 +647,25 @@ def monkey
 
       context "clears the unsaved remote urls when nil is assigned" do
         subject { instance.remote_images_urls }
-        let(:remote_images_url) { ['invalid'] }
+        before { instance.remote_images_urls = ['invalid'] }
 
         before do
           instance.remote_images_urls = nil
         end
 
         it { is_expected.to be_empty }
       end
+
+      context "when a file is already stored" do
+        before do
+          allow(instance).to receive(:read_uploader).and_return(['bork.txt'])
+          instance.remote_images_urls = ["http://www.example.com/test.jpg"]
+        end
+
+        it "marks the previously uploaded file as removed" do
+          expect(instance.send(:_mounter, :images).instance_variable_get(:@removed_uploaders).map(&:identifier)).to eq ['bork.txt']
+        end
+      end
     end
 
     describe '#store_images!' do
@@ -716,6 +729,22 @@ def monkey
       end
     end
 
+    describe '#remove_images=' do
+      context "when a file is already stored" do
+        before do
+          attribute = ['bork.txt']
+          allow(instance).to receive(:read_uploader) { attribute }
+          allow(instance).to receive(:write_uploader) { |_, value| attribute.replace(value || []) }
+        end
+
+        it "marks the previously uploaded file as removed" do
+          instance.remove_images = true
+          instance.write_images_identifier
+          expect(instance.send(:_mounter, :images).instance_variable_get(:@removed_uploaders).map(&:identifier)).to eq ['bork.txt']
+        end
+      end
+    end
+
     describe '#remove_images?' do
       subject { instance.remove_images? }
 

spec/mount_single_spec.rb

@@ -304,6 +304,12 @@ def default_url
         @instance.image_cache = ''
         expect(@instance.image.current_path).to match(/test.jpg$/)
       end
+
+      it "marks the previously uploaded file as removed" do
+        allow(@instance).to receive(:read_uploader).and_return('bork.txt')
+        @instance.image_cache = '1369894322-123-0123-1234/test.jpg'
+        expect(@instance.send(:_mounter, :image).instance_variable_get(:@removed_uploaders).map(&:identifier)).to eq ['bork.txt']
+      end
     end
 
     describe "#remote_image_url" do
@@ -384,6 +390,12 @@ def default_url
 
         expect(@instance.remote_image_url).to be_nil
       end
+
+      it "marks the previously uploaded file as removed" do
+        allow(@instance).to receive(:read_uploader).and_return('bork.txt')
+        @instance.remote_image_url = "http://www.example.com/test.jpg"
+        expect(@instance.send(:_mounter, :image).instance_variable_get(:@removed_uploaders).map(&:identifier)).to eq ['bork.txt']
+      end
     end
 
     describe '#store_image!' do
@@ -452,6 +464,17 @@ def default_url
 
     end
 
+    describe '#remove_image=' do
+      it "marks the previously uploaded file as removed" do
+        @attribute = 'bork.txt'
+        allow(@instance).to receive(:read_uploader) { @attribute }
+        allow(@instance).to receive(:write_uploader) { |_, value| @attribute = value }
+        @instance.remove_image = true
+        @instance.write_image_identifier
+        expect(@instance.send(:_mounter, :image).instance_variable_get(:@removed_uploaders).map(&:identifier)).to eq ['bork.txt']
+      end
+    end
+
     describe '#remove_image?' do
 
       it "should be true when the value is truthy" do

spec/orm/activerecord_spec.rb

@@ -548,6 +548,37 @@ def monkey
           @event.remove_image?
         }.from(true).to(false)
       end
+
+      it "should remove the existing image file" do
+        # Use another instance to simulate the real use case
+        @another = Event.find(@event.id)
+        @another.remove_image = true
+        @another.save!
+        expect(@event.image.file).not_to exist
+      end
+    end
+
+    describe "image_cache=" do
+      let(:cache_id) { Event.new(image: stub_file('test.jpg')).image_cache }
+
+      before do
+      end
+
+      it "sets the cached image" do
+        @event.image_cache = cache_id
+        @event.save!
+        expect(@event.image.to_s).to eq '/uploads/test.jpg'
+      end
+
+      it "removes the previous image file on save" do
+        @event.image = stub_file('test.jpeg')
+        @event.save!
+        # Use another instance to simulate the real use case
+        @another = Event.find(@event.id)
+        @another.image_cache = cache_id
+        @another.save!
+        expect(@event.image.file).not_to exist
+      end
     end
 
     describe "#remote_image_url=" do
@@ -575,6 +606,16 @@ def monkey
         expect(@event.errors).to be_empty
       end
 
+      it "should remove the existing image" do
+        @event.image = stub_file('test.jpeg')
+        @event.save!
+        # Use another instance to simulate the real use case
+        @another = Event.find(@event.id)
+        @another.remote_image_url = 'http://www.example.com/test.jpg'
+        @another.save!
+        expect(@event.image.file).not_to exist
+      end
+
       context 'when validating download' do
         before do
           @uploader.class_eval do
@@ -1567,6 +1608,37 @@ def monkey
           @event.remove_images?
         }.from(true).to(false)
       end
+
+      it "should remove the existing image file" do
+        # Use another instance to simulate the real use case
+        @another = Event.find(@event.id)
+        @another.remove_images = true
+        @another.save!
+        expect(@event.images.first.file).not_to exist
+      end
+    end
+
+    describe "images_cache=" do
+      let(:cache_id) { Event.new(images: [stub_file('test.jpg')]).images_cache }
+
+      before do
+      end
+
+      it "sets the cached image" do
+        @event.images_cache = cache_id
+        @event.save!
+        expect(@event.images.first.to_s).to eq '/uploads/test.jpg'
+      end
+
+      it "removes the previous image file on save" do
+        @event.images = [stub_file('test.jpeg')]
+        @event.save!
+        # Use another instance to simulate the real use case
+        @another = Event.find(@event.id)
+        @another.images_cache = cache_id
+        @another.save!
+        expect(@event.images.first.file).not_to exist
+      end
     end
 
     describe "#remote_images_urls=" do
@@ -1594,6 +1666,16 @@ def monkey
         expect(@event.errors).to be_empty
       end
 
+      it "should remove the existing image" do
+        @event.images = [stub_file('test.jpeg')]
+        @event.save!
+        # Use another instance to simulate the real use case
+        @another = Event.find(@event.id)
+        @another.remote_images_urls = ['http://www.example.com/test.jpg']
+        @another.save!
+        expect(@event.images.first.file).not_to exist
+      end
+
       context 'when validating download' do
         before do
           @uploader.class_eval do