Skip to content

Commit f4aa7ad

Browse files
committed
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 rails#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' ```
1 parent a9506eb commit f4aa7ad

File tree

2 files changed

+28
-7
lines changed

2 files changed

+28
-7
lines changed

activesupport/lib/active_support/cache.rb

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -740,15 +740,13 @@ def merged_options(call_options)
740740
expires_at = call_options.delete(:expires_at)
741741
call_options[:expires_in] = (expires_at - Time.now) if expires_at
742742

743+
if call_options[:expires_in].is_a?(Time)
744+
expires_in = call_options[:expires_in]
745+
raise ArgumentError.new("expires_in parameter should not be a Time. Did you mean to use expires_at? Got: #{expires_in}")
746+
end
743747
if call_options[:expires_in]&.negative?
744748
expires_in = call_options.delete(:expires_in)
745-
error = ArgumentError.new("Cache expiration time is invalid, cannot be negative: #{expires_in}")
746-
if ActiveSupport::Cache::Store.raise_on_invalid_cache_expiration_time
747-
raise error
748-
else
749-
ActiveSupport.error_reporter&.report(error, handled: true, severity: :warning)
750-
logger.error("#{error.class}: #{error.message}") if logger
751-
end
749+
handle_invalid_expires_in("Cache expiration time is invalid, cannot be negative: #{expires_in}")
752750
end
753751

754752
if options.empty?
@@ -761,6 +759,16 @@ def merged_options(call_options)
761759
end
762760
end
763761

762+
def handle_invalid_expires_in(message)
763+
error = ArgumentError.new(message)
764+
if ActiveSupport::Cache::Store.raise_on_invalid_cache_expiration_time
765+
raise error
766+
else
767+
ActiveSupport.error_reporter&.report(error, handled: true, severity: :warning)
768+
logger.error("#{error.class}: #{error.message}") if logger
769+
end
770+
end
771+
764772
# Normalize aliased options to their canonical form
765773
def normalize_options(options)
766774
options = options.dup

activesupport/test/cache/behaviors/cache_store_behavior.rb

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -602,6 +602,7 @@ def test_invalid_expiration_time_raises_an_error_when_raise_on_invalid_cache_exp
602602
@cache.write(key, "bar", expires_in: -60)
603603
end
604604
assert_equal "Cache expiration time is invalid, cannot be negative: -60", error.message
605+
assert_nil @cache.read(key)
605606
end
606607
end
607608

@@ -612,13 +613,25 @@ def test_invalid_expiration_time_reports_and_logs_when_raise_on_invalid_cache_ex
612613
logs = capture_logs do
613614
key = SecureRandom.uuid
614615
@cache.write(key, "bar", expires_in: -60)
616+
assert_equal "bar", @cache.read(key)
615617
end
616618
assert_includes logs, "ArgumentError: #{error_message}"
617619
end
618620
assert_includes report.error.message, error_message
619621
end
620622
end
621623

624+
def test_expires_in_from_now_raises_an_error
625+
time = 1.minute.from_now
626+
627+
key = SecureRandom.uuid
628+
error = assert_raises(ArgumentError) do
629+
@cache.write(key, "bar", expires_in: time)
630+
end
631+
assert_equal "expires_in parameter should not be a Time. Did you mean to use expires_at? Got: #{time}", error.message
632+
assert_nil @cache.read(key)
633+
end
634+
622635
def test_race_condition_protection_skipped_if_not_defined
623636
key = SecureRandom.alphanumeric
624637
@cache.write(key, "bar")

0 commit comments

Comments
 (0)