Advice needed on making Devise-Omniauth work with Zeitwerk
Closed this issue · 9 comments
I am looking for some best practice advice on how gems that require a user-provided class at runtime (eg. to be used as a middleware) should be set up to work with Zeitwerk in a Rails context. That's a bit of a mouthful, so here's a specific example:
I'm trying to understand how to get Devise Omniauth working with a custom strategy using Zeitwerk. (To be honest there's probably 3 different repos I could create this issue on... feel free to nudge me in a different direction.)
I'm using this guide: https://github.com/heartcombo/devise/wiki/OmniAuth:-Overview. Specifically this bit. A more realistic use case here, is if you have a custom strategy (eg. for an internal service, or a specific customer) that doesn't make sense to publish as an omniauth-
gem.
# app/strategies/my_strategy.rb
module Strategies
class MyStrategy
include OmniAuth::Strategy
end
end
# config/initiailzers/devise.rb
Devise.setup do
config.omniauth :my_service, :strategy_class => Strategies::MyStrategy
end
With this setup, I get this warning when I ran rails zeitwerk:check
:
bin/rails zeitwerk:check
W, [2020-11-24T15:43:34.789269 #72186] WARN -- : DEPRECATION WARNING: Initialization autoloaded the constants Strategies and Strategies::MyStrategy.
Being able to do this is deprecated. Autoloading during initialization is going
to be an error condition in future versions of Rails.
Reloading does not reboot the application, and therefore code executed during
initialization does not run again. So, if you reload Strategies, for example,
the expected changes won't be reflected in that stale Module object.
These autoloaded constants have been unloaded.
Please, check the "Autoloading and Reloading Constants" guide for solutions.
(called from <main> at config/environment.rb:5)
While the app boots and operates fine, it makes me think I've done something very wrong.
https://edgeguides.rubyonrails.org/autoloading_and_reloading_constants.html#autoloading-when-the-application-boots suggests Rails.application.reloader.to_prepare
.
# config/initiailzers/devise.rb
Rails.application.reloader.to_prepare do
Devise.setup do
config.omniauth :my_service, :strategy_class => Strategies::MyStrategy
end
end
This doesn't work, because the Devise omniauth config needs to have been called by the time this line in its Railtie runs. As far as I can tell, to_prepare
is called when app boot completes, and again on every subsequent reload. So with this code, when this line runs, Devise.omniauth_configs
will be empty, so the appropriate omniauth middleware won't be set up.
You can confirm this by adding a link to omniauth_authorize_path(:my_service)
in a view - it will crash as the method isn't defined. omniauth_authorize_path
is defined here, and that module is included here but only if Devise.omniauth_configs
isn't empty.
Annoyingly by the time the app fully boots, Devise.omniauth_configs
is not empty, so if you check its value in a console you may be surprised.
For the record, same thing happens with this approach:
# config/initiailzers/devise.rb
Devise.setup do
Rails.application.reloader.to_prepare do
config.omniauth :my_service, :strategy_class => Strategies::MyStrategy
end
end
It looks like a possible workaround would be to replicate Devise's initializer in an initializer of my own, wrapped with to_prepare
. But I'd rather not do that in this specific case, and in general, I think it would be good for everyone in the community to have general advice on how to structure & use gems to avoid these issues.
So to reiterate / summarise.
- Devise + Omniauth requires you to provide a reference to a class (which gets used as middleware) in an initializer.
- Using the sample code Devise recommends results in Zeitwerk deprecation warnings.
- Using the recommended workaround in the Rails docs breaks in subtle ways.
- Unless I'm missing it, there isn't a documented best practice that works without warnings.
I expect the correct strategy might involve some changes to devise
. That's fine - I can make those, based on your advice.
Finally, thanks for the fantastic gem, and thanks for reading my very long issue!
Interesting use case. Thanks a lot for the excellent issue description.
This is too dependent on things that have to happen at boot time. In some use cases, you can delay and be reload-friendly by asking the user to pass a class name, rather than a class object. Then, the code that needs that class performs a const_get
every time it needs it. In the end, const_get
is what the interpreter does anyway to do a constant lookup, so basically you are being dynamic to play well with reloading.
But in your case, I do not think this would work because the class object needs to end up in the middleware stack, which is not refreshed on reloads.
I believe the best and easiest solution is to have the strategy defined in lib
, perform a require
in the initializer, and configure as the documentation says.
Thanks @fxn. I will test that out today and see if it works, then update devise
docs if it does.
I assume if you require
the strategy file directly, it won't be loaded by Zeitwerk at all, and so if you make changes to it in development you'll need to restart your server? That's not a blocker by any means, just something I will also document.
Exactly, if you change the class, you need to restart.
The middleware stack has objects that respond to a certain API. Changes in middleware need a restart in general.
Oh, forgot to address the question about Zeitwerk.
Correct, since Zeitwerk does not manage lib
(unless the application configures it by hand, in which case you'd need a different place), that file is loaded the same way require "nokogiri"
works, Zeitwerk is not conceptually involved (technically it is because it decorates require
, but if it does not manage the file, it is a transparent decoration, it does nothing).
Okay, great. Thank you for the help!
My pleasure! I have added something about this in the guide thanks to this ticket, will ship with 6.1.
🙌
FWIW, the trick I used for rodauth-rails is to create a wrapper middleware that would dynamically resolve the middleware class I wanted to keep reloadable.
I believe this could be generalized for any middleware class as follows:
class ReloadableMiddleware
def initialize(app, middleware_name, *args)
@app = app
@middleware_name = middleware_name
@args = args
end
def call(env)
middleware = Object.const_get(@middleware_name)
middleware.new(@app, *@args).call(env)
end
end
Rails.application.config.middleware.use ReloadableMiddleware, "MyMiddleware", *my_args
I'm not sure how this can be used in Devise case specifically, as in this case Devise is the one adding strategies to the middleware stack.