Fix IntervalStrategy#decrement in ActiveSupport 7.1 #54
+17
−1
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
ActiveSupport 7.1 introduced a breaking change in
ActiveRecord::Cache#decrement
's behavior:decrement
on a new key does nothing and returns nildecrement
on a new key sets its value to 0 first and then decrements, returning a negative valueThe new behavior is at odds with
Prop::IntervalStrategy#decrement
's intended behavior, which sets new keys to 0 when they are decremented.Changes
This PR fixes
IntervalStrategy#decrement
by detecting if the return value ofActiveRecord::Cache#decrement
is nil (< 7.1) or-amount
(>= 7.1) before clamping the value to 0. This is as vulnerable to a race condition as the existing code, so it's not worse in that regard.Prop::IntervalStrategy#increment
does not need a similar change because incrementing a new key is the same as incrementing an existing key set to 0 in ActiveSupport 7.1, just like we intended, and thus the fallback towrite
isn't used in that case.It also adds some missing
require
statements to the test helper: the time-related ActiveSupport extensions (e.g..minutes
) are no longer automatically loaded since Rails 7.1. In turn,active_support/core_ext/numeric/time
relies on new deprecation-related functionality that isn't automatically loaded either, so we try to load that as well.