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

per model config can no longer be defined before associations #3490

Closed
q3aiml opened this issue Mar 9, 2022 · 1 comment
Closed

per model config can no longer be defined before associations #3490

q3aiml opened this issue Mar 9, 2022 · 1 comment

Comments

@q3aiml
Copy link
Contributor

q3aiml commented Mar 9, 2022

Describe the bug
While testing an upgrade from rails_admin 2.x to 3.0.0.rc3, I noticed that a number of our association fields were incorrectly using the String type. Previously on 2.x these fields correctly used an association type such as BelongsToAssociation.

I tracked this down to a behavior change when rails_admin config is defined in a model before the associations. In 2.x the execution of the rails_admin block is deferred and runs after the associations are defined. In 3.x the block runs immediately. As a result in 3.x any field declarations effectively disable the field factories and force a string type. The declarations no longer modify an existing field automatically created from the association and instead create the field.

I believe this may be caused by e4ae669.

Reproduction steps

Here is a standalone example. Uncomment the relevant gemfile section for either 2.x or 3.x.

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  # >> uncomment this section to show rails_admin 3.x behavior
  # gem 'rails', '~> 7.0.0'
  # gem 'rails_admin', '~> 3.0.0.rc3', require: false
  # >> end

  # >> uncomment this section to show rails_admin 2.x behavior
  # gem 'rails', '~> 6.0.0'
  # gem 'rails_admin', '~> 2.2.1', require: false
  # gem 'sprockets', '<4'
  # >> end

  gem 'sqlite3'
end

require 'active_record/railtie'
require 'active_storage/engine'
require 'rails_admin'
require 'logger'

class TestApp < Rails::Application
  config.root = __dir__
  config.eager_load = false
  config.session_store :cookie_store, key: 'cookie_store_key'
  secrets.secret_key_base = 'secret_key_base'

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger
end

ENV['DATABASE_URL'] = 'sqlite3::memory:'

Rails.application.initialize!

RailsAdmin.config do |config|
  config.included_models =
    %w[
      Comment
    ]
end

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  rails_admin do
    base do
      field :post do
        # Another way to see this is to include an option such as `inline_add`
        # which is valid for the association type but not string. In 3.x it will
        # raise an error.
        inline_add false
      end
    end
  end

  belongs_to :post
end

fields = RailsAdmin::AbstractModel.all[0].config.fields.to_h { |field| [field.name, field] }
puts fields

The output on 2.x:

{:post=>#<RailsAdmin::Config::Fields::Types::BelongsToAssociation[post] @parent=RailsAdmin::Config::Sections::Base, @root=RailsAdmin::Config::Model, @abstract_model=#<RailsAdmin::AbstractModel:0x00007f99b2d365c8 @model_name="Comment", @adapter=:active_record>, @defined=true, @name=:post, @order=1, @properties=#<RailsAdmin::Adapters::ActiveRecord::Association:0x00007f99c1f10a10 @association=#<ActiveRecord::Reflection::BelongsToReflection:0x00007f99b39c61a8 @name=:post, @scope=nil, @options={}, @active_record=Comment(id: integer, post_id: integer), @klass=nil, @plural_name="posts", @type=nil, @foreign_type=nil, @constructable=true, @foreign_key="post_id">, @model=Comment(id: integer, post_id: integer)>, @section=RailsAdmin::Config::Sections::Base, @children_fields_registered=Array, @visible_registered=true>}

The output on 3.x:

{:post=>#<RailsAdmin::Config::Fields::Types::String[post] @parent=RailsAdmin::Config::Sections::Base, @root=RailsAdmin::Config::Model, @abstract_model=#<RailsAdmin::AbstractModel:0x00007fdd685a4400 @model_name="Comment", @adapter=:active_record>, @defined=true, @name=:post, @order=1, @properties=nil, @section=RailsAdmin::Config::Sections::Base>}

Expected behavior

For our use ideally the 2.x behavior would be reinstated. We separate our per-model config into concerns and we group the includes of concerns near the top of our models. We could simply move our includes but the risk remains that someone could define the rails_admin config in the wrong place in the future and be confused when association fields do not work as expected.

That said this is a major release and I do think the reloading of config in development mode is a large productivity boost. Perhaps those and other benefits are worth sacrificing the past behavior. If so I think the change is worthy of a note in the upgrade docs. Ideally there would also be a way to warn when a field is defined prior to its association, but I don't have a solution handy for that.

If sticking with the new behavior I think it may also make sense to update the RailsAdmin.config comment which seems to hint that config block execution will be deferred later than it now is:

# If only a block is passed it is stored to initializer stack to be evaluated
# on first request in production mode and on each request in development. If
# initialization has already occured (in other words RailsAdmin.setup has
# been called) the block will be added to stack and evaluated at once.

Additional context

  • rails version: 7.0
  • rails_admin version: 3.0.0.rc3

❤️ thanks so much for all the work on rails_admin ❤️

q3aiml added a commit to q3aiml/rails_admin that referenced this issue Mar 10, 2022
Attempt to restore 2.x config loading behavior where 1) initializer config
block evaluates immediately so that routes are configured correctly while 2)
per-model config blocks only evaluate after all the models are loaded and
associations are defined.

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 railsadminteam#3490
q3aiml added a commit to q3aiml/rails_admin that referenced this issue Mar 10, 2022
Attempt to restore 2.x config loading behavior where 1) initializer config
block evaluates immediately so that routes are configured correctly while 2)
per-model config blocks only evaluate after all the models are loaded and
associations are defined.

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 railsadminteam#3490
q3aiml added a commit to q3aiml/rails_admin that referenced this issue Mar 10, 2022
Attempt to restore 2.x config loading behavior where 1) initializer config
block evaluates immediately so that routes are configured correctly while 2)
per-model config blocks only evaluate after all the models are loaded and
associations are defined.

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 railsadminteam#3490
q3aiml added a commit to q3aiml/rails_admin that referenced this issue Mar 10, 2022
Attempt to restore 2.x config loading behavior where 1) initializer config
block evaluates immediately so that routes are configured correctly while 2)
per-model config blocks only evaluate after all the models are loaded and
associations are defined.

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 railsadminteam#3490
q3aiml added a commit to q3aiml/rails_admin that referenced this issue Mar 10, 2022
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 railsadminteam#3490
q3aiml added a commit to q3aiml/rails_admin that referenced this issue Mar 10, 2022
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 railsadminteam#3490
q3aiml added a commit to q3aiml/rails_admin that referenced this issue Mar 25, 2022
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 railsadminteam#3490
@mshibuya
Copy link
Member

Closed by #3541.

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

Successfully merging a pull request may close this issue.

2 participants