Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache: warning if expires_in is given an incorrect value #47891

Merged
merged 1 commit into from
Apr 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 15 additions & 7 deletions activesupport/lib/active_support/cache.rb
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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