Merge pull request rails#48043 from joshuay03/always-raise-on-nil-cache-key

Consistently raise an `ArgumentError` if the `ActiveSupport::Cache` key is blank

Author: byroot

activesupport/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Consistently raise an `ArgumentError` if the `ActiveSupport::Cache` key is blank.
+
+    *Joshua Young*
+
 *   Deprecate usage of the singleton `ActiveSupport::Deprecation`.
 
     All usage of `ActiveSupport::Deprecation` as a singleton is deprecated, the most common one being

activesupport/lib/active_support/cache.rb

@@ -783,10 +783,14 @@ def normalize_options(options)
           options
         end
 
-        # Expands and namespaces the cache key. May be overridden by
-        # cache stores to do additional normalization.
+        # Expands and namespaces the cache key.
+        # Raises an exception when the key is +nil+ or an empty string.
+        # May be overridden by cache stores to do additional normalization.
         def normalize_key(key, options = nil)
-          namespace_key expanded_key(key), options
+          str_key = expanded_key(key)
+          raise(ArgumentError, "key cannot be blank") if !str_key || str_key.empty?
+
+          namespace_key str_key, options
         end
 
         # Prefix the key with a namespace string:

activesupport/test/cache/behaviors/cache_instrumentation_behavior.rb

@@ -42,18 +42,6 @@ def test_instrumentation_with_fetch_multi_as_super_operation
     assert_equal @cache.class.name, events[0].payload[:store]
   end
 
-  def test_instrumentation_empty_fetch_multi
-    events = with_instrumentation "read_multi" do
-      @cache.fetch_multi() { |key| key * 2 }
-    end
-
-    assert_equal %w[ cache_read_multi.active_support ], events.map(&:name)
-    assert_equal :fetch_multi, events[0].payload[:super_operation]
-    assert_equal [], events[0].payload[:key]
-    assert_equal [], events[0].payload[:hits]
-    assert_equal @cache.class.name, events[0].payload[:store]
-  end
-
   def test_read_multi_instrumentation
     key_1 = SecureRandom.uuid
     @cache.write(key_1, SecureRandom.alphanumeric)
@@ -70,17 +58,6 @@ def test_read_multi_instrumentation
     assert_equal @cache.class.name, events[0].payload[:store]
   end
 
-  def test_empty_read_multi_instrumentation
-    events = with_instrumentation "read_multi" do
-      @cache.read_multi()
-    end
-
-    assert_equal %w[ cache_read_multi.active_support ], events.map(&:name)
-    assert_equal [], events[0].payload[:key]
-    assert_equal [], events[0].payload[:hits]
-    assert_equal @cache.class.name, events[0].payload[:store]
-  end
-
   private
     def with_instrumentation(method)
       event_name = "cache_#{method}.active_support"

activesupport/test/cache/behaviors/cache_store_behavior.rb

@@ -159,12 +159,6 @@ def test_read_multi_with_expires
     end
   end
 
-  def test_read_multi_with_empty_keys_and_a_logger_and_no_namespace
-    cache = lookup_store(namespace: nil)
-    cache.logger = ActiveSupport::Logger.new(nil)
-    assert_equal({}, cache.read_multi)
-  end
-
   def test_fetch_multi
     key = SecureRandom.uuid
     other_key = SecureRandom.uuid
@@ -464,6 +458,22 @@ def test_keys_are_case_sensitive
     assert_nil @cache.read(key.upcase)
   end
 
+  def test_blank_key
+    invalid_keys = [nil, "", [], {}]
+    invalid_keys.each do |key|
+      assert_raises(ArgumentError) { @cache.write(key, "bar") }
+      assert_raises(ArgumentError) { @cache.read(key) }
+      assert_raises(ArgumentError) { @cache.delete(key) }
+    end
+
+    valid_keys = ["foo", ["bar"], { foo: "bar" }, 0, 1, InstanceTest.new("foo", 2)]
+    valid_keys.each do |key|
+      assert_nothing_raised { @cache.write(key, "bar") }
+      assert_nothing_raised { @cache.read(key) }
+      assert_nothing_raised { @cache.delete(key) }
+    end
+  end
+
   def test_exist
     key = SecureRandom.alphanumeric
     @cache.write(key, "bar")

activesupport/test/cache/stores/redis_cache_store_test.rb

@@ -164,12 +164,6 @@ def test_fetch_multi_with_namespace
       end
     end
 
-    def test_fetch_multi_without_names
-      assert_not_called(@cache.redis, :mget) do
-        @cache.fetch_multi() { }
-      end
-    end
-
     def test_write_expires_at
       @cache.write "key_with_expires_at", "bar", expires_at: 30.minutes.from_now
       assert @cache.redis.ttl("#{@namespace}:key_with_expires_at") > 0