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

Advice needed on making Devise-Omniauth work with Zeitwerk #143

Closed
ghiculescu opened this issue Nov 25, 2020 · 9 comments
Closed

Advice needed on making Devise-Omniauth work with Zeitwerk #143

ghiculescu opened this issue Nov 25, 2020 · 9 comments

Comments

@ghiculescu
Copy link

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!

@ghiculescu ghiculescu changed the title Advice needed on making Devise omniauth zeitwerk-compatible Advice needed on making Devise-Omniauth work with Zeitwerk Nov 25, 2020
@fxn
Copy link
Owner

fxn commented Nov 25, 2020

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.

@ghiculescu
Copy link
Author

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.

@fxn
Copy link
Owner

fxn commented Nov 25, 2020

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.

@fxn
Copy link
Owner

fxn commented Nov 25, 2020

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

@ghiculescu
Copy link
Author

Okay, great. Thank you for the help!

@fxn
Copy link
Owner

fxn commented Nov 25, 2020

My pleasure! I have added something about this in the guide thanks to this ticket, will ship with 6.1.

@ghiculescu
Copy link
Author

@fxn
Copy link
Owner

fxn commented Nov 25, 2020

🙌

@janko
Copy link

janko commented Dec 1, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants