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

Restore previous behavior of parallel test databases #38179

Merged
merged 1 commit into from Jan 7, 2020

Conversation

@eileencodes
Copy link
Member

eileencodes commented Jan 7, 2020

This commit is somewhat of a bandaid fix for a bug that was revealed in #38029
and then #38151. #38151 can cause problems in certain cases when an app has
a 3-tier config, with replicas, because it reorders the configuration
and changes the implict default connection that gets picked up.

If an app calls establish_connection with no arguments or doesn't call
connects_to in ApplicationRecord AND uses parallel testing
databases, the application may pick up the wrong configuration.

This is because when the code in #38151 loops through the configurations
it will update the non-replica configurations and then put them at the
end of the list. If you don't specify which connection you want, Rails
will pick up the first connection for that environment. So given the
following configuration:

test:
  primary:
    database: my_db
  replica:
    database: my_db
    replica: true

The database configurations will get reordered to be replica,
primary and when Rails calls establish_connection with no arguments
it will pick up replica because it's first in the list.

Looking at this bug it shows that calling establish_connection with no
arguments (which will pick up the default env + first configuration in
the list) OR when establish_connection is called with an environment
like :test it will also pick up that env's first configuration. This
can have surprising behavior in a couple cases:

  1. In the parallel testing changes we saw users getting the wrong db
    configuration and hitting an ActiveRecord::ReadOnlyError
  2. Writing a configuration that puts replica before primary, also
    resulting in a ActiveRecord::ReadOnlyError

The real fix for this issue is to deprecate calling
establish_connection with an env or nothing and require an explcit
configuration (like primary). This would also involve blessing
:primary as the default connection Rails looks for on boot. In
addition, this would require us deprecating connection specification
name "primary" in favor of the class name always since that will get
mega-confusing (seriously, it's already mega-confusing).

We'll work on fixing these underlying issues, but wanted to get a fix
out that restores previous behavior.

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

cc/ @georgeclaghorn

This commit is somewhat of a bandaid fix for a bug that was revealed in #38029
and then #38151. #38151 can cause problems in certain cases when an app has
a 3-tier config, with replicas, because it reorders the configuration
and changes the implict default connection that gets picked up.

If an app calls `establish_connection` with no arguments or doesn't call
`connects_to` in `ApplicationRecord` AND uses parallel testing
databases, the application may pick up the wrong configuration.

This is because when the code in #38151 loops through the configurations
it will update the non-replica configurations and then put them at the
end of the list. If you don't specify which connection you want, Rails
will pick up the _first_ connection for that environment. So given the
following configuration:

```
test:
  primary:
    database: my_db
  replica:
    database: my_db
    replica: true
```

The database configurations will get reordered to be `replica`,
`primary` and when Rails calls `establish_connection` with no arguments
it will pick up `replica` because it's first in the list.

Looking at this bug it shows that calling `establish_connection` with no
arguments (which will pick up the default env + first configuration in
the list) OR when `establish_connection` is called with an environment
like `:test` it will also pick up that env's first configuration. This
can have surprising behavior in a couple cases:

1) In the parallel testing changes we saw users getting the wrong db
configuration and hitting an `ActiveRecord::ReadOnlyError`
2) Writing a configuration that puts `replica` before `primary`, also
resulting in a `ActiveRecord::ReadOnlyError`

The real fix for this issue is to deprecate calling
`establish_connection` with an env or nothing and require an explcit
configuration (like `primary`). This would also involve blessing
`:primary` as the default connection Rails looks for on boot. In
addition, this would require us deprecating connection specification
name "primary" in favor of the class name always since that will get
mega-confusing (seriously, it's already mega-confusing).

We'll work on fixing these underlying issues, but wanted to get a fix
out that restores previous behavior.

Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
@rails-bot rails-bot bot added the activerecord label Jan 7, 2020
@eileencodes eileencodes added this to the 6.1.0 milestone Jan 7, 2020
@eileencodes eileencodes merged commit 09fbee8 into rails:master Jan 7, 2020
2 checks passed
2 checks passed
build
Details
buildkite/rails Build #66255 passed (18 minutes, 23 seconds)
Details
@eileencodes eileencodes deleted the eileencodes:fix-resolve-symbol-conn branch Jan 7, 2020
eileencodes added a commit to seejohnrun/rails that referenced this pull request Jan 8, 2020
…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 added a commit to seejohnrun/rails that referenced this pull request Jan 8, 2020
…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 added a commit to seejohnrun/rails that referenced this pull request Jan 8, 2020
…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 added a commit to seejohnrun/rails that referenced this pull request Jan 8, 2020
…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 added a commit to seejohnrun/rails that referenced this pull request Jan 8, 2020
…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 added a commit to seejohnrun/rails that referenced this pull request Jan 8, 2020
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.