Patch run_callbacks instead of _run_commit_callbacks#602
Patch run_callbacks instead of _run_commit_callbacks#602adrianna-chang-shopify merged 1 commit intomainfrom
run_callbacks instead of _run_commit_callbacks#602Conversation
|
Tests for this pass locally for me with all three gemfiles. |
lib/identity_cache/query_api.rb
Outdated
| if destroyed? || transaction_changed_attributes.present? | ||
| expire_cache | ||
| if ActiveRecord.version >= Gem::Version.new("7.1") | ||
| def run_callbacks(kind, type = nil, ignore_override: false) |
There was a problem hiding this comment.
Where does the ignore_override come from ?
I should RTFM
|
hey folks 👋 . How did you deal this issues, are you running this branch in production? Because we're running into the same issue, so let me know if I can help get the PR over the finish line. Would be nice to have a identity_cache release that works with Rails 8.1. |
|
👋 We are indeed running this branch in production. We wanted to ensure the fix in this PR wouldn't have negative side effect before merging it and cutting a new release. With BFCM and deploy restrictions it delayed things a bit, but we should be able to merge this now since we confirm it works with no issue. |
|
👋 Hi, yes we've validated this works in production. My goal is to get this out today! |
|
❤️ |
Ref: rails/rails@207a254 _run_commit_callbacks is now a no-op; Rails aliases it to a `run_commit_callbacks!` implementation. We could patch the `!` version, but it can lead to issues depending on when callbacks are defined and when the `IdentityCache` module is included. Overriding `run_callbacks` is a more robust solution.
adrianna-chang-shopify
force-pushed
the
ac-fix-commit-callback-patch
branch
from
eebc8d8 to
8c6a19f
Compare
|
Failures are apparent on main too, so going to merge this and relase. |
|
@byroot this is in the v1.6.4 release for your use :) |
3 participants
Summary
This PR updates
IdentityCache's callback override mechanism to be compatible with updates to Rails' callback handling code, as of rails/rails@207a254. The change in Rails introduces a "fast path" for callbacks and no-ops the_run_#{callback}_callbacksmethods up until the point where a callback is actually defined, at which point Rails aliases_run_#{callback}_callbacksto a!version of the method.Rather than patching the ! version of the callbacks, we patch the
run_callbacksmethod. This way, we don't need to worry about callbacks potentially being defined before we include theIdentityCachegem, which can lead to Rails aliasing the non-patched version of the_run_commit_callbacks!methods. Plus,run_callbacksis public API, and it makes more sense for us to invalidate the cache any time callbacks are run (and not just when they're run via a call tocommitted!within Active Record transaction code.)Changes
run_callbacksinstead of_run_commit_callbacks: Rails 8.1 made_run_commit_callbacksa no-op and now uses run_commit_callbacks! internally (rails/rails@207a254).Overriding run_callbacks is more robust and avoids ordering issues when the IdentityCache module is included.
enabling safer rollout.
TODO: Remove
ignore_overridebefore shipping, after we verify the change doesn't cause any unintended side effects in Shopify's Core monolith.