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

Fix keyword argument warning on Action Mailer #38105

Closed
wants to merge 1 commit into from

Conversation

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 27, 2019

No description provided.

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

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 27, 2019
Author Member

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.

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 27, 2019
Author Member

@eregon @mame ideas on how to avoid making this braking change to applications while still removing this warning?

This comment has been minimized.

@eregon

eregon Dec 27, 2019
Contributor

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

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 27, 2019
Author Member

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.

This comment has been minimized.

This comment has been minimized.

@eregon

eregon Dec 29, 2019
Contributor

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

This comment has been minimized.

@eregon

eregon Dec 29, 2019
Contributor

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 # deserialize

But obviously we'd want something more direct.

This comment has been minimized.

@eregon

eregon Dec 29, 2019
Contributor

Remembering the flag during serialization, together with ruby2_keywords by default, actually fixes the warning.

This comment has been minimized.

@mame

mame Dec 29, 2019

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?

This comment has been minimized.

@eregon

eregon Dec 29, 2019
Contributor

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.

mame added a commit to mame/ruby that referenced this pull request Jan 6, 2020
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.
mame added a commit to mame/ruby that referenced this pull request Jan 7, 2020
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.
mame added a commit to mame/ruby that referenced this pull request Jan 17, 2020
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.
mame added a commit to mame/ruby that referenced this pull request Jan 17, 2020
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,
mame added a commit to mame/ruby that referenced this pull request Jan 17, 2020
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]
mame added a commit to ruby/ruby that referenced this pull request Jan 17, 2020
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]
kamipo added a commit to kamipo/rails that referenced this pull request Jan 19, 2020
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
Copy link
Member

@kamipo kamipo commented Jan 20, 2020

This has fixed by #38260.

@kamipo kamipo closed this Jan 20, 2020
@rafaelfranca
Copy link
Member Author

@rafaelfranca rafaelfranca commented Jan 20, 2020

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.

@kamipo
Copy link
Member

@kamipo kamipo commented Jan 20, 2020

Reverted 99f18b6.

@kamipo kamipo reopened this Jan 20, 2020
kamipo added a commit to kamipo/rails that referenced this pull request Jan 20, 2020
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.
@rafaelfranca
Copy link
Member Author

@rafaelfranca rafaelfranca commented Jan 20, 2020

#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 ruby2_keywords.

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.

@rafaelfranca rafaelfranca deleted the rm-fix-kwargs-warning-actionmailbox branch Feb 6, 2020
matzbot pushed a commit to ruby/ruby that referenced this pull request Feb 18, 2020
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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants