Skip to content

Bust caches in User::active_profile and User::pending_profile#9760

Merged
matthinz merged 9 commits intomainfrom
matthinz/theres-something-strange-in-the-neighborhood
Dec 15, 2023
Merged

Bust caches in User::active_profile and User::pending_profile#9760
matthinz merged 9 commits intomainfrom
matthinz/theres-something-strange-in-the-neighborhood

Conversation

@matthinz
Copy link
Contributor

🛠 Summary of changes

User::active_profile and User::pending_profile both cache their results in instance variables. This is fine, but sometimes, especially in tests, those caches get stale, and there's not a straightforward way to bust them without creating a new User instance (for example, User::reload does not clear those cached instance variables).

This PR updates active_profile and pending_profile to check whether the cached Profile they're about to return are still active or pending, and busts the cache if something has changed.

If the `active` state of a Profile does not match what's expected when calling User::active_profile or User::pending_profile, clear the cached value.

This is mostly a convenience for tests.

[skip changelog]
Since they were referencing a profile that's not actually pending anymore.
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@aduth
Copy link
Contributor

aduth commented Dec 14, 2023

Typically I might imagine the invalidation would happen at the same time as the state transition, but I suppose that's a little tricky here with how this method is defined in a separate class.

Not actually required to achieve the same effect.
Since these calls are preceded by .reload (which busts the cache), these are no longer the _exact same_ objects, even though they refer to the same records
@matthinz matthinz force-pushed the matthinz/theres-something-strange-in-the-neighborhood branch from e292f4e to 0909766 Compare December 14, 2023 22:05
@matthinz matthinz requested a review from jmhooper December 14, 2023 22:29
@matthinz
Copy link
Contributor Author

Sorry for the churn here, all. I got confused and didn't realize that we aren't currently caching nil values for active_profile (but we are for pending_profile). To reduce the scope of this PR, I'm keeping the existing behavior, but adding the following cache busts:

  • When active_profile would return a Profile that is not active
  • When pending_profile would return a Profile that is active
  • When User::reload is used

Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great

@matthinz matthinz merged commit f1c573e into main Dec 15, 2023
@matthinz matthinz deleted the matthinz/theres-something-strange-in-the-neighborhood branch December 15, 2023 19:35
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.

6 participants