From ec03eb6fba7db37c1c74963a6ec4b37098146e1c Mon Sep 17 00:00:00 2001 From: Andy Clayton Date: Thu, 10 Mar 2022 09:50:32 -0600 Subject: [PATCH 1/2] restore 2.x config loading behavior Attempt to restore 2.x config loading behavior where 1) per-model config blocks only evaluate after all the models are loaded and associations are defined while 2) initializer config block evaluates immediately so that routes are configured correctly. One remaining change compared to 2.x is RailsAdmin::Config#model no longer returns the model when given a block. With the removal of LazyModel in e4ae669 it is tricky to both defer initialization and return the model class. Fixes https://github.com/railsadminteam/rails_admin/issues/3490 --- .rubocop.yml | 2 +- lib/rails_admin.rb | 4 ++- lib/rails_admin/adapters/mongoid/extension.rb | 4 +-- lib/rails_admin/config.rb | 32 +++++++++++++++---- lib/rails_admin/engine.rb | 5 --- spec/rails_admin/config_spec.rb | 24 ++++++++++++++ 6 files changed, 55 insertions(+), 16 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index f9e7add9f9..812b176a02 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -118,7 +118,7 @@ Metrics/MethodLength: Max: 29 # TODO: Lower to 15 Metrics/ModuleLength: - Max: 217 # TODO: Lower to 100 + Max: 228 # TODO: Lower to 100 Metrics/ParameterLists: Max: 8 # TODO: Lower to 4 diff --git a/lib/rails_admin.rb b/lib/rails_admin.rb index 7f70d4d507..fee9e267d6 100644 --- a/lib/rails_admin.rb +++ b/lib/rails_admin.rb @@ -29,7 +29,9 @@ def self.config(entity = nil, &block) if entity RailsAdmin::Config.model(entity, &block) elsif block_given? - RailsAdmin::Config.apply(&block) + # Immediately evaluate non-model (initializer) config. It's needed to + # properly configure routes when there are custom actions. + yield 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 531b6701b4..48c0426403 100644 --- a/lib/rails_admin/config.rb +++ b/lib/rails_admin/config.rb @@ -24,6 +24,7 @@ module Config # Variables to track initialization process @initialized = false + @initializing = false @deferred_blocks = [] class << self @@ -87,6 +88,9 @@ class << self # Finish initialization by executing deferred configuration blocks def initialize! + return if @initializing + + @initializing = true @deferred_blocks.each { |block| block.call(self) } @deferred_blocks.clear @initialized = true @@ -226,6 +230,7 @@ def default_search_operator=(operator) # pool of all found model names from the whole application def models_pool + initialize! (viable_models - excluded_models.collect(&:to_s)).uniq.sort end @@ -238,7 +243,7 @@ def models_pool # # If a block is given it is evaluated in the context of configuration instance. # - # Returns given model's configuration + # Otherwise returns given model's configuration. # # @see RailsAdmin::Config.registry def model(entity, &block) @@ -254,9 +259,24 @@ 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] + # When a block is provided defer evaluation until initialize so that: + # + # 1) performing configuration in the initializer does not attempt to + # autoload model classes, and + # 2) model config blocks run after models are fully loaded and + # associations are defined. + # + # Do not defer when called without a block as the model must be returned. + if block + RailsAdmin::Config.apply do + m = model(entity) + m.instance_eval(&block) if @registry[key].abstract_model + end + nil + else + initialize! + @registry[key] ||= RailsAdmin::Config::Model.new(entity) + end end def asset_source @@ -318,6 +338,8 @@ def models # # @see RailsAdmin::Config.registry def reset + @initialized = @initializing = false + @deferred_blocks.clear @compact_show_view = true @browser_validations = true @authenticate = nil @@ -356,10 +378,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/engine.rb b/lib/rails_admin/engine.rb index 788071eff6..b462fb557d 100644 --- a/lib/rails_admin/engine.rb +++ b/lib/rails_admin/engine.rb @@ -68,11 +68,6 @@ class Engine < Rails::Engine ERROR end - RailsAdmin::Config.initialize! - - # Force route reload, since it doesn't reflect RailsAdmin action configuration yet - app.reload_routes! - RailsAdmin::Version.warn_with_js_version end end diff --git a/spec/rails_admin/config_spec.rb b/spec/rails_admin/config_spec.rb index 521515960c..bf1f27dd04 100644 --- a/spec/rails_admin/config_spec.rb +++ b/spec/rails_admin/config_spec.rb @@ -327,6 +327,24 @@ class TestController < ActionController::Base; end end end + context 'when a block is not provided' do + let(:model) { described_class.model(Team) } + it 'returns the model config' do + expect(model).to be_a(RailsAdmin::Config::Model) + end + end + + context 'when a block is provided' do + let(:model) do + described_class.model(Team) do + field :fans + end + end + it 'does not return the model config' do + expect(model).to be_nil + end + end + context 'when model expanded' do before do described_class.model(Team) do @@ -387,6 +405,7 @@ class TestController < ActionController::Base; end end end before { RailsAdmin::Config.instance_variable_set(:@initialized, false) } + before { RailsAdmin::Config.instance_variable_set(:@initializing, false) } after do RailsAdmin::Config.instance_variable_set(:@initialized, true) RailsAdmin::Config.instance_variable_set(:@deferred_blocks, []) @@ -445,6 +464,11 @@ class TestController < ActionController::Base; end expect(RailsAdmin::Config.model(Team).fields.find { |f| f.name == :color }.type).to eq :color end + it 'does not immediately apply model configuration' do + expect(RailsAdmin::Config).not_to receive(:initialize!) + RailsAdmin::Config.reload! + 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| From e2ec3b9f3ecacae217709a5c26d50663641715ad Mon Sep 17 00:00:00 2001 From: Andy Clayton Date: Mon, 14 Mar 2022 14:18:52 -0500 Subject: [PATCH 2/2] maintain 3.x deferred initializer block config In 3.x the initializer config blocks are intentionally deferred to allow referencing model classes without quoting. Here is a first pass at keeping that behavior on top of ec03eb6f. It could probably use a little cleanup but here's an idea of an approach for feedback. See https://github.com/railsadminteam/rails_admin/pull/3492#issuecomment-1064915362 for details. --- .rubocop.yml | 4 +- lib/rails_admin.rb | 4 +- lib/rails_admin/config.rb | 64 +++++++++++++------ lib/rails_admin/engine.rb | 5 ++ .../config/initializers/rails_admin.rb | 2 +- spec/rails_admin/config_spec.rb | 29 +++++---- spec/spec_helper.rb | 1 + 7 files changed, 71 insertions(+), 38 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 812b176a02..030bae93ff 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -115,10 +115,10 @@ Metrics/CyclomaticComplexity: Metrics/MethodLength: CountComments: false - Max: 29 # TODO: Lower to 15 + Max: 30 # TODO: Lower to 15 Metrics/ModuleLength: - Max: 228 # TODO: Lower to 100 + Max: 248 # TODO: Lower to 100 Metrics/ParameterLists: Max: 8 # TODO: Lower to 4 diff --git a/lib/rails_admin.rb b/lib/rails_admin.rb index fee9e267d6..f58f254ade 100644 --- a/lib/rails_admin.rb +++ b/lib/rails_admin.rb @@ -29,9 +29,7 @@ def self.config(entity = nil, &block) if entity RailsAdmin::Config.model(entity, &block) elsif block_given? - # Immediately evaluate non-model (initializer) config. It's needed to - # properly configure routes when there are custom actions. - yield RailsAdmin::Config + RailsAdmin::Config.apply_core(&block) else RailsAdmin::Config end diff --git a/lib/rails_admin/config.rb b/lib/rails_admin/config.rb index 48c0426403..431c7eeace 100644 --- a/lib/rails_admin/config.rb +++ b/lib/rails_admin/config.rb @@ -23,9 +23,12 @@ module Config DEFAULT_CURRENT_USER = proc {} # Variables to track initialization process - @initialized = false - @initializing = false - @deferred_blocks = [] + @initialized_core = false + @initializing_core = false + @deferred_core_blocks = [] + @initialized_models = false + @initializing_models = false + @deferred_model_blocks = [] class << self # Application title, can be an array of two elements @@ -86,22 +89,42 @@ 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! - return if @initializing + # Finish initialization by executing deferred non-model configuration blocks + def initialize_core! + return if @initializing_core - @initializing = true - @deferred_blocks.each { |block| block.call(self) } - @deferred_blocks.clear - @initialized = true + @initializing_core = true + @deferred_core_blocks.each { |block| block.call(self) } + @deferred_core_blocks.clear + @initialized_core = true end - # Evaluate the given block either immediately or lazily, based on initialization status. - def apply(&block) - if @initialized + # Evaluate the given non-model block either immediately or lazily, based on initialization status. + def apply_core(&block) + if @initialized_core yield(self) else - @deferred_blocks << block + @deferred_core_blocks << block + end + end + + # Finish initialization by executing deferred model configuration blocks + def initialize_models! + return if @initializing_models + + initialize_core! + @initializing_models = true + @deferred_model_blocks.each { |block| block.call(self) } + @deferred_model_blocks.clear + @initialized_models = true + end + + # Evaluate the given model block either immediately or lazily, based on initialization status. + def apply_model(&block) + if @initialized_models + yield(self) + else + @deferred_model_blocks << block end end @@ -230,7 +253,7 @@ def default_search_operator=(operator) # pool of all found model names from the whole application def models_pool - initialize! + initialize_models! (viable_models - excluded_models.collect(&:to_s)).uniq.sort end @@ -268,13 +291,13 @@ def model(entity, &block) # # Do not defer when called without a block as the model must be returned. if block - RailsAdmin::Config.apply do + RailsAdmin::Config.apply_model do m = model(entity) m.instance_eval(&block) if @registry[key].abstract_model end nil else - initialize! + initialize_models! @registry[key] ||= RailsAdmin::Config::Model.new(entity) end end @@ -338,8 +361,10 @@ def models # # @see RailsAdmin::Config.registry def reset - @initialized = @initializing = false - @deferred_blocks.clear + @initialized_core = @initializing_core = false + @deferred_core_blocks.clear + @initialized_models = @initializing_models = false + @deferred_model_blocks.clear @compact_show_view = true @browser_validations = true @authenticate = nil @@ -380,6 +405,7 @@ def reset_model(model) def reload! reset load RailsAdmin::Engine.config.initializer_path + initialize_core! end # Get all models that are configured as visible sorted by their weight and label. diff --git a/lib/rails_admin/engine.rb b/lib/rails_admin/engine.rb index b462fb557d..ef3042da21 100644 --- a/lib/rails_admin/engine.rb +++ b/lib/rails_admin/engine.rb @@ -68,6 +68,11 @@ class Engine < Rails::Engine ERROR end + RailsAdmin::Config.initialize_core! + + # Force route reload, since it doesn't reflect RailsAdmin action configuration yet + app.reload_routes! + RailsAdmin::Version.warn_with_js_version end end diff --git a/spec/dummy_app/config/initializers/rails_admin.rb b/spec/dummy_app/config/initializers/rails_admin.rb index 6f3d9b9ade..6f81505b39 100644 --- a/spec/dummy_app/config/initializers/rails_admin.rb +++ b/spec/dummy_app/config/initializers/rails_admin.rb @@ -2,7 +2,7 @@ RailsAdmin.config do |c| c.asset_source = CI_ASSET - c.model 'Team' do + c.model Team do include_all_fields field :color, :color end diff --git a/spec/rails_admin/config_spec.rb b/spec/rails_admin/config_spec.rb index bf1f27dd04..6fadce48e3 100644 --- a/spec/rails_admin/config_spec.rb +++ b/spec/rails_admin/config_spec.rb @@ -395,8 +395,8 @@ class TestController < ActionController::Base; end end end - describe '.apply' do - subject { RailsAdmin::Config.apply(&block) } + describe '.apply_core' do + subject { RailsAdmin::Config.apply_core(&block) } let(:block) do proc do |config| config.model Team do @@ -404,32 +404,35 @@ class TestController < ActionController::Base; end end end end - before { RailsAdmin::Config.instance_variable_set(:@initialized, false) } - before { RailsAdmin::Config.instance_variable_set(:@initializing, false) } - after do - RailsAdmin::Config.instance_variable_set(:@initialized, true) - RailsAdmin::Config.instance_variable_set(:@deferred_blocks, []) - end + after { RailsAdmin::Config.reset } 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 + it "doesn't evaluate the block when initialize_core! is finished" do + expect_any_instance_of(RailsAdmin::Config::Model).not_to receive(:register_instance_option).with('parameter') + subject + RailsAdmin::Config.initialize_core! + end + + it 'evaluates block when initialize_models! is finished' do expect_any_instance_of(RailsAdmin::Config::Model).to receive(:register_instance_option).with('parameter') subject - RailsAdmin::Config.initialize! + RailsAdmin::Config.initialize_models! 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! + RailsAdmin::Config.initialize_models! + RailsAdmin::Config.initialize_models! end context 'with a non-existent class' do + before { RailsAdmin::Config.reset } + let(:block) do proc do |config| config.model UnknownClass do @@ -465,7 +468,7 @@ class TestController < ActionController::Base; end end it 'does not immediately apply model configuration' do - expect(RailsAdmin::Config).not_to receive(:initialize!) + expect(RailsAdmin::Config).not_to receive(:initialize_models!) RailsAdmin::Config.reload! end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index e4d9fa8feb..e728bc5d3d 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -93,6 +93,7 @@ DatabaseCleaner.start RailsAdmin::Config.reset RailsAdmin::Config.asset_source = CI_ASSET + RailsAdmin::Config.initialize_core! end config.after(:each) do