Fix keyword arguments warnings in Active Job #38260

kamipo · 2020-01-19T03:34:37Z

This is a reattempt to fix keyword arguments warnings in Active Job.

Now Ruby (master) has Hash.ruby2_keywords_hash{?,} and that will be
backported to 2.7.1.

ruby/ruby#2818
https://bugs.ruby-lang.org/issues/16486

I've emulated that for 2.7.0 and older versions.

Related rails#38053, rails#38187, rails#38105. This is a reattempt to fix keyword arguments warnings in Active Job. Now Ruby (master) has `Hash.ruby2_keywords_hash{?,}` and that will be backported to 2.7.1. ruby/ruby#2818 https://bugs.ruby-lang.org/issues/16486 I've emulated that for 2.7.0 and older versions.

kamipo · 2020-01-19T04:06:59Z

activejob/test/cases/argument_serialization_test.rb

@@ -170,7 +170,7 @@ class ClassArgument; end
  end

  test "allows for keyword arguments" do
-    KwargsJob.perform_later(argument: 2)
+    KwargsJob.perform_now(argument: 2)


This change is to excercise perform_now with keyword arguments, perform_later is already tested by MultipleKwargsJob.

rails/activejob/test/cases/test_helper_test.rb

Lines 561 to 570 in d72f940

def test_assert_enqueued_with_selective_args

args = ->(job_args) do

assert_equal 1, job_args.first[:argument1]

assert job_args.first[:argument2].key?(:b)

end

assert_enqueued_with(job: MultipleKwargsJob, args: args) do

MultipleKwargsJob.perform_later(argument2: { b: 2, a: 1 }, argument1: 1)

end

end

Fix keyword arguments warnings in Active Job

rafaelfranca · 2020-01-20T16:42:34Z

You should have wait the review of this PR before merging. This is increasing the storage on the background job database because of a warning on Ruby. I don't think it is a good solution. Can we revert it?

rafaelfranca · 2020-01-20T16:44:54Z

This PR also don't address the problem that will be caused when we have to really split keyword arguments and regular arguments. When the split happen the MailDeliveryJob signature will have to change and it means that workers running the old code will start to fail as soon a job with the new signature arrives. We need to take care of that as well.

rafaelfranca · 2020-01-20T16:48:53Z

activejob/lib/active_job/arguments.rb

          result = serialize_hash(argument)
          result[SYMBOL_KEYS_KEY] = symbol_keys
+          result[RUBY2_KEYWORDS_KEY] = true if Hash.ruby2_keywords_hash?(argument)


This mean storing 25 characters, ~96 more bytes to the database just to get rid of a warning on Ruby. I don't think this is a good trade off.

rafaelfranca · 2020-01-20T16:51:28Z

actionmailer/lib/action_mailer/parameterized.rb

@@ -161,7 +162,8 @@ def delivery_job_class

        def arguments_for(delivery_job, delivery_method)
          if delivery_job <= MailDeliveryJob
-            [@mailer_class.name, @action.to_s, delivery_method.to_s, params: @params, args: @args]
+            ruby2_keywords_arguments \
+              @mailer_class.name, @action.to_s, delivery_method.to_s, params: @params, args: @args


This is still not future safe. Workers with old code will still break as soon we change the code to actually split the arguments. We should at least add a kwargs argument to the job with a default as nil right now so, when we have to split the arguments for real, the workers are ready to receive a pass them. But I'm not sure if we can even pass them right now, what makes this solution still not good enough.

I'm not sure why "Workers with old code will still break as soon we change the code to actually split the arguments", but I agree that "We should at least add a kwargs argument to the job with a default" as a future plan, that is to be required if Ruby team will decide to remove internal kwargs flag (my hope that removal should not be happen since there is a serialization/deserialization problem, though).

Anyway, I'll revert this for now.

Let's say we deploy Rails X to production, the first request running on 6.1 add a job to the queue with the arguments and keyword arguments split. The job worker that fetches that job is still using Rails X-1, given the deploy is still happening. That same worker would have code that doesn't know how to deal with the split keyword arguments so it would only pass around the regular arguments and leave the keyword arguments out of the method call.

Thanks for the explanation. I see that concern is almost not for this change but for a future plan.

Right. As soon we release the code to deal with this is better. It means that the entire 6.1+ releases will be able to run Ruby 3.0 without having to release a version to prepare and a version to actually add support.

…activejob" This reverts commit 80e72c5, reversing changes made to 0dad1e3.

Related rails#38053, rails#38187, rails#38105, rails#38260. This is a reattempt to fix keyword arguments warnings in Active Job. Now Ruby (master) has `Hash.ruby2_keywords_hash{?,}` and that will be backported to 2.7.1. ruby/ruby#2818 https://bugs.ruby-lang.org/issues/16486 I've emulated that for 2.7.0 and older versions.

rails-bot bot added the activejob label Jan 19, 2020

kamipo commented Jan 19, 2020

View reviewed changes

Fix keyword arguments warnings in Action Mailer

c10b580

rails-bot bot added the actionmailer label Jan 19, 2020

kamipo merged commit 80e72c5 into rails:master Jan 20, 2020

kamipo deleted the fix_kwargs_warning_for_activejob branch January 20, 2020 00:31

kamipo added a commit that referenced this pull request Jan 20, 2020

Merge pull request #38260 from kamipo/fix_kwargs_warning_for_activejob

7d65478

Fix keyword arguments warnings in Active Job

kamipo mentioned this pull request Jan 20, 2020

Fix keyword argument warning on Action Mailer #38105

Closed

rafaelfranca reviewed Jan 20, 2020

View reviewed changes

kamipo added a commit that referenced this pull request Jan 20, 2020

Revert "Merge pull request #38260 from kamipo/fix_kwargs_warning_for_…

99f18b6

…activejob" This reverts commit 80e72c5, reversing changes made to 0dad1e3.

kamipo added a commit that referenced this pull request Jan 20, 2020

Revert "Merge pull request #38260 from kamipo/fix_kwargs_warning_for_…

482f6d6

…activejob" This reverts commit 80e72c5, reversing changes made to 0dad1e3.

kamipo mentioned this pull request Jan 20, 2020

Fix keyword arguments warnings in Active Job #38267

Merged

kamipo mentioned this pull request Jul 1, 2020

PERF: 45% faster attributes for readonly usage #39612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix keyword arguments warnings in Active Job #38260

Fix keyword arguments warnings in Active Job #38260

kamipo commented Jan 19, 2020

kamipo Jan 19, 2020

rafaelfranca commented Jan 20, 2020

rafaelfranca commented Jan 20, 2020

rafaelfranca Jan 20, 2020

rafaelfranca Jan 20, 2020 •

edited

kamipo Jan 20, 2020

rafaelfranca Jan 20, 2020

kamipo Jan 20, 2020

rafaelfranca Jan 20, 2020

	def test_assert_enqueued_with_selective_args
	args = ->(job_args) do
	assert_equal 1, job_args.first[:argument1]
	assert job_args.first[:argument2].key?(:b)
	end

	assert_enqueued_with(job: MultipleKwargsJob, args: args) do
	MultipleKwargsJob.perform_later(argument2: { b: 2, a: 1 }, argument1: 1)
	end
	end

Fix keyword arguments warnings in Active Job #38260

Fix keyword arguments warnings in Active Job #38260

Conversation

kamipo commented Jan 19, 2020

kamipo Jan 19, 2020

Choose a reason for hiding this comment

rafaelfranca commented Jan 20, 2020

rafaelfranca commented Jan 20, 2020

rafaelfranca Jan 20, 2020

Choose a reason for hiding this comment

rafaelfranca Jan 20, 2020 • edited

Choose a reason for hiding this comment

kamipo Jan 20, 2020

Choose a reason for hiding this comment

rafaelfranca Jan 20, 2020

Choose a reason for hiding this comment

kamipo Jan 20, 2020

Choose a reason for hiding this comment

rafaelfranca Jan 20, 2020

Choose a reason for hiding this comment

rafaelfranca Jan 20, 2020 •

edited