Make `role` required when using `shard` in `connected_to`

While working on some more indepth changes to Rails internal connection
management we noticed that it's confusing in some cases that the `role`
is implcit when using a `shard`. For example, if you passed a `shard`
and not a `role` in an un-nested block the default `role` would be
`writing`.

```
ActiveRecord::Base.connected_to(shard: :one) do
  # connected to writing
end
```

However in cases where nesting is used it could be confusing to
application authors that the role is inherited:

```
ActiveRecord::Base.connected_to(role: :reading) do
  ActiveRecord::Base.connected_to(shard: :one) do
    # will read from shard one replica, not write to primary
  end
end
```

Since this could be potentially confusing, and extremely hard to track
in complex applications, the best approach is to require `role` when
using `shard` which is what this PR does.

Note: the code for this method is...getting unweildy. Once the
`database` argument is fully deprecated we can remove most of the guards
and make `role` required by removing `nil` from the keyword argument.
Until then we need to support required arguments in this round about way.

activerecord/lib/active_record/connection_handling.rb

@@ -160,7 +160,11 @@ def connected_to(database: nil, role: nil, shard: nil, prevent_writes: false, &b
 
         with_handler(role, &blk)
       elsif shard
-        with_shard(shard, role || current_role, prevent_writes, &blk)
+        unless role
+          raise ArgumentError, "`connected_to` cannot accept a `shard` argument without a `role`."
+        end
+
+        with_shard(shard, role, prevent_writes, &blk)
       elsif role
         with_role(role, prevent_writes, &blk)
       else

activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb

@@ -26,7 +26,7 @@ def teardown
 
       unless in_memory_db?
         def test_establishing_a_connection_in_connected_to_block_uses_current_role_and_shard
-          ActiveRecord::Base.connected_to(shard: :shard_one) do
+          ActiveRecord::Base.connected_to(role: :writing, shard: :shard_one) do
             db_config = ActiveRecord::Base.configurations.configs_for(env_name: "arunit", name: "primary")
             ActiveRecord::Base.establish_connection(db_config)
             assert_nothing_raised { Person.first }
@@ -213,11 +213,6 @@ def test_retrieves_proper_connection_with_nested_connected_to
               assert_equal "primary_replica", ActiveRecord::Base.connection_pool.db_config.name
             end
 
-            # Uses the current role
-            ActiveRecord::Base.connected_to(shard: :default) do
-              assert_equal "primary_replica", ActiveRecord::Base.connection_pool.db_config.name
-            end
-
             # Resets correctly
             assert_equal "primary_shard_one_replica", ActiveRecord::Base.connection_pool.db_config.name
           end
@@ -282,7 +277,7 @@ 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" } } }
 
-        ActiveRecord::Base.connected_to(shard: :one) do
+        ActiveRecord::Base.connected_to(role: :writing, shard: :one) do
           ShardConnectionTestModel.connection.execute("CREATE TABLE `shard_connection_test_models` (shard_key VARCHAR (255))")
           ShardConnectionTestModel.create!(shard_key: "test_model_default")
 
@@ -301,7 +296,7 @@ def test_sharding_separation
         }
 
         [:default, :one].each do |shard_name|
-          ActiveRecord::Base.connected_to(shard: shard_name) do
+          ActiveRecord::Base.connected_to(role: :writing, shard: shard_name) do
             ShardConnectionTestModel.connection.execute("CREATE TABLE `shard_connection_test_models` (shard_key VARCHAR (255))")
           end
         end
@@ -310,13 +305,13 @@ def test_sharding_separation
         ShardConnectionTestModel.create!(shard_key: "foo")
 
         # Make sure we can read it when explicitly connecting to :default
-        ActiveRecord::Base.connected_to(shard: :default) do
+        ActiveRecord::Base.connected_to(role: :writing, shard: :default) do
           assert ShardConnectionTestModel.find_by_shard_key("foo")
         end
 
         # Switch to shard and make sure we can't read the record from :default
         # Also add a new record on :one
-        ActiveRecord::Base.connected_to(shard: :one) do
+        ActiveRecord::Base.connected_to(role: :writing, shard: :one) do
           assert_not ShardConnectionTestModel.find_by_shard_key("foo")
           ShardConnectionTestModel.create!(shard_key: "bar")
         end
@@ -337,7 +332,7 @@ def test_swapping_shards_in_a_multi_threaded_environment
         }
 
         [:default, :one].each do |shard_name|
-          ActiveRecord::Base.connected_to(shard: shard_name) do
+          ActiveRecord::Base.connected_to(role: :writing, shard: shard_name) do
             ShardConnectionTestModel.connection.execute("CREATE TABLE `shard_connection_test_models` (shard_key VARCHAR (255))")
             ShardConnectionTestModel.connection.execute("INSERT INTO `shard_connection_test_models` VALUES ('shard_key_#{shard_name}')")
           end
@@ -356,7 +351,7 @@ def test_swapping_shards_in_a_multi_threaded_environment
           shard_one_latch.count_down
         end
 
-        ActiveRecord::Base.connected_to(shard: :one) do
+        ActiveRecord::Base.connected_to(role: :writing, shard: :one) do
           shard_default_latch.count_down
           assert_equal "shard_key_one", ShardConnectionTestModel.connection.select_value("SELECT shard_key from shard_connection_test_models")
           shard_one_latch.wait
@@ -382,12 +377,12 @@ def test_swapping_shards_and_roles_in_a_multi_threaded_environment
         }
 
         [:default, :one].each do |shard_name|
-          ActiveRecord::Base.connected_to(shard: shard_name) do
+          ActiveRecord::Base.connected_to(role: :writing, shard: shard_name) do
             ShardConnectionTestModel.connection.execute("CREATE TABLE `shard_connection_test_models` (shard_key VARCHAR (255))")
             ShardConnectionTestModel.connection.execute("INSERT INTO `shard_connection_test_models` VALUES ('shard_key_#{shard_name}')")
           end
 
-          ActiveRecord::Base.connected_to(shard: shard_name, role: :secondary) do
+          ActiveRecord::Base.connected_to(role: :secondary, shard: shard_name) do
             ShardConnectionTestModel.connection.execute("CREATE TABLE `shard_connection_test_models` (shard_key VARCHAR (255))")
             ShardConnectionTestModel.connection.execute("INSERT INTO `shard_connection_test_models` VALUES ('shard_key_#{shard_name}_secondary')")
           end

guides/source/active_record_multiple_databases.md

@@ -349,14 +349,15 @@ class ApplicationRecord < ActiveRecord::Base
 end
 ```
 
-Then models can swap connections manually via the `connected_to` API:
+Then models can swap connections manually via the `connected_to` API. If
+using sharding both a `role` and `shard` must be passed:
 
 ```ruby
-ActiveRecord::Base.connected_to(shard: :default) do
+ActiveRecord::Base.connected_to(role: :writing, shard: :default) do
   @id = Record.create! # creates a record in shard one
 end
 
-ActiveRecord::Base.connected_to(shard: :shard_one) do
+ActiveRecord::Base.connected_to(role: :writing, shard: :shard_one) do
   Record.find(@id) # can't find record, doesn't exist
 end
 ```