Merge pull request rails#49873 from Mangara/mangara-memcache-fallback

Make return value of `Cache::Store#write` consistent

Author: byroot

activesupport/CHANGELOG.md

@@ -1,3 +1,11 @@
+*   Make return values of `Cache::Store#write` consistent.
+
+    The return value was not specified before. Now it returns `true` on a successful write,
+    `nil` if there was an error talking to the cache backend, and `false` if the write failed
+    for another reason (e.g. the key already exists and `unless_exist: true` was passed).
+
+    *Sander Verdonschot*
+
 *   Fix logged cache keys not always matching actual key used by cache action.
 
     *Hartley McGuire*

activesupport/lib/active_support/cache.rb

@@ -166,7 +166,7 @@ def retrieve_store_class(store)
     #   cache = ActiveSupport::Cache::MemoryStore.new
     #
     #   cache.read('city')   # => nil
-    #   cache.write('city', "Duckburgh")
+    #   cache.write('city', "Duckburgh") # => true
     #   cache.read('city')   # => "Duckburgh"
     #
     #   cache.write('not serializable', Proc.new {}) # => TypeError
@@ -630,6 +630,9 @@ def fetch_multi(*names)
       # Writes the value to the cache with the key. The value must be supported
       # by the +coder+'s +dump+ and +load+ methods.
       #
+      # Returns +true+ if the write succeeded, +nil+ if there was an error talking
+      # to the cache backend, or +false+ if the write failed for another reason.
+      #
       # By default, cache entries larger than 1kB are compressed. Compression
       # allows more data to be stored in the same memory footprint, leading to
       # fewer cache evictions and higher hit rates.

activesupport/lib/active_support/cache/mem_cache_store.rb

@@ -263,10 +263,10 @@ def write_serialized_entry(key, payload, **options)
             # Set the memcache expire a few minutes in the future to support race condition ttls on read
             expires_in += 5.minutes
           end
-          rescue_error_with false do
+          rescue_error_with nil do
             # Don't pass compress option to Dalli since we are already dealing with compression.
             options.delete(:compress)
-            @data.with { |c| c.send(method, key, payload, expires_in, **options) }
+            @data.with { |c| !!c.send(method, key, payload, expires_in, **options) }
           end
         end
 

activesupport/lib/active_support/cache/redis_cache_store.rb

@@ -368,8 +368,8 @@ def write_serialized_entry(key, payload, raw: false, unless_exist: false, expire
           if pipeline
             pipeline.set(key, payload, **modifiers)
           else
-            failsafe :write_entry, returning: false do
-              redis.then { |c| c.set key, payload, **modifiers }
+            failsafe :write_entry, returning: nil do
+              redis.then { |c| !!c.set(key, payload, **modifiers) }
             end
           end
         end

activesupport/test/cache/behaviors/cache_store_behavior.rb

@@ -7,14 +7,14 @@
 module CacheStoreBehavior
   def test_should_read_and_write_strings
     key = SecureRandom.uuid
-    assert @cache.write(key, "bar")
+    assert_equal true, @cache.write(key, "bar")
     assert_equal "bar", @cache.read(key)
   end
 
   def test_should_overwrite
     key = SecureRandom.uuid
-    @cache.write(key, "bar")
-    @cache.write(key, "baz")
+    assert_equal true, @cache.write(key, "bar")
+    assert_equal true, @cache.write(key, "baz")
     assert_equal "baz", @cache.read(key)
   end
 
@@ -116,25 +116,25 @@ def test_fetch_with_forced_cache_miss_without_block
 
   def test_should_read_and_write_hash
     key = SecureRandom.uuid
-    assert @cache.write(key, a: "b")
+    assert_equal true, @cache.write(key, a: "b")
     assert_equal({ a: "b" }, @cache.read(key))
   end
 
   def test_should_read_and_write_integer
     key = SecureRandom.uuid
-    assert @cache.write(key, 1)
+    assert_equal true, @cache.write(key, 1)
     assert_equal 1, @cache.read(key)
   end
 
   def test_should_read_and_write_nil
     key = SecureRandom.uuid
-    assert @cache.write(key, nil)
+    assert_equal true, @cache.write(key, nil)
     assert_nil @cache.read(key)
   end
 
   def test_should_read_and_write_false
     key = SecureRandom.uuid
-    assert @cache.write(key, false)
+    assert_equal true, @cache.write(key, false)
     assert_equal false, @cache.read(key)
   end
 

activesupport/test/cache/behaviors/failure_safety_behavior.rb

@@ -49,10 +49,10 @@ def test_read_multi_failure_returns_empty_hash
     end
   end
 
-  def test_write_failure_returns_false
+  def test_write_failure_returns_nil
     key = SecureRandom.uuid
     emulating_unavailability do |cache|
-      assert_equal false, cache.write(key, SecureRandom.alphanumeric)
+      assert_nil cache.write(key, SecureRandom.alphanumeric)
     end
   end
 

activesupport/test/cache/stores/mem_cache_store_test.rb

@@ -207,6 +207,11 @@ def test_write_expires_at
     end
   end
 
+  def test_write_with_unless_exist
+    assert_equal true, @cache.write("foo", 1)
+    assert_equal false, @cache.write("foo", 1, unless_exist: true)
+  end
+
   def test_increment_expires_in
     cache = lookup_store(raw: true, namespace: nil)
     assert_called_with client(cache), :incr, [ "foo", 1, 60, 1 ] do

activesupport/test/cache/stores/redis_cache_store_test.rb

@@ -195,6 +195,11 @@ def test_write_expires_at
       end
     end
 
+    def test_write_with_unless_exist
+      assert_equal true, @cache.write("foo", 1)
+      assert_equal false, @cache.write("foo", 1, unless_exist: true)
+    end
+
     def test_increment_ttl
       # existing key
       redis_backend(@cache_no_ttl) { |r| r.set "#{@namespace}:jar", 10 }