palkan / anyway_config

Configuration library for Ruby gems and applications
MIT License
778 stars 52 forks source link

Upgrade to v2 messes up Forest Admin schema #66

Closed YoranBrondsema closed 4 years ago

YoranBrondsema commented 4 years ago

What did you do?

I use both isolator (which depends on sniffer which depends on anyway_config) and forest-rails in my app. Forest Admin is a service that provides an admin panel.

I upgraded anyway_config to 2.0.6 and it messed up the .forestadmin-schema.json file. This file is generated by Forest Admin upon starting the Rails server and it contains a description of the database schema based on the ActiveRecord models in the app.

What did you expect to happen?

The .forestadmin-schema.json file to be untouched by the upgrade of the anyway_config gem, as they are unrelated. anyway_config does not appear in the dependency tree of forest-rails.

What actually happened?

After the anyway_config upgrade to 2.0.6, all ActiveRecord models disappeared from the .forestadmin-schema.json file. It's like forest-rails couldn't find any ActiveRecord models anymore.

Additional context

I'm using forest-rails 5.2.1.

Environment

Ruby Version: ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c)

Framework Version (Rails, whatever): Rails 6.0.3.2

Anyway Config Version: I upgraded from 1.4.4 to 2.0.6

palkan commented 4 years ago

The .forestadmin-schema.json file to be untouched by the upgrade of the anyway_config gem, as they are unrelated.

They are. So, you ran bundle update and .forestadmin-schema.json changed? Or it's getting updated during the application boot?

Does downgrading to 1.4.4 solves the problem?

YoranBrondsema commented 4 years ago

Thanks for your reply, and sorry for not being more complete in the initial bug report.

So, you ran bundle update and .forestadmin-schema.json changed? Or it's getting updated during the application boot?

.forestadmin-schema.json does NOT change after bundle update. It does change during application boot.

Does downgrading to 1.4.4 solves the problem?

Correct. I fixed anyway_config to ~> 1 in my Gemfile in the meantime.

palkan commented 4 years ago

Thanks for clarification! It doesn't seem to be easy to reproduce. Maybe, you know where to find a demo/example app for Forest?

So far, I found that .json is loaded here: https://github.com/ForestAdmin/forest-rails/blob/a36791da9aa0321a20614a851cbf8c0f676402ba/lib/forest_liana/bootstrapper.rb#L26

So, my guess is that ForestLiana.env_secret is missing at this point. Could you check please? (e.g., by running rails c and checking the value).

YoranBrondsema commented 4 years ago

Thanks for diving into it! I'll try and set up a minimum Rails app that reproduces the error. This will make it easier to debug I think. Let me set it up and get back to you (might be in the next few days).

YoranBrondsema commented 4 years ago

I haven't managed to reproduce it in a repo but I got some info.

The bug starts happening from 2.0.0.rc1. So 2.0.0.pre2 is the last version for which there is no issue.

Furthermore, I think it has to do with Zeitwerk autoloading. I use Rails 6 but with 5.2 defaults meaning that Zeitwerk is not enabled, even though it is a dependency in Rails 6.

# rails console with 2.0.0.pre2

> Zeitwerk
NameError (uninitialized constant Zeitwerk)
# rails console with 2.0.0.rc1

> Zeitwerk
=> Zeitwerk

So what I think is that 2.0.0.rc1 and later forces the loading of Zeitwerk (maybe in https://github.com/palkan/anyway_config/blob/v2.0.0.rc1/lib/anyway/rails/settings.rb?). Then in Forest there's the section in https://github.com/ForestAdmin/forest-rails/blob/v5.2.2/lib/forest_liana/engine.rb#L60:

# ForestAdmin/forest-rails/blob/v5.2.2/lib/forest_liana/engine.rb

if Rails::VERSION::MAJOR > 5 && defined?(Zeitwerk::Loader)
  Zeitwerk::Loader.eager_load_all
else
  app.eager_load!
end

When Zeitwerk does not exist (which is the case for 2.0.0.pre2), it uses app.eager_load!. However, since 2.0.0.rc1 loads Zeitwerk, it uses Zeitwerk::Loader.eager_load_all, which does not load all the models in the application and thereby writes an incomplete .forestadmin-schema.json.

Does this make sense?

I admit that our app is partially at fault as well. You'd expect Rails 6 apps to all use Zeitwerk. We've done the Rails 6 upgrade but not the migration to Zeitwerk yet. I think you detect the presence of Zeitwerk with:

# lib/anyway/rails/settings.rb

begin
  require "zeitwerk"
  require "active_support/dependencies/zeitwerk_integration"
rescue LoadError
end

which passes in our case, because Rails 6 has zeitwerk as a dependency. But our app doesn't actually use Zeitwerk. So maybe there's another way of detecting whether Zeitwerk is being used in the app?

Of course, ideally we'd migrate to Zeitwerk as well. We'll do that soon but in the mean time maybe we can find a fix in anyway_config to detect cases like ours!

Curious to hear what you think.

palkan commented 4 years ago

Does this make sense?

Yeah! Awesome investigation. Thanks!

Curious to hear what you think.

I think, the problem should be fixed at the Forest side. The follow check is incorrect:

if Rails::VERSION::MAJOR > 5 && defined?(Zeitwerk::Loader)

Zeitwerk::Loader could be used by other gems (I believe, more gems will adopt it in the future), it's not a Rails-only library.

I'm not sure what was the purpose of adding this check, but looks like we can replace it with the following:

# Eager load all the code managed by zeitwerk (within Rails and outside of it)
Zeitwerk::Loader.eager_load_all if defined?(Zeitwerk::Loader)
# Eager loads Rails app (independently of the autoload mode)
app.eager_load!

That would work the same way as config.eager_load = true. See https://github.com/rails/rails/blob/838d3f73ddd4b333295b7cd942505ce775d03ee0/railties/lib/rails/application/finisher.rb#L127-L136

YoranBrondsema commented 4 years ago

Cool, thanks for your comments! I'll see with the people over at Forest. I'll close this in the mean time.