Skip to content

Commit d2761cf

Browse files
committed
Raise an error if pool_config is nil in set_pool_config
In rails#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. Note: In the test I made a new connection handler so that we have isolated pools to test against. Otherwise we'll be testing against the existing pools but we don't want to mess those up. Closes rails#41549
1 parent a787d6c commit d2761cf

File tree

2 files changed

+37
-1
lines changed

2 files changed

+37
-1
lines changed

activerecord/lib/active_record/connection_adapters/pool_manager.rb

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,11 @@ def get_pool_config(role, shard)
3636
end
3737

3838
def set_pool_config(role, shard, pool_config)
39-
@name_to_role_mapping[role][shard] = pool_config
39+
if pool_config
40+
@name_to_role_mapping[role][shard] = pool_config
41+
else
42+
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."
43+
end
4044
end
4145
end
4246
end

activerecord/test/cases/connection_adapters/connection_swapping_nested_test.rb

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,38 @@ class ApplicationRecord < ActiveRecord::Base
434434
self.abstract_class = true
435435
end
436436

437+
def test_using_a_role_other_than_writing_raises
438+
connection_handler = ActiveRecord::Base.connection_handler
439+
ActiveRecord::Base.connection_handler = ActiveRecord::ConnectionAdapters::ConnectionHandler.new
440+
Object.const_set(:ApplicationRecord, ApplicationRecord)
441+
442+
ApplicationRecord.connects_to(shards: { default: { all: :arunit }, one: { all: :arunit } })
443+
444+
assert_raises(ArgumentError) { setup_shared_connection_pool }
445+
ensure
446+
ActiveRecord.application_record_class = nil
447+
Object.send(:remove_const, :ApplicationRecord)
448+
ActiveRecord::Base.connection_handler = connection_handler
449+
ActiveRecord::Base.establish_connection :arunit
450+
end
451+
452+
def test_setting_writing_role_is_used_over_writing
453+
connection_handler = ActiveRecord::Base.connection_handler
454+
ActiveRecord::Base.connection_handler = ActiveRecord::ConnectionAdapters::ConnectionHandler.new
455+
old_role, ActiveRecord.writing_role = ActiveRecord.writing_role, :all
456+
Object.const_set(:ApplicationRecord, ApplicationRecord)
457+
458+
ApplicationRecord.connects_to(shards: { default: { all: :arunit }, one: { all: :arunit } })
459+
460+
assert_nothing_raised { setup_shared_connection_pool }
461+
ensure
462+
ActiveRecord.writing_role = old_role
463+
ActiveRecord.application_record_class = nil
464+
Object.send(:remove_const, :ApplicationRecord)
465+
ActiveRecord::Base.connection_handler = connection_handler
466+
ActiveRecord::Base.establish_connection :arunit
467+
end
468+
437469
def test_application_record_prevent_writes_can_be_changed
438470
Object.const_set(:ApplicationRecord, ApplicationRecord)
439471

0 commit comments

Comments
 (0)