From 3f3ec51e0d409146a655bb02c09e80d1843879d0 Mon Sep 17 00:00:00 2001
From: eileencodes <eileencodes@gmail.com>
Date: Wed, 23 Jun 2021 10:29:03 -0400
Subject: [PATCH] Fix nil pool_config in legacy connection handling

This is a followup to #42537 because that PR only fixed the bug for the
new connection handling but not legacy connection handling.

In #41549 a user was getting an error that the `pool_config` was `nil`
so the `all_connection_pools` method would raise an error. After getting
a reproduction application I saw that if the application is
misconfigured this can happen. For example, if an application uses
`:all` for the writing role but does not set
`config.active_record.writing_role = :all` then the
`setup_shared_connection_pool` pool method will get a `nil` value for
the `writing_pool_config` and set that in `set_pool_config`. I
considered fixing this in `setup_shared_connection_pool` directly and
raising an error there, but there's a possibility this _can_ happen in
an external gem or application code if they're using these private APIs
and realistically we never want any code, Rails or otherwise, to be able
to set a `nil` pool config in the pools.
---
 .../legacy_pool_manager.rb                    |  8 +++--
 ...egacy_connection_handlers_multi_db_test.rb | 34 +++++++++++++++++++
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/activerecord/lib/active_record/connection_adapters/legacy_pool_manager.rb b/activerecord/lib/active_record/connection_adapters/legacy_pool_manager.rb
index eaa879e4a7..c6e216e864 100644
--- a/activerecord/lib/active_record/connection_adapters/legacy_pool_manager.rb
+++ b/activerecord/lib/active_record/connection_adapters/legacy_pool_manager.rb
@@ -23,8 +23,12 @@ module ActiveRecord
         @name_to_pool_config[shard]
       end
 
-      def set_pool_config(_, shard, pool_config)
-        @name_to_pool_config[shard] = pool_config
+      def set_pool_config(role, shard, pool_config)
+        if pool_config
+          @name_to_pool_config[shard] = pool_config
+        else
+          raise ArgumentError, "The `pool_config` for the :#{role} role and :#{shard} shard was `nil`. Please check your configuration. If you want your writing role to be something other than `:writing` set `config.active_record.writing_role` in your application configuration. The same setting should be applied for the `reading_role` if applicable."
+        end
       end
     end
   end
diff --git a/activerecord/test/cases/connection_adapters/legacy_connection_handlers_multi_db_test.rb b/activerecord/test/cases/connection_adapters/legacy_connection_handlers_multi_db_test.rb
index 8ffe1570a4..e0d21c285d 100644
--- a/activerecord/test/cases/connection_adapters/legacy_connection_handlers_multi_db_test.rb
+++ b/activerecord/test/cases/connection_adapters/legacy_connection_handlers_multi_db_test.rb
@@ -99,6 +99,40 @@ module ActiveRecord
       end
 
       unless in_memory_db?
+        def test_not_setting_writing_role_while_using_another_named_role_raises
+          old_handler = ActiveRecord::Base.connection_handler
+          assert_deprecated do
+            ActiveRecord::Base.connection_handlers = { writing: ConnectionHandler.new }
+          end
+          ActiveRecord::Base.connection_handler = ActiveRecord::Base.connection_handlers[:writing]
+          ActiveRecord::Base.establish_connection :arunit
+
+          ActiveRecord::Base.connects_to(shards: { default: { all: :arunit }, one: { all: :arunit } })
+
+          assert_raises(ArgumentError) { setup_shared_connection_pool }
+        ensure
+          ActiveRecord::Base.connection_handler = old_handler
+          ActiveRecord::Base.establish_connection :arunit
+        end
+
+        def test_setting_writing_role_while_using_another_named_role_does_not_raise
+          old_role, ActiveRecord.writing_role = ActiveRecord.writing_role, :all
+          old_handler = ActiveRecord::Base.connection_handler
+          assert_deprecated do
+            ActiveRecord::Base.connection_handlers = { all: ConnectionHandler.new }
+          end
+          ActiveRecord::Base.connection_handler = ActiveRecord::Base.connection_handlers[:all]
+          ActiveRecord::Base.establish_connection :arunit
+
+          ActiveRecord::Base.connects_to(shards: { default: { all: :arunit }, one: { all: :arunit } })
+
+          assert_nothing_raised { setup_shared_connection_pool }
+        ensure
+          ActiveRecord.writing_role = old_role
+          ActiveRecord::Base.connection_handler = old_handler
+          ActiveRecord::Base.establish_connection :arunit
+        end
+
         def test_establish_connection_using_3_levels_config
           previous_env, ENV["RAILS_ENV"] = ENV["RAILS_ENV"], "default_env"