Add suffix to filename on store when path collides with the existing …

…ones

Closes #1855

lib/carrierwave/mounter.rb

@@ -139,8 +139,14 @@ def remote_urls=(urls)
     end
 
     def store!
-      uploaders.each(&:store!)
-      @added_uploaders += uploaders.reject(&:staged)
+      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
     end
 
     def write_identifier

lib/carrierwave/storage/abstract.rb

@@ -14,7 +14,7 @@ def initialize(uploader)
       end
 
       def identifier
-        uploader.filename
+        uploader.deduplicated_filename
       end
 
       def store!(file)

lib/carrierwave/uploader/store.rb

@@ -11,7 +11,7 @@ module Store
         prepend Module.new {
           def initialize(*)
             super
-            @file, @filename, @cache_id, @identifier = nil
+            @file, @filename, @cache_id, @identifier, @deduplication_index = nil
           end
         }
       end
@@ -34,9 +34,25 @@ def filename
         @filename
       end
 
+      ##
+      # Returns a filename which doesn't conflict with already-stored files.
+      #
+      # === Returns
+      #
+      # [String] the filename with suffix added for deduplication
+      #
+      def deduplicated_filename
+        return unless filename
+
+        parts = filename.split('.')
+        basename = parts.shift
+        basename.sub!(/ ?\(\d+\)\z/, '')
+        ([basename.to_s + (@deduplication_index ? "(#{@deduplication_index})" : '')] + parts).join('.')
+      end
+
       ##
       # Calculates the path where the file should be stored. If +for_file+ is given, it will be
-      # used as the filename, otherwise +CarrierWave::Uploader#filename+ is assumed.
+      # used as the identifier, otherwise +CarrierWave::Uploader#identifier+ is assumed.
       #
       # === Parameters
       #
@@ -46,7 +62,7 @@ def filename
       #
       # [String] the store path
       #
-      def store_path(for_file=filename)
+      def store_path(for_file=identifier)
         File.join([store_dir, full_filename(for_file)].compact)
       end
 
@@ -69,7 +85,8 @@ def store!(new_file=nil)
               cache_storage.delete_dir!(cache_path(nil))
             end
             @file = new_file
-            @cache_id = @identifier = nil
+            @identifier = storage.identifier
+            @cache_id = @deduplication_index = nil
             @staged = false
           end
         end
@@ -89,6 +106,25 @@ def retrieve_from_store!(identifier)
         end
       end
 
+      ##
+      # Look for a store path which doesn't collide with the given already-stored paths.
+      # 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
+      #
+      def deduplicate(current_paths)
+        return unless current_paths.include?(store_path)
+
+        (2..current_paths.size + 1).each do |i|
+          @deduplication_index = i
+          break unless current_paths.include?(store_path)
+        end
+      end
+
     private
 
       def full_filename(for_file)

lib/carrierwave/uploader/versions.rb

@@ -41,6 +41,11 @@ def build(superclass)
             def move_to_cache
               false
             end
+
+            # Need to rely on the parent version's identifier, as versions don't have its own one.
+            def identifier
+              parent_version.identifier
+            end
           RUBY
           @blocks.each { |block| @klass.class_eval(&block) }
           @klass

spec/mount_multiple_spec.rb

@@ -265,17 +265,38 @@ def monkey
         end
       end
 
+      describe "when caching files of the same filename" do
+        before { FileUtils.cp(file_path('bork.json'), tmp_path('bork.txt')) }
+        after { FileUtils.rm(tmp_path('bork.txt')) }
+
+        it "accepts them without confusion" do
+          instance.images = [text_file_stub, File.open(tmp_path('bork.txt'))]
+          expect(instance.images[0].cache_path).not_to eq instance.images[1].cache_path
+          expect(instance.images[0].read).not_to eq instance.images[1].read
+        end
+      end
+
       describe "with cached files" do
         before do
           instance.images = [text_file_stub, test_file_stub]
         end
         let(:cache_names) { instance.images.map(&:cache_name) }
-        let(:identifiers) { instance.images.map(&:identifier) }
 
         it "accepts cache name and retrieves from cache" do
           instance.images = [cache_names[1]]
           expect(instance.images.map { |u| u.file.filename }).to eq ['test.jpg']
         end
+
+        context "when adding a file which has the same filename with the exsting one" do
+          before { FileUtils.cp(file_path('bork.json'), tmp_path('bork.txt')) }
+          after { FileUtils.rm(tmp_path('bork.txt')) }
+
+          it "accepts it without confusion" do
+            instance.images = [instance.images[0].cache_name, File.open(tmp_path('bork.txt'))]
+            expect(instance.images[0].cache_path).not_to eq instance.images[1].cache_path
+            expect(instance.images[0].read).not_to eq instance.images[1].read
+          end
+        end
       end
 
       describe "with stored files" do
@@ -340,6 +361,18 @@ def monkey
           instance.images = [instance.images[0]]
           expect(instance.images.map(&:identifier)).to eq ['bork.txt']
         end
+
+        context "when adding a file which has the same filename with the exsting one" do
+          before { FileUtils.cp(file_path('bork.json'), tmp_path('bork.txt')) }
+          after { FileUtils.rm(tmp_path('bork.txt')) }
+
+          it "renames the latter file to avoid filename duplication" do
+            instance.images = ['bork.txt', File.open(tmp_path('bork.txt'))]
+            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
+          end
+        end
       end
     end
 

spec/mount_single_spec.rb

@@ -403,6 +403,22 @@ def default_url
         @instance.store_image!
         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
+        before { FileUtils.cp(file_path('bork.txt'), tmp_path('test.jpg')) }
+        after { FileUtils.rm(tmp_path('test.jpg')) }
+
+        it "renames the latter file to avoid overwriting the old one" do
+          @instance.image = stub_file('test.jpg')
+          @instance.store_image!
+          old_uploader = @instance.image
+
+          @instance.image = File.open(tmp_path('test.jpg'))
+          @instance.store_image!
+          expect(@instance.image.identifier).to eq 'test(2).jpg'
+          expect(old_uploader.read).to eq 'this is stuff'
+        end
+      end
     end
 
     describe '#remove_image!' do