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

Rails Admin loads every single file in autoload paths! #2770

Closed
glebm opened this issue Nov 15, 2016 · 0 comments
Closed

Rails Admin loads every single file in autoload paths! #2770

glebm opened this issue Nov 15, 2016 · 0 comments

Comments

@glebm
Copy link
Contributor

glebm commented Nov 15, 2016

Every single file in autoload_paths is eagerly loaded by Rails admin. Below, I'll explain how this happens, why this shouldn't happen, and will propose a solution.

I came across this when trying to figure a bizarre issue in Thredded + rails_admin compatibility: thredded/thredded#280.

How does this happen?

Here is what RailsAdmin tries to load, lib/rails_admin/config.rb#L322:

def viable_models
  included_models.collect(&:to_s).presence || begin
    @@system_models ||= # memoization for tests
        ([Rails.application] + Rails::Engine.subclasses.collect(&:instance)).flat_map do |app|
          (app.paths['app/models'].to_a + app.config.autoload_paths).collect do |load_path|
            Dir.glob(app.root.join(load_path)).collect do |load_dir|
              Dir.glob(load_dir + '/**/*.rb').collect do |filename|
                # app/models/module/class.rb => module/class.rb => module/class => Module::Class
                lchomp(filename, "#{app.root.join(load_dir)}/").chomp('.rb').camelize
              end
            end
          end
        end.flatten.reject { |m| m.starts_with?('Concerns::') } # rubocop:disable MultilineBlockChain
  end
end

It loads these by calling constantize in lib/rails_admin/abstract_model.rb#L13-L25:

def all(adapter = nil)
  @@all ||= Config.models_pool.collect { |m| new(m) }.compact
  adapter ? @@all.select { |m| m.adapter == adapter } : @@all
end

alias_method :old_new, :new
def new(m)
  m = m.constantize unless m.is_a?(Class)
  (am = old_new(m)).model && am.adapter ? am : nil
rescue LoadError, NameError
  puts "[RailsAdmin] Could not load model #{m}, assuming model is non existing. (#{$ERROR_INFO})" unless Rails.env.test?
  nil
end

Why this shouldn't happen?

If something is in autoload_paths, it doesn't mean it can or should always be loaded. Rails explicitly makes this distinction with eager_load.

E.g. if I'm developing a Rails engine, I might want to add lib to autoload paths (to ease the development of the engine), but that doesn't mean I want to eagerly load ruby files in lib/generators/templates!

Proposed solution

There are a few options here:

  1. Remove app.config.autoload_paths from the files RailsAdmin tries to autoload. Only load things from app/models directories.
  2. Replace app.config.autoload_paths with app.paths.eager_load.
  3. Stop messing with the loading system! Just include a module into the base class!

3 is ideal but is also the most effort and I don't have the time. 1 breaks backwards compatibility.
So I'll send a PR for 2 for now.
Perhaps one day somebody can do 3.

@mshibuya @bbenezech @sferik

glebm added a commit to glebm/rails_admin that referenced this issue Nov 15, 2016
If something is in `autoload_paths`, it doesn't mean it can or should *always* be loaded. Rails explicitly makes this distinction with `eager_load`.

E.g. if I'm developing a Rails engine, I might want to add `lib` to autoload paths (to ease the development of the engine), but that doesn't mean I want to eagerly load ruby files in `lib/generators/templates`!

Resolves railsadminteam#2770
glebm added a commit to glebm/rails_admin that referenced this issue Mar 18, 2017
If something is in `autoload_paths`, it doesn't mean it can or should *always* be loaded. Rails explicitly makes this distinction with `eager_load`.

E.g. if I'm developing a Rails engine, I might want to add `lib` to autoload paths (to ease the development of the engine), but that doesn't mean I want to eagerly load ruby files in `lib/generators/templates`!

Resolves railsadminteam#2770
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

1 participant