[wip]

lib/carrierwave/error.rb

@@ -4,5 +4,6 @@ class IntegrityError < UploadError; end
   class InvalidParameter < UploadError; end
   class ProcessingError < UploadError; end
   class DownloadError < UploadError; end
+  class DuplicateFilenameError < UploadError; end
   class UnknownStorageError < StandardError; end
 end

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/uploader/configuration.rb

@@ -25,6 +25,7 @@ module Configuration
         add_config :remove_previously_stored_files_after_update
         add_config :downloader
         add_config :force_extension
+        add_config :resolve_duplication
 
         # fog
         add_deprecated_config :fog_provider
@@ -204,6 +205,7 @@ def reset_config
             config.remove_previously_stored_files_after_update = true
             config.downloader = CarrierWave::Downloader::Base
             config.force_extension = false
+            config.resolve_duplication = true
             config.ignore_integrity_errors = true
             config.ignore_processing_errors = true
             config.ignore_download_errors = true

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
@@ -31,7 +31,7 @@ def initialize(*)
       # [String] a filename
       #
       def filename
-        @filename
+        add_deduplication_suffix(@filename)
       end
 
       ##
@@ -89,6 +89,31 @@ 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 resolve_duplication && current_paths.include?(store_path)
+
+        i = 2
+        loop do
+          @deduplication_index = i
+          break unless current_paths.include?(store_path)
+          i += 1
+          raise CarrierWave::DuplicateFilenameError, <<~MSG if i > current_paths.size
+            Unable to find a non-duplicate filename. This typically happens when the uploader's #filename is overridden and doesn't allow CarrierWave to modify it to a non-colliding one.
+            Consider changing your #filename implementation to `def filename; add_deduplication_suffix(...); end` to make it compatible with that feature.
+          MSG
+        end
+      end
+
     private
 
       def full_filename(for_file)
@@ -99,6 +124,14 @@ def storage
         @storage ||= self.class.storage.new(self)
       end
 
+      def add_deduplication_suffix(filename)
+        return unless filename
+
+        parts = filename.split('.')
+        basename = parts.shift
+        basename.sub!(/ ?\(\d+\)\z/, '')
+        ([basename.to_s + (@deduplication_index ? "(#{@deduplication_index})" : '')] + parts).join('.')
+      end
     end # Store
   end # Uploader
 end # CarrierWave

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

spec/uploader/store_spec.rb

@@ -404,4 +404,77 @@ def filename
     end
   end
 
+  describe "#deduplicate" do
+    let(:file) { stub_file('test.jpg') }
+
+    before do
+      @uploader.cache!(file)
+    end
+
+    it "tries to find a non-duplicate filename" do
+      @uploader.deduplicate(['uploads/test.jpg'])
+      expect(@uploader.filename).to eq('test(2).jpg')
+    end
+
+    it "does nothing when filename doesn't collide" do
+      @uploader.deduplicate(['uploads/file.jpg'])
+      expect(@uploader.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'])
+      expect(@uploader.filename).to eq('test(3).jpg')
+    end
+
+    context "when #filename is overridden and duplication can't be resolved" do
+      before do
+        @uploader_class.class_eval do
+          def filename
+            'test.jpg'
+          end
+        end
+      end
+
+      it "raises DuplicateFilenameError" do
+        expect { @uploader.deduplicate(['uploads/test.jpg']) }.to raise_error CarrierWave::DuplicateFilenameError
+      end
+    end
+
+    context "when #resolve_duplication is set to false" do
+      before do
+        @uploader_class.resolve_duplication = false
+      end
+
+      it "keeps the old behavior of not trying to resolve duplication" do
+        @uploader.deduplicate(['uploads/test.jpg'])
+        expect(@uploader.filename).to eq 'test.jpg'
+      end
+    end
+  end
+
+  describe "#add_deduplication_suffix" do
+    it "does not change the filename when deduplication index is not set" do
+      expect(@uploader.send(:add_deduplication_suffix, 'filename.jpg')).to eq('filename.jpg')
+    end
+
+    it "appends the deduplication index as suffix" do
+      @uploader.instance_variable_set :@deduplication_index, 1
+      expect(@uploader.send(:add_deduplication_suffix, 'filename.jpg')).to eq('filename(1).jpg')
+    end
+
+    it "reuses the parentheses" do
+      @uploader.instance_variable_set :@deduplication_index, 269
+      expect(@uploader.send(:add_deduplication_suffix, 'filename (42).jpg')).to eq('filename(269).jpg')
+    end
+
+    it "reuses the parentheses when there's no space before that" do
+      @uploader.instance_variable_set :@deduplication_index, 2
+      expect(@uploader.send(:add_deduplication_suffix, 'filename(1).jpg')).to eq('filename(2).jpg')
+    end
+
+    it "does not reuse the parentheses when non-numbers are enclosed" do
+      @uploader.instance_variable_set :@deduplication_index, 2
+      expect(@uploader.send(:add_deduplication_suffix, 'filename (A).jpg')).to eq('filename (A)(2).jpg')
+    end
+  end
 end