Merge pull request #3096 from phuibonhoa/master

FileStore key_file_path does not properly limit filenames to 255 characters
rails · Sep 22, 2011 · 52edadf · 52edadf
1 parent d879213
commit 52edadf
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 10 deletions.
diff --git a/activesupport/lib/active_support/cache/file_store.rb b/activesupport/lib/active_support/cache/file_store.rb
@@ -13,6 +13,7 @@ class FileStore < Store
       attr_reader :cache_path
 
       DIR_FORMATTER = "%03X"
+      FILENAME_MAX_SIZE = 230 # max filename size on file system is 255, minus room for timestamp and random characters appended by Tempfile (used by atomic write)
 
       def initialize(cache_path, options = nil)
         super(options)
@@ -141,15 +142,13 @@ def key_file_path(key)
           hash, dir_1 = hash.divmod(0x1000)
           dir_2 = hash.modulo(0x1000)
           fname_paths = []
-          # Make sure file name is < 255 characters so it doesn't exceed file system limits.
+
-          if fname.size <= 255
+          # Make sure file name doesn't exceed file system limits.
-            fname_paths << fname
+          begin
-          else
+            fname_paths << fname[0...FILENAME_MAX_SIZE]
-            while fname.size <= 255
+            fname = fname[FILENAME_MAX_SIZE..-1]
-              fname_path << fname[0, 255]
+          end until fname.blank?
-              fname = fname[255, -1]
+
-            end
-          end
           File.join(cache_path, DIR_FORMATTER % dir_1, DIR_FORMATTER % dir_2, *fname_paths)
         end
 

diff --git a/activesupport/test/caching_test.rb b/activesupport/test/caching_test.rb
@@ -350,7 +350,7 @@ def test_crazy_key_characters
 
   def test_really_long_keys
     key = ""
-    1000.times{key << "x"}
+    900.times{key << "x"}
     assert_equal true, @cache.write(key, "bar")
     assert_equal "bar", @cache.read(key)
     assert_equal "bar", @cache.fetch(key)
@@ -570,6 +570,15 @@ def test_key_transformation_with_pathname
     key = @cache_with_pathname.send(:key_file_path, "views/index?id=1")
     assert_equal "views/index?id=1", @cache_with_pathname.send(:file_path_key, key)
   end
+
+  # Because file systems have a maximum filename size, filenames > max size should be split in to directories
+  # If filename is 'AAAAB', where max size is 4, the returned path should be AAAA/B
+  def test_key_transformation_max_filename_size
+    key = "#{'A' * ActiveSupport::Cache::FileStore::FILENAME_MAX_SIZE}B"
+    path = @cache.send(:key_file_path, key)    
+    assert path.split('/').all? { |dir_name| dir_name.size <= ActiveSupport::Cache::FileStore::FILENAME_MAX_SIZE}
+    assert_equal 'B', File.basename(path)
+  end
 end
 
 class MemoryStoreTest < ActiveSupport::TestCase