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

Fix IntervalStrategy#decrement in ActiveSupport 7.1 #54

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

meqif
Copy link

@meqif meqif commented Oct 10, 2023

Context

ActiveSupport 7.1 introduced a breaking change in ActiveRecord::Cache#decrement's behavior:

  • prior to 7.1, decrement on a new key does nothing and returns nil
  • since 7.1, decrement on a new key sets its value to 0 first and then decrements, returning a negative value

The 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 of ActiveRecord::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 to write 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.

@meqif meqif requested review from a team as code owners October 10, 2023 10:31
@meqif meqif changed the title Add support for ActiveSupport 7.1 Fix IntervalStrategy#decrement in ActiveSupport 7.1 Oct 10, 2023
razumau added a commit that referenced this pull request Dec 29, 2023
Until it gets fixed in #54
razumau added a commit that referenced this pull request Dec 29, 2023
Until it gets fixed in #54
razumau added a commit that referenced this pull request Dec 29, 2023
Until it gets fixed in #54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant