Merge pull request #22215 from grosser/grosser/normalize_key

send normalized keys to the cache backends so they do not need to man…

activesupport/lib/active_support/cache.rb

@@ -275,7 +275,7 @@ def mute
       def fetch(name, options = nil)
         if block_given?
           options = merged_options(options)
-          key = namespaced_key(name, options)
+          key = normalize_key(name, options)
 
           instrument(:read, name, options) do |payload|
             cached_entry = read_entry(key, options) unless options[:force]
@@ -302,7 +302,7 @@ def fetch(name, options = nil)
       # Options are passed to the underlying cache implementation.
       def read(name, options = nil)
         options = merged_options(options)
-        key = namespaced_key(name, options)
+        key = normalize_key(name, options)
         instrument(:read, name, options) do |payload|
           entry = read_entry(key, options)
           if entry
@@ -334,7 +334,7 @@ def read_multi(*names)
         instrument_multi(:read, names, options) do |payload|
           results = {}
           names.each do |name|
-            key = namespaced_key(name, options)
+            key = normalize_key(name, options)
             entry = read_entry(key, options)
             if entry
               if entry.expired?
@@ -386,7 +386,7 @@ def write(name, value, options = nil)
 
         instrument(:write, name, options) do
           entry = Entry.new(value, options)
-          write_entry(namespaced_key(name, options), entry, options)
+          write_entry(normalize_key(name, options), entry, options)
         end
       end
 
@@ -397,7 +397,7 @@ def delete(name, options = nil)
         options = merged_options(options)
 
         instrument(:delete, name) do
-          delete_entry(namespaced_key(name, options), options)
+          delete_entry(normalize_key(name, options), options)
         end
       end
 
@@ -408,7 +408,7 @@ def exist?(name, options = nil)
         options = merged_options(options)
 
         instrument(:exist?, name) do
-          entry = read_entry(namespaced_key(name, options), options)
+          entry = read_entry(normalize_key(name, options), options)
           (entry && !entry.expired?) || false
         end
       end
@@ -529,16 +529,17 @@ def expanded_key(key) # :nodoc:
 
         # Prefix a key with the namespace. Namespace and key will be delimited
         # with a colon.
-        def namespaced_key(key, options)
+        def normalize_key(key, options)
           key = expanded_key(key)
           namespace = options[:namespace] if options
           prefix = namespace.is_a?(Proc) ? namespace.call : namespace
           key = "#{prefix}:#{key}" if prefix
           key
         end
+        alias namespaced_key normalize_key
 
         def instrument(operation, key, options = nil)
-          log { "Cache #{operation}: #{namespaced_key(key, options)}#{options.blank? ? "" : " (#{options.inspect})"}" }
+          log { "Cache #{operation}: #{normalize_key(key, options)}#{options.blank? ? "" : " (#{options.inspect})"}" }
 
           payload = { :key => key }
           payload.merge!(options) if options.is_a?(Hash)

activesupport/lib/active_support/cache/file_store.rb

@@ -60,41 +60,38 @@ def delete_matched(matcher, options = nil)
           matcher = key_matcher(matcher, options)
           search_dir(cache_path) do |path|
             key = file_path_key(path)
-            delete_entry(key, options) if key.match(matcher)
+            delete_entry(path, options) if key.match(matcher)
           end
         end
       end
 
       protected
 
         def read_entry(key, options)
-          file_name = key_file_path(key)
-          if File.exist?(file_name)
-            File.open(file_name) { |f| Marshal.load(f) }
+          if File.exist?(key)
+            File.open(key) { |f| Marshal.load(f) }
           end
         rescue => e
           logger.error("FileStoreError (#{e}): #{e.message}") if logger
           nil
         end
 
         def write_entry(key, entry, options)
-          file_name = key_file_path(key)
-          return false if options[:unless_exist] && File.exist?(file_name)
-          ensure_cache_path(File.dirname(file_name))
-          File.atomic_write(file_name, cache_path) {|f| Marshal.dump(entry, f)}
+          return false if options[:unless_exist] && File.exist?(key)
+          ensure_cache_path(File.dirname(key))
+          File.atomic_write(key, cache_path) {|f| Marshal.dump(entry, f)}
           true
         end
 
         def delete_entry(key, options)
-          file_name = key_file_path(key)
-          if File.exist?(file_name)
+          if File.exist?(key)
             begin
-              File.delete(file_name)
-              delete_empty_directories(File.dirname(file_name))
+              File.delete(key)
+              delete_empty_directories(File.dirname(key))
               true
             rescue => e
               # Just in case the error was caused by another process deleting the file first.
-              raise e if File.exist?(file_name)
+              raise e if File.exist?(key)
               false
             end
           end
@@ -118,7 +115,8 @@ def lock_file(file_name, &block) # :nodoc:
         end
 
         # Translate a key into a file path.
-        def key_file_path(key)
+        def normalize_key(key, options)
+          key = super
           fname = URI.encode_www_form_component(key)
 
           if fname.size > FILEPATH_MAX_SIZE
@@ -175,7 +173,7 @@ def search_dir(dir, &callback)
         # Modifies the amount of an already existing integer value that is stored in the cache.
         # If the key is not found nothing is done.
         def modify_value(name, amount, options)
-          file_name = key_file_path(namespaced_key(name, options))
+          file_name = normalize_key(name, options)
 
           lock_file(file_name) do
             options = merged_options(options)

activesupport/lib/active_support/cache/mem_cache_store.rb

@@ -97,7 +97,7 @@ def read_multi(*names)
         options = merged_options(options)
 
         instrument_multi(:read, names, options) do
-          keys_to_names = Hash[names.map{|name| [escape_key(namespaced_key(name, options)), name]}]
+          keys_to_names = Hash[names.map{|name| [normalize_key(name, options), name]}]
           raw_values = @data.get_multi(keys_to_names.keys, :raw => true)
           values = {}
           raw_values.each do |key, value|
@@ -115,7 +115,7 @@ def read_multi(*names)
       def increment(name, amount = 1, options = nil) # :nodoc:
         options = merged_options(options)
         instrument(:increment, name, :amount => amount) do
-          @data.incr(escape_key(namespaced_key(name, options)), amount)
+          @data.incr(normalize_key(name, options), amount)
         end
       rescue Dalli::DalliError => e
         logger.error("DalliError (#{e}): #{e.message}") if logger
@@ -129,7 +129,7 @@ def increment(name, amount = 1, options = nil) # :nodoc:
       def decrement(name, amount = 1, options = nil) # :nodoc:
         options = merged_options(options)
         instrument(:decrement, name, :amount => amount) do
-          @data.decr(escape_key(namespaced_key(name, options)), amount)
+          @data.decr(normalize_key(name, options), amount)
         end
       rescue Dalli::DalliError => e
         logger.error("DalliError (#{e}): #{e.message}") if logger
@@ -153,7 +153,7 @@ def stats
       protected
         # Read an entry from the cache.
         def read_entry(key, options) # :nodoc:
-          deserialize_entry(@data.get(escape_key(key), options))
+          deserialize_entry(@data.get(key, options))
         rescue Dalli::DalliError => e
           logger.error("DalliError (#{e}): #{e.message}") if logger
           nil
@@ -168,15 +168,15 @@ def write_entry(key, entry, options) # :nodoc:
             # Set the memcache expire a few minutes in the future to support race condition ttls on read
             expires_in += 5.minutes
           end
-          @data.send(method, escape_key(key), value, expires_in, options)
+          @data.send(method, key, value, expires_in, options)
         rescue Dalli::DalliError => e
           logger.error("DalliError (#{e}): #{e.message}") if logger
           false
         end
 
         # Delete an entry from the cache.
         def delete_entry(key, options) # :nodoc:
-          @data.delete(escape_key(key))
+          @data.delete(key)
         rescue Dalli::DalliError => e
           logger.error("DalliError (#{e}): #{e.message}") if logger
           false
@@ -187,8 +187,8 @@ def delete_entry(key, options) # :nodoc:
         # Memcache keys are binaries. So we need to force their encoding to binary
         # before applying the regular expression to ensure we are escaping all
         # characters properly.
-        def escape_key(key)
-          key = key.to_s.dup
+        def normalize_key(key, options)
+          key = super.dup
           key = key.force_encoding(Encoding::ASCII_8BIT)
           key = key.gsub(ESCAPE_KEY_CHARS){ |match| "%#{match.getbyte(0).to_s(16).upcase}" }
           key = "#{key[0, 213]}:md5:#{Digest::MD5.hexdigest(key)}" if key.size > 250

activesupport/lib/active_support/cache/strategy/local_cache.rb

@@ -93,14 +93,14 @@ def cleanup(options = nil) # :nodoc:
         def increment(name, amount = 1, options = nil) # :nodoc:
           return super unless local_cache
           value = bypass_local_cache{super}
-          set_cache_value(value, name, amount, options)
+          set_cache_value(name, value, options)
           value
         end
 
         def decrement(name, amount = 1, options = nil) # :nodoc:
           return super unless local_cache
           value = bypass_local_cache{super}
-          set_cache_value(value, name, amount, options)
+          set_cache_value(name, value, options)
           value
         end
 
@@ -123,7 +123,8 @@ def delete_entry(key, options) # :nodoc:
             super
           end
 
-          def set_cache_value(value, name, amount, options) # :nodoc:
+          def set_cache_value(name, value, options) # :nodoc:
+            name = normalize_key(name, options)
             cache = local_cache
             cache.mute do
               if value

activesupport/test/caching_test.rb

@@ -416,7 +416,7 @@ def test_expires_in
 
   def test_race_condition_protection_skipped_if_not_defined
     @cache.write('foo', 'bar')
-    time = @cache.send(:read_entry, 'foo', {}).expires_at
+    time = @cache.send(:read_entry, @cache.send(:normalize_key, 'foo', {}), {}).expires_at
 
     Time.stub(:now, Time.at(time)) do
       result = @cache.fetch('foo') do
@@ -783,21 +783,21 @@ def test_long_uri_encoded_keys
   end
 
   def test_key_transformation
-    key = @cache.send(:key_file_path, "views/index?id=1")
+    key = @cache.send(:normalize_key, "views/index?id=1", {})
     assert_equal "views/index?id=1", @cache.send(:file_path_key, key)
   end
 
   def test_key_transformation_with_pathname
     FileUtils.touch(File.join(cache_dir, "foo"))
-    key = @cache_with_pathname.send(:key_file_path, "views/index?id=1")
+    key = @cache_with_pathname.send(:normalize_key, "views/index?id=1", {})
     assert_equal "views/index?id=1", @cache_with_pathname.send(:file_path_key, key)
   end
 
   # Test that generated cache keys are short enough to have Tempfile stuff added to them and
   # remain valid
   def test_filename_max_size
     key = "#{'A' * ActiveSupport::Cache::FileStore::FILENAME_MAX_SIZE}"
-    path = @cache.send(:key_file_path, key)
+    path = @cache.send(:normalize_key, key, {})
     Dir::Tmpname.create(path) do |tmpname, n, opts|
       assert File.basename(tmpname+'.lock').length <= 255, "Temp filename too long: #{File.basename(tmpname+'.lock').length}"
     end
@@ -807,7 +807,7 @@ def test_filename_max_size
   # 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)
+    path = @cache.send(:normalize_key, key, {})
     assert path.split('/').all? { |dir_name| dir_name.size <= ActiveSupport::Cache::FileStore::FILENAME_MAX_SIZE}
     assert_equal 'B', File.basename(path)
   end