Skip to content

Commit

Permalink
Fix nil pool_config in legacy connection handling
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
eileencodes committed Jun 23, 2021
1 parent e4b1e97 commit 3f3ec51
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 2 deletions.
Expand Up @@ -23,8 +23,12 @@ def get_pool_config(_, shard)
@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
Expand Down
Expand Up @@ -99,6 +99,40 @@ def test_loading_relations_with_multi_db_connection_handlers
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"

Expand Down

0 comments on commit 3f3ec51

Please sign in to comment.