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

Author: smudge

CHANGELOG.md

@@ -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
@@ -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

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'
 

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

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

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)

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

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 }
 

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

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