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

Jump to bottom

Deprecate "primary" as a connection_specification_name for ActiveRecord::Base #38190

Merged
merged 1 commit into from Jan 9, 2020
Merged

Deprecate "primary" as a connection_specification_name for ActiveRecord::Base #38190

merged 1 commit into from Jan 9, 2020

Conversation

eileencodes
Copy link
Member

@eileencodes eileencodes commented Jan 8, 2020 •
edited

As multiple databases have evolved it's becoming more and more
confusing that we have a connection_specification_name that defaults
to "primary" and a spec_name on the database objects that defaults to
"primary" (my bad).

Even more confusing is that we use the class name for all
non-ActiveRecord::Base abstract classes that establish connections. For
example connections established on class MyOtherDatabaseModel < ApplicationRecord will use "MyOtherDatabaseModel" as it's connection
specification name while ActiveRecord::Base uses "primary".

This PR deprecates the use of the name "primary" as the
connection_specification_name for ActiveRecord::Base in favor of
using "ActiveRecord::Base".

In this PR the following is true:

  • If handler.establish_connection(:primary) is called, "primary"
    will not throw a deprecation warning and can still be used for the
    connection_specification_name. This also fixes a bug where using this
    method to establish a connection could accidentally overwrite the actual
    ActiveRecord::Base connection IF that connection was not using a
    configuration named :primary.
  • Calling handler.retrieve_connection "primary" when
    handler.establish_connection :primary has never been called will
    return the connection for ActiveRecord::Base and throw a deprecation
    warning.
  • Calling handler.remove_connection "primary" when
    handler.establish_connection :primary has never been called will
    remove the connection for ActiveRecord::Base and throw a deprecation
    warning.

See #38179 for details on more motivations for this change.

Co-authored-by: John Crepezzi john.crepezzi@gmail.com

@eileencodes eileencodes added this to the 6.1.0 milestone Jan 8, 2020
@eileencodes eileencodes force-pushed the deprecate-primary-as-connection_specification_name branch 2 times, most recently from b11eeb6 to 3daae24 Compare Jan 8, 2020
@eileencodes @seejohnrun
Deprecate "primary" as a connection_specification_name for ActiveReco…
b74fbe4 
…rd::Base

As multiple databases have evolved it's becoming more and more
confusing that we have a `connection_specification_name` that defaults
to "primary" and a `spec_name` on the database objects that defaults to
"primary" (my bad).

Even more confusing is that we use the class name for all
non-ActiveRecord::Base abstract classes that establish connections. For
example connections established on `class MyOtherDatabaseModel <
ApplicationRecord` will use `"MyOtherDatabaseModel"` as it's connection
specification name while `ActiveRecord::Base` uses `"primary"`.

This PR deprecates the use of the name `"primary"` as the
`connection_specification_name` for `ActiveRecord::Base` in favor of
using `"ActiveRecord::Base"`.

In this PR the following is true:

* If `handler.establish_connection(:primary)` is called, `"primary"`
will not throw a deprecation warning and can still be used for the
`connection_specification_name`. This also fixes a bug where using this
method to establish a connection could accidentally overwrite the actual
`ActiveRecord::Base` connection IF that connection was not using a
configuration named `:primary`.
* Calling `handler.retrieve_connection "primary"` when
`handler.establish_connection :primary` has never been called will
return the connection for `ActiveRecord::Base` and throw a deprecation
warning.
* Calling `handler.remove_connection "primary"` when
`handler.establish_connection :primary` has never been called will
remove the connection for `ActiveRecord::Base` and throw a deprecation
warning.

See rails#38179 for details on more motivations for this change.

Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
@eileencodes eileencodes force-pushed the deprecate-primary-as-connection_specification_name branch from 3daae24 to b74fbe4 Compare Jan 8, 2020
@eileencodes eileencodes merged commit 4a477c0 into rails:master Jan 9, 2020
2 checks passed
@eileencodes eileencodes deleted the deprecate-primary-as-connection_specification_name branch Jan 9, 2020
eileencodes added a commit to eileencodes/rails that referenced this issue Feb 20, 2020
@eileencodes
Deprecate spec_name and use namespace for configurations
e2e5fc4 
I have so. many. regrets. about using `spec_name` for database
configurations and now I'm finally putting this mistake to an end.

Back when I started multi-db work I assumed that eventually
`connection_specification_name` (sometimes called `spec_name`) and
`spec_name` for configurations would one day be the same thing. After
2 years I no longer believe they will ever be the same thing.

This PR deprecates `spec_name` on database configurations in favor of
`namespace`. It's the same behavior, just a better name, or at least a
less confusing name.

`connection_specification_name` refers to the parent class name (ie
ActiveRecord::Base, AnimalsBase, etc) that holds the connection for it's
models. In some places like ConnectionHandler it shortens this to
`spec_name`, hence the major confusion.

Recently I've been working with some new folks on database stuff and
connection management and realize how confusing it was to explain that
`db_config.spec_name` was not `spec_name` and
`connection_specification_name`. Worse than that one is a symbole while
the other is a class name. This was made even more complicated by the
fact that `ActiveRecord::Base` used `primary` as the
`connection_specification_name` until rails#38190.

After spending 2 years with connection management I don't believe that
we can ever use the symbols from the database configs as a way to
connect the database without the class name being _somewhere_ because
a db_config does not know who it's owner class is until it's been
connected and a model has no idea what db_config belongs to it until
it's connected. The model is the only way to tie a primary/writer config
to a replica/reader config. This could change in the future but I don't
see value in adding a class name to the db_configs before connection or
telling a model what config belongs to it before connection. That would
probably break a lot of application assumptions. If we do ever end up in
that world, we can use namespace, because tbh `spec_name` and
`connection_specification_name` were always confusing to me.
@eileencodes eileencodes mentioned this pull request Feb 20, 2020
Merged
eileencodes added a commit to eileencodes/rails that referenced this issue Feb 20, 2020
@eileencodes
Deprecate spec_name and use namespace for configurations
dc686be 
I have so. many. regrets. about using `spec_name` for database
configurations and now I'm finally putting this mistake to an end.

Back when I started multi-db work I assumed that eventually
`connection_specification_name` (sometimes called `spec_name`) and
`spec_name` for configurations would one day be the same thing. After
2 years I no longer believe they will ever be the same thing.

This PR deprecates `spec_name` on database configurations in favor of
`namespace`. It's the same behavior, just a better name, or at least a
less confusing name.

`connection_specification_name` refers to the parent class name (ie
ActiveRecord::Base, AnimalsBase, etc) that holds the connection for it's
models. In some places like ConnectionHandler it shortens this to
`spec_name`, hence the major confusion.

Recently I've been working with some new folks on database stuff and
connection management and realize how confusing it was to explain that
`db_config.spec_name` was not `spec_name` and
`connection_specification_name`. Worse than that one is a symbole while
the other is a class name. This was made even more complicated by the
fact that `ActiveRecord::Base` used `primary` as the
`connection_specification_name` until rails#38190.

After spending 2 years with connection management I don't believe that
we can ever use the symbols from the database configs as a way to
connect the database without the class name being _somewhere_ because
a db_config does not know who it's owner class is until it's been
connected and a model has no idea what db_config belongs to it until
it's connected. The model is the only way to tie a primary/writer config
to a replica/reader config. This could change in the future but I don't
see value in adding a class name to the db_configs before connection or
telling a model what config belongs to it before connection. That would
probably break a lot of application assumptions. If we do ever end up in
that world, we can use namespace, because tbh `spec_name` and
`connection_specification_name` were always confusing to me.
eileencodes added a commit to eileencodes/rails that referenced this issue Feb 21, 2020
@eileencodes
Deprecate spec_name and use namespace for configurations
782c8d3 
I have so. many. regrets. about using `spec_name` for database
configurations and now I'm finally putting this mistake to an end.

Back when I started multi-db work I assumed that eventually
`connection_specification_name` (sometimes called `spec_name`) and
`spec_name` for configurations would one day be the same thing. After
2 years I no longer believe they will ever be the same thing.

This PR deprecates `spec_name` on database configurations in favor of
`namespace`. It's the same behavior, just a better name, or at least a
less confusing name.

`connection_specification_name` refers to the parent class name (ie
ActiveRecord::Base, AnimalsBase, etc) that holds the connection for it's
models. In some places like ConnectionHandler it shortens this to
`spec_name`, hence the major confusion.

Recently I've been working with some new folks on database stuff and
connection management and realize how confusing it was to explain that
`db_config.spec_name` was not `spec_name` and
`connection_specification_name`. Worse than that one is a symbole while
the other is a class name. This was made even more complicated by the
fact that `ActiveRecord::Base` used `primary` as the
`connection_specification_name` until rails#38190.

After spending 2 years with connection management I don't believe that
we can ever use the symbols from the database configs as a way to
connect the database without the class name being _somewhere_ because
a db_config does not know who it's owner class is until it's been
connected and a model has no idea what db_config belongs to it until
it's connected. The model is the only way to tie a primary/writer config
to a replica/reader config. This could change in the future but I don't
see value in adding a class name to the db_configs before connection or
telling a model what config belongs to it before connection. That would
probably break a lot of application assumptions. If we do ever end up in
that world, we can use namespace, because tbh `spec_name` and
`connection_specification_name` were always confusing to me.
eileencodes added a commit to eileencodes/rails that referenced this issue Feb 21, 2020
@eileencodes
Deprecate spec_name and use namespace for configurations
1d60673 
I have so. many. regrets. about using `spec_name` for database
configurations and now I'm finally putting this mistake to an end.

Back when I started multi-db work I assumed that eventually
`connection_specification_name` (sometimes called `spec_name`) and
`spec_name` for configurations would one day be the same thing. After
2 years I no longer believe they will ever be the same thing.

This PR deprecates `spec_name` on database configurations in favor of
`namespace`. It's the same behavior, just a better name, or at least a
less confusing name.

`connection_specification_name` refers to the parent class name (ie
ActiveRecord::Base, AnimalsBase, etc) that holds the connection for it's
models. In some places like ConnectionHandler it shortens this to
`spec_name`, hence the major confusion.

Recently I've been working with some new folks on database stuff and
connection management and realize how confusing it was to explain that
`db_config.spec_name` was not `spec_name` and
`connection_specification_name`. Worse than that one is a symbole while
the other is a class name. This was made even more complicated by the
fact that `ActiveRecord::Base` used `primary` as the
`connection_specification_name` until rails#38190.

After spending 2 years with connection management I don't believe that
we can ever use the symbols from the database configs as a way to
connect the database without the class name being _somewhere_ because
a db_config does not know who it's owner class is until it's been
connected and a model has no idea what db_config belongs to it until
it's connected. The model is the only way to tie a primary/writer config
to a replica/reader config. This could change in the future but I don't
see value in adding a class name to the db_configs before connection or
telling a model what config belongs to it before connection. That would
probably break a lot of application assumptions. If we do ever end up in
that world, we can use namespace, because tbh `spec_name` and
`connection_specification_name` were always confusing to me.
eileencodes added a commit to eileencodes/rails that referenced this issue Feb 24, 2020
@eileencodes
Deprecate spec_name and use name for configurations
854d4e0 
I have so. many. regrets. about using `spec_name` for database
configurations and now I'm finally putting this mistake to an end.

Back when I started multi-db work I assumed that eventually
`connection_specification_name` (sometimes called `spec_name`) and
`spec_name` for configurations would one day be the same thing. After
2 years I no longer believe they will ever be the same thing.

This PR deprecates `spec_name` on database configurations in favor of
`name`. It's the same behavior, just a better name, or at least a
less confusing name.

`connection_specification_name` refers to the parent class name (ie
ActiveRecord::Base, AnimalsBase, etc) that holds the connection for it's
models. In some places like ConnectionHandler it shortens this to
`spec_name`, hence the major confusion.

Recently I've been working with some new folks on database stuff and
connection management and realize how confusing it was to explain that
`db_config.spec_name` was not `spec_name` and
`connection_specification_name`. Worse than that one is a symbole while
the other is a class name. This was made even more complicated by the
fact that `ActiveRecord::Base` used `primary` as the
`connection_specification_name` until rails#38190.

After spending 2 years with connection management I don't believe that
we can ever use the symbols from the database configs as a way to
connect the database without the class name being _somewhere_ because
a db_config does not know who it's owner class is until it's been
connected and a model has no idea what db_config belongs to it until
it's connected. The model is the only way to tie a primary/writer config
to a replica/reader config. This could change in the future but I don't
see value in adding a class name to the db_configs before connection or
telling a model what config belongs to it before connection. That would
probably break a lot of application assumptions. If we do ever end up in
that world, we can use name, because tbh `spec_name` and
`connection_specification_name` were always confusing to me.
eileencodes added a commit to eileencodes/rails that referenced this issue Feb 24, 2020
@eileencodes
Deprecate spec_name and use name for configurations
79ce7d9 
I have so. many. regrets. about using `spec_name` for database
configurations and now I'm finally putting this mistake to an end.

Back when I started multi-db work I assumed that eventually
`connection_specification_name` (sometimes called `spec_name`) and
`spec_name` for configurations would one day be the same thing. After
2 years I no longer believe they will ever be the same thing.

This PR deprecates `spec_name` on database configurations in favor of
`name`. It's the same behavior, just a better name, or at least a
less confusing name.

`connection_specification_name` refers to the parent class name (ie
ActiveRecord::Base, AnimalsBase, etc) that holds the connection for it's
models. In some places like ConnectionHandler it shortens this to
`spec_name`, hence the major confusion.

Recently I've been working with some new folks on database stuff and
connection management and realize how confusing it was to explain that
`db_config.spec_name` was not `spec_name` and
`connection_specification_name`. Worse than that one is a symbole while
the other is a class name. This was made even more complicated by the
fact that `ActiveRecord::Base` used `primary` as the
`connection_specification_name` until rails#38190.

After spending 2 years with connection management I don't believe that
we can ever use the symbols from the database configs as a way to
connect the database without the class name being _somewhere_ because
a db_config does not know who it's owner class is until it's been
connected and a model has no idea what db_config belongs to it until
it's connected. The model is the only way to tie a primary/writer config
to a replica/reader config. This could change in the future but I don't
see value in adding a class name to the db_configs before connection or
telling a model what config belongs to it before connection. That would
probably break a lot of application assumptions. If we do ever end up in
that world, we can use name, because tbh `spec_name` and
`connection_specification_name` were always confusing to me.
@eileencodes eileencodes mentioned this pull request May 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
activerecord
Projects
None yet
Milestone
6.1.0
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@eileencodes