Refactor #33254.

Firstly, increment and decrement shouldn't care about the particulars of
key expiry. They should only know that they have to pass that responsibility
on to somewhere else.

Secondly, it moves the key normalization back inside the instrumentation like
it was originally. I think that matches the original design intention or at
the very least it lets users catch haywire key truncation.

Thirdly, it moves the changelog entry to the top of the file, where new entries
go. I couldn't understand what the entry was saying so I tried to rewrite it.

3 files changed, 28 insertions, 29 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 8031d351a9..24ad72c45d 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,13 @@
+*   RedisCacheStore: support key expiry in increment/decrement.
+
+    Pass `:expires_in` to `#increment` and `#decrement` to set a Redis EXPIRE on the key.
+
+    If the key is already set to expire, RedisCacheStore won't extend its expiry.
+
+        Rails.cache.increment("some_key", 1, expires_in: 2.minutes)
+
+    *Jason Lee*
+
 *   Allow Range#=== and Range#cover? on Range
 
     `Range#cover?` can now accept a range argument like `Range#include?` and
@@ -131,16 +141,5 @@
 
     *Eileen M. Uchitelle*, *Aaron Patterson*
 
-*   RedisCacheStore: Support expiring counters.
-    Pass `expires_in: [seconds]` to `#increment` and `#decrement` options
-    to set the Redis EXPIRE if the counter doesn't exist.
-    If the counter exists, Redis doesn't extend its expiry when it's exist.
-
-    ```
-    Rails.cache.increment("my_counter", 1, expires_in: 2.minutes)
-    ```
-
-    *Jason Lee*
-
 
 Please check [5-2-stable](https://github.com/rails/rails/blob/5-2-stable/activesupport/CHANGELOG.md) for previous changes.
diff --git a/activesupport/lib/active_support/cache/redis_cache_store.rb b/activesupport/lib/active_support/cache/redis_cache_store.rb
index a05718b663..5737450b4a 100644
--- a/activesupport/lib/active_support/cache/redis_cache_store.rb
+++ b/activesupport/lib/active_support/cache/redis_cache_store.rb
@@ -256,18 +256,15 @@ module ActiveSupport
       #
       # Failsafe: Raises errors.
       def increment(name, amount = 1, options = nil)
-        options = merged_options(options)
-        key = normalize_key(name, options)
-        expires_in = options[:expires_in].to_i
-
         instrument :increment, name, amount: amount do
           failsafe :increment do
+            options = merged_options(options)
+            key = normalize_key(name, options)
+
             redis.with do |c|
-              val = c.incrby key, amount
-              if expires_in > 0 && c.ttl(key) < 0
-                c.expire key, expires_in
+              c.incrby(key, amount).tap do
+                write_key_expiry(c, key, options)
               end
-              val
             end
           end
         end
@@ -282,18 +279,15 @@ module ActiveSupport
       #
       # Failsafe: Raises errors.
       def decrement(name, amount = 1, options = nil)
-        options = merged_options(options)
-        key = normalize_key(name, options)
-        expires_in = options[:expires_in].to_i
-
         instrument :decrement, name, amount: amount do
           failsafe :decrement do
+            options = merged_options(options)
+            key = normalize_key(name, options)
+
             redis.with do |c|
-              val = c.decrby key, amount
-              if expires_in > 0 && c.ttl(key) < 0
-                c.expire key, expires_in
+              c.decrby(key, amount).tap do
+                write_key_expiry(c, key, options)
               end
-              val
             end
           end
         end
@@ -405,6 +399,12 @@ module ActiveSupport
           end
         end
 
+        def write_key_expiry(client, key, options)
+          if options[:expires_in] && client.ttl(key).negative?
+            client.expire key, options[:expires_in].to_i
+          end
+        end
+
         # Delete an entry from the cache.
         def delete_entry(key, options)
           failsafe :delete_entry, returning: false do
diff --git a/activesupport/test/cache/stores/redis_cache_store_test.rb b/activesupport/test/cache/stores/redis_cache_store_test.rb
index 845254ac69..3b873de383 100644
--- a/activesupport/test/cache/stores/redis_cache_store_test.rb
+++ b/activesupport/test/cache/stores/redis_cache_store_test.rb
@@ -152,7 +152,7 @@ module ActiveSupport::Cache::RedisCacheStoreTests
       # key and ttl exist
       @cache.redis.setex "#{@namespace}:bar", 120, 1
       assert_not_called @cache.redis, :expire do
-        @cache.increment "bar", 1, expires_in: 120
+        @cache.increment "bar", 1, expires_in: 2.minutes
       end
 
       # key exist but not have expire
@@ -172,7 +172,7 @@ module ActiveSupport::Cache::RedisCacheStoreTests
       # key and ttl exist
       @cache.redis.setex "#{@namespace}:bar", 120, 1
       assert_not_called @cache.redis, :expire do
-        @cache.decrement "bar", 1, expires_in: 120
+        @cache.decrement "bar", 1, expires_in: 2.minutes
       end
 
       # key exist but not have expire