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

Support Ruby 3.0 kwarg-handling within .delay API #7

Merged
merged 8 commits into from
Nov 30, 2021
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
11 changes: 11 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,16 @@ and this project aims to adhere to [Semantic Versioning](http://semver.org/spec/
### Removed <!-- for now removed features. -->
### Fixed <!-- for any bug fixes. -->

## [0.4.0] - 2021-11-30
### Fixed
- Fix Ruby 3.0 kwarg compatibility issue when executing jobs enqueued via the
`Delayed::MessageSending` APIs (`.delay` and `handle_asynchronously`).
### Changed
- `Delayed::PerformableMethod` now splits `kwargs` out into a separate attribute, while still being
backwards-compatible with jobs enqueued via the previous gem version. This is an undocumented
internal API and is not considered a breaking change, but if you had previously relied on
`payload_object.args.last` to access keyword arguments, you must now use `payload_object.kwargs`.

## [0.3.0] - 2021-10-26
### Added
- Add more official support for Rails 7.0 (currently alpha2). There were no gem conflicts, but this
Expand Down Expand Up @@ -43,6 +53,7 @@ and this project aims to adhere to [Semantic Versioning](http://semver.org/spec/
ancestor repos (`delayed_job` and `delayed_job_active_record`), plus the changes from Betterment's
internal forks.

[0.4.0]: https://github.com/betterment/delayed/compare/v0.3.0...v0.4.0
[0.3.0]: https://github.com/betterment/delayed/compare/v0.2.0...v0.3.0
[0.2.0]: https://github.com/betterment/delayed/compare/v0.1.1...v0.2.0
[0.1.1]: https://github.com/betterment/delayed/compare/v0.1.0...v0.1.1
Expand Down
3 changes: 2 additions & 1 deletion delayed.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ 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.3.0'
spec.version = '0.4.0'
spec.metadata = {
'changelog_uri' => 'https://github.com/betterment/delayed/blob/main/CHANGELOG.md',
'bug_tracker_uri' => 'https://github.com/betterment/delayed/issues',
'source_code_uri' => 'https://github.com/betterment/delayed',
'rubygems_mfa_required' => 'true',
}
spec.required_ruby_version = '>= 2.6'

Expand Down
18 changes: 4 additions & 14 deletions lib/delayed/message_sending.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ def initialize(payload_class, target, options)
@options = options
end

def method_missing(method, *args)
Job.enqueue({ payload_object: @payload_class.new(@target, method.to_sym, args) }.merge(@options))
def method_missing(method, *args, **kwargs)
Job.enqueue({ payload_object: @payload_class.new(@target, method.to_sym, args, kwargs) }.merge(@options))
end
end

Expand All @@ -18,16 +18,6 @@ def delay(options = {})
DelayProxy.new(PerformableMethod, self, options)
end
alias __delay__ delay

def send_later(method, *args)
warn '[DEPRECATION] `object.send_later(:method)` is deprecated. Use `object.delay.method'
__delay__.__send__(method, *args)
end

def send_at(time, method, *args)
warn '[DEPRECATION] `object.send_at(time, :method)` is deprecated. Use `object.delay(:run_at => time).method'
__delay__(run_at: time).__send__(method, *args)
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing these -- they were already deprecated prior to our fork.

end

module MessageSendingClassMethods
Expand All @@ -36,7 +26,7 @@ def handle_asynchronously(method, opts = {}) # rubocop:disable Metrics/Perceived
punctuation = $1 # rubocop:disable Style/PerlBackrefs
with_method = "#{aliased_method}_with_delay#{punctuation}"
without_method = "#{aliased_method}_without_delay#{punctuation}"
define_method(with_method) do |*args|
define_method(with_method) do |*args, **kwargs|
curr_opts = opts.clone
curr_opts.each_key do |key|
next unless (val = curr_opts[key]).is_a?(Proc)
Expand All @@ -47,7 +37,7 @@ def handle_asynchronously(method, opts = {}) # rubocop:disable Metrics/Perceived
val.call
end
end
delay(curr_opts).__send__(without_method, *args)
delay(curr_opts).__send__(without_method, *args, **kwargs)
end

alias_method without_method, method
Expand Down
2 changes: 1 addition & 1 deletion lib/delayed/performable_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module Delayed
class PerformableMailer < PerformableMethod
def perform
mailer = object.send(method_name, *args)
mailer = super
mailer.respond_to?(:deliver_now) ? mailer.deliver_now : mailer.deliver
end
end
Expand Down
13 changes: 10 additions & 3 deletions lib/delayed/performable_method.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
module Delayed
class PerformableMethod
attr_accessor :object, :method_name, :args
attr_accessor :object, :method_name, :args, :kwargs

def initialize(object, method_name, args)
def initialize(object, method_name, args, kwargs)
raise NoMethodError, "undefined method `#{method_name}' for #{object.inspect}" unless object.respond_to?(method_name, true)

if !her_model?(object) && object.respond_to?(:persisted?) && !object.persisted?
Expand All @@ -11,6 +11,7 @@ def initialize(object, method_name, args)

self.object = object
self.args = args
self.kwargs = kwargs
self.method_name = method_name.to_sym
end

Expand All @@ -23,7 +24,13 @@ def display_name
end

def perform
object.send(method_name, *args) if object
return unless object

if kwargs.nil? || (RUBY_VERSION < '2.7' && kwargs.empty?)
Copy link
Member Author

@smudge smudge Nov 29, 2021

Choose a reason for hiding this comment

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

There's a proposed PR in the one of the ancestor repos, but for this PR I took a very different strategy, because we want to both maintain Ruby 2.6 support and support the Ruby 3 case where both positional and keyword arguments are present. Holding onto kwargs separately resolves edge cases around argument auto-merging behaviors, but then on Ruby 2.6 we need to fall back in the **{} case (since on 2.6 and below that causes {} to get passed as a positional argument).

Copy link
Member Author

Choose a reason for hiding this comment

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

But I think that this should resolve collectiveidea/delayed_job#1134 and might be worth upstreaming, if someone gets a chance.

Choose a reason for hiding this comment

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

You can check if the callee uses kwargs. First get the method object with object.method(method_name) and check its parameter list for :key, :keyreq or :keyrest. (From experience, Rails Mailer magical methods end up with a single :rest argument that you'd need to account for as well.)

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, I don't think we want it to actually care about the arity of the method it's about to call -- only the arity that was provided to it by the enqueuing call. (For example, if I do obj.delay.something(test: 123) on a method that does not accept kwargs, I want to ensure that the job raises an error and tells me that my arguments are wrong.)

So the fix here is really just to prevent **{} from breaking on Ruby 2.6 (because trying to pass that as a kwarg results in it passing {} as a positional arg). This quirk was fixed in 2.7.

object.send(method_name, *args)
else
object.send(method_name, *args, **kwargs)
end
end

def method(sym)
Expand Down
1 change: 1 addition & 0 deletions lib/delayed/psych_ext.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ def encode_with(coder)
'object' => object,
'method_name' => method_name,
'args' => args,
'kwargs' => kwargs,
}
end
end
Expand Down
19 changes: 19 additions & 0 deletions spec/delayed/active_job_adapter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,25 @@ def perform; end
end
end

context 'when ActiveJob has both positional and keyword arguments' do
let(:job_class) do
Class.new(ActiveJob::Base) do # rubocop:disable Rails/ApplicationJob
cattr_accessor(:result)

def perform(arg, kwarg:)
self.class.result = [arg, kwarg]
end
end
end

it 'passes arguments through to the perform method' do
JobClass.perform_later('foo', kwarg: 'bar')

Delayed::Worker.new.work_off
expect(JobClass.result).to eq %w(foo bar)
end
end

context 'when using the ActiveJob test adapter' do
let(:queue_adapter) { :test }

Expand Down
2 changes: 1 addition & 1 deletion spec/delayed/job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ def create_job(opts = {})
context 'large handler' do
before do
text = 'Lorem ipsum dolor sit amet. ' * 1000
@job = described_class.enqueue Delayed::PerformableMethod.new(text, :length, {})
@job = described_class.enqueue Delayed::PerformableMethod.new(text, :length, [], {})
end

it 'has an id' do
Expand Down
56 changes: 33 additions & 23 deletions spec/message_sending_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,27 @@
end

describe 'handle_asynchronously' do
class Story
def tell!(_arg); end
handle_asynchronously :tell!
let(:test_class) do
Class.new do
def tell!(_arg, _kwarg:); end
handle_asynchronously :tell!
end
end

it 'aliases original method' do
expect(Story.new).to respond_to(:tell_without_delay!)
expect(Story.new).to respond_to(:tell_with_delay!)
expect(test_class.new).to respond_to(:tell_without_delay!)
expect(test_class.new).to respond_to(:tell_with_delay!)
end

it 'creates a PerformableMethod' do
story = Story.create
obj = test_class.new
expect {
job = story.tell!(1)
job = obj.tell!('a', kwarg: 'b')
expect(job.payload_object.class).to eq(Delayed::PerformableMethod)
expect(job.payload_object.method_name).to eq(:tell_without_delay!)
expect(job.payload_object.args).to eq([1])
}.to(change { Delayed::Job.count })
expect(job.payload_object.args).to eq(['a'])
expect(job.payload_object.kwargs).to eq(kwarg: 'b')
}.to change { Delayed::Job.count }.by(1)
end

describe 'with options' do
Expand Down Expand Up @@ -64,26 +67,33 @@ def spin; end
end

context 'delay' do
class FairyTail
attr_accessor :happy_ending
let(:fairy_tail_class) do
Class.new do
attr_accessor :happy_ending

def self.princesses; end
def self.princesses; end

def tell
@happy_ending = true
def tell(arg, kwarg:)
@happy_ending = [arg, kwarg]
end
end
end

before do
stub_const('FairyTail', fairy_tail_class)
end

after do
Delayed::Worker.default_queue_name = nil
end

it 'creates a new PerformableMethod job' do
expect {
job = 'hello'.delay.count('l')
job = FairyTail.new.delay.tell('arg', kwarg: 'kwarg')
expect(job.payload_object.class).to eq(Delayed::PerformableMethod)
expect(job.payload_object.method_name).to eq(:count)
expect(job.payload_object.args).to eq(['l'])
expect(job.payload_object.method_name).to eq(:tell)
expect(job.payload_object.args).to eq(['arg'])
expect(job.payload_object.kwargs).to eq(kwarg: 'kwarg')
}.to change { Delayed::Job.count }.by(1)
end

Expand Down Expand Up @@ -111,8 +121,8 @@ def tell
fairy_tail = FairyTail.new
expect {
expect {
fairy_tail.delay.tell
}.to change { fairy_tail.happy_ending }.from(nil).to(true)
fairy_tail.delay.tell('a', kwarg: 'b')
}.to change { fairy_tail.happy_ending }.from(nil).to %w(a b)
}.not_to(change { Delayed::Job.count })
end

Expand All @@ -121,7 +131,7 @@ def tell
fairy_tail = FairyTail.new
expect {
expect {
fairy_tail.delay.tell
fairy_tail.delay.tell('a', kwarg: 'b')
}.not_to change { fairy_tail.happy_ending }
}.to change { Delayed::Job.count }.by(1)
end
Expand All @@ -131,7 +141,7 @@ def tell
fairy_tail = FairyTail.new
expect {
expect {
fairy_tail.delay.tell
fairy_tail.delay.tell('a', kwarg: 'b')
}.not_to change { fairy_tail.happy_ending }
}.to change { Delayed::Job.count }.by(1)
end
Expand All @@ -141,8 +151,8 @@ def tell
fairy_tail = FairyTail.new
expect {
expect {
fairy_tail.delay.tell
}.to change { fairy_tail.happy_ending }.from(nil).to(true)
fairy_tail.delay.tell('a', kwarg: 'b')
}.to change { fairy_tail.happy_ending }.from(nil).to %w(a b)
}.not_to(change { Delayed::Job.count })
end
end
Expand Down
Loading