From 6b110d7528c0148016ec0a660b3c97dde7e508df Mon Sep 17 00:00:00 2001 From: eileencodes Date: Wed, 28 Oct 2020 18:07:04 -0400 Subject: [PATCH] Don't allow granular swapping for shards in legacy This is a separate commit because I want it to be easy to revert if we change our minds. After some discussion I think it is confusing that you could swap shard but not role granularly in legacy mode. This change forces users to always either have global swapping until moved off legacy mode. This will prevent a situation where `AnimalsBase` can change the shard granularly but the role globally. --- .../lib/active_record/connection_handling.rb | 16 +++++++----- activerecord/test/cases/base_test.rb | 4 +-- ...cy_connection_handlers_sharding_db_test.rb | 26 ++++++------------- 3 files changed, 20 insertions(+), 26 deletions(-) diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index a5e063a4d8..7a5ba005f0 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -135,17 +135,21 @@ module ActiveRecord # # The database kwarg is deprecated and will be removed in 6.2.0 without replacement. def connected_to(database: nil, role: nil, shard: nil, prevent_writes: false, &blk) - if shard && role && (!abstract_class? || self != Base) - raise NotImplementedError, "calling `connected_to` with `role` and `shard` is only allowed on ActiveRecord::Base or abstract classes" unless abstract_class? || self == Base - elsif legacy_connection_handling && self != Base - raise NotImplementedError, "`connected_to` can only be called on ActiveRecord::Base" - elsif database - ActiveSupport::Deprecation.warn("The database key in `connected_to` is deprecated. It will be removed in Rails 6.2.0 without replacement.") + if legacy_connection_handling + if self != Base + raise NotImplementedError, "`connected_to` can only be called on ActiveRecord::Base with legacy connection handling." + end + else + if self != Base && !abstract_class + raise NotImplementedError, "calling `connected_to` is only allowed on ActiveRecord::Base or abstract classes." + end end if database && (role || shard) raise ArgumentError, "`connected_to` cannot accept a `database` argument with any other arguments." elsif database + ActiveSupport::Deprecation.warn("The database key in `connected_to` is deprecated. It will be removed in Rails 6.2.0 without replacement.") + if database.is_a?(Hash) role, database = database.first role = role.to_sym diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index ac917b767e..603802bc9d 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1632,7 +1632,7 @@ class BasicsTest < ActiveRecord::TestCase Bird.connected_to(role: :reading) { } end - assert_equal "`connected_to` can only be called on ActiveRecord::Base", error.message + assert_equal "`connected_to` can only be called on ActiveRecord::Base with legacy connection handling.", error.message ensure clean_up_legacy_connection_handlers ActiveRecord::Base.legacy_connection_handling = old_value @@ -1643,7 +1643,7 @@ class BasicsTest < ActiveRecord::TestCase Bird.connected_to(role: :reading, shard: :default) { } end - assert_equal "calling `connected_to` with `role` and `shard` is only allowed on ActiveRecord::Base or abstract classes", error.message + assert_equal "calling `connected_to` is only allowed on ActiveRecord::Base or abstract classes.", error.message end test "can call connected_to with role and shard on abstract classes" do diff --git a/activerecord/test/cases/connection_adapters/legacy_connection_handlers_sharding_db_test.rb b/activerecord/test/cases/connection_adapters/legacy_connection_handlers_sharding_db_test.rb index 238c631d40..3dea31b4b5 100644 --- a/activerecord/test/cases/connection_adapters/legacy_connection_handlers_sharding_db_test.rb +++ b/activerecord/test/cases/connection_adapters/legacy_connection_handlers_sharding_db_test.rb @@ -308,27 +308,17 @@ module ActiveRecord assert_equal "primary_shard_one_replica", ActiveRecord::Base.connection_pool.db_config.name assert_equal "secondary_shard_one_replica", SecondaryBase.connection_pool.db_config.name - SecondaryBase.connected_to(role: :writing, shard: :default) do - # Note: Currently there is only granular shard swapping, not role swapping - # so this switches the shard only for SecondaryBase but role for both - assert_equal "primary_shard_one", ActiveRecord::Base.connection_pool.db_config.name + ActiveRecord::Base.connected_to(role: :writing, shard: :default) do + assert_equal "primary", ActiveRecord::Base.connection_pool.db_config.name assert_equal "secondary", SecondaryBase.connection_pool.db_config.name - ActiveRecord::Base.connected_to(role: :writing, shard: :default) do - assert_equal "primary", ActiveRecord::Base.connection_pool.db_config.name - assert_equal "secondary", SecondaryBase.connection_pool.db_config.name - end - - SecondaryBase.connected_to(role: :reading, shard: :shard_two) do - # granular switching means only SecondaryBase uses shard_two - assert_equal "primary_shard_one_replica", ActiveRecord::Base.connection_pool.db_config.name + ActiveRecord::Base.connected_to(role: :reading, shard: :shard_two) do + # ActiveRecord::Base has no shard_two assert_equal "secondary_shard_two_replica", SecondaryBase.connection_pool.db_config.name end ActiveRecord::Base.connected_to(role: :writing) do - # shard is inherited for AR::Base from outer most block :shard_one - assert_equal "primary_shard_one", ActiveRecord::Base.connection_pool.db_config.name - # shard is inherited for SecondaryBase by second block :default + assert_equal "primary", ActiveRecord::Base.connection_pool.db_config.name assert_equal "secondary", SecondaryBase.connection_pool.db_config.name end end @@ -559,13 +549,13 @@ module ActiveRecord shard_one_latch.count_down end - SecondaryBase.connected_to(shard: :one, role: :secondary) do + ActiveRecord::Base.connected_to(shard: :one, role: :secondary) do shard_default_latch.count_down assert_equal "shard_key_one_secondary", ShardConnectionTestModel.connection.select_value("SELECT shard_key from shard_connection_test_models") - assert_equal "shard_key_default_secondary_b", ShardConnectionTestModelB.connection.select_value("SELECT shard_key from shard_connection_test_models") + assert_equal "shard_key_one_secondary_b", ShardConnectionTestModelB.connection.select_value("SELECT shard_key from shard_connection_test_models") - SomeOtherBase.connected_to(shard: :one, role: :secondary) do + ActiveRecord::Base.connected_to(shard: :one, role: :secondary) do assert_equal "shard_key_one_secondary", ShardConnectionTestModel.connection.select_value("SELECT shard_key from shard_connection_test_models") assert_equal "shard_key_one_secondary_b", ShardConnectionTestModelB.connection.select_value("SELECT shard_key from shard_connection_test_models") end