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

Handle proper values for ActiveJob.enqueue_after_transaction_commit for Rails 7.2 support #43

Merged
merged 1 commit into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,18 @@ jobs:
- gemfiles/rails_6_1.gemfile
- gemfiles/rails_7_0.gemfile
- gemfiles/rails_7_1.gemfile
- gemfiles/rails_7_2.gemfile
exclude:
- ruby: '3.2'
gemfile: gemfiles/rails_5_2.gemfile
- ruby: '3.1'
gemfile: gemfiles/rails_5_2.gemfile
- ruby: '3.0'
gemfile: gemfiles/rails_7_2.gemfile
- ruby: '3.0'
gemfile: gemfiles/rails_5_2.gemfile
- ruby: '2.7'
gemfile: gemfiles/rails_7_2.gemfile
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is fine for now! Fully dropping Ruby 2.6 doesn't need to be part of this PR.

- ruby: '2.7'
gemfile: gemfiles/rails_7_1.gemfile
- ruby: '2.6'
Expand Down
1 change: 1 addition & 0 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ Style/FrozenStringLiteralComment:
- 'gemfiles/rails_6_1.gemfile'
- 'gemfiles/rails_7_0.gemfile'
- 'gemfiles/rails_7_1.gemfile'
- 'gemfiles/rails_7_2.gemfile'
- 'gemfiles/rails_main.gemfile'
- 'lib/delayed.rb'
- 'lib/delayed/active_job_adapter.rb'
Expand Down
6 changes: 6 additions & 0 deletions Appraisals
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ appraise 'rails-7-1' do
gem 'activerecord', '~> 7.1.0'
end

appraise 'rails-7-2' do
gem 'actionmailer', '~> 7.2.0'
gem 'activejob', '~> 7.2.0'
gem 'activerecord', '~> 7.2.0'
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Finish Appraisal set up for Rails 7.2

Copy link
Member

Choose a reason for hiding this comment

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

Do you intend to do that in this PR?

(While you're at it, mind bumping the delayed gem's version?)

Copy link
Contributor Author

@warmerzega warmerzega Aug 13, 2024

Choose a reason for hiding this comment

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

Yep. Was struggling to get the incantation working to build/install mysql. Should be good now

Copy link
Member

Choose a reason for hiding this comment

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

I think the version bump requires an update to all of the lockfiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumped


appraise 'rails-main' do
gem 'actionmailer', github: 'rails/rails', glob: 'actionmailer/*.gemspec'
gem 'activejob', github: 'rails/rails', glob: 'activejob/*.gemspec'
Expand Down
2 changes: 1 addition & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
delayed (0.5.4)
delayed (0.5.5)
activerecord (>= 5.2)
concurrent-ruby

Expand Down
2 changes: 1 addition & 1 deletion delayed.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Gem::Specification.new do |spec|
spec.require_paths = ['lib']
spec.summary = 'a multi-threaded, SQL-driven ActiveJob backend used at Betterment to process millions of background jobs per day'

spec.version = '0.5.4'
spec.version = '0.5.5'
spec.metadata = {
'changelog_uri' => 'https://github.com/betterment/delayed/blob/main/CHANGELOG.md',
'bug_tracker_uri' => 'https://github.com/betterment/delayed/issues',
Expand Down
18 changes: 18 additions & 0 deletions gemfiles/rails_7_2.gemfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# This file was generated by Appraisal

source "https://rubygems.org"

gem "actionmailer", "~> 7.2.0"
gem "activejob", "~> 7.2.0"
gem "activerecord", "~> 7.2.0"
gem "appraisal"
gem "betterlint"
gem "mysql2"
gem "pg"
gem "rake"
gem "rspec"
gem "sqlite3", "~> 1.7.3"
gem "timecop"
gem "zeitwerk"

gemspec path: "../"
2 changes: 1 addition & 1 deletion lib/delayed/active_job_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

@warmerzega warmerzega Aug 13, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, feels like there is a missing validation on the value you are passing in.

Copy link
Member

@smudge smudge Oct 25, 2024

Choose a reason for hiding this comment

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

raise UnsafeEnqueueError, "The ':delayed' ActiveJob adapter is not compatible with enqueue_after_transaction_commit"
end

Expand Down
15 changes: 13 additions & 2 deletions spec/delayed/active_job_adapter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -288,16 +288,27 @@ def perform(arg, kwarg:)
end

if ActiveJob.gem_version.release >= Gem::Version.new('7.2')
context 'when the given job sets enqueue_after_transaction_commit to true' do
context 'when the given job sets enqueue_after_transaction_commit to :always' do
before do
JobClass.include ActiveJob::EnqueueAfterTransactionCommit # normally run in an ActiveJob railtie
JobClass.enqueue_after_transaction_commit = true
JobClass.enqueue_after_transaction_commit = :always
end

it 'raises an exception on enqueue' do
expect { JobClass.perform_later }.to raise_error(Delayed::ActiveJobAdapter::UnsafeEnqueueError)
end
end

context 'when the given job sets enqueue_after_transaction_commit to :never' do
before do
JobClass.include ActiveJob::EnqueueAfterTransactionCommit # normally run in an ActiveJob railtie
JobClass.enqueue_after_transaction_commit = :never
end

it 'does not raises an exception on enqueue' do
expect { JobClass.perform_later }.not_to raise_error(Delayed::ActiveJobAdapter::UnsafeEnqueueError)
end
end
end

context 'when using the ActiveJob test adapter' do
Expand Down
Loading