Fix keyword argument warning on Action Mailer #38105
Conversation
| @@ -143,9 +143,9 @@ def enqueue_delivery(delivery_method, options = {}) | |||
|
|
|||
| def arguments_for(delivery_job, delivery_method) | |||
| if delivery_job <= MailDeliveryJob | |||
| [@mailer_class.name, @action.to_s, delivery_method.to_s, args: @args] | |||
| [@mailer_class.name, @action.to_s, delivery_method.to_s, args: @args, kwargs: @kwargs] | |||
There was a problem hiding this comment.
This change is a breaking change and I saw no way to make this work without a breaking change. Applications upgrading will lost jobs because the job worker can be using the old code and not accepting this new argument. We have the same problem in the else branch.
There was a problem hiding this comment.
I suspect @jeremyevans and @mame's answer would be something like:
Use ruby2_keywords, and only use *args, **kwargs for delegation when dropping Ruby 2.x support, since that doesn't delegate correctly in 2.6 (**kwargs passes an extra positional {} if no kwargs passed) and in 2.7 (delegate({}) drops positional {}).
There was a problem hiding this comment.
This would not fix the warning, unless application developers call ruby2_keywords in all the actions of their mailers. I don't think application developers should call that method since this is implementation detail of the framework.
There was a problem hiding this comment.
Here is the changeset with ruby2_keywords https://github.com/rails/rails/compare/a55fa7911a. And the warning will be found on this CI job https://buildkite.com/rails/rails/builds/65996#ef0e092f-3e67-46da-8fba-fc7587e63bd8
There was a problem hiding this comment.
I also tried to make the real test work without warnings, which is much more complicated than this minimal example.
I couldn't find a solution after hours of debugging it and adding ruby2_keywords in 8 places.
I wonder who can.
BTW, a way to detect if a Hash is flagged as keyword arguments would be useful for debugging.
To run the test:
git clone rails
cd rails/actionmailbox
bundle install
bundle exec rake test TEST=test/unit/mailbox/bouncing_test.rb
The challenge is to fix this warning:
rails/actionpack/lib/abstract_controller/base.rb:195: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
rails/actionmailbox/test/test_helper.rb:51: warning: The called method `bounce' is defined here
There was a problem hiding this comment.
After a lot of debugging, with my branch enabling ruby2_keywords by default and the help of Hash#flagged? I found that [SPOILER] it's related to serialization and deserialization in https://github.com/rails/rails/blob/09cc7967/activejob/lib/active_job/arguments.rb
That effectively loses the "keyword arguments" flag.
It's not too difficult to remember whether the Hash is flagged, it can be done similarly to how symbols keys are handled there, but we need a way to find out if a Hash is flagged, and a way to flag a Hash.
In my case I used:
private def flag_hash(*args) # would need ruby2_keywords if not using my branch
args[-1]
end
is_flagged = argument.flagged? # serialize, Hash#flagged? is not in 2.7.0
...
result = flag_hash(**result) if is_flagged # deserializeBut obviously we'd want something more direct.
There was a problem hiding this comment.
Remembering the flag during serialization, together with ruby2_keywords by default, actually fixes the warning.
There was a problem hiding this comment.
Solved :-)
https://gist.github.com/mame/18cd2dfc7d965d0bad1216f2fdd008ee
I admit that it is super hacky, though.
ActiveJob serializes and deserializes the argument objects, which removes the ruby2_keywords flag. This patch preserves the flag explicitly.
This may indicate the need of a feature to inspect and add the ruby2_keywords flag to a hash. @jeremyevans What do you think?
There was a problem hiding this comment.
Congrats, almost at the same time than me :)
This is the full patch with ruby2_keywords by default for comparison:
https://gist.github.com/eregon/cd60d95fd7c448379e94afb21e2f8a7c
There is no need to find the 8 methods that might or not need ruby2_keywords.
It was found that a feature to check and add ruby2_keywords flag to an existing Hash is needed when arguments are serialized and deserialized. It is possible to do the same without explicit APIs, but it would be good to provide them as a core feature. rails/rails#38105 (comment) Hash.ruby2_keywords?(hash) checks if hash is flagged or not. Hash.ruby2_keywords!(hash) flags a given hash.
It was found that a feature to check and add ruby2_keywords flag to an existing Hash is needed when arguments are serialized and deserialized. It is possible to do the same without explicit APIs, but it would be good to provide them as a core feature. rails/rails#38105 (comment) Hash.ruby2_keywords?(hash) checks if hash is flagged or not. Hash.ruby2_keywords!(hash) flags a given hash.
It was found that a feature to check and add ruby2_keywords flag to an existing Hash is needed when arguments are serialized and deserialized. It is possible to do the same without explicit APIs, but it would be good to provide them as a core feature. rails/rails#38105 (comment) Hash.ruby2_keywords?(hash) checks if hash is flagged or not. Hash.ruby2_keywords!(hash) flags a given hash.
It was found that a feature to check and add ruby2_keywords flag to an existing Hash is needed when arguments are serialized and deserialized. It is possible to do the same without explicit APIs, but it would be good to provide them as a core feature. rails/rails#38105 (comment) Hash.ruby2_keywords_hash?(hash) checks if hash is flagged or not. Hash.ruby2_keywords_hash(hash) returns a duplicated hash that has a ruby2_keywords flag,
It was found that a feature to check and add ruby2_keywords flag to an existing Hash is needed when arguments are serialized and deserialized. It is possible to do the same without explicit APIs, but it would be good to provide them as a core feature. rails/rails#38105 (comment) Hash.ruby2_keywords_hash?(hash) checks if hash is flagged or not. Hash.ruby2_keywords_hash(hash) returns a duplicated hash that has a ruby2_keywords flag, [Bug #16486]
It was found that a feature to check and add ruby2_keywords flag to an existing Hash is needed when arguments are serialized and deserialized. It is possible to do the same without explicit APIs, but it would be good to provide them as a core feature. rails/rails#38105 (comment) Hash.ruby2_keywords_hash?(hash) checks if hash is flagged or not. Hash.ruby2_keywords_hash(hash) returns a duplicated hash that has a ruby2_keywords flag, [Bug #16486]
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.
|
This has fixed by #38260. |
|
Wait, what? That fix is not acceptable. Can we revert? We should not be increasing the storage on redis or any other system because Ruby is showing warnings. |
|
Reverted 99f18b6. |
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.
|
#38260 has most of what we need, so let's reopen it. We need to make the code future proof so we don't break again when we drop support to We really need to discuss if it is really the right thing to do to ask users to store more data on they datastores to remove a warning in the language I don't think this makes sense, and this unfortunately was a case that Ruby core missed when making this decision. If you serialize method arguments anywhere you will have to increase the serialization storage and some systems can't easily make room for this new space. Can we solve this problem with a version check? First, make the job work with the arguments split or not. Then make another version that always split on Ruby 2.7 or don't split on Ruby 2.6. |
It was found that a feature to check and add ruby2_keywords flag to an existing Hash is needed when arguments are serialized and deserialized. It is possible to do the same without explicit APIs, but it would be good to provide them as a core feature. rails/rails#38105 (comment) Hash.ruby2_keywords_hash?(hash) checks if hash is flagged or not. Hash.ruby2_keywords_hash(hash) returns a duplicated hash that has a ruby2_keywords flag, [Bug #16486]
No description provided.