From c2ce4a659f4ebc40047446c6b59c4da0cbd8922f Mon Sep 17 00:00:00 2001 From: "M.Shibuya" Date: Sun, 12 Dec 2021 18:38:46 +0900 Subject: [PATCH 1/3] Partially revert "Reload configuration on change in development mode" This partially reverts commit e4ae6698e52e56a1cefdf5c4097b29b8306f21e2, except for reload-related part. --- .../initializers/active_record_extensions.rb | 4 +- lib/rails_admin.rb | 2 +- lib/rails_admin/adapters/mongoid/extension.rb | 4 +- lib/rails_admin/config.rb | 28 +------- lib/rails_admin/config/lazy_model.rb | 70 +++++++++++++++++++ lib/rails_admin/engine.rb | 9 +-- spec/rails_admin/config/lazy_model_spec.rb | 47 +++++++++++++ spec/rails_admin/config_spec.rb | 56 +-------------- 8 files changed, 126 insertions(+), 94 deletions(-) create mode 100644 lib/rails_admin/config/lazy_model.rb create mode 100644 spec/rails_admin/config/lazy_model_spec.rb diff --git a/config/initializers/active_record_extensions.rb b/config/initializers/active_record_extensions.rb index 0e8a6550b2..a2c1a7f3c0 100644 --- a/config/initializers/active_record_extensions.rb +++ b/config/initializers/active_record_extensions.rb @@ -4,9 +4,7 @@ module ActiveRecord class Base def self.rails_admin(&block) - RailsAdmin.config do |config| - config.model(self, &block) - end + RailsAdmin.config(self, &block) end def rails_admin_default_object_label_method diff --git a/lib/rails_admin.rb b/lib/rails_admin.rb index 5a43a0d7b1..cfaacf9f3e 100644 --- a/lib/rails_admin.rb +++ b/lib/rails_admin.rb @@ -29,7 +29,7 @@ def self.config(entity = nil, &block) if entity RailsAdmin::Config.model(entity, &block) elsif block_given? - RailsAdmin::Config.apply(&block) + block.call(RailsAdmin::Config) else RailsAdmin::Config end diff --git a/lib/rails_admin/adapters/mongoid/extension.rb b/lib/rails_admin/adapters/mongoid/extension.rb index e669c693a5..3803525a24 100644 --- a/lib/rails_admin/adapters/mongoid/extension.rb +++ b/lib/rails_admin/adapters/mongoid/extension.rb @@ -11,9 +11,7 @@ module Extension self.nested_attributes_options = {} class << self def rails_admin(&block) - RailsAdmin.config do |config| - config.model(self, &block) - end + RailsAdmin.config(self, &block) end alias_method :accepts_nested_attributes_for_without_rails_admin, :accepts_nested_attributes_for diff --git a/lib/rails_admin/config.rb b/lib/rails_admin/config.rb index 8bed81fbc6..706dc64e38 100644 --- a/lib/rails_admin/config.rb +++ b/lib/rails_admin/config.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'rails_admin/config/model' +require 'rails_admin/config/lazy_model' require 'rails_admin/config/sections/list' require 'active_support/core_ext/module/attribute_accessors' @@ -22,10 +22,6 @@ module Config DEFAULT_CURRENT_USER = proc {} - # Variables to track initialization process - @initialized = false - @deferred_blocks = [] - class << self # Application title, can be an array of two elements attr_accessor :main_app_name @@ -88,22 +84,6 @@ class << self # Set where RailsAdmin fetches JS/CSS from, defaults to :sprockets attr_writer :asset_source - # Finish initialization by executing deferred configuration blocks - def initialize! - @deferred_blocks.each { |block| block.call(self) } - @deferred_blocks.clear - @initialized = true - end - - # Evaluate the given block either immediately or lazily, based on initialization status. - def apply(&block) - if @initialized - yield(self) - else - @deferred_blocks << block - end - end - # Setup authentication to be run as a before filter # This is run inside the controller instance so you can setup any authentication you need to # @@ -257,8 +237,8 @@ def model(entity, &block) entity.class.name.to_sym end - @registry[key] ||= RailsAdmin::Config::Model.new(entity) - @registry[key].instance_eval(&block) if block && @registry[key].abstract_model + @registry[key] ||= RailsAdmin::Config::LazyModel.new(entity) + @registry[key].add_deferred_block(&block) if block @registry[key] end @@ -360,10 +340,8 @@ def reset_model(model) # Perform reset, then load RailsAdmin initializer again def reload! - @initialized = false reset load RailsAdmin::Engine.config.initializer_path - initialize! end # Get all models that are configured as visible sorted by their weight and label. diff --git a/lib/rails_admin/config/lazy_model.rb b/lib/rails_admin/config/lazy_model.rb new file mode 100644 index 0000000000..be18c9d9c1 --- /dev/null +++ b/lib/rails_admin/config/lazy_model.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require 'rails_admin/config/model' + +module RailsAdmin + module Config + class LazyModel < BasicObject + def initialize(entity, &block) + @entity = entity + @deferred_blocks = [*block] + @existing_blocks = [] + end + + def add_deferred_block(&block) + @deferred_blocks << block + end + + def target + @model ||= ::RailsAdmin::Config::Model.new(@entity) + # When evaluating multiple configuration blocks, the order of + # execution is important. As one would expect (in my opinion), + # options defined within a resource should take precedence over + # more general options defined in an initializer. This way, + # general settings for a number of resources could be specified + # in the initializer, while models could override these settings + # later, if required. + # + # CAVEAT: It cannot be guaranteed that blocks defined in an initializer + # will be loaded (and adde to @deferred_blocks) first. For instance, if + # the initializer references a model class before defining + # a RailsAdmin configuration block, the configuration from the + # resource will get added to @deferred_blocks first: + # + # # app/models/some_model.rb + # class SomeModel + # rails_admin do + # : + # end + # end + # + # # config/initializers/rails_admin.rb + # model = 'SomeModel'.constantize # blocks from SomeModel get loaded + # model.config model do # blocks from initializer gets loaded + # : + # end + # + # Thus, sort all blocks to excute for a resource by Proc.source_path, + # to guarantee that blocks from 'config/initializers' evaluate before + # blocks defined within a model class. + unless @deferred_blocks.empty? + @existing_blocks += @deferred_blocks + @existing_blocks. + partition { |block| block.source_location.first =~ %r{config/initializers} }. + flatten. + each { |block| @model.instance_eval(&block) } + @deferred_blocks = [] + end + @model + end + + def method_missing(method_name, *args, &block) + target.send(method_name, *args, &block) + end + + def respond_to_missing?(method_name, include_private = false) + super || target.respond_to?(method_name, include_private) + end + end + end +end diff --git a/lib/rails_admin/engine.rb b/lib/rails_admin/engine.rb index 54cdb03cbf..036a95e7b8 100644 --- a/lib/rails_admin/engine.rb +++ b/lib/rails_admin/engine.rb @@ -35,14 +35,7 @@ class Engine < Rails::Engine end end - initializer 'RailsAdmin apply configuration', after: :eager_load! do |app| - RailsAdmin::Config.initialize! - - # Force route reload, since it doesn't reflect RailsAdmin action configuration yet - app.reload_routes! - end - - initializer 'RailsAdmin precompile hook', group: :all, after: 'RailsAdmin apply configuration' do |app| + initializer 'RailsAdmin precompile hook', group: :all do |app| case RailsAdmin.config.asset_source when :sprockets if defined?(Sprockets) diff --git a/spec/rails_admin/config/lazy_model_spec.rb b/spec/rails_admin/config/lazy_model_spec.rb new file mode 100644 index 0000000000..b8976b851f --- /dev/null +++ b/spec/rails_admin/config/lazy_model_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe RailsAdmin::Config::LazyModel do + subject { RailsAdmin::Config::LazyModel.new(:Team, &block) } + let(:block) { proc { register_instance_option('parameter') } } # an arbitrary instance method we can spy on + + describe '#store' do + it "doesn't evaluate the block immediately" do + expect_any_instance_of(RailsAdmin::Config::Model).not_to receive(:register_instance_option) + subject + end + + it 'evaluates block when reading' do + expect_any_instance_of(RailsAdmin::Config::Model).to receive(:register_instance_option).with('parameter') + subject.groups # an arbitrary instance method on RailsAdmin::Config::Model to wake up lazy_model + end + + it 'evaluates config block only once' do + expect_any_instance_of(RailsAdmin::Config::Model).to receive(:register_instance_option).once.with('parameter') + + subject.groups + subject.groups + end + end + + context 'when a method is defined in Kernel' do + before do + Kernel.module_eval do + def weight + 42 + end + end + end + + after do + Kernel.module_eval do + undef weight + end + end + + it 'proxies calls for the method to @object' do + expect(subject.weight).to eq 0 + end + end +end diff --git a/spec/rails_admin/config_spec.rb b/spec/rails_admin/config_spec.rb index 70a060cc8f..504f4fbd31 100644 --- a/spec/rails_admin/config_spec.rb +++ b/spec/rails_admin/config_spec.rb @@ -377,54 +377,6 @@ class TestController < ActionController::Base; end end end - describe '.apply' do - subject { RailsAdmin::Config.apply(&block) } - let(:block) do - proc do |config| - config.model Team do - register_instance_option('parameter') # an arbitrary instance method we can spy on - end - end - end - before { RailsAdmin::Config.instance_variable_set(:@initialized, false) } - after do - RailsAdmin::Config.instance_variable_set(:@initialized, true) - RailsAdmin::Config.instance_variable_set(:@deferred_blocks, []) - end - - it "doesn't evaluate the block immediately" do - expect_any_instance_of(RailsAdmin::Config::Model).not_to receive(:register_instance_option) - subject - end - - it 'evaluates block when initialize! is finished' do - expect_any_instance_of(RailsAdmin::Config::Model).to receive(:register_instance_option).with('parameter') - subject - RailsAdmin::Config.initialize! - end - - it 'evaluates config block only once' do - expect_any_instance_of(RailsAdmin::Config::Model).to receive(:register_instance_option).once.with('parameter') - subject - RailsAdmin::Config.initialize! - RailsAdmin::Config.initialize! - end - - context 'with a non-existent class' do - let(:block) do - proc do |config| - config.model UnknownClass do - field :name - end - end - end - - it "doesn't break immediately" do - subject - end - end - end - describe '.reload!' do before do RailsAdmin.config Player do @@ -446,12 +398,8 @@ class TestController < ActionController::Base; end end it "applies the initializer's configuration first, then models' configurations" do - # simulate the situation that Team model is loaded in the middle of processing RailsAdmin initializer - allow_any_instance_of(RailsAdmin::Config::Model).to receive(:include_all_fields).and_wrap_original do |method| - Team.rails_admin do - field :color, :integer - end - method.call + Team.rails_admin do + field :color, :integer end RailsAdmin::Config.reload! From 2545e6df5d3baa2503ddfdd5774ed193e50eace3 Mon Sep 17 00:00:00 2001 From: Mitsuhiro Shibuya Date: Sat, 2 Jul 2022 20:02:04 +0900 Subject: [PATCH 2/3] Introduce ConstLoadSuppressor to allow use of class constants without loading the actual classes --- lib/rails_admin.rb | 5 +- lib/rails_admin/config.rb | 17 ++++--- .../config/const_load_suppressor.rb | 50 +++++++++++++++++++ lib/rails_admin/config/lazy_model.rb | 12 +++-- lib/rails_admin/config/model.rb | 4 +- lib/rails_admin/engine.rb | 16 +++--- spec/dummy_app/app/active_record/team.rb | 4 ++ .../config/initializers/rails_admin.rb | 4 +- spec/rails_admin/config/lazy_model_spec.rb | 17 ++++++- spec/rails_admin/config_spec.rb | 15 ++---- 10 files changed, 107 insertions(+), 37 deletions(-) create mode 100644 lib/rails_admin/config/const_load_suppressor.rb diff --git a/lib/rails_admin.rb b/lib/rails_admin.rb index cfaacf9f3e..41fd01419d 100644 --- a/lib/rails_admin.rb +++ b/lib/rails_admin.rb @@ -3,6 +3,7 @@ require 'rails_admin/engine' require 'rails_admin/abstract_model' require 'rails_admin/config' +require 'rails_admin/config/const_load_suppressor' require 'rails_admin/extension' require 'rails_admin/extensions/cancancan' require 'rails_admin/extensions/pundit' @@ -12,6 +13,8 @@ require 'yaml' module RailsAdmin + extend RailsAdmin::Config::ConstLoadSuppressor + # Setup RailsAdmin # # Given the first argument is a model class, a model class name @@ -29,7 +32,7 @@ def self.config(entity = nil, &block) if entity RailsAdmin::Config.model(entity, &block) elsif block_given? - block.call(RailsAdmin::Config) + suppress_const_load { yield(RailsAdmin::Config) } else RailsAdmin::Config end diff --git a/lib/rails_admin/config.rb b/lib/rails_admin/config.rb index 706dc64e38..b2540566a1 100644 --- a/lib/rails_admin/config.rb +++ b/lib/rails_admin/config.rb @@ -229,7 +229,7 @@ def model(entity, &block) case entity when RailsAdmin::AbstractModel entity.model.try(:name).try :to_sym - when Class + when Class, ConstLoadSuppressor::ConstProxy entity.name.to_sym when String, Symbol entity.to_sym @@ -237,7 +237,7 @@ def model(entity, &block) entity.class.name.to_sym end - @registry[key] ||= RailsAdmin::Config::LazyModel.new(entity) + @registry[key] ||= RailsAdmin::Config::LazyModel.new(key.to_s) @registry[key].add_deferred_block(&block) if block @registry[key] end @@ -245,11 +245,14 @@ def model(entity, &block) def asset_source @asset_source ||= begin - warn <<~MSG - [Warning] After upgrading RailsAdmin to 3.x you haven't set asset_source yet, using :sprockets as the default. - To suppress this message, run 'rails rails_admin:install' to setup the asset delivery method suitable to you. - MSG - :sprockets + detected = defined?(Sprockets) ? :sprockets : :invalid + unless ARGV.join(' ').include? 'rails_admin:install' + warn <<~MSG + [Warning] After upgrading RailsAdmin to 3.x you haven't set asset_source yet, using :#{detected} as the default. + To suppress this message, run 'rails rails_admin:install' to setup the asset delivery method suitable to you. + MSG + end + detected end end diff --git a/lib/rails_admin/config/const_load_suppressor.rb b/lib/rails_admin/config/const_load_suppressor.rb new file mode 100644 index 0000000000..53cb0170d7 --- /dev/null +++ b/lib/rails_admin/config/const_load_suppressor.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module RailsAdmin + module Config + module ConstLoadSuppressor + def suppress_const_load + original = Object.method(:const_missing) + Object.define_singleton_method(:const_missing) do |name| + ConstProxy.new(name.to_s) + end + yield + ensure + Object.define_singleton_method(:const_missing, original) + end + + class ConstProxy < BasicObject + attr_reader :name + + def initialize(name) + @name = name + end + + def klass + @klass ||= + begin + unless ::Object.const_defined?(name) + ::Kernel.raise <<~MESSAGE + The constant #{name} is not loaded yet upon the execution of the RailsAdmin initializer. + We don't recommend to do this and may lead to issues, but if you really have to do so you can explicitly require it by adding: + + require '#{name.underscore}' + + on top of config/initializers/rails_admin.rb. + MESSAGE + end + name.constantize + end + end + + def method_missing(method_name, *args, &block) + klass.send(method_name, *args, &block) + end + + def respond_to_missing?(method_name, include_private = false) + super || klass.respond_to?(method_name, include_private) + end + end + end + end +end diff --git a/lib/rails_admin/config/lazy_model.rb b/lib/rails_admin/config/lazy_model.rb index be18c9d9c1..c96bb4083e 100644 --- a/lib/rails_admin/config/lazy_model.rb +++ b/lib/rails_admin/config/lazy_model.rb @@ -8,11 +8,15 @@ class LazyModel < BasicObject def initialize(entity, &block) @entity = entity @deferred_blocks = [*block] - @existing_blocks = [] + @initialized = false end def add_deferred_block(&block) - @deferred_blocks << block + if @initialized + @model.instance_eval(&block) + else + @deferred_blocks << block + end end def target @@ -48,13 +52,13 @@ def target # to guarantee that blocks from 'config/initializers' evaluate before # blocks defined within a model class. unless @deferred_blocks.empty? - @existing_blocks += @deferred_blocks - @existing_blocks. + @deferred_blocks. partition { |block| block.source_location.first =~ %r{config/initializers} }. flatten. each { |block| @model.instance_eval(&block) } @deferred_blocks = [] end + @initialized = true @model end diff --git a/lib/rails_admin/config/model.rb b/lib/rails_admin/config/model.rb index 16a041a8fd..68688ec747 100644 --- a/lib/rails_admin/config/model.rb +++ b/lib/rails_admin/config/model.rb @@ -36,8 +36,10 @@ def initialize(entity) case entity when RailsAdmin::AbstractModel entity - when Class, String, Symbol + when Class, String RailsAdmin::AbstractModel.new(entity) + when Symbol + RailsAdmin::AbstractModel.new(entity.to_s) else RailsAdmin::AbstractModel.new(entity.class) end diff --git a/lib/rails_admin/engine.rb b/lib/rails_admin/engine.rb index 036a95e7b8..1532c8bcf8 100644 --- a/lib/rails_admin/engine.rb +++ b/lib/rails_admin/engine.rb @@ -38,15 +38,13 @@ class Engine < Rails::Engine initializer 'RailsAdmin precompile hook', group: :all do |app| case RailsAdmin.config.asset_source when :sprockets - if defined?(Sprockets) - app.config.assets.precompile += %w[ - rails_admin/application.js - rails_admin/application.css - ] - app.config.assets.paths << RailsAdmin::Engine.root.join('src') - require 'rails_admin/support/esmodule_preprocessor' - Sprockets.register_preprocessor 'application/javascript', RailsAdmin::ESModulePreprocessor - end + app.config.assets.precompile += %w[ + rails_admin/application.js + rails_admin/application.css + ] + app.config.assets.paths << RailsAdmin::Engine.root.join('src') + require 'rails_admin/support/esmodule_preprocessor' + Sprockets.register_preprocessor 'application/javascript', RailsAdmin::ESModulePreprocessor when :importmap self.importmap = Importmap::Map.new.draw(app.root.join('config/importmap.rails_admin.rb')) end diff --git a/spec/dummy_app/app/active_record/team.rb b/spec/dummy_app/app/active_record/team.rb index 800d5d55bb..b8d183049b 100644 --- a/spec/dummy_app/app/active_record/team.rb +++ b/spec/dummy_app/app/active_record/team.rb @@ -28,4 +28,8 @@ def color_enum scope :green, -> { where(color: 'red') } scope :red, -> { where(color: 'red') } scope :white, -> { where(color: 'white') } + + rails_admin do + field :color, :color + end end diff --git a/spec/dummy_app/config/initializers/rails_admin.rb b/spec/dummy_app/config/initializers/rails_admin.rb index 6f3d9b9ade..b8f793d0e3 100644 --- a/spec/dummy_app/config/initializers/rails_admin.rb +++ b/spec/dummy_app/config/initializers/rails_admin.rb @@ -2,8 +2,8 @@ RailsAdmin.config do |c| c.asset_source = CI_ASSET - c.model 'Team' do + c.model Team do include_all_fields - field :color, :color + field :color, :hidden end end diff --git a/spec/rails_admin/config/lazy_model_spec.rb b/spec/rails_admin/config/lazy_model_spec.rb index b8976b851f..1a2d23a0aa 100644 --- a/spec/rails_admin/config/lazy_model_spec.rb +++ b/spec/rails_admin/config/lazy_model_spec.rb @@ -6,7 +6,7 @@ subject { RailsAdmin::Config::LazyModel.new(:Team, &block) } let(:block) { proc { register_instance_option('parameter') } } # an arbitrary instance method we can spy on - describe '#store' do + describe '#initialize' do it "doesn't evaluate the block immediately" do expect_any_instance_of(RailsAdmin::Config::Model).not_to receive(:register_instance_option) subject @@ -25,6 +25,21 @@ end end + describe '#add_deferred_block' do + let(:another_block) { proc { register_instance_option('parameter2') } } + + it "doesn't evaluate the block immediately" do + expect_any_instance_of(RailsAdmin::Config::Model).not_to receive(:register_instance_option).with('parameter2') + subject.add_deferred_block(&another_block) + end + + it 'evaluates the block immediately after initialization' do + subject.target + expect_any_instance_of(RailsAdmin::Config::Model).to receive(:register_instance_option).with('parameter2') + subject.add_deferred_block(&another_block) + end + end + context 'when a method is defined in Kernel' do before do Kernel.module_eval do diff --git a/spec/rails_admin/config_spec.rb b/spec/rails_admin/config_spec.rb index 504f4fbd31..cabae96792 100644 --- a/spec/rails_admin/config_spec.rb +++ b/spec/rails_admin/config_spec.rb @@ -383,7 +383,7 @@ class TestController < ActionController::Base; end field :name end RailsAdmin.config Team do - field :color, :hidden + field :color, :integer end end @@ -392,18 +392,9 @@ class TestController < ActionController::Base; end expect(RailsAdmin::Config.model(Player).fields.map(&:name)).to include :number end - it 're-applies the configuration from the initializer' do - RailsAdmin::Config.reload! - expect(RailsAdmin::Config.model(Team).fields.find { |f| f.name == :color }.type).to eq :color - end - - it "applies the initializer's configuration first, then models' configurations" do - Team.rails_admin do - field :color, :integer - end - + it 'reloads the configuration from the initializer' do RailsAdmin::Config.reload! - expect(RailsAdmin::Config.model(Team).fields.find { |f| f.name == :color }.type).to eq :integer + expect(RailsAdmin::Config.model(Team).fields.find { |f| f.name == :color }.type).to eq :hidden end end end From 27002eb3e2d4e24e14f743e90f755a5541bd1e83 Mon Sep 17 00:00:00 2001 From: Mitsuhiro Shibuya Date: Sun, 3 Jul 2022 23:05:50 +0900 Subject: [PATCH 3/3] Fix Rails 6.x builds failing with FrozenError --- .../app/{active_record => }/eager_loaded/basketball.rb | 0 spec/dummy_app/app/mongoid/eager_loaded/basketball.rb | 4 ---- spec/dummy_app/config/application.rb | 2 +- 3 files changed, 1 insertion(+), 5 deletions(-) rename spec/dummy_app/app/{active_record => }/eager_loaded/basketball.rb (100%) delete mode 100644 spec/dummy_app/app/mongoid/eager_loaded/basketball.rb diff --git a/spec/dummy_app/app/active_record/eager_loaded/basketball.rb b/spec/dummy_app/app/eager_loaded/basketball.rb similarity index 100% rename from spec/dummy_app/app/active_record/eager_loaded/basketball.rb rename to spec/dummy_app/app/eager_loaded/basketball.rb diff --git a/spec/dummy_app/app/mongoid/eager_loaded/basketball.rb b/spec/dummy_app/app/mongoid/eager_loaded/basketball.rb deleted file mode 100644 index 28dbe974d4..0000000000 --- a/spec/dummy_app/app/mongoid/eager_loaded/basketball.rb +++ /dev/null @@ -1,4 +0,0 @@ -# frozen_string_literal: true - -class Basketball < Ball -end diff --git a/spec/dummy_app/config/application.rb b/spec/dummy_app/config/application.rb index 34129fd94e..795a4f707a 100644 --- a/spec/dummy_app/config/application.rb +++ b/spec/dummy_app/config/application.rb @@ -36,7 +36,7 @@ class Application < Rails::Application # -- all .rb files in that directory are automatically loaded. config.load_defaults Rails.version[0, 3] config.eager_load_paths.reject! { |p| p =~ %r{/app/([^/]+)} && !%W[controllers jobs locales mailers #{CI_ORM}].include?(Regexp.last_match[1]) } - config.eager_load_paths += %W[#{config.root}/app/#{CI_ORM}/eager_loaded] + config.eager_load_paths += %W[#{config.root}/app/eager_loaded] config.autoload_paths += %W[#{config.root}/lib] config.i18n.load_path += Dir[Rails.root.join('app', 'locales', '*.{rb,yml}').to_s] config.active_record.time_zone_aware_types = %i[datetime time] if CI_ORM == :active_record