Skip to content

Commit

Permalink
Support aliases to expires_in for cache stores
Browse files Browse the repository at this point in the history
The `expires_in` option is easy to misremember or mistype as `expire_in`
or `expired_in`, with potentially harmful results. If a developer wants
to cache a value for only 2 seconds but mistakenly types
`expire_in: 2.seconds`, that value will instead be cached for the
default time, likely 6 hours, meaning that users of the site will see
the same data for much longer than they should, and the only recovery
(short of waiting for the 6 hours to elapse) is to manually expire all
relevant keys. This commit allows cache stores to recognize these common
typos as aliases by normalizing them before consuming the options.

In general, we should be careful about adding too many aliases for
options to the cache stores, since each option key used by the base
Cache::Store class is one fewer key that each cache implementation can
customize for itself. This case was approved because of the similarity
of the aliases to the root key and the potential damage caused by
mistaking them.

Fixes #39850.
  • Loading branch information
Nate Pinsky committed Dec 29, 2020
1 parent 96446a9 commit fd8c367
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 3 deletions.
25 changes: 22 additions & 3 deletions activesupport/lib/active_support/cache.rb
Expand Up @@ -22,7 +22,12 @@ module Cache

# These options mean something to all cache implementations. Individual cache
# implementations may support additional options.
UNIVERSAL_OPTIONS = [:namespace, :compress, :compress_threshold, :expires_in, :race_condition_ttl, :coder, :skip_nil]
UNIVERSAL_OPTIONS = [:namespace, :compress, :compress_threshold, :expires_in, :expire_in, :expired_in, :race_condition_ttl, :coder, :skip_nil]

# Mapping of canonical option names to aliases that a store will recognize.
OPTION_ALIASES = {
expires_in: [:expire_in, :expired_in]
}.freeze

module Strategy
autoload :LocalCache, "active_support/cache/strategy/local_cache"
Expand Down Expand Up @@ -186,7 +191,7 @@ def ensure_connection_pool_added!
# except for <tt>:namespace</tt> which can be used to set the global
# namespace for the cache.
def initialize(options = nil)
@options = options ? options.dup : {}
@options = options ? normalize_options(options) : {}
@coder = @options.delete(:coder) { self.class::DEFAULT_CODER } || NullCoder
end

Expand Down Expand Up @@ -249,7 +254,9 @@ def mute
# All caches support auto-expiring content after a specified number of
# seconds. This value can be specified as an option to the constructor
# (in which case all entries will be affected), or it can be supplied to
# the +fetch+ or +write+ method to effect just one entry.
# the +fetch+ or +write+ method to affect just one entry.
# <tt>:expire_in</tt> and <tt>:expired_in</tt> are aliases for
# <tt>:expires_in</tt>.
#
# cache = ActiveSupport::Cache::MemoryStore.new(expires_in: 5.minutes)
# cache.write(key, value, expires_in: 1.minute) # Set a lower value for one entry
Expand Down Expand Up @@ -634,6 +641,7 @@ def delete_multi_entries(entries, **options)
# Merges the default options with ones specific to a method call.
def merged_options(call_options)
if call_options
call_options = normalize_options(call_options)
if options.empty?
call_options
else
Expand All @@ -644,6 +652,17 @@ def merged_options(call_options)
end
end

# Normalize aliased options to their canonical form
def normalize_options(options)
options = options.dup
OPTION_ALIASES.each do |canonical_name, aliases|
alias_key = aliases.detect { |key| options.key?(key) }
options[canonical_name] ||= options[alias_key] if alias_key
options.except!(*aliases)
end
options
end

# Expands and namespaces the cache key. May be overridden by
# cache stores to do additional normalization.
def normalize_key(key, options = nil)
Expand Down
34 changes: 34 additions & 0 deletions activesupport/test/cache/behaviors/cache_store_behavior.rb
Expand Up @@ -406,6 +406,40 @@ def test_expires_in
end
end

def test_expire_in_is_alias_for_expires_in
time = Time.local(2008, 4, 24)

Time.stub(:now, time) do
@cache.write("foo", "bar", expire_in: 20)
assert_equal "bar", @cache.read("foo")
end

Time.stub(:now, time + 10) do
assert_equal "bar", @cache.read("foo")
end

Time.stub(:now, time + 21) do
assert_nil @cache.read("foo")
end
end

def test_expired_in_is_alias_for_expires_in
time = Time.local(2008, 4, 24)

Time.stub(:now, time) do
@cache.write("foo", "bar", expired_in: 20)
assert_equal "bar", @cache.read("foo")
end

Time.stub(:now, time + 10) do
assert_equal "bar", @cache.read("foo")
end

Time.stub(:now, time + 21) do
assert_nil @cache.read("foo")
end
end

def test_race_condition_protection_skipped_if_not_defined
@cache.write("foo", "bar")
time = @cache.send(:read_entry, @cache.send(:normalize_key, "foo", {}), **{}).expires_at
Expand Down

0 comments on commit fd8c367

Please sign in to comment.