From f4aa7ad9a61bc4cbd1bb585cb965109b1f707e96 Mon Sep 17 00:00:00 2001 From: Alex Ghiculescu Date: Fri, 7 Apr 2023 14:47:35 -0600 Subject: [PATCH] Cache: warning if `expires_in` is given an incorrect value In Rails 7, if you do `Rails.cache.write(key, value, expires_in: 1.minute.from_now)`, it will work. The actual expiration will be much more than a minute away, but it won't raise. (The correct code is `expires_in: 1.minute` or `expires_at: 1.minute.from_now`.) Since https://github.com/rails/rails/pull/45892 the same code will error with: ``` NoMethodError: undefined method `negative?' for 2008-04-24 00:01:00 -0600:Time /Users/alex/Code/rails/activesupport/lib/active_support/cache.rb:743:in `merged_options' /Users/alex/Code/rails/activesupport/lib/active_support/cache.rb:551:in `write' ``` To make it a bit easier to upgrade to Rails 7.1, this PR introduces a better error if you pass a `Time` object to `expires_in:` ``` ArgumentError: expires_in parameter should not be a Time. Did you mean to use expires_at? Got: 2023-04-07 14:47:45 -0600 /Users/alex/Code/rails/activesupport/lib/active_support/cache.rb:765:in `handle_invalid_expires_in' /Users/alex/Code/rails/activesupport/lib/active_support/cache.rb:745:in `merged_options' /Users/alex/Code/rails/activesupport/lib/active_support/cache.rb:551:in `write' ``` --- activesupport/lib/active_support/cache.rb | 22 +++++++++++++------ .../cache/behaviors/cache_store_behavior.rb | 13 +++++++++++ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb index 186e3da52e996..cfbdde31c90aa 100644 --- a/activesupport/lib/active_support/cache.rb +++ b/activesupport/lib/active_support/cache.rb @@ -740,15 +740,13 @@ def merged_options(call_options) expires_at = call_options.delete(:expires_at) call_options[:expires_in] = (expires_at - Time.now) if expires_at + if call_options[:expires_in].is_a?(Time) + expires_in = call_options[:expires_in] + raise ArgumentError.new("expires_in parameter should not be a Time. Did you mean to use expires_at? Got: #{expires_in}") + end if call_options[:expires_in]&.negative? expires_in = call_options.delete(:expires_in) - error = ArgumentError.new("Cache expiration time is invalid, cannot be negative: #{expires_in}") - if ActiveSupport::Cache::Store.raise_on_invalid_cache_expiration_time - raise error - else - ActiveSupport.error_reporter&.report(error, handled: true, severity: :warning) - logger.error("#{error.class}: #{error.message}") if logger - end + handle_invalid_expires_in("Cache expiration time is invalid, cannot be negative: #{expires_in}") end if options.empty? @@ -761,6 +759,16 @@ def merged_options(call_options) end end + def handle_invalid_expires_in(message) + error = ArgumentError.new(message) + if ActiveSupport::Cache::Store.raise_on_invalid_cache_expiration_time + raise error + else + ActiveSupport.error_reporter&.report(error, handled: true, severity: :warning) + logger.error("#{error.class}: #{error.message}") if logger + end + end + # Normalize aliased options to their canonical form def normalize_options(options) options = options.dup diff --git a/activesupport/test/cache/behaviors/cache_store_behavior.rb b/activesupport/test/cache/behaviors/cache_store_behavior.rb index aabd997817d2a..8a32583b5911d 100644 --- a/activesupport/test/cache/behaviors/cache_store_behavior.rb +++ b/activesupport/test/cache/behaviors/cache_store_behavior.rb @@ -602,6 +602,7 @@ def test_invalid_expiration_time_raises_an_error_when_raise_on_invalid_cache_exp @cache.write(key, "bar", expires_in: -60) end assert_equal "Cache expiration time is invalid, cannot be negative: -60", error.message + assert_nil @cache.read(key) end end @@ -612,6 +613,7 @@ def test_invalid_expiration_time_reports_and_logs_when_raise_on_invalid_cache_ex logs = capture_logs do key = SecureRandom.uuid @cache.write(key, "bar", expires_in: -60) + assert_equal "bar", @cache.read(key) end assert_includes logs, "ArgumentError: #{error_message}" end @@ -619,6 +621,17 @@ def test_invalid_expiration_time_reports_and_logs_when_raise_on_invalid_cache_ex end end + def test_expires_in_from_now_raises_an_error + time = 1.minute.from_now + + key = SecureRandom.uuid + error = assert_raises(ArgumentError) do + @cache.write(key, "bar", expires_in: time) + end + assert_equal "expires_in parameter should not be a Time. Did you mean to use expires_at? Got: #{time}", error.message + assert_nil @cache.read(key) + end + def test_race_condition_protection_skipped_if_not_defined key = SecureRandom.alphanumeric @cache.write(key, "bar")