Set default_shard from connects_to hash

If an application is using sharding, they may not want to use `default`
as the `default_shard`. Unfortunately Rails expects there to be a shard
named `default` for certain actions internally. This leads to some
errors on boot and the application is left manually setting
`default_shard=` in their model or updating their shards in
`connects_to` to name `shard_one` to `default`. Neither are a great
solution, especially if Rails can do this for you. Changes to Active
Record are:

* Simplify `connects_to` by merging `database` into `shards` kwarg so we
can do a single loop through provided options.
* Set the `self.default_shard` to the first keys in the shards kwarg.
* Add a test for this behavior
* Update existing test that wasn't testing this to use `default`. I
could have left this test but it really messes with connections in the
other tests and since this isn't testing shard behavior specifically, I
updated it to use `default` as the default shard name.

This is a slight change in behavior from existing applications but
arguably this is fixing a bug because without this an application won't
boot. I originally thought that this would require a huge refactoring to
fix but realized that it makes a lot of sense to take the first shard as
they default. They should all have the same schema so we can assume it's
fine to take the first one.

Fixes: #45390
This commit is contained in:
eileencodes 2023-05-31 11:45:24 -04:00
parent ae02cd6539
commit 0d41bfd3cc
No known key found for this signature in database
GPG Key ID: BA5C575120BBE8DF
5 changed files with 44 additions and 9 deletions

@ -1,3 +1,25 @@
* Use the first key in the `shards` hash from `connected_to` for the `default_shard`.
Some applications may not want to use `:default` as a shard name in their connection model. Unfortunately Active Record expects there to be a `:default` shard because it must assume a shard to get the right connection from the pool manager. Rather than force applications to manually set this, `connects_to` can infer the default shard name from the hash of shards and will now assume that the first shard is your default.
For example if your model looked like this:
```ruby
class ShardRecord < ApplicationRecord
self.abstract_class = true
connects_to shards: {
shard_one: { writing: :shard_one },
shard_two: { writing: :shard_two }
}
```
Then the `default_shard` for this class would be set to `shard_one`.
Fixes: #45390
*Eileen M. Uchitelle*
* Fix mutation detection for serialized attributes backed by binary columns.
*Jean Boussier*

@ -87,13 +87,12 @@ def connects_to(database: {}, shards: {})
connections = []
database.each do |role, database_key|
db_config = resolve_config_for_connection(database_key)
self.connection_class = true
connections << connection_handler.establish_connection(db_config, owner_name: self, role: role)
if shards.empty?
shards[:default] = database
end
self.default_shard = shards.keys.first
shards.each do |shard, database_keys|
database_keys.each do |role, database_key|
db_config = resolve_config_for_connection(database_key)

@ -309,6 +309,14 @@ class SomeOtherBase < ActiveRecord::Base
class ShardConnectionTestModelB < SomeOtherBase
end
def test_default_shard_is_chosen_by_first_key_or_default
SecondaryBase.connects_to shards: { not_default: { writing: { database: ":memory:", adapter: "sqlite3" } } }
SomeOtherBase.connects_to database: { writing: { database: ":memory:", adapter: "sqlite3" } }
assert_equal :not_default, SecondaryBase.default_shard
assert_equal :default, SomeOtherBase.default_shard
end
def test_same_shards_across_clusters
SecondaryBase.connects_to shards: { one: { writing: { database: ":memory:", adapter: "sqlite3" } } }
SomeOtherBase.connects_to shards: { one: { writing: { database: ":memory:", adapter: "sqlite3" } } }

@ -535,6 +535,7 @@ def test_connection_notification_is_called
assert_equal @connection_test_model_class.name, payloads[0][:connection_name]
assert_equal ActiveRecord::Base.default_shard, payloads[0][:shard]
ensure
@connection_test_model_class.remove_connection
ActiveSupport::Notifications.unsubscribe(subscription) if subscription
end
@ -543,12 +544,13 @@ def test_connection_notification_is_called_for_shard
subscription = ActiveSupport::Notifications.subscribe("!connection.active_record") do |name, started, finished, unique_id, payload|
payloads << payload
end
@connection_test_model_class.connects_to shards: { shard_two: { writing: :arunit } }
@connection_test_model_class.connects_to shards: { default: { writing: :arunit } }
assert_equal [:config, :connection_name, :shard], payloads[0].keys.sort
assert_equal @connection_test_model_class.name, payloads[0][:connection_name]
assert_equal :shard_two, payloads[0][:shard]
assert_equal :default, payloads[0][:shard]
ensure
@connection_test_model_class.remove_connection
ActiveSupport::Notifications.unsubscribe(subscription) if subscription
end

@ -426,17 +426,21 @@ class ApplicationRecord < ActiveRecord::Base
end
```
You are not required to use `default` as the first shard name. Rails will assume the first
shard name in the `connects_to` hash is the "default" connection. This connection is used
internally to load type data and other information where the schema is the same across shards.
Then models can swap connections manually via the `connected_to` API. If
using sharding, both a `role` and a `shard` must be passed:
```ruby
ActiveRecord::Base.connected_to(role: :writing, shard: :default) do
@id = Person.create! # Creates a record in shard default
@id = Person.create! # Creates a record in shard named ":default"
end
ActiveRecord::Base.connected_to(role: :writing, shard: :shard_one) do
Person.find(@id) # Can't find record, doesn't exist because it was created
# in the default shard
# in the shard named ":default".
end
```