From fd8c36707f64776ce0f48379816083401ffe40fc Mon Sep 17 00:00:00 2001 From: Nate Pinsky Date: Mon, 28 Dec 2020 14:13:46 -0800 Subject: [PATCH] Support aliases to expires_in for cache stores 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. --- activesupport/lib/active_support/cache.rb | 25 ++++++++++++-- .../cache/behaviors/cache_store_behavior.rb | 34 +++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb index f1cfe5e81a..246692837a 100644 --- a/activesupport/lib/active_support/cache.rb +++ b/activesupport/lib/active_support/cache.rb @@ -22,7 +22,12 @@ module ActiveSupport # 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" @@ -186,7 +191,7 @@ module ActiveSupport # except for :namespace 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 @@ -249,7 +254,9 @@ module ActiveSupport # 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. + # :expire_in and :expired_in are aliases for + # :expires_in. # # cache = ActiveSupport::Cache::MemoryStore.new(expires_in: 5.minutes) # cache.write(key, value, expires_in: 1.minute) # Set a lower value for one entry @@ -634,6 +641,7 @@ module ActiveSupport # 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 @@ -644,6 +652,17 @@ module ActiveSupport 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) diff --git a/activesupport/test/cache/behaviors/cache_store_behavior.rb b/activesupport/test/cache/behaviors/cache_store_behavior.rb index a44b9696ee..429d5e106f 100644 --- a/activesupport/test/cache/behaviors/cache_store_behavior.rb +++ b/activesupport/test/cache/behaviors/cache_store_behavior.rb @@ -406,6 +406,40 @@ module CacheStoreBehavior 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