Rails 6 RC2 regression with multidb support

[navied]

Steps to reproduce

Git clone test repo: https://github.com/navied/autoloaderbug

rails db:create
rails db:migrate
rails s

Expected behavior

Code should reload on code change in development without causing an error.

Actual behavior

When a code change happens with autoloader set to classic the first hit to the application causes a NameError:

System configuration

Rails version: 6.0.0 RC2

Ruby version: 2.5.5

[fxn]

I have been able to reproduce.

Problem is, there is a class object which is not assigned to any constant (#<Class:0x00007fc694a12770>), and whose name is primary::SchemaMigration. That is not a valid constant name.

This does not happen in zeitwerk mode because Zeitwerk assumes less things about the stuff it does not manage.

On a first thought, we could perhaps fix classic to not assume in autoloaded? the name of an arbitrary class passed as argument has to yield a valid constant name (unless anonymous). I'll work on it.

/cc @eileencodes (just FYI)

[shime]

I'm experiencing the same issue on 6.0.0 after updating from 5.2.3 when running rake db:migrate.

This is happening when using either :zeitwerk or :classic as config.autoloader.

[OmriSama]

This seems to be supposedly fixed, but I'm trying to upgrade my Rails app to 6.0 right now and am experiencing a similar issue.

I have a method in a module that gets the DB Schema and returns it as a string. Calling it raises the NameError: wrong constant name primary exception:

Interestingly, this is happening in one of my Specs, but not in rails console. Here's the apparent traceback:

Marty is the name of the application.

[rails-bot]

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-0-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

[tconst]

@shime I was also having trouble with the db:migrate task. In this case I tracked it down to a gem that was calling self.class.model_name during an after_commit callback on that primary named class. I would check to see if you have anything being included into ActiveRecord that does something similar.

https://github.com/betterup/wisper-activerecord-publisher/blob/master/lib/wisper/activerecord/publisher.rb#L46

[fxn]

I have tried the original application with the current stable version and cannot reproduce anymore.

I have also tried

 % bin/rails c
Loading development environment (Rails 6.0.2.2)
irb(main):001:0> ActiveRecord::Base.connection.migration_context.needs_migration?
   (0.7ms)  SELECT "schema_migrations"."version" FROM "schema_migrations" ORDER BY "schema_migrations"."version" ASC
=> false

and does not raise anything in both classic and zeitwerk modes.

Let's close then.

[shime]

Thanks for pointing me in the right direction, @tconst. For posterity, here's what happened in my project.

Our Gemfile contained wisper-activerecord which caused problems. Not sure how, but that gem clashes with Zeitwerk.

The solution I used was to patch Active Support's Inflector to skip trying to constantize "primary".

In the meantime we have removed Wisper from the Gemfile and we no longer experience issues. This is indeed not an issue with Zeitwerk, but with Wisper.

Good call on closing this and thank you for your hard work on the new loader, @fxn.

[alecslupu]

I can confirm @shime statement. The wisper active record causes the issue.

A fix has been proposed in krisleech/wisper-activerecord#30

[fxn]

Can you help me understand the root cause of this?

Given a vanilla rails new application that has wisper-activerecord in the Gemfile, which would be a minimal change I can do to trigger the error?

[alecslupu]

@fxn , I have managed to replicate several times the issue having a migration like :

class ConvertPersonToActiveStorage < ActiveRecord::Migration[6.0]
  def up
    Person.where.not(image_file_name: nil).find_each do |entity|
      Some::BackGroundJob.perform_later(entity.id)
    end
  end

  def down
    raise ActiveRecord::IrreversibleMigration
  end
end

The issue resides in the fact that wisper-activerecord will trigger the after_commit hook at the end of the migration (as described in https://github.com/krisleech/wisper-activerecord/blob/master/lib/wisper/active_record/publisher.rb#L41).

The culprit of the error is the below function:

      def broadcast_model_name_key
        self.class.model_name.param_key rescue false
      end

And the values, while describing the issue:

[3] pry(#<primary::SchemaMigration>)> self.class
=> #<Class:0x0000557c6257c360>(version: string)
[4] pry(#<primary::SchemaMigration>)> self.class.model_name
NameError: wrong constant name primary
from /home/alecslupu/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/activesupport-6.0.3.1/lib/active_support/inflector/methods.rb:282:in `const_get'
[5] pry(#<primary::SchemaMigration>)> 

If additional details are required, let us know

LE: Using Rails V 6.0.3.1, wisper-activerecord v 1.0.0

[fxn]

Aaahhh, I have reproduced. It is enough to throw

Wisper::ActiveRecord.extend_all

in an initializer.

With that setting, Wisper is involved in migrations because ActiveRecord::SchemaMigration is an internal Rails model used to manage the table schema_migrations, and therefore when AR tries to write to that table broadcasts are triggered. Is in those models that Wisper errs, but I believe that is not Wisper's fault.

The name of the schema migration class is decorated in an unusual way.

So the root problem here is:

• Multi database support creates a class object that is assigned to no constant (via Class.new), but overrides name and to_s that way so that they look like primary::SchemaMigration.
• Wisper calls self.class.model_name in a callback. That is a legit call.
• AMo::Naming#model_name in turn calls Module#module_parents, which performs a constantize call.

Bottom line, this does not seem related to autoloading. The class objects created to manage schema_migrations do not play well with the AMo naming interface.

I am not sure right now about which is the best way to address this. @eileencodes do you have any thoughts about this? Why are those classes defined that way? Could we make them conform to the AMo interface?

[fxn]

Minimal way to reproduce in a vanilla Rails application (without Wisper):

1. rails new foo.
2. rails g migration bar.
3. Write schema_migration.model_name in the change method.
4. rails db:migrate.

You'll get

NameError: wrong constant name primary

Reason is the name of the class is "primary::SchemaMigration" and therefore model_name cannot compute schema_migration.module_parents.

[eileencodes]

I opened #39500, I think that will fix 6.0.

[byroot]

I believe this is fixed.