This PR adds support for `if_exists` on `remove_column` and
`if_not_exists` on `add_column` to support silently ignoring migrations
if the remove tries to remove a non-existent column or an add tries to
add an already existing column.
We (GitHub) have custom monkey-patched support for these features and
would like to upstream this behavior.
This matches the same behavior that is supported for `create_table` and
`drop_table`. The behavior for sqlite is different from mysql/postgres
and sqlite for remove column and that is reflected in the tests.
Fixes#38332
If the `connected_to` block returns a relation and does not inspect, or
load that relation before returning it, the block will exit before the
database is queried. This causes the wrong database connection to be
queried.
The consequences of this are getting records from the primary instead of
the replica, and potentially having database performance impact.
Relations lazily query the database. If you return the relation from the
block like:
```
posts = ActiveRecord::Base.connected_to(role: :reading) { Post.where(id: 1) }
```
`posts.first` will be queried from the `writing` connection instead
because it's lazy and performed outside the block. Any query that loads
the relation (ie `to_a`) inside the block would eagerly load the
relation's records and not exhibit this bug.
`connected_to` now checks if the return value is a `Relation` and if so
calls `load`.
Co-authored-by: Aaron Patterson <aaron.patterson@gmail.com>
`default_scoped` is an only way to enforce returning a scope with
default scopes in a scoping, and it is needed for migration to avoid
leaking scope (#35280, #37727).
Closes#38241.
This PR moves advisory lock to it's own connection instead of
`ActiveRecord::Base` to fix#37748. As a note the issue is present on
both mysql and postgres. We don't see it on sqlite3 because sqlite3
doesn't support advisory locks.
The underlying problem only appears if:
1) the app is using multiple databases, and therefore establishing a new
connetion in the abstract models
2) the app has a migration that loads a model (ex `Post.update_all`)
which causes that new connection to get established.
This is because when Rails runs migrations the default connections are
established, the lock is taken out on the `ActiveRecord::Base`
connection. When the migration that calls a model is loaded, a new
connection will be established and the lock will automatically be
released.
When Rails goes to release the lock in the ensure block it will find
that the connection has been closed. Even if the connection wasn't
closed the lock would no longer exist on that connection.
We originally considered checking if the connection was active, but
ultimately that would hide that the advisory locks weren't working
correctly because there'd be no lock to release.
We also considered making the lock more granular - that it only blocked
on each migration individually instead of all the migrations for that
connection. This might be the right move going forward, but right now
multi-db migrations that load models are very broken in Rails 6.0 and
master.
John and I don't love this fix, it requires a bit too much knowledge of
internals and how Rails picks up connections. However, it does fix the
issue, makes the lock more global, and makes the lock more resilient to
changing connections.
Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
This updates the database tasks for dumping the Active Record schema cache as
well as clearing the schema cache file, allowing the path to be defined in the
database configuration YAML file.
As before, the value can also be defined in an ENV variable, though this would
not work for a multi-db application. If the value is specified neither in the
DB config, nor in the ENV, then the path will continue to be derived from the
DB config spec_name.
Note that in order to make this change cleaner I also moved a bit of logic
out of a rake task and into the DatabaseTasks class, for symmetry.
We have two rake tasks for the schema cache:
$ rake db:schema:cache:dump
$ rake db:schema:cache:clear
The cache:dump task was implemented in DatabaseTasks, but the
cache:clear one was not.
I also added some tests for the behavior that I was changing, since some of
the code paths weren't tested.
Calling `#remove_connection` on the handler is deprecated in favor of
`#remove_connection_pool`. This change was made to support changing the
return value from a hash to a `DatabaseConfig` object.
`#remove_connection` will be removed in 6.2.
NOTE: `#remove_connection` on `ActiveRecord::Base` will also now return
a `DatabaseConfig` object. We didn't use a deprecation here since
it's not documented that this method used to return a `Hash`.
Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
Database configurations are now objects almost everywhere, so we don't
need to fake access to a hash with `#default_hash` or it's alias `#[]`.
Applications should `configs_for` and pass `env_name` and `spec_name` to
get the database config object. If you're looking for the default for
the test environment you can pass `configs_for(env_name: "test", spec_name:
"primary")`. Change test to developement to get the dev config, etc.
`#default_hash` and `#[]` will be removed in 6.2.
Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
Currently the database selector middleware only passes the request to
the context.
This is enough for the resolver to decide whether to switch
to the primary, but for a custom resolver and a custom context class,
it's not enough to persist any information for subsequent requests. For
example, say you want to use a cookie to decide whether when to switch.
It's not possible to set the response cookie from this middleware, since
neither the context nor the resolver have access to it.
This includes an extra step to update the context after the response has
been computed.
- When a column with a precision that is higher than what the system
allows, it would result in an error:
```sh
require "bigdecimal/util"
123.4.to_d(20) => ArgumentError, precision is too high
```
To fix that problem we need to check what the max number of digits
a Float is allowed to have, we can achieve that with `BigDecimal.double_fig`
Fix#38209
- ### Problem
It's no longer possible to define a numeric validation on abstract
class:
```ruby
class AnimalsBase < ApplicationRecord
self.abstract_class = true
validates :age, numericality: { min: 18 }
end
class Dog < AnimalsBase
end
Dog.create!(age: 0) => ActiveRecord::TableNotSpecified: Dog has no table configured. Set one with Dog.table_name=
```
### Solution
Instead of trying to get the type for attribute on the class
defining the validation, get it from the record being validated.
Since test fixtures share connections (due to transactional tests) we
end up overwriting the reading configuration so Rails doesn't recognize
it as a replica connection.
This change ensures that if we're using the `reading` role that
connections will always have prevent writes turned on.
If you need a replica connection that does not block writes, you should
use a different role name other than `:reading`.
The db selector test and connection handlers test have been updated to
test for these changes. In the db selector test we don't always have a
writing handler so I updated test fixtures to return if that's nil.
Lastly one test needed to be updated to use a different handler name due
to it needing to write to successfully test what it needs to test.
Fixes#37765
This test wasn't correct. If we're calling `resolver.read` and want to
actually read from the replicas then the role would be reading not
writing. This was because the session store needed to be changed so that
we actually are "reading from the replicas" instead of reading from the
primary.
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>
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>
The Preloader relies on other objects to bind the retrieved records to their
parents. When executed across a hash, it assumes that the results of
`preloaded_records` is the appropriate set of records to pass in to the next
layer.
Filtering based on the reflection properties in `preloaded_records` allows us to
avoid excessive preloading in the instance where we are loading across a
`has_one` association distinguished by an order (e.g. "last comment" or
similar), by dropping these records before they are returned to the
Preloader. In this situation, we avoid potentially very long key lists in
generated queries and the consequential AR object instantiations.
This is mostly relevant if the underlying linked set has relatively many
records, because this is effectively a multiplier on the number of records
returned on the far side of the preload. Unfortunately, avoiding the
over-retrieval of the `has_one` association seems to require substantial changes
to the preloader design, and probably adaptor-specific logic -- it is a
top-by-group problem.
If we defined a callback before an association that updates the object, then this may end up being
manipulated to being `false` when it should be `true`. We guard this be only defining it once.
The implication of it being false, in this case, is that the children aren't updated with the parent_id
and so they fail to associate to one another.
See https://github.com/rails/rails/issues/38120 for more details
