Fix deduplicated filename not being persisted

lib/carrierwave/mounter.rb

@@ -139,20 +139,22 @@ def remote_urls=(urls)
     end
 
     def store!
-      additions, remains = uploaders.partition(&:cached?)
-      existing_paths = (@removed_uploaders + remains).map(&:store_path)
-      additions.each do |uploader|
-        uploader.deduplicate(existing_paths)
-        uploader.store!
-        existing_paths << uploader.store_path
-      end
-      @added_uploaders += additions
+      uploaders.each(&:store!)
+      @added_uploaders += uploaders.reject(&:staged)
     end
 
     def write_identifier
       return if record.frozen?
 
       clear! if remove?
+
+      additions, remains = uploaders.partition(&:cached?)
+      existing_identifiers = (@removed_uploaders + remains).map(&:identifier)
+      additions.each do |uploader|
+        uploader.deduplicate(existing_identifiers)
+        existing_identifiers << uploader.identifier
+      end
+
       record.write_uploader(serialization_column, identifier)
     end
 

lib/carrierwave/uploader/store.rb

@@ -108,22 +108,22 @@ def retrieve_from_store!(identifier)
       end
 
       ##
-      # Look for a store path which doesn't collide with the given already-stored paths.
+      # Look for an identifier which doesn't collide with the given already-stored identifiers.
       # It is done by adding a index number as the suffix.
       # For example, if there's 'image.jpg' and the @deduplication_index is set to 2,
       # The stored file will be named as 'image(2).jpg'.
       #
       # === Parameters
       #
-      # [current_paths (Array[String])] List of paths for already-stored files
+      # [current_identifiers (Array[String])] List of identifiers for already-stored files
       #
-      def deduplicate(current_paths)
+      def deduplicate(current_identifiers)
         @deduplication_index = nil
-        return unless current_paths.include?(store_path)
+        return unless current_identifiers.include?(identifier)
 
-        (1..current_paths.size + 1).each do |i|
+        (1..current_identifiers.size + 1).each do |i|
           @deduplication_index = i
-          break unless current_paths.include?(store_path)
+          break unless current_identifiers.include?(identifier)
         end
       end
 

spec/mount_multiple_spec.rb

@@ -368,6 +368,7 @@ def monkey
 
           it "renames the latter file to avoid filename duplication" do
             instance.images = ['bork.txt', File.open(tmp_path('bork.txt'))]
+            instance.write_images_identifier
             instance.store_images!
             expect(instance.images.map(&:identifier)).to eq ['bork.txt', 'bork(2).txt']
             expect(instance.images[0].read).not_to eq instance.images[1].read

spec/mount_single_spec.rb

@@ -404,7 +404,7 @@ def default_url
         expect(@instance.image.current_path).to eq(public_path('uploads/test.jpg'))
       end
 
-      context "when adding a file which has the same filename with the exsting one" do
+      context "when adding a file which has the same filename with the existing one" do
         before { FileUtils.cp(file_path('bork.txt'), tmp_path('test.jpg')) }
         after { FileUtils.rm(tmp_path('test.jpg')) }
 
@@ -414,6 +414,7 @@ def default_url
           old_uploader = @instance.image
 
           @instance.image = File.open(tmp_path('test.jpg'))
+          @instance.write_image_identifier
           @instance.store_image!
           expect(@instance.image.identifier).to eq 'test(2).jpg'
           expect(old_uploader.read).to eq 'this is stuff'

spec/orm/activerecord_spec.rb

@@ -777,6 +777,13 @@ def filename
         expect(File.exist?(public_path('uploads/old.jpeg'))).to be_falsey
       end
 
+      it "should persist the deduplicated filename" do
+        @event.image = stub_file('old.jpeg')
+        expect(@event.save).to be_truthy
+        @event.reload
+        expect(@event.image.current_path).to eq public_path('uploads/old(2).jpeg')
+      end
+
       it "should not remove file if validations fail on save" do
         Event.validate { |r| r.errors.add :textfile, "FAIL!" }
         @event.image = stub_file('new.jpeg')
@@ -1682,6 +1689,13 @@ def filename
         expect(@event.images[0].current_path).to eq public_path('uploads/old(2).jpeg')
       end
 
+      it "should persist the deduplicated filename" do
+        @event.images = [stub_file('old.jpeg')]
+        expect(@event.save).to be_truthy
+        @event.reload
+        expect(@event.images[0].current_path).to eq public_path('uploads/old(2).jpeg')
+      end
+
       it "should not remove file if validations fail on save" do
         Event.validate { |r| r.errors.add :textfile, "FAIL!" }
         @event.images = [stub_file('new.jpeg')]

spec/uploader/store_spec.rb

@@ -415,23 +415,23 @@ def filename
     end
 
     it "tries to find a non-duplicate filename" do
-      @uploader.deduplicate(['uploads/test.jpg'])
+      @uploader.deduplicate(['test.jpg'])
       expect(@uploader.deduplicated_filename).to eq('test(2).jpg')
     end
 
     it "does nothing when filename doesn't collide" do
-      @uploader.deduplicate(['uploads/file.jpg'])
+      @uploader.deduplicate(['file.jpg'])
       expect(@uploader.deduplicated_filename).to eq('test.jpg')
     end
 
     it "chooses the first non-colliding name" do
-      @uploader.deduplicate(['uploads/test.jpg', 'uploads/test(2).jpg', 'uploads/test(4).jpg'])
+      @uploader.deduplicate(['test.jpg', 'test(2).jpg', 'test(4).jpg'])
       expect(@uploader.deduplicated_filename).to eq('test(3).jpg')
     end
 
     it "resets the deduplication index value from the previous attempt" do
-      @uploader.deduplicate(['uploads/test.jpg'])
-      @uploader.deduplicate(['uploads/test.png'])
+      @uploader.deduplicate(['test.jpg'])
+      @uploader.deduplicate(['test.png'])
       expect(@uploader.deduplicated_filename).to eq('test.jpg')
     end