From 0d41bfd3cc080dfff0c5af8fe40418f25e1c00c4 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Wed, 31 May 2023 11:45:24 -0400 Subject: [PATCH] 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 --- activerecord/CHANGELOG.md | 22 +++++++++++++++++++ .../lib/active_record/connection_handling.rb | 9 ++++---- .../connection_handlers_sharding_db_test.rb | 8 +++++++ .../test/cases/connection_pool_test.rb | 6 +++-- .../active_record_multiple_databases.md | 8 +++++-- 5 files changed, 44 insertions(+), 9 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 45c544392f..e30a0a174a 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -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* diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index b054d86abf..9cb733a267 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -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) diff --git a/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb b/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb index 8d4aee1e85..6df45a8312 100644 --- a/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb @@ -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" } } } diff --git a/activerecord/test/cases/connection_pool_test.rb b/activerecord/test/cases/connection_pool_test.rb index daf88b6b11..b6ce296bd0 100644 --- a/activerecord/test/cases/connection_pool_test.rb +++ b/activerecord/test/cases/connection_pool_test.rb @@ -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 diff --git a/guides/source/active_record_multiple_databases.md b/guides/source/active_record_multiple_databases.md index b04ab3b47d..3453e86cf1 100644 --- a/guides/source/active_record_multiple_databases.md +++ b/guides/source/active_record_multiple_databases.md @@ -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 ```