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 for Ruby 3.0 added #1130

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

webhoernchen
Copy link

Some methods are not worked with Ruby 3.0

Comment on lines 27 to 31
if args_is_a_hash?
object.send(method_name, **args.first)
else
object.send(method_name, *args)
end if object
Copy link
Member

Choose a reason for hiding this comment

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

This fails if the only argument is intentionally a hash, for example sending partial request params to a job.

@jdelStrother
Copy link

@albus522 -

This fails if the only argument is intentionally a hash, for example sending partial request params to a job.

I started looking into this - think we'd want to capture the PerformableMethod's args and kwargs separately (see below). It's going to be painful to continue supporting ruby 2.6 for this, though - how would you feel about only supporting 2.7+ for the next release?

diff --git a/lib/delayed/message_sending.rb b/lib/delayed/message_sending.rb
index a2808fd..34cc4f6 100644
--- a/lib/delayed/message_sending.rb
+++ b/lib/delayed/message_sending.rb
@@ -7,8 +7,8 @@ module Delayed
     end
 
     # rubocop:disable MethodMissing
-    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
     # rubocop:enable MethodMissing
   end
diff --git a/lib/delayed/performable_method.rb b/lib/delayed/performable_method.rb
index 9f64a61..a8765f6 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 object.respond_to?(:persisted?) && !object.persisted?
@@ -11,6 +11,7 @@ module Delayed
 
       self.object       = object
       self.args         = args
+      self.kwargs       = kwargs
       self.method_name  = method_name.to_sym
     end
 
@@ -26,19 +27,9 @@ module Delayed
     # Otherwise the following error is thrown
     # ArgumentError:
     #   wrong number of arguments (given 1, expected 0; required keywords:
     if RUBY_VERSION >= '3.0'
       def perform
-        return unless object
-
-        if args_is_a_hash?
-          object.send(method_name, **args.first)
-        else
-          object.send(method_name, *args)
-        end
-      end
-
-      def args_is_a_hash?
-        args.size == 1 && args.first.is_a?(Hash)
+        object.send(method_name, *args, **kwargs) if object
       end
     else
       def perform
diff --git a/spec/message_sending_spec.rb b/spec/message_sending_spec.rb
index ca58edc..281f8b2 100644
--- a/spec/message_sending_spec.rb
+++ b/spec/message_sending_spec.rb
@@ -64,12 +64,17 @@ describe Delayed::MessageSending do
 
   context 'delay' do
     class FairyTail
-      attr_accessor :happy_ending
+      attr_accessor :happy_ending, :ogre, :dead
       def self.princesses; end
 
       def tell
         @happy_ending = true
       end
+
+      def defeat(ogre_params, dead: true)
+        @ogre = ogre_params
+        @dead = dead
+      end
     end
 
     after do
@@ -143,5 +148,14 @@ describe Delayed::MessageSending do
         end.to change(fairy_tail, :happy_ending).from(nil).to(true)
       end.not_to(change { Delayed::Job.count })
     end
+
+    it 'can handle a mix of params and kwargs' do
+      Delayed::Worker.delay_jobs = false
+      fairy_tail = FairyTail.new
+      expect do
+        fairy_tail.delay.defeat({:name => 'shrek'}, :dead => false)
+      end.to change(fairy_tail, :ogre).from(nil).to(:name => 'shrek').
+        and(change(fairy_tail, :dead).from(nil).to(false))
+    end
   end
 end
diff --git a/spec/performable_method_spec.rb b/spec/performable_method_spec.rb
index 17179c6..c5d3e78 100644
--- a/spec/performable_method_spec.rb
+++ b/spec/performable_method_spec.rb
@@ -34,6 +34,17 @@ describe Delayed::PerformableMethod do
     end
   end
 
+  describe 'perform with hash object and kwargs' do
+    before do
+      @method = Delayed::PerformableMethod.new('foo', :count, [{:o => true}], :o2 => false)
+    end
+
+    it 'calls the method on the object' do
+      expect(@method.object).to receive(:count).with({:o => true}, :o2 => false)
+      @method.perform
+    end
+  end
+
   describe 'perform with many hash objects' do
     before do
       @method = Delayed::PerformableMethod.new('foo', :count, [{:o => true}, {:o2 => true}])
@@ -110,7 +121,7 @@ describe Delayed::PerformableMethod do
         end
       end
 
-      @method = Delayed::PerformableMethod.new(klass.new, :test_method, [{:name => 'name', :any => 'any'}])
+      @method = Delayed::PerformableMethod.new(klass.new, :test_method, [], :name => 'name', :any => 'any')
     end
 
     it 'calls the method on the object' do

@jacobjlevine
Copy link

Hi folks. Any updates on this? As far as I can tell, it appears Delayed Job still doesn't support keyword args for background jobs in Ruby 3.

@infoman
Copy link

infoman commented Sep 17, 2021

Does not work for me on Ruby 3.0.2 and Rails 6.0.4:

Something.some_method(arg, other_arg: value)
  • success
Something.delay.some_method(arg, other_arg: value)
  • wrong number of arguments (given 2, expected 1)

@jdelStrother
Copy link

I've built on this here - #1158 - which has been working well in production for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants