Support Rails 7.1+ by adding ActiveJob Que adapter removed from Rails #403
Conversation
(Tests not yet passing)
This no longer exists in Rails, but is still needed. I simply copied the class from the [removal PR](rails/rails#46005), and removed the unnecessary comments, and adjusted the formatting slightly. (Tests not yet passing)
I'd thought we'd need to now use `ActiveJob::Base.queue_adapter = ActiveJob::QueueAdapters::QueAdapter.new` rather than `ActiveJob::Base.queue_adapter = :que`, but it seems that by keeping the adapter class namespace unchanged, we can still use the latter.
…equire_job_options_kwarg? We don't need to support old versions of Que in the adapter anymore! Que's codebase always uses the latest Que =P I considered removing & inlining Que::ActiveJob::WrapperExtensions::ClassMethods#enqueue into our Rails 7.1+ QueAdapter, but it still needs to exist in place for support for older versions of Rails =\ Keeping it in place conditionally based on the Rails version would be [a bit complex](#399 (comment)), and so perhaps not an improvement.
|
Any ETA on the PR being merged and a new gem being cut? Thanks in advance. |
|
Thanks @ZimbiX, let's do it. |
|
Gah, unfortunately this has broken the build. Trying to work out why. Maybe Rails 7.1.1 release a few days ago breaks support for postgres 9 somehow? See #405 EDIT: Looks like this PR may have broken postgres 9 support. I might just drop the rails version to 7.0 for the postgres 9 test. EDIT: Fixed in this PR. I am just going to drop the rails version back down temporarily until we can bump it up again. |
See this comment for more info: https://github.com/que-rb/que/pull/403\#issuecomment-1764134027
Done. @ZimbiX I might need to you announce it on discord, I don't seem to have permission. |
Done =) Thanks for working out the failure! I agree with your workaround (#405). I imagine their fix would be included in the next release of Rails, so it looks like excluding 7.1.1 was another option. |
See this comment for more info: https://github.com/que-rb/que/pull/403\#issuecomment-1764134027
Rails wants to slim down the number of adapters it ships with. This approach is not dissimilar from que-rb/que#403, but we have included all of the tests from Active Job to ensure it remains stable. Not all of the tests are necessary, we can trim down the tests for non-resque adapters for example. But I wanted to start with a clean base, and find a pattern that can be applied to all of the adapters we want to extract. The reason for the extension and `const_remove` is that the class name serialized for the job cannot change, otherwise we risk losing jobs. In the future, after the adapter is removed downstream, you should be able to version guard that part at least. And eventually if you so choose, rename the constant, assuming that you give users ample time to migrate their queues but that seems unnecessary.
ActiveJob's Que adapter was removed from Rails in rails/rails#46005 (issue: rails/rails#45899). This is a good thing for simplicity - Que and ActiveJob won't have to keep patching over each other's changes. But it's unfortunately broken Que in Rails.
Sorry, I didn't realise this would be an issue in removing the Que adapter from Rails! 😬 I really should have tested that before giving the go ahead.
The adapter is still necessary, and so this PR incorporates it into Que.
I'd thought we'd need to now use
ActiveJob::Base.queue_adapter = ActiveJob::QueueAdapters::QueAdapter.newrather thanActiveJob::Base.queue_adapter = :que, but it seems that by keeping the adapter class namespace unchanged, we can still use the latter. We have to keep it unchanged for backwards compatibility, as the class name ofActiveJob::QueueAdapters::QueAdapter::JobWrappermust remain the same, given this string would be in old job records in the database.Because the class name needs to remain the same, but we still need to support older versions of ActiveJob where that class exists there, I've defined the adapter here only when the version of ActiveJob is 7.1+.
I was able to simplify the adapter slightly by removing
#require_job_options_kwarg?, since the Que codebase only has to cater for one version of Que =PI considered removing & inlining
Que::ActiveJob::WrapperExtensions::ClassMethods#enqueueinto our Rails 7.1+ QueAdapter, but it still needs to exist in place to support older versions of Rails =\ Keeping it in place conditionally based on the Rails version would be a bit complex, and so perhaps not an improvement.Supersedes #401.