From 6d9d831a731f20595f99655c3ae5436003caaeca Mon Sep 17 00:00:00 2001 From: Nathan Griffith Date: Tue, 30 Nov 2021 16:30:50 -0500 Subject: [PATCH] Support Ruby 3.0 kwarg-handling within `.delay` API (#7) Resolves #6. While we already fully support Ruby 3.0 kwargs via ActiveJob, the legacy `delay` and `handle_asynchronously` APIs did not support the new [separation of positional and keyword arguments](https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/) introduced in 2.7 and enforced in 3.0. This means that methods accepting both positional and keyword arguments (e.g. `def foo(a, b:)`) would fail with a `wrong number of arguments (given 2, expected 1; required keyword:)` error on Ruby 3. In order to resolve this, this PR changes `Delayed::PerformableMethod` to handle kwargs separately from args, with a backwards compatibility check for any Ruby 2.6 methods that do not accept keyword arguments. It should also support jobs that were enqueued by the prior `delayed` gem version (where the new `kwargs` accessor would be nil, and `args` contains the kwargs as its last item). --- CHANGELOG.md | 11 ++++ delayed.gemspec | 3 +- lib/delayed/message_sending.rb | 18 ++---- lib/delayed/performable_mailer.rb | 2 +- lib/delayed/performable_method.rb | 13 +++- lib/delayed/psych_ext.rb | 1 + spec/delayed/active_job_adapter_spec.rb | 19 ++++++ spec/delayed/job_spec.rb | 2 +- spec/message_sending_spec.rb | 56 +++++++++------- spec/performable_mailer_spec.rb | 86 ++++++++++++++----------- spec/performable_method_spec.rb | 47 ++++++++++---- spec/psych_ext_spec.rb | 24 +++++++ 12 files changed, 190 insertions(+), 92 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 411ed701..7bdfb4aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,16 @@ and this project aims to adhere to [Semantic Versioning](http://semver.org/spec/ ### Removed ### Fixed +## [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 @@ -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 diff --git a/delayed.gemspec b/delayed.gemspec index 97353e8f..71401473 100644 --- a/delayed.gemspec +++ b/delayed.gemspec @@ -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' diff --git a/lib/delayed/message_sending.rb b/lib/delayed/message_sending.rb index cba0c88a..800140ca 100644 --- a/lib/delayed/message_sending.rb +++ b/lib/delayed/message_sending.rb @@ -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 @@ -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 end module MessageSendingClassMethods @@ -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) @@ -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 diff --git a/lib/delayed/performable_mailer.rb b/lib/delayed/performable_mailer.rb index 8535c452..7c5d6d7b 100644 --- a/lib/delayed/performable_mailer.rb +++ b/lib/delayed/performable_mailer.rb @@ -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 diff --git a/lib/delayed/performable_method.rb b/lib/delayed/performable_method.rb index 483e7531..8b6d46d9 100644 --- a/lib/delayed/performable_method.rb +++ b/lib/delayed/performable_method.rb @@ -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? @@ -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 @@ -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?) + object.send(method_name, *args) + else + object.send(method_name, *args, **kwargs) + end end def method(sym) diff --git a/lib/delayed/psych_ext.rb b/lib/delayed/psych_ext.rb index 210ba633..f4f21105 100644 --- a/lib/delayed/psych_ext.rb +++ b/lib/delayed/psych_ext.rb @@ -6,6 +6,7 @@ def encode_with(coder) 'object' => object, 'method_name' => method_name, 'args' => args, + 'kwargs' => kwargs, } end end diff --git a/spec/delayed/active_job_adapter_spec.rb b/spec/delayed/active_job_adapter_spec.rb index f737fde0..31914f3f 100644 --- a/spec/delayed/active_job_adapter_spec.rb +++ b/spec/delayed/active_job_adapter_spec.rb @@ -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 } diff --git a/spec/delayed/job_spec.rb b/spec/delayed/job_spec.rb index cf6542db..72609706 100644 --- a/spec/delayed/job_spec.rb +++ b/spec/delayed/job_spec.rb @@ -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 diff --git a/spec/message_sending_spec.rb b/spec/message_sending_spec.rb index 918f9fd6..f630a09e 100644 --- a/spec/message_sending_spec.rb +++ b/spec/message_sending_spec.rb @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 diff --git a/spec/performable_mailer_spec.rb b/spec/performable_mailer_spec.rb index 89154b6f..ee6bb0c2 100644 --- a/spec/performable_mailer_spec.rb +++ b/spec/performable_mailer_spec.rb @@ -1,58 +1,41 @@ require 'helper' -class MyMailer < ActionMailer::Base - def signup(email) - mail to: email, subject: 'Delaying Emails', from: 'delayedjob@example.com', body: 'Delaying Emails Body' - end -end +describe Delayed::PerformableMailer do + let(:mailer_class) do + Class.new(ActionMailer::Base) do + cattr_accessor(:emails) { [] } -describe ActionMailer::Base do - describe 'delay' do - it 'enqueues a PerformableEmail job' do - expect { - job = MyMailer.delay.signup('john@example.com') - expect(job.payload_object.class).to eq(Delayed::PerformableMailer) - expect(job.payload_object.method_name).to eq(:signup) - expect(job.payload_object.args).to eq(['john@example.com']) - }.to change { Delayed::Job.count }.by(1) + def signup(email, beta_tester: false) + mail to: email, subject: "Delaying Emails (beta: #{beta_tester})", from: 'delayedjob@example.com', body: 'Delaying Emails Body' + end end end - describe 'delay on a mail object' do - it 'raises an exception' do - expect { - MyMailer.signup('john@example.com').delay - }.to raise_error(RuntimeError) - end + before do + stub_const('MyMailer', mailer_class) end - describe Delayed::PerformableMailer do - describe 'perform' do - it 'calls the method and #deliver on the mailer' do - email = double('email', deliver: true) - mailer_class = double('MailerClass', signup: email) - mailer = described_class.new(mailer_class, :signup, ['john@example.com']) + describe 'perform' do + it 'calls the method and #deliver on the mailer' do + mailer = MyMailer.new + email = double('email', deliver: true) + allow(mailer).to receive(:mail).and_return(email) + mailer_job = described_class.new(mailer, :signup, ['john@example.com'], {}) - expect(mailer_class).to receive(:signup).with('john@example.com') - expect(email).to receive(:deliver) - mailer.perform - end + expect(email).to receive(:deliver) + mailer_job.perform end end -end -if defined?(ActionMailer::Parameterized::Mailer) - describe ActionMailer::Parameterized::Mailer do + describe ActionMailer::Base do describe 'delay' do it 'enqueues a PerformableEmail job' do expect { - job = MyMailer.with(foo: 1, bar: 2).delay.signup('john@example.com') + job = MyMailer.delay.signup('john@example.com', beta_tester: true) expect(job.payload_object.class).to eq(Delayed::PerformableMailer) - expect(job.payload_object.object.class).to eq(described_class) - expect(job.payload_object.object.instance_variable_get('@mailer')).to eq(MyMailer) - expect(job.payload_object.object.instance_variable_get('@params')).to eq(foo: 1, bar: 2) expect(job.payload_object.method_name).to eq(:signup) expect(job.payload_object.args).to eq(['john@example.com']) + expect(job.payload_object.kwargs).to eq(beta_tester: true) }.to change { Delayed::Job.count }.by(1) end end @@ -60,9 +43,36 @@ def signup(email) describe 'delay on a mail object' do it 'raises an exception' do expect { - MyMailer.with(foo: 1, bar: 2).signup('john@example.com').delay + MyMailer.signup('john@example.com').delay }.to raise_error(RuntimeError) end end end + + if defined?(ActionMailer::Parameterized::Mailer) + describe ActionMailer::Parameterized::Mailer do + describe 'delay' do + it 'enqueues a PerformableEmail job' do + expect { + job = MyMailer.with(foo: 1, bar: 2).delay.signup('john@example.com', beta_tester: false) + expect(job.payload_object.class).to eq(Delayed::PerformableMailer) + expect(job.payload_object.object.class).to eq(described_class) + expect(job.payload_object.object.instance_variable_get('@mailer')).to eq(MyMailer) + expect(job.payload_object.object.instance_variable_get('@params')).to eq(foo: 1, bar: 2) + expect(job.payload_object.method_name).to eq(:signup) + expect(job.payload_object.args).to eq(['john@example.com']) + expect(job.payload_object.kwargs).to eq(beta_tester: false) + }.to change { Delayed::Job.count }.by(1) + end + end + + describe 'delay on a mail object' do + it 'raises an exception' do + expect { + MyMailer.with(foo: 1, bar: 2).signup('john@example.com').delay + }.to raise_error(RuntimeError) + end + end + end + end end diff --git a/spec/performable_method_spec.rb b/spec/performable_method_spec.rb index 5a032d4b..a37fae27 100644 --- a/spec/performable_method_spec.rb +++ b/spec/performable_method_spec.rb @@ -2,8 +2,18 @@ describe Delayed::PerformableMethod do describe 'perform' do + let(:test_class) do + Class.new do + cattr_accessor :result + + def foo(arg, kwarg:) + self.class.result = [arg, kwarg] + end + end + end + before do - @method = described_class.new('foo', :count, ['o']) + @method = described_class.new(test_class.new, :foo, ['a'], { kwarg: 'b' }) end context 'with the persisted record cannot be found' do @@ -17,14 +27,29 @@ end it 'calls the method on the object' do - expect(@method.object).to receive(:count).with('o') - @method.perform + expect { @method.perform } + .to change { test_class.result } + .from(nil).to %w(a b) + end + + if RUBY_VERSION < '3.0' + context 'when kwargs are nil (job was delayed via prior gem version)' do + before do + @method = described_class.new(test_class.new, :foo, ['a', { kwarg: 'b' }], nil) + end + + it 'calls the method on the object' do + expect { @method.perform } + .to change { test_class.result } + .from(nil).to %w(a b) + end + end end end it "raises a NoMethodError if target method doesn't exist" do expect { - described_class.new(Object, :method_that_does_not_exist, []) + described_class.new(Object, :method_that_does_not_exist, [], {}) }.to raise_error(NoMethodError) end @@ -33,29 +58,29 @@ def private_method; end private :private_method end - expect { described_class.new(clazz.new, :private_method, []) }.not_to raise_error + expect { described_class.new(clazz.new, :private_method, [], {}) }.not_to raise_error end context 'when it receives an object that is not persisted' do let(:object) { double(persisted?: false, expensive_operation: true) } it 'raises an ArgumentError' do - expect { described_class.new(object, :expensive_operation, []) }.to raise_error ArgumentError + expect { described_class.new(object, :expensive_operation, [], {}) }.to raise_error ArgumentError end it 'does not raise ArgumentError if the object acts like a Her model' do allow(object.class).to receive(:save_existing).and_return(true) - expect { described_class.new(object, :expensive_operation, []) }.not_to raise_error + expect { described_class.new(object, :expensive_operation, [], {}) }.not_to raise_error end end describe 'display_name' do it 'returns class_name#method_name for instance methods' do - expect(described_class.new('foo', :count, ['o']).display_name).to eq('String#count') + expect(described_class.new('foo', :count, ['o'], {}).display_name).to eq('String#count') end it 'returns class_name.method_name for class methods' do - expect(described_class.new(Class, :inspect, []).display_name).to eq('Class.inspect') + expect(described_class.new(Class, :inspect, [], {}).display_name).to eq('Class.inspect') end end @@ -84,7 +109,7 @@ def private_method; end end it 'delegates failure hook to object' do - method = described_class.new('object', :size, []) + method = described_class.new('object', :size, [], {}) expect(method.object).to receive(:failure) method.failure end @@ -114,7 +139,7 @@ def private_method; end end it 'delegates failure hook to object' do - method = described_class.new('object', :size, []) + method = described_class.new('object', :size, [], {}) expect(method.object).to receive(:failure) method.failure end diff --git a/spec/psych_ext_spec.rb b/spec/psych_ext_spec.rb index b2c02f9b..8663bf64 100644 --- a/spec/psych_ext_spec.rb +++ b/spec/psych_ext_spec.rb @@ -11,6 +11,30 @@ end end + context Delayed::PerformableMethod do + it 'serializes with object, method_name, args, and kwargs' do + Delayed::PerformableMethod.new(String, :new, ['hello'], capacity: 20).tap do |pm| + serialized = YAML.dump_dj(pm) + expect(serialized).to eq <<~YAML + --- !ruby/object:Delayed::PerformableMethod + object: !ruby/class 'String' + method_name: :new + args: + - hello + kwargs: + :capacity: 20 + YAML + + deserialized = YAML.load_dj(serialized) + expect(deserialized).to be_an_instance_of(Delayed::PerformableMethod) + expect(deserialized.object).to eq String + expect(deserialized.method_name).to eq :new + expect(deserialized.args).to eq ['hello'] + expect(deserialized.kwargs).to eq(capacity: 20) + end + end + end + context ActiveRecord::Base do it 'serializes and deserializes in a version-independent way' do Story.create.tap do |story|