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

Treat ActiveRecord::Base and ApplicationRecord as "primary" #36394

Merged

Conversation

eileencodes
Copy link
Member

When someone has a multi-db application their ApplicationRecord will
look like:

class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true

  connects_to database: { writing: :primary, reading: :replica }
end

This will cause us to open 2 connections to ActiveRecord::Base's
database when we actually only want 1. This is because Rails sees
ApplicationRecord and thinks it's a new connection, not the existing
ActiveRecord::Base connection because the
connection_specification_name is different.

This PR changes ApplicationRecord classes to consider themselves the
same as the "primary" connection.

Fixes #36382

Note: this needs a backport to 6-0-stable

cc/ @tenderlove @jhawthorn @rafaelfranca @matthewd @morgoth

@eileencodes eileencodes added this to the 6.0.0 milestone Jun 3, 2019
@tenderlove
Copy link
Member

Does this mean that AR::Base must have the same connection configuration as ApplicationRecord?

@eileencodes
Copy link
Member Author

I'm not sure why you'd set up one that's different. ApplicationRecord already assumes that it's ActiveRecord::Base, it's just that we never call establish_connection from it before. Do you know of a use case where you'd want a different connection configuration for AR::Base and ApplicationRecord?

@eileencodes eileencodes force-pushed the treat-application-record-as-primary branch from 676574b to 8666d09 Compare June 4, 2019 18:47
When someone has a multi-db application their `ApplicationRecord` will
look like:

```ruby
class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true

  connects_to database: { writing: :primary, reading: :replica }
end
```

This will cause us to open 2 connections to ActiveRecord::Base's
database when we actually only want 1. This is because Rails sees
`ApplicationRecord` and thinks it's a new connection, not the existing
`ActiveRecord::Base` connection because the
`connection_specification_name` is different.

This PR changes `ApplicationRecord` classes to consider themselves the
same as the "primary" connection.

Fixes rails#36382
@eileencodes eileencodes force-pushed the treat-application-record-as-primary branch from 8666d09 to 2f8b397 Compare June 5, 2019 13:34
@rails-bot rails-bot bot added the railties label Jun 5, 2019
@eileencodes
Copy link
Member Author

To followup on this a bit Aaron and I talked on Slack about "why can't Rails just automatically connect on boot to the replica instead of manually connecting to the replica in ApplicationRecord?"

There are a few reasons why:

  1. Since we don't associate replicas to their primaries Rails doesn't actually know which one of the replica definitions is for the primary database, aka one that ActiveRecord::Base connects to. Eventually I'd like to change the configurations to be able to handle this but we had a ton of work to do for Rails 6 and that just wasn't going to make it in.
  2. On boot Rails calls establish_connection with no arguments. In current Rails 6.0 and master this will establish a connection to the primary database in the environment you're in (ie development primary). We could theoretically loop through the configurations and look up a replica for the development primary database but that replica would have to have a specific name that cannot change (primary_replica or something like that). I'm not sure at this point in the 6.0 release it would be a good idea to force users to use a specific name for the replica.
  3. To be totally selfish this wouldn't work for GitHub without us changing our replica keys because ours is called something other than primary_replica. This isn't "hard", but isn't convenient either.

As for the concerns regarding "what if someone is on a legacy app and they don't have an ApplicationRecord?"

  • This is solved by Rails requiring as of 5.0 that you must add an ApplicationRecord

And lastly "what if someone is connecting to one database for ActiveRecord::Base and another for ApplicationRecord?"

  • Normally I don't like to use this argument but I'd say that you're doing it wrong. ApplicationRecord was always meant to be tied to ActiveRecord::Base and each connection should have their own abstract class to deal with their connection.

@@ -116,7 +116,7 @@ class User < ActiveRecord::Base
RUBY

app_file "app/models/post.rb", <<-MODEL
class Post < ActiveRecord::Base
class Post < ApplicationRecord
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this because technically in a new app this is what Post would inherit from and checking defined? loaded ApplicationRecord in this test. In master if I change this line the same thing happens so I think that this more accurately demonstrates what a real app would load/unload.

@eileencodes eileencodes merged commit 71a019e into rails:master Jun 5, 2019
@eileencodes eileencodes deleted the treat-application-record-as-primary branch June 5, 2019 14:01
eileencodes added a commit that referenced this pull request Jun 5, 2019
…s-primary

Treat ActiveRecord::Base and ApplicationRecord as "primary"
alpaca-tc added a commit to alpaca-tc/rails that referenced this pull request Dec 30, 2020
Fixed: rails#40559

When calling `connects_to` from `ApplicationRecord`, `owner_name` will be
`ActiveRecord::Base` instead of `ApplicationRecord`. It is unexpected behavior.

It means `ApplicationRecord.connection` can't resolve correct `owner_name`
and can't fetch own `preventing_writes?`.

This behavior introduced by rails#36394
But, I think that the new established connection is completely independent connection.
If we want to consider the same as the `ActiveRecord::Base`, we can call
`ActiveRecord::Base.connects_to` instead of.

Co-authored-by: Ryuta Kamizono <kamipo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple db connection issues
2 participants