Skip to content

Commit

Permalink
Cache: warning if expires_in is given an incorrect value
Browse files Browse the repository at this point in the history
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 #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'
```
  • Loading branch information
ghiculescu committed Apr 9, 2023
1 parent a9506eb commit f4aa7ad
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 7 deletions.
22 changes: 15 additions & 7 deletions activesupport/lib/active_support/cache.rb
Expand Up @@ -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?
Expand All @@ -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
Expand Down
13 changes: 13 additions & 0 deletions activesupport/test/cache/behaviors/cache_store_behavior.rb
Expand Up @@ -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

Expand All @@ -612,13 +613,25 @@ 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
assert_includes report.error.message, error_message
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")
Expand Down

0 comments on commit f4aa7ad

Please sign in to comment.