diff --git a/activesupport/lib/active_support/cache/redis_cache_store.rb b/activesupport/lib/active_support/cache/redis_cache_store.rb index 6f91b3ea80ad1..8683681812b86 100644 --- a/activesupport/lib/active_support/cache/redis_cache_store.rb +++ b/activesupport/lib/active_support/cache/redis_cache_store.rb @@ -438,18 +438,26 @@ def change_counter(key, amount, options) redis.then do |c| c = c.node_for(key) if c.is_a?(Redis::Distributed) - if options[:expires_in] && supports_expire_nx? - c.pipelined do |pipeline| - pipeline.incrby(key, amount) - pipeline.call(:expire, key, options[:expires_in].to_i, "NX") - end.first + expires_in = options[:expires_in] + + if expires_in + if supports_expire_nx? + count, _ = c.pipelined do |pipeline| + pipeline.incrby(key, amount) + pipeline.call(:expire, key, expires_in.to_i, "NX") + end + else + count, ttl = c.pipelined do |pipeline| + pipeline.incrby(key, amount) + pipeline.ttl(key) + end + c.expire(key, expires_in.to_i) if ttl < 0 + end else count = c.incrby(key, amount) - if count != amount && options[:expires_in] && c.ttl(key) < 0 - c.expire(key, options[:expires_in].to_i) - end - count end + + count end end diff --git a/activesupport/test/cache/stores/redis_cache_store_test.rb b/activesupport/test/cache/stores/redis_cache_store_test.rb index 6d66f8689295f..36a8e1d75c868 100644 --- a/activesupport/test/cache/stores/redis_cache_store_test.rb +++ b/activesupport/test/cache/stores/redis_cache_store_test.rb @@ -141,6 +141,8 @@ class StoreTest < ActiveSupport::TestCase @cache = lookup_store(expires_in: 60) # @cache.logger = Logger.new($stdout) # For test debugging + @cache_no_ttl = lookup_store + # For LocalCacheBehavior tests @peek = lookup_store(expires_in: 60) end @@ -193,6 +195,23 @@ def test_write_expires_at end end + def test_increment_ttl + # existing key + redis_backend(@cache_no_ttl) { |r| r.set "#{@namespace}:jar", 10 } + @cache_no_ttl.increment "jar", 1 + redis_backend(@cache_no_ttl) do |r| + assert r.get("#{@namespace}:jar").to_i == 11 + assert r.ttl("#{@namespace}:jar") < 0 + end + + # new key + @cache_no_ttl.increment "kar", 1 + redis_backend(@cache_no_ttl) do |r| + assert r.get("#{@namespace}:kar").to_i == 1 + assert r.ttl("#{@namespace}:kar") < 0 + end + end + def test_increment_expires_in @cache.increment "foo", 1, expires_in: 60 redis_backend do |r| @@ -208,13 +227,30 @@ def test_increment_expires_in end # key exist but not have expire - redis_backend { |r| r.set "#{@namespace}:dar", 10 } - @cache.increment "dar", 1, expires_in: 60 - redis_backend do |r| + redis_backend(@cache_no_ttl) { |r| r.set "#{@namespace}:dar", 10 } + @cache_no_ttl.increment "dar", 1, expires_in: 60 + redis_backend(@cache_no_ttl) do |r| assert r.ttl("#{@namespace}:dar") > 0 end end + def test_decrement_ttl + # existing key + redis_backend(@cache_no_ttl) { |r| r.set "#{@namespace}:jar", 10 } + @cache_no_ttl.decrement "jar", 1 + redis_backend(@cache_no_ttl) do |r| + assert r.get("#{@namespace}:jar").to_i == 9 + assert r.ttl("#{@namespace}:jar") < 0 + end + + # new key + @cache_no_ttl.decrement "kar", 1 + redis_backend(@cache_no_ttl) do |r| + assert r.get("#{@namespace}:kar").to_i == -1 + assert r.ttl("#{@namespace}:kar") < 0 + end + end + def test_decrement_expires_in @cache.decrement "foo", 1, expires_in: 60 redis_backend do |r| @@ -230,9 +266,9 @@ def test_decrement_expires_in end # key exist but not have expire - redis_backend { |r| r.set "#{@namespace}:dar", 10 } - @cache.decrement "dar", 1, expires_in: 60 - redis_backend do |r| + redis_backend(@cache_no_ttl) { |r| r.set "#{@namespace}:dar", 10 } + @cache_no_ttl.decrement "dar", 1, expires_in: 60 + redis_backend(@cache_no_ttl) do |r| assert r.ttl("#{@namespace}:dar") > 0 end end @@ -252,8 +288,8 @@ def test_decrement_expires_in end end - def redis_backend - @cache.redis.with do |r| + def redis_backend(cache = @cache) + cache.redis.with do |r| yield r if block_given? return r end