Add counter to cache_id

Old version of generate_cache_id was collision-prone: for complex factories with multiple
uploaders we had quite a few flaky specs because of that. With a counter, this
(and similar situations like #1232)
should be resolved completely.

features/caching.feature

@@ -23,6 +23,6 @@ Feature: uploader with file storage
   Scenario: retrieving a file from cache
     Given an uploader class that uses the 'file' storage
     And an instance of that class
-    And the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/bork.txt'
-    When I retrieve the cache name '1369894322-345-2255/bork.txt' from the cache
-    Then the uploader should have 'public/uploads/tmp/1369894322-345-2255/bork.txt' as its current path
+    And the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/bork.txt'
+    When I retrieve the cache name '1369894322-345-1234-2255/bork.txt' from the cache
+    Then the uploader should have 'public/uploads/tmp/1369894322-345-1234-2255/bork.txt' as its current path

features/file_storage.feature

@@ -30,8 +30,8 @@ Feature: uploader with file storage
     And the file at 'public/uploads/bork.txt' should be identical to the file at 'fixtures/bork.txt'
 
   Scenario: retrieving a file from cache then storing
-    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/bork.txt'
-    When I retrieve the cache name '1369894322-345-2255/bork.txt' from the cache
+    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/bork.txt'
+    When I retrieve the cache name '1369894322-345-1234-2255/bork.txt' from the cache
     And I store the file
     Then there should be a file at 'public/uploads/bork.txt'
     And the file at 'public/uploads/bork.txt' should be identical to the file at 'fixtures/bork.txt'

features/file_storage_overridden_filename.feature

@@ -31,8 +31,8 @@ Feature: uploader with file storage and overriden filename
     And the file at 'public/uploads/txt.krob' should be identical to the file at 'fixtures/bork.txt'
 
   Scenario: retrieving a file from cache then storing
-    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/bork.txt'
-    When I retrieve the cache name '1369894322-345-2255/bork.txt' from the cache
+    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/bork.txt'
+    When I retrieve the cache name '1369894322-345-1234-2255/bork.txt' from the cache
     And I store the file
     Then there should be a file at 'public/uploads/txt.krob'
     And the file at 'public/uploads/txt.krob' should be identical to the file at 'fixtures/bork.txt'

features/file_storage_overridden_store_dir.feature

@@ -31,8 +31,8 @@ Feature: uploader with file storage and overridden store dir
     And the file at 'public/monkey/llama/bork.txt' should be identical to the file at 'fixtures/bork.txt'
 
   Scenario: retrieving a file from cache then storing
-    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/bork.txt'
-    When I retrieve the cache name '1369894322-345-2255/bork.txt' from the cache
+    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/bork.txt'
+    When I retrieve the cache name '1369894322-345-1234-2255/bork.txt' from the cache
     And I store the file
     Then there should be a file at 'public/monkey/llama/bork.txt'
     And the file at 'public/monkey/llama/bork.txt' should be identical to the file at 'fixtures/bork.txt'

features/file_storage_reversing_processor.feature

@@ -36,8 +36,8 @@ Feature: uploader with file storage and a processor that reverses the file
     And the file at 'public/uploads/bork.txt' should be the reverse of the file at 'fixtures/bork.txt'
 
   Scenario: retrieving a file from cache then storing
-    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/bork.txt'
-    When I retrieve the cache name '1369894322-345-2255/bork.txt' from the cache
+    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/bork.txt'
+    When I retrieve the cache name '1369894322-345-1234-2255/bork.txt' from the cache
     And I store the file
     Then there should be a file at 'public/uploads/bork.txt'
     And the file at 'public/uploads/bork.txt' should be identical to the file at 'fixtures/bork.txt'

features/versions_basics.feature

@@ -33,9 +33,9 @@ Feature: uploader with file storage and versions
     And the uploader's version 'thumb' should have the url '/uploads/thumb_bork.txt'
 
   Scenario: retrieving a file from cache then storing
-    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/bork.txt'
-    Given the file 'fixtures/monkey.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/thumb_bork.txt'
-    When I retrieve the cache name '1369894322-345-2255/bork.txt' from the cache
+    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/bork.txt'
+    Given the file 'fixtures/monkey.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/thumb_bork.txt'
+    When I retrieve the cache name '1369894322-345-1234-2255/bork.txt' from the cache
     And I store the file
     Then there should be a file at 'public/uploads/bork.txt'
     Then there should be a file at 'public/uploads/thumb_bork.txt'

features/versions_nested_versions.feature

@@ -43,11 +43,11 @@ Feature: uploader with nested versions
     And the uploader's nested version 'micro' nested in 'thumb' should have the url '/uploads/thumb_micro_bork.txt'
 
   Scenario: retrieving a file from cache then storing
-    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/bork.txt'
-    Given the file 'fixtures/monkey.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/thumb_bork.txt'
-    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/thumb_mini_bork.txt'
-    Given the file 'fixtures/monkey.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/thumb_micro_bork.txt'
-    When I retrieve the cache name '1369894322-345-2255/bork.txt' from the cache
+    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/bork.txt'
+    Given the file 'fixtures/monkey.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/thumb_bork.txt'
+    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/thumb_mini_bork.txt'
+    Given the file 'fixtures/monkey.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/thumb_micro_bork.txt'
+    When I retrieve the cache name '1369894322-345-1234-2255/bork.txt' from the cache
     And I store the file
     Then there should be a file at 'public/uploads/bork.txt'
     Then there should be a file at 'public/uploads/thumb_bork.txt'

features/versions_overridden_filename.feature

@@ -34,9 +34,9 @@ Feature: uploader with file storage and overriden filename
       And the uploader's version 'thumb' should have the url '/uploads/thumb_grark.png'
 
     Scenario: retrieving a file from cache then storing
-      Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/bork.txt'
-      Given the file 'fixtures/monkey.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/thumb_bork.txt'
-      When I retrieve the cache name '1369894322-345-2255/bork.txt' from the cache
+      Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/bork.txt'
+      Given the file 'fixtures/monkey.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/thumb_bork.txt'
+      When I retrieve the cache name '1369894322-345-1234-2255/bork.txt' from the cache
       And I store the file
       Then there should be a file at 'public/uploads/grark.png'
       Then there should be a file at 'public/uploads/thumb_grark.png'

features/versions_overriden_store_dir.feature

@@ -31,9 +31,9 @@ Feature: uploader with file storage and versions with overridden store dir
     And the file at 'public/monkey/llama/thumb_bork.txt' should be identical to the file at 'fixtures/bork.txt'
 
   Scenario: retrieving a file from cache then storing
-    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/bork.txt'
-    Given the file 'fixtures/monkey.txt' is cached file at 'public/uploads/tmp/1369894322-345-2255/thumb_bork.txt'
-    When I retrieve the cache name '1369894322-345-2255/bork.txt' from the cache
+    Given the file 'fixtures/bork.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/bork.txt'
+    Given the file 'fixtures/monkey.txt' is cached file at 'public/uploads/tmp/1369894322-345-1234-2255/thumb_bork.txt'
+    When I retrieve the cache name '1369894322-345-1234-2255/bork.txt' from the cache
     And I store the file
     Then there should be a file at 'public/uploads/bork.txt'
     Then there should be a file at 'public/monkey/llama/thumb_bork.txt'

lib/carrierwave/uploader/cache.rb

@@ -7,16 +7,27 @@ def message
       "You tried to assign a String or a Pathname to an uploader, for security reasons, this is not allowed.\n\n If this is a file upload, please check that your upload form is multipart encoded."
     end
   end
+
+  class CacheCounter
+    @@counter = 0
+    def self.increment
+      @@counter += 1
+    end  
+  end
 
   ##
   # Generates a unique cache id for use in the caching system
   #
   # === Returns
   #
-  # [String] a cache id in the format TIMEINT-PID-RND
+  # [String] a cache id in the format TIMEINT-PID-COUNTER-RND
   #
   def self.generate_cache_id
-    Time.now.utc.to_i.to_s + '-' + Process.pid.to_s + '-' + ("%04d" % rand(9999))
+    [Time.now.utc.to_i,
+      Process.pid, 
+      '%04d' % (CarrierWave::CacheCounter.increment % 1000), 
+      '%04d' % rand(9999)
+    ].map(&:to_s).join('-')
   end
 
   module Uploader
@@ -83,7 +94,7 @@ def sanitized_file
       #
       # === Returns
       #
-      # [String] a cache name, in the format YYYYMMDD-HHMM-PID-RND/filename.txt
+      # [String] a cache name, in the format TIMEINT-PID-COUNTER-RND/filename.txt
       #
       def cache_name
         File.join(cache_id, full_original_filename) if cache_id and original_filename
@@ -178,7 +189,9 @@ def workfile_path(for_file=original_filename)
       alias_method :full_original_filename, :original_filename
 
       def cache_id=(cache_id)
-        raise CarrierWave::InvalidParameter, "invalid cache id" unless cache_id =~ /\A[\d]+\-[\d]+\-[\d]{4}\z/
+        # Earlier cache_id contained only 3 pieces, the "counter" part was introduced later.
+        # Thus we should allow for the cache_id to have both 3-piece and 4-piece formats.
+        raise CarrierWave::InvalidParameter, "invalid cache id" unless cache_id =~ /\A[\d]+\-[\d]+(\-[\d]{4})?\-[\d]{4}\z/
         @cache_id = cache_id
       end
 

spec/mount_multiple_spec.rb

@@ -249,8 +249,8 @@ def monkey
       it "should be the cache name when a file has been cached" do
         @instance.images = [stub_file('test.jpg'), stub_file('old.jpeg')]
         res = JSON.parse(@instance.images_cache)
-        expect(res[0]).to match(%r(^[\d]+\-[\d]+\-[\d]{4}/test\.jpg$))
-        expect(res[1]).to match(%r(^[\d]+\-[\d]+\-[\d]{4}/old\.jpeg$))
+        expect(res[0]).to match(%r(^[\d]+\-[\d]+\-[\d]{4}\-[\d]{4}/test\.jpg$))
+        expect(res[1]).to match(%r(^[\d]+\-[\d]+\-[\d]{4}\-[\d]{4}/old\.jpeg$))
       end
     end
 
@@ -259,7 +259,7 @@ def monkey
       before do
         allow(@instance).to receive(:write_uploader)
         allow(@instance).to receive(:read_uploader).and_return(nil)
-        CarrierWave::SanitizedFile.new(file_path('test.jpg')).copy_to(public_path('uploads/tmp/1369894322-123-1234/test.jpg'))
+        CarrierWave::SanitizedFile.new(file_path('test.jpg')).copy_to(public_path('uploads/tmp/1369894322-123-0123-1234/test.jpg'))
       end
 
       it "should do nothing when nil is assigned" do
@@ -273,13 +273,13 @@ def monkey
       end
 
       it "retrieve from cache when a cache name is assigned" do
-        @instance.images_cache = ['1369894322-123-1234/test.jpg'].to_json
-        expect(@instance.images[0].current_path).to eq(public_path('uploads/tmp/1369894322-123-1234/test.jpg'))
+        @instance.images_cache = ['1369894322-123-0123-1234/test.jpg'].to_json
+        expect(@instance.images[0].current_path).to eq(public_path('uploads/tmp/1369894322-123-0123-1234/test.jpg'))
       end
 
       it "should not write over a previously assigned file" do
         @instance.images = [stub_file('test.jpg')]
-        @instance.images_cache = ['1369894322-123-1234/monkey.jpg'].to_json
+        @instance.images_cache = ['1369894322-123-0123-1234/monkey.jpg'].to_json
         expect(@instance.images[0].current_path).to match(/test.jpg$/)
       end
     end

spec/mount_single_spec.rb

@@ -252,7 +252,7 @@ def default_url
 
       it "should be the cache name when a file has been cached" do
         @instance.image = stub_file('test.jpg')
-        expect(@instance.image_cache).to match(%r(^[\d]+\-[\d]+\-[\d]{4}/test\.jpg$))
+        expect(@instance.image_cache).to match(%r(^[\d]+\-[\d]+\-[\d]{4}\-[\d]{4}/test\.jpg$))
       end
 
     end
@@ -262,7 +262,7 @@ def default_url
       before do
         allow(@instance).to receive(:write_uploader)
         allow(@instance).to receive(:read_uploader).and_return(nil)
-        CarrierWave::SanitizedFile.new(file_path('test.jpg')).copy_to(public_path('uploads/tmp/1369894322-123-1234/test.jpg'))
+        CarrierWave::SanitizedFile.new(file_path('test.jpg')).copy_to(public_path('uploads/tmp/1369894322-123-0123-1234/test.jpg'))
       end
 
       it "should do nothing when nil is assigned" do
@@ -276,13 +276,13 @@ def default_url
       end
 
       it "retrieve from cache when a cache name is assigned" do
-        @instance.image_cache = '1369894322-123-1234/test.jpg'
-        expect(@instance.image.current_path).to eq(public_path('uploads/tmp/1369894322-123-1234/test.jpg'))
+        @instance.image_cache = '1369894322-123-0123-1234/test.jpg'
+        expect(@instance.image.current_path).to eq(public_path('uploads/tmp/1369894322-123-0123-1234/test.jpg'))
       end
 
       it "should not write over a previously assigned file" do
         @instance.image = stub_file('test.jpg')
-        @instance.image_cache = '1369894322-123-1234/monkey.jpg'
+        @instance.image_cache = '1369894322-123-0123-1234/monkey.jpg'
         expect(@instance.image.current_path).to match(/test.jpg$/)
       end
     end