Handle proper values for ActiveJob.enqueue_after_transaction_commit for Rails 7.2 support #43
Conversation
| @@ -17,7 +17,7 @@ def enqueue_at(job, timestamp) | |||
| private | |||
|
|
|||
| def _enqueue(job, opts = {}) | |||
| if job.class.respond_to?(:enqueue_after_transaction_commit) && job.class.enqueue_after_transaction_commit | |||
| if job.class.respond_to?(:enqueue_after_transaction_commit) && job.class.enqueue_after_transaction_commit == :always | |||
There was a problem hiding this comment.
This isn't the perfect solution to this problem. The implementation in ActiveJob::EnqueueAfterTransactionCommit does allow for unset (or non :always/:never) values which then gets delegated to job_adapter.enqueue_after_transaction_commit?. Given the job adapter will always be delayed, if a non :always value is passed in, it will always be false
There was a problem hiding this comment.
Yup, that was my read as well.
But, gosh, that Rails API seems like it's going to cause such confusion. I.e. you could set def self.enqueue_after_transaction_commit; true; end and then it goes down the false codepath because nothing about the API itself checks for completeness on :always/:never/:default. 🤔
There was a problem hiding this comment.
Yep, feels like there is a missing validation on the value you are passing in.
There was a problem hiding this comment.
| gem 'actionmailer', '~> 7.2.0' | ||
| gem 'activejob', '~> 7.2.0' | ||
| gem 'activerecord', '~> 7.2.0' | ||
| end |
There was a problem hiding this comment.
TODO: Finish Appraisal set up for Rails 7.2
There was a problem hiding this comment.
Do you intend to do that in this PR?
(While you're at it, mind bumping the delayed gem's version?)
There was a problem hiding this comment.
Yep. Was struggling to get the incantation working to build/install mysql. Should be good now
There was a problem hiding this comment.
I think the version bump requires an update to all of the lockfiles.
warmerzega
force-pushed
the
main
branch
3 times, most recently
from
b93fb8f to
528cb72
Compare
| - ruby: '3.0' | ||
| gemfile: gemfiles/rails_5_2.gemfile | ||
| - ruby: '2.7' | ||
| gemfile: gemfiles/rails_7_2.gemfile |
There was a problem hiding this comment.
Rails 7.2 has a minimum version of Ruby 3.1. Not sure if I should add an exclusion for Ruby 2.6. Got a sense from this PR that we may be removing Ruby 2.6 support? #42
There was a problem hiding this comment.
Yeah, this is fine for now! Fully dropping Ruby 2.6 doesn't need to be part of this PR.
|
@warmerzega released! |
Hi Betterment friends!
As addressed in @smudge's PR #40, Rails 7.2 introduces the concept of
enqueue_after_transaction_commit, which should not be used in conjunction with Delayed. The implementation, and the associated tests, make the assumption that the values used by ActiveJob aretrueandfalse. The implementation in ActiveJob::EnqueueAfterTransactionCommit (https://github.com/rails/rails/blob/ac1d7681d05906b2b050eced76c6a2165f420821/activejob/lib/active_job/enqueue_after_transaction_commit.rb#L7-L14) uses the values of:alwaysand:never. The issues arises when hitting the conditional check (https://github.com/Betterment/delayed/compare/main...warmerzega:delayed:main?expand=1#diff-325ce2458ac2c3e05143813a4899c9f996ae51f54cdb8fa5f79f50efab872f9aR20) where!!:never == trueand causes the condition to be true (and raising the exception) whenjob.class.enqueue_after_transaction_commit == :never.