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

Teach ActiveJob to set configs on itself #45664

Merged

Conversation

sambostock
Copy link
Contributor

Summary

Previously configs of the form config.active_job.X were only forwarded to ActiveJob::Base. This teaches Active Job to set them on ActiveJob directly instead, if the setter exists.

Other Information

This is a follow up to #45618, which introduces the first config set on ActiveJob directly. @adrianna-chang-shopify pointed out that despite having set the config

# config/application.rb
config.active_job.use_big_decimal_serializer = true

the config was still set to the default value after the application was booted:

Rails.application.config.active_job.use_big_decimal_serializer # => true
ActiveJob.use_big_decimal_serializer                           # => false

In #45618, I had considered if it was more appropriate to set the config on ActiveJob::Base. While that might have allowed individual job classes to opt or out of the serializer (which is of questionable value), it would have required a change to the Arguments.serialize API to inject the job class. I assumed that the config would be automatically forwarded, without realizing that each framework is responsible for setting their config independently (perhaps an opportunity for helper extraction).

@rails-bot rails-bot bot added the activejob label Jul 26, 2022
activejob/CHANGELOG.md Outdated Show resolved Hide resolved
activejob/lib/active_job/railtie.rb Show resolved Hide resolved
@skipkayhil
Copy link
Member

Probably worth testing in railties/test/application/configuration_test.rb like

test "custom serializers should be able to set via config.active_job.custom_serializers in an initializer" do

@sambostock sambostock force-pushed the teach-active-job-to-set-configs-on-itself branch from a49dfe0 to fd0a8e8 Compare July 27, 2022 05:01
@rails-bot rails-bot bot added the railties label Jul 27, 2022
@sambostock sambostock force-pushed the teach-active-job-to-set-configs-on-itself branch 2 times, most recently from fed886a to 5848b53 Compare July 27, 2022 05:22
Copy link
Contributor Author

@sambostock sambostock left a comment

Choose a reason for hiding this comment

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

Ahh, thank you @skipkayhil. I've added tests for the default value, and overriding it.

Comment on lines +2672 to +2691
# When loaded, ActiveJob::Base triggers the :active_job load hooks, which is where config is attached.
# Referencing the constant auto-loads it.
ActiveJob::Base
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This took a long time to figure out, as the assertion would just fail without it. It was particularly hard to test because the tests run in parallel with their output interleaved, and this suite would crash due to running out of file descriptors... (See #45667 😅)

It feels kind of weird that we have to do this. Nothing in the implementation BigDecimalSerializer, its use in Assertions, or let alone the configuration code references ActiveJob::Base, so it's weird that we have to reference it to get this to work. Is there a better way to do this, or is this acceptable?

I wonder if maybe I should have just set the config on ActiveJob::Base instead? That just felt like it attached it to a job, which Arguments isn't. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably shouldn't be set on on_load, Active Record does it on after_initialize, might as well be consistent:

config.after_initialize do
configs.each do |k, v|
next if k == :encryption
setter = "#{k}="
if ActiveRecord.respond_to?(setter)
ActiveRecord.send(setter, v)
end
end
end
ActiveSupport.on_load(:active_record) do
# Configs used in other initializers
configs = configs.except(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've mirrored the approach, including the "double copying" for consistency. ActiveRecord does it for backwards compatibility though, which we don't technically need to support here (since use_big_decimal_serializer is the first config on ActiveJob directly).

@sambostock sambostock force-pushed the teach-active-job-to-set-configs-on-itself branch from 5848b53 to 8af90a4 Compare July 27, 2022 16:16
@@ -25,16 +25,29 @@ class Railtie < Rails::Railtie # :nodoc:
options = app.config.active_job
options.queue_adapter ||= :async

options.after_initialize do
Copy link
Member

Choose a reason for hiding this comment

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

Should not this be app.config or just config? And why do it twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@byroot recommended copying the approach from ActiveRecord::Railtie for consistency (#45664 (comment)), which is why we're doing it twice. I'm glad to change this to whatever is determined to be the correct way to do it.

However, I do see that I misread, and ActiveRecord::Railtie is calling after_initialize on config, not configs, so I'll fix that. It is clearer here with the (existing) name options, which stands out as different.

@byroot
Copy link
Member

byroot commented Jul 27, 2022

@sambostock can you also add a fix for the issue @rafaelfranca noticed in #45618 (comment) ? It can go in the same commit as this.

@sambostock sambostock force-pushed the teach-active-job-to-set-configs-on-itself branch from 8af90a4 to 2ba9c21 Compare July 30, 2022 04:07
@sambostock sambostock force-pushed the teach-active-job-to-set-configs-on-itself branch from 2ba9c21 to d7a5261 Compare July 30, 2022 04:18
@sambostock
Copy link
Contributor Author

@byroot done! I've also added the contributor documentation comment, as discussed. Hopefully I got it all correct.

@byroot
Copy link
Member

byroot commented Jul 30, 2022

Thanks Sam. I'll try to review this later today, in the meantime can you squash your commits? Otherwise I can't merge.

Comment on lines 94 to 95
# 4. Add a commented out section in the new_framework_defaults file, setting the new value.
# 5. Update the guide on configuration.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# 4. Add a commented out section in the new_framework_defaults file, setting the new value.
# 5. Update the guide on configuration.
# 4. Add a commented out section in the new_framework_defaults template, setting the new value.
# 5. Update the guide in `configuring.md`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. I also realized I omitted the step of actually implementing the change, and its corresponding removal step, so I added that too.

Commits squashed too.

Previously configs of the form `config.active_job.X` were only forwarded to
`ActiveJob::Base`. This teaches Active Job to set them on `ActiveJob` directly
instead, if the setter exists.

For consistency, this more or less mirrors the way that Active Record does it.

Co-authored-by: Adrianna Chang <adrianna.chang@shopify.com>
Co-authored-by: Sam Bostock <sam.bostock@shopify.com>

---

Fix use_big_decimal_serializer Rails 7.1 default

This config should be enabled for new Rails 7.1 apps, or apps that have updated
their config to `load_defaults 7.1`, not disabled.

This also clarifies the config accessor comment.

---

Add contributor documentation comment to load_defaults

The process for introducing a change in behavior in Rails can be confusing to
new contributors, so a comment is added roughly explaining how to do so, and
what belongs in `load_defaults` and `new_framework_defaults`.

This comment is aimed at contributors, not consumers, so it is added within the
method, rather than above it.
@sambostock sambostock force-pushed the teach-active-job-to-set-configs-on-itself branch from d7a5261 to b4fffc3 Compare July 30, 2022 15:12
@byroot byroot merged commit 73d2cc8 into rails:main Aug 1, 2022
@sambostock sambostock deleted the teach-active-job-to-set-configs-on-itself branch August 1, 2022 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants