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.
Currently, if you run `rails g migration remove_column_from_models`,
there is an empty line before `remove_column` line because we forgot to
use `-%>` in the template:
$ bin/rails g migration remove_title_from_posts title:string
invoke active_record
create db/migrate/20200114061235_remove_title_from_posts.rb
$ cat db/migrate/20200114061235_remove_title_from_posts.rb
class RemoveTitleFromPosts < ActiveRecord::Migration[6.1]
def change
remove_column :posts, :title, :string
end
end
This commit adds the missing `-` in front of `-%>` to make it removes
the empty line.
- 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>
Although consuming code will almost certainly retraverse this set, we can avoid
walking it twice here. As an extra upside, we can avoid the double-use of an
identity-sensitive hash; this is convenient, because it is another collection we
actually don't need to build.
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
The ArgumentError occurs even though structures is compatible.
Because some query methods keep duplicate values.
For example, the behavior of `joins` method is as following:
```ruby
relation = Post.joins(:author).joins(:author)
relation.joins_values
#=> [:author, :author]
relation.or(Post.joins(:author))
#=> ArgumentError: Relation passed to #or must be structurally compatible. Incompatible values: [:joins]
```
This commit changes to not keep duplicate values.
Fixes#38052
In 154abca we switched from using `Rails.env` to fetch the `env_name` to
`ActiveRecord::ConnectionHandling::DEFAULT_ENV.call.to_sym` which
changed the type from a `String` to a `Symbol`.
This commit brings things back to the original state, so we can find the
configurations correctly!
It also modifies the configuration in the configurations array, so that
future connections can find the database with the updated keyword value.
Add ActiveRecord::Relation#cache_key_with_version. This method will be
used by ActionController::ConditionalGet to ensure that when collection
cache versioning is enabled, requests using ConditionalGet don't return
the same ETag header after a collection is modified.
Prior to the introduction of collection cache versioning in
4f2ac80d4cdb01c4d3c1765637bed76cc91c1e35, all collection cache keys
included a version. However, with cache versioning enabled, collection
cache keys remain constant. In turn, ETag headers remain constant,
rendering them ineffective.
This commit takes the cache_key_with_version method used for individual
Active Record objects (from aa8749eb52d7919a438940c9218cad98d892f9ad),
and adds it to collections.
In arel, there is a space between the table name and columns list when INSERT.
3c28e79b61/activerecord/lib/arel/visitors/to_sql.rb (L56)
```ruby
User.create!(name: "foo")
# INSERT INTO `users` (`name`) VALUES ('foo')
^^^^^^^^^^^^^^^^
```
But SQL of insert_all is not separated.
```ruby
User.insert_all!([{name: "foo"}])
# INSERT INTO `users`(`name`) VALUES ('foo')
^^^^^^^^^^^^^^^
```
This is not a problem (because it is correct as SQL), but fixing this will make unified behavior.
A known issue is that database_rewinder fails to parse table name on insert_all.
https://github.com/amatsuda/database_rewinder/blob/v0.9.1/lib/database_rewinder.rb#L51-L54
```ruby
statement = "INSERT INTO `users`(`name`) VALUES ('foo')"
match = statement.match(/\A\s*INSERT(?:\s+IGNORE)?(?:\s+INTO)?\s+(?:\.*[`"]?([^.\s`"]+)[`"]?)*/i)
# => #<MatchData "INSERT INTO `users`(`name`)" 1:")">
# Expected behavior is
# => #<MatchData "INSERT INTO `users`" 1:"users">
```
When using PostgreSQL, it's useful to be able to specify NULLS FIRST and NULLS LAST on ordered columns. With this commit you can do that now, as in:
```ruby
User.arel_table[:first_name].desc.nulls_last
```
The `_trigger_update_callback` and `_trigger_destroy_callback`
attributes were added in 9252da96597fbffe2246704556524c4804239552 to
avoid running transactional callbacks when an attempt to modify a record
fails inside a transaction due to the record being invalid, for example.
However the values weren't being reset between transactions, which meant
they leaked from one transaction to another and caused false positives
where unsuccessful modifications still triggered callbacks. Clearing
them when a transaction commits or is rolled back fixes the problem.
With collection_cache_versioning enabled, a collection's volatile info
(size & max updated_at timestamp) is included in
ActiveRecord::Relation#cache_version, not #cache_key.
Avoid the SQL query to used determine this volatile info when generating
an un-versioned cache key. This query does not need to be executed
unless cache_version is called separately.
If `AR::Enum` is used for boolean field, it would be not expected
behavior for us.
fixes#38075
Problem:
In case of using boolean for enum, we can set with string (hash key)
to instance, but we cannot set with actual value (hash value).
```ruby
class Post < ActiveRecord::Base
enum status: { enabled: true, disabled: false }
end
post.status = 'enabled'
post.status # 'enabled'
post.status = true
post.status # 'enabled'
post.status = 'disabled'
post.status # 'disabled'
post.status = false
post.status # nil (This is not expected behavior)
```
After looking into `AR::Enum::EnumType#cast`, I found that `blank?`
method converts from false value to nil (it seems it may not intentional behavior).
In this patch, I improved that if it defines enum with boolean,
it returns reasonable behavior.
The regular expression did not match CREATE TABLE statements printed out by AWS Aurora MySQL 5.6 instances, because they lack the required space at that position.
Fixes https://github.com/rails/rails/issues/28827.
The steps to reproduce are as follows:
git clone git@github.com:bbuchalter/rails-issue-28827.git
cd rails-issue-28827
bundle install
bin/rails db:create
Observe that we create two databases when invoking db:create: development and test. Now observe what happens when we invoke our drop command while using DATABASE_URL.
DATABASE_URL=sqlite3://$(pwd)/db/database_url.sqlite3 bin/rails db:create
As expected, the development environment now uses the DATABASE_URL. What is unexpected is that the test environment does not.
It's unclear what the expected behavior should be in this case, but the cause of it is this: 9f2c74eda0/activerecord/lib/active_record/tasks/database_tasks.rb (L494)
Because of each_local_configuration, there seems to be no way invoke these database rake on only the development environment to ensure DATABASE_URL is respected.
The smallest scope of change I can think to make would be to conditionalize this behavior so it does not get applied when DATABASE_URL is present.
This adds `:declare`, `:fetch`, `:move`, and `:close` to allowed queries
when `while_preventing_writes` is set. I didn't support `:open` because
AFAICT `:declar` implcitly opens a cursor and all of my attempts to
write `@connection.execute("OPEN cur_ex")` threw a syntax error on
`OPEN`. It seems like open isn't supported with the client.
Fixes: #37960
`:begin, :commit, :explain, :release, :rollback, :savepoint, :select,
:with` are used by all the adapters. This commit adds a new constant
called `DEFAULT_READ_QUERY` so that we have somewhere to put the
queries used by all the adapters. Then we can set adapter specific query
parts in those adapters.
I also alphabetized these.
We want to introduce an object-based DSL for building and modifying
configuration objects. As part of that we want to make sure that users
don't think they can modify configuration_hash values and have them
change the configuration. For that reason we're going to freeze the
Hash here, and have modified places in tests where we were modifying
these hashes.
The commit here also adds a test for the Test Databases and in that work
we found that we were calling `Rails.env` and Active Record doesn't load
Rails.
Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
These tests weren't calling assert, so if the execute didn't raise but
also didn't return anything it would be a broken test that never fails.
We need to always add an assertion so we know what the expected behavior
is.
Related #36456.
I grepped the code base by `git grep -n 'connection_id: '` then I found
extra `connection_id: object_id` which is added at #20818 but unused.
Actually the `connection_id: object_id` is not a connection's object_id
but a connection_handler's object_id, it is very confusing.
Since the `:connection_id` in an internal instrument is not used, we can
just remove the incorrect information.
`name` is used by Rails to find the configuration by connection
specification name, but database adapters don't need to use `name` in
order to establish a connection. This is part of our work to separate
what the database needs to connect (the configuration hash) and the
what Rails needs to find connections (everything else).
Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
