Expose primary_abstract_class public API
#41258
Conversation
Previously Rails was treating `ApplicationRecord` as special in the `primary_class?` check. The reason we need to treat it differtently than other connection classes is that `ActiveRecord::Base` will establish a connection to the primary database on boot. The established connection is to your primary database, or the first database defined in your configuration. We need to do this so that 2 connections aren't opened to the same database since `ActiveRecord::Base` and `AppliationRecord` are different classes, on connection the connection_speicification_name would be different. However, there is no guarantee that an application is using `ApplicationRecord` as it's primary abstract class. This exposes a public method for setting a class to a `primary_abstract_class` like this: ``` class PrimaryApplicationRecord < ActiveRecord::Base self.primary_abstract_class end ``` Calling `primary_abstract_class` will automatically set `self.abstract_class = true`. This change is backwards compatible because we weren't supporting multiple application records previously, and if you had an `ApplicationRecord` we assumed that was the primary class. This change continues to assume that `ApplicationRecord` is your primary class. You only need to set `primary_abstract_class` if your application record is not ApplicationRecord and you're using multiple databases. Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
| if Base.application_record_class | ||
| self == Base.application_record_class | ||
| else | ||
| if defined?(ApplicationRecord) && self == ApplicationRecord |
There was a problem hiding this comment.
This is here only for backward compatibility right? Should we deprecate?
There was a problem hiding this comment.
It's less backwards compatibility and more "most likely to be the default". I was thinking that I didn't want to force any existing application using ApplicationRecord to have to change anything. We could update the generators to add primary_abstract_class to ApplicationRecord and deprecate this to get everyone on board, but it only matters in apps that are using multiple databases AND have a non-application record application record. I don't think the number of apps doing this is high enough to warrant everyone changing their existing applications. What do you think?
There was a problem hiding this comment.
Agree with what you said. I'd still update the generator to use primary_abstract_class, but no need to deprecate or force apps to change.
One thing to investigate is if we can remove this connection on boot. Connection on boot is a huge resiliency problem because it means that you application that could be serving request that don't depend on the primary database will not be able to boot if the primary database is down. |
This has existed for a very long time so changing it will need to take that into account. But I agree it would be great if Rails didn't need to connect to a db on boot. It's something on my list but I don't know exactly when I'll get to it. |
Followup on rails#41258 (comment) This makes sure that newly generated applications get their `ApplicationRecord` set to `primary_abstract_class`. Existing applications can opt-in to this if they're using multiple databases or have changed their `ApplicationRecord` to a class with a different name. Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
In rails#41258 we exposed a public API for changing the default application record - ie the class used to connect to the database on boot in a non-eager loaded environment. After attempting to implement this in GitHub we realized that this won't work unless the class that is the primary class is eager loaded. Instead of marking a class as the `primary_abstract_class` this moves it to a config setting. The config setting must use a string because the class is not loaded yet. When the database connects it will be constantized. Without this change application envs that don't eager load like development would see an error that ApplicationRecord doesn't exist - because the custom application record class is not loaded yet. Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
followup to #41030, cc @rafaelfranca
We went with primary_abstract_class because we want that to set the class that is the primary abstract class, not have to look for which one is set to true. We'd have to store the class name somewhere, so figured better on the caller.
Previously Rails was treating
ApplicationRecordas special in theprimary_class?check. The reason we need to treat it differently thanother connection classes is that
ActiveRecord::Basewill establish aconnection to the primary database on boot. The established connection
is to your primary database, or the first database defined in your
configuration. We need to do this so that 2 connections aren't opened to
the same database since
ActiveRecord::BaseandApplicationRecordare different classes, on connection the connection_specification_name
would be different.
However, there is no guarantee that an application is using
ApplicationRecordas it's primary abstract class. This exposes apublic method for setting a class to a
primary_abstract_classlikethis:
Calling
primary_abstract_classwill automatically setself.abstract_class = true. This change is backwards compatiblebecause we weren't supporting multiple application records previously,
and if you had an
ApplicationRecordwe assumed that was the primaryclass. This change continues to assume that
ApplicationRecordis yourprimary class. You only need to set
primary_abstract_classif yourapplication record is not ApplicationRecord and you're using multiple
databases.
Co-authored-by: John Crepezzi john.crepezzi@gmail.com