diff --git a/lib/carrierwave/mounter.rb b/lib/carrierwave/mounter.rb index e0463d492..aeba22e65 100644 --- a/lib/carrierwave/mounter.rb +++ b/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 diff --git a/lib/carrierwave/uploader/store.rb b/lib/carrierwave/uploader/store.rb index 87ed24bd9..16a09ca04 100644 --- a/lib/carrierwave/uploader/store.rb +++ b/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 diff --git a/spec/mount_multiple_spec.rb b/spec/mount_multiple_spec.rb index a2cd4a7b7..08e425d57 100644 --- a/spec/mount_multiple_spec.rb +++ b/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 diff --git a/spec/mount_single_spec.rb b/spec/mount_single_spec.rb index 478c5044f..ed1ecbd38 100644 --- a/spec/mount_single_spec.rb +++ b/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' diff --git a/spec/orm/activerecord_spec.rb b/spec/orm/activerecord_spec.rb index eb727166b..7421b3fe9 100644 --- a/spec/orm/activerecord_spec.rb +++ b/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')] diff --git a/spec/uploader/store_spec.rb b/spec/uploader/store_spec.rb index c4a02c4a4..a3bdca975 100644 --- a/spec/uploader/store_spec.rb +++ b/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