kotovalexarian-likes-github
/
rails--rails
mirror of https://github.com/rails/rails.git
1
0
Fork 0
Code Releases Activity

Merge ConnectionSpecification + Role -> Role 

In order to move schema_cache off of DatabaseConfiguration we needed to
make role accessible on pool.

While looking at `ConnectionSpecification`, `Role`, and `DatabaseConfig` John
and I noticed that this could be achieved by merging `ConnectionSpecification`
and `Role` into one `Role` class. This allows us to eliminate the `spec`
concept which is confusing. `spec` is a private method so renaming
to `resolve_role` is ok.

In the `Role` class we took `name` (renamed to `connection_specification_name`
for clarity since it's not a `role` name) and `db_config` from
`ConnectionSpecification` and the `pool` methods from `Role` and combined
them into one `Role` class.

This feels a lot cleaner to us because it clarifies the purposes of the
classes/methods/variables, and makes it easier to drop
`connection_specification_name` keyed on the parent class in the future.

There are a lot of changes in here but the majority of them are find and
replace `spec` -> `role`, `spec.name` ->
`role.connection_specification_name`, `Resolver#spec` ->
`Resolver#resolve_role`.

This PR also moves the `schema_cache` from `DatabaseConfig` to the new
combined `Role` class.

Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
This commit is contained in:
eileencodes 2019-10-16 13:26:43 -04:00
parent b305f0e206
commit eeacc03454
13 changed files with 165 additions and 170 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
activerecord/lib/active_record/connection_adapters.rb
View File

@ -9,7 +9,7 @@ module ActiveRecord
end
autoload :Column
autoload :ConnectionSpecification
autoload :Role
autoload :Resolver
autoload_at "active_record/connection_adapters/abstract/schema_definitions" do

79
activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
View File

@ -370,20 +370,21 @@ module ActiveRecord
include ConnectionAdapters::AbstractPool
attr_accessor :automatic_reconnect, :checkout_timeout
attr_reader :db_config, :size, :reaper
attr_reader :db_config, :size, :reaper, :role
delegate :schema_cache, :schema_cache=, to: :db_config
delegate :schema_cache, :schema_cache=, to: :role
# Creates a new ConnectionPool object. +spec+ is a ConnectionSpecification
# Creates a new ConnectionPool object. +role+ is a Role
# object which describes database connection information (e.g. adapter,
# host name, username, password, etc), as well as the maximum size for
# this ConnectionPool.
#
# The default ConnectionPool maximum size is 5.
def initialize(db_config)
def initialize(role)
super()
@db_config = db_config
@role = role
@db_config = role.db_config
@checkout_timeout = db_config.checkout_timeout
@idle_timeout = db_config.idle_timeout
@ -950,60 +951,6 @@ module ActiveRecord
end
end
class Role # :nodoc:
include Mutex_m
attr_reader :db_config
INSTANCES = ObjectSpace::WeakMap.new
private_constant :INSTANCES
class << self
def discard_pools!
INSTANCES.each_key(&:discard_pool!)
end
end
def initialize(db_config)
super()
@db_config = db_config
@pool = nil
INSTANCES[self] = self
end
def disconnect!
ActiveSupport::ForkTracker.check!
return unless @pool
synchronize do
return unless @pool
@pool.automatic_reconnect = false
@pool.disconnect!
end
nil
end
def pool
ActiveSupport::ForkTracker.check!
@pool || synchronize { @pool ||= ConnectionAdapters::ConnectionPool.new(db_config) }
end
def discard_pool!
return unless @pool
synchronize do
return unless @pool
@pool.discard!
@pool = nil
end
end
end
# ConnectionHandler is a collection of ConnectionPool objects. It is used
# for keeping separate connection pools that connect to different databases.
#
@ -1056,7 +1003,7 @@ module ActiveRecord
private_constant :FINALIZER
def initialize
# These caches are keyed by spec.name (ConnectionSpecification#name).
# These caches are keyed by role.connection_specification_name (Role#connection_specification_name).
@owner_to_role = Concurrent::Map.new(initial_capacity: 2)
# Backup finalizer: if the forked child skipped Kernel#fork the early discard has not occurred
@ -1094,21 +1041,21 @@ module ActiveRecord
def establish_connection(config)
resolver = Resolver.new(Base.configurations)
spec = resolver.spec(config)
db_config = spec.db_config
role = resolver.resolve_role(config)
db_config = role.db_config
remove_connection(spec.name)
remove_connection(role.connection_specification_name)
message_bus = ActiveSupport::Notifications.instrumenter
payload = {
connection_id: object_id
}
if spec
payload[:spec_name] = spec.name
if role
payload[:spec_name] = role.connection_specification_name
payload[:config] = db_config.configuration_hash
end
role = owner_to_role[spec.name] = Role.new(db_config)
role = owner_to_role[role.connection_specification_name] = Role.new(role.connection_specification_name, db_config)
message_bus.instrument("!connection.active_record", payload) do
role.pool

15
activerecord/lib/active_record/connection_adapters/connection_specification.rb
View File

@ -1,15 +0,0 @@
# frozen_string_literal: true
require "uri"
module ActiveRecord
module ConnectionAdapters
class ConnectionSpecification # :nodoc:
attr_reader :name, :db_config
def initialize(name, db_config)
@name, @db_config = name, db_config
end
end
end
end

12
activerecord/lib/active_record/connection_adapters/resolver.rb
View File

@ -2,7 +2,7 @@
module ActiveRecord
module ConnectionAdapters
# Builds a ConnectionSpecification from user input.
# Builds a Role from user input.
class Resolver # :nodoc:
attr_reader :configurations
@ -11,17 +11,17 @@ module ActiveRecord
@configurations = configurations
end
# Returns an instance of ConnectionSpecification for a given adapter.
# Returns an instance of Role for a given adapter.
# Accepts a hash one layer deep that contains all connection information.
#
# == Example
#
# config = { "production" => { "host" => "localhost", "database" => "foo", "adapter" => "sqlite3" } }
# spec = Resolver.new(config).spec(:production)
# spec.db_config.configuration_hash
# role = Resolver.new(config).resolve_role(:production)
# role.db_config.configuration_hash
# # => { host: "localhost", database: "foo", adapter: "sqlite3" }
#
def spec(config)
def resolve_role(config)
pool_name = config if config.is_a?(Symbol)
db_config = resolve(config, pool_name)
@ -53,7 +53,7 @@ module ActiveRecord
raise AdapterNotFound, "database configuration specifies nonexistent #{db_config.adapter} adapter"
end
ConnectionSpecification.new(db_config.configuration_hash.delete(:name) || "primary", db_config)
Role.new(db_config.configuration_hash.delete(:name) || "primary", db_config)
end
# Returns fully resolved connection, accepts hash, string or symbol.

63
activerecord/lib/active_record/connection_adapters/role.rb Normal file
View File

@ -0,0 +1,63 @@
# frozen_string_literal: true
module ActiveRecord
module ConnectionAdapters
class Role # :nodoc:
include Mutex_m
attr_reader :db_config, :connection_specification_name
attr_accessor :schema_cache
INSTANCES = ObjectSpace::WeakMap.new
private_constant :INSTANCES
class << self
def discard_pools!
INSTANCES.each_key(&:discard_pool!)
end
end
def initialize(connection_specification_name, db_config)
super()
@connection_specification_name = connection_specification_name
@db_config = db_config
@pool = nil
INSTANCES[self] = self
end
def disconnect!
ActiveSupport::ForkTracker.check!
return unless @pool
synchronize do
return unless @pool
@pool.automatic_reconnect = false
@pool.disconnect!
end
nil
end
def pool
ActiveSupport::ForkTracker.check!
@pool || synchronize { @pool ||= ConnectionAdapters::ConnectionPool.new(self) }
end
def discard_pool!
return unless @pool
synchronize do
return unless @pool
@pool.discard!
@pool = nil
end
end
end
end
end
ActiveSupport::ForkTracker.after_fork { ActiveRecord::ConnectionAdapters::Role.discard_pools! }

4
activerecord/lib/active_record/database_configurations/database_config.rb
View File

@ -8,8 +8,6 @@ module ActiveRecord
class DatabaseConfig # :nodoc:
attr_reader :env_name, :spec_name
attr_accessor :schema_cache
def initialize(env_name, spec_name)
@env_name = env_name
@spec_name = spec_name
@ -61,5 +59,3 @@ module ActiveRecord
end
end
end
ActiveSupport::ForkTracker.after_fork { ActiveRecord::ConnectionAdapters::Role.discard_pools! }

3
activerecord/test/cases/connection_adapters/adapter_leasing_test.rb
View File

@ -40,7 +40,8 @@ module ActiveRecord
def test_close
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new("test", "primary", {})
pool = Pool.new(db_config)
role = ActiveRecord::ConnectionAdapters::Role.new("primary", db_config)
pool = Pool.new(role)
pool.insert_connection_for_test! @adapter
@adapter.pool = pool

29
activerecord/test/cases/connection_pool_test.rb
View File

@ -12,7 +12,9 @@ module ActiveRecord
super
# Keep a duplicate pool so we do not bother others
@pool = ConnectionPool.new(ActiveRecord::Base.connection_pool.db_config)
@db_config = ActiveRecord::Base.connection_pool.db_config
@role = ActiveRecord::ConnectionAdapters::Role.new("primary", @db_config)
@pool = ConnectionPool.new(@role)
if in_memory_db?
# Separate connections to an in-memory database create an entirely new database,
@ -198,9 +200,9 @@ module ActiveRecord
def test_idle_timeout_configuration
@pool.disconnect!
db_config = ActiveRecord::Base.connection_pool.db_config
db_config.configuration_hash.merge!(idle_timeout: "0.02")
@pool = ConnectionPool.new(db_config)
@db_config.configuration_hash.merge!(idle_timeout: "0.02")
role = ActiveRecord::ConnectionAdapters::Role.new("primary", @db_config)
@pool = ConnectionPool.new(role)
idle_conn = @pool.checkout
@pool.checkin(idle_conn)
@ -223,9 +225,9 @@ module ActiveRecord
def test_disable_flush
@pool.disconnect!
db_config = ActiveRecord::Base.connection_pool.db_config
db_config.configuration_hash.merge!(idle_timeout: -5)
@pool = ConnectionPool.new(db_config)
@db_config.configuration_hash.merge!(idle_timeout: -5)
role = ActiveRecord::ConnectionAdapters::Role.new("primary", @db_config)
@pool = ConnectionPool.new(role)
idle_conn = @pool.checkout
@pool.checkin(idle_conn)
@ -315,7 +317,7 @@ module ActiveRecord
end
def test_checkout_behaviour
pool = ConnectionPool.new(ActiveRecord::Base.connection_pool.db_config)
pool = ConnectionPool.new(@role)
main_connection = pool.connection
assert_not_nil main_connection
threads = []
@ -448,7 +450,7 @@ module ActiveRecord
end
def test_automatic_reconnect_restores_after_disconnect
pool = ConnectionPool.new(ActiveRecord::Base.connection_pool.db_config)
pool = ConnectionPool.new(@role)
assert pool.automatic_reconnect
assert pool.connection
@ -457,7 +459,7 @@ module ActiveRecord
end
def test_automatic_reconnect_can_be_disabled
pool = ConnectionPool.new(ActiveRecord::Base.connection_pool.db_config)
pool = ConnectionPool.new(@role)
pool.disconnect!
pool.automatic_reconnect = false
@ -718,12 +720,13 @@ module ActiveRecord
private
def with_single_connection_pool
old_config = ActiveRecord::Base.connection_pool.db_config.configuration_hash
old_config = @db_config.configuration_hash
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new("arunit", "primary", old_config.dup)
db_config.configuration_hash[:pool] = 1 # this is safe to do, because .dupped ConnectionSpecification also auto-dups its config
db_config.configuration_hash[:pool] = 1 # this is safe to do, because .dupped Role also auto-dups its config
yield(pool = ConnectionPool.new(db_config))
role = ActiveRecord::ConnectionAdapters::Role.new("primary", db_config)
yield(pool = ConnectionPool.new(role))
ensure
pool.disconnect! if pool
end

82
activerecord/test/cases/connection_specification/resolver_test.rb
View File

@ -4,23 +4,23 @@ require "cases/helper"
module ActiveRecord
module ConnectionAdapters
class ConnectionSpecification
class Role
class ResolverTest < ActiveRecord::TestCase
def resolve(spec, config = {})
def resolve(role, config = {})
configs = ActiveRecord::DatabaseConfigurations.new(config)
resolver = ConnectionAdapters::Resolver.new(configs)
resolver.resolve(spec, spec).configuration_hash
resolver.resolve(role, role).configuration_hash
end
def spec(spec, config = {})
def resolve_role(role, config = {})
configs = ActiveRecord::DatabaseConfigurations.new(config)
resolver = ConnectionAdapters::Resolver.new(configs)
resolver.spec(spec)
resolver.resolve_role(role)
end
def test_url_invalid_adapter
error = assert_raises(LoadError) do
spec "ridiculous://foo?encoding=utf8"
resolve_role "ridiculous://foo?encoding=utf8"
end
assert_match "Could not load the 'ridiculous' Active Record adapter. Ensure that the adapter is spelled correctly in config/database.yml and that you've added the necessary adapter gem to your Gemfile.", error.message
@ -28,7 +28,7 @@ module ActiveRecord
def test_error_if_no_adapter_method
error = assert_raises(AdapterNotFound) do
spec "abstract://foo?encoding=utf8"
resolve_role "abstract://foo?encoding=utf8"
end
assert_match "database configuration specifies nonexistent abstract adapter", error.message
@ -38,121 +38,121 @@ module ActiveRecord
# checks that the adapter file can be required in.
def test_url_from_environment
spec = resolve :production, "production" => "abstract://foo?encoding=utf8"
role = resolve :production, "production" => "abstract://foo?encoding=utf8"
assert_equal({
adapter: "abstract",
host: "foo",
encoding: "utf8",
name: "production"
}, spec)
}, role)
end
def test_url_sub_key
spec = resolve :production, "production" => { "url" => "abstract://foo?encoding=utf8" }
role = resolve :production, "production" => { "url" => "abstract://foo?encoding=utf8" }
assert_equal({
adapter: "abstract",
host: "foo",
encoding: "utf8",
name: "production"
}, spec)
}, role)
end
def test_url_sub_key_merges_correctly
hash = { "url" => "abstract://foo?encoding=utf8&", "adapter" => "sqlite3", "host" => "bar", "pool" => "3" }
spec = resolve :production, "production" => hash
role = resolve :production, "production" => hash
assert_equal({
adapter: "abstract",
host: "foo",
encoding: "utf8",
pool: "3",
name: "production"
}, spec)
}, role)
end
def test_url_host_no_db
spec = resolve "abstract://foo?encoding=utf8"
role = resolve "abstract://foo?encoding=utf8"
assert_equal({
adapter: "abstract",
host: "foo",
encoding: "utf8"
}, spec)
}, role)
end
def test_url_missing_scheme
spec = resolve "foo"
assert_equal({ database: "foo" }, spec)
role = resolve "foo"
assert_equal({ database: "foo" }, role)
end
def test_url_host_db
spec = resolve "abstract://foo/bar?encoding=utf8"
role = resolve "abstract://foo/bar?encoding=utf8"
assert_equal({
adapter: "abstract",
database: "bar",
host: "foo",
encoding: "utf8"
}, spec)
}, role)
end
def test_url_port
spec = resolve "abstract://foo:123?encoding=utf8"
role = resolve "abstract://foo:123?encoding=utf8"
assert_equal({
adapter: "abstract",
port: 123,
host: "foo",
encoding: "utf8"
}, spec)
}, role)
end
def test_encoded_password
password = "am@z1ng_p@ssw0rd#!"
encoded_password = URI.encode_www_form_component(password)
spec = resolve "abstract://foo:#{encoded_password}@localhost/bar"
assert_equal password, spec[:password]
role = resolve "abstract://foo:#{encoded_password}@localhost/bar"
assert_equal password, role[:password]
end
def test_url_with_authority_for_sqlite3
spec = resolve "sqlite3:///foo_test"
assert_equal("/foo_test", spec[:database])
role = resolve "sqlite3:///foo_test"
assert_equal("/foo_test", role[:database])
end
def test_url_absolute_path_for_sqlite3
spec = resolve "sqlite3:/foo_test"
assert_equal("/foo_test", spec[:database])
role = resolve "sqlite3:/foo_test"
assert_equal("/foo_test", role[:database])
end
def test_url_relative_path_for_sqlite3
spec = resolve "sqlite3:foo_test"
assert_equal("foo_test", spec[:database])
role = resolve "sqlite3:foo_test"
assert_equal("foo_test", role[:database])
end
def test_url_memory_db_for_sqlite3
spec = resolve "sqlite3::memory:"
assert_equal(":memory:", spec[:database])
role = resolve "sqlite3::memory:"
assert_equal(":memory:", role[:database])
end
def test_url_sub_key_for_sqlite3
spec = resolve :production, "production" => { "url" => "sqlite3:foo?encoding=utf8" }
role = resolve :production, "production" => { "url" => "sqlite3:foo?encoding=utf8" }
assert_equal({
adapter: "sqlite3",
database: "foo",
encoding: "utf8",
name: "production"
}, spec)
}, role)
end
def test_spec_name_on_key_lookup
spec = spec(:readonly, "readonly" => { "adapter" => "sqlite3" })
assert_equal "readonly", spec.name
def test_role_connection_specification_name_on_key_lookup
role = resolve_role(:readonly, "readonly" => { "adapter" => "sqlite3" })
assert_equal "readonly", role.connection_specification_name
end
def test_spec_name_with_inline_config
spec = spec("adapter" => "sqlite3")
assert_equal "primary", spec.name, "should default to primary id"
def test_role_connection_specification_name_with_inline_config
role = resolve_role("adapter" => "sqlite3")
assert_equal "primary", role.connection_specification_name, "should default to primary id"
end
def test_spec_with_invalid_type
def test_role_with_invalid_type
assert_raises TypeError do
spec(Object.new)
resolve_role(Object.new)
end
end
end

4
activerecord/test/cases/disconnected_test.rb
View File

@ -14,8 +14,8 @@ class TestDisconnectedAdapter < ActiveRecord::TestCase
teardown do
return if in_memory_db?
spec = ActiveRecord::Base.connection_config
ActiveRecord::Base.establish_connection(spec)
role = ActiveRecord::Base.connection_config
ActiveRecord::Base.establish_connection(role)
end
unless in_memory_db?

2
activerecord/test/cases/fixtures_test.rb
View File

@ -1415,7 +1415,7 @@ class MultipleDatabaseFixturesTest < ActiveRecord::TestCase
private
def with_temporary_connection_pool
role = ActiveRecord::Base.connection_handler.send(:owner_to_role).fetch("primary")
new_pool = ActiveRecord::ConnectionAdapters::ConnectionPool.new(role.db_config)
new_pool = ActiveRecord::ConnectionAdapters::ConnectionPool.new(role)
role.stub(:pool, new_pool) do
yield

2
activerecord/test/cases/query_cache_test.rb
View File

@ -556,7 +556,7 @@ class QueryCacheTest < ActiveRecord::TestCase
private
def with_temporary_connection_pool
role = ActiveRecord::Base.connection_handler.send(:owner_to_role).fetch("primary")
new_pool = ActiveRecord::ConnectionAdapters::ConnectionPool.new(role.db_config)
new_pool = ActiveRecord::ConnectionAdapters::ConnectionPool.new(role)
role.stub(:pool, new_pool) do
yield

38
activerecord/test/cases/reaper_test.rb
View File

@ -59,8 +59,8 @@ module ActiveRecord
def test_pool_has_reaper
config = ActiveRecord::Base.configurations.configs_for(env_name: "arunit", spec_name: "primary")
spec = ConnectionSpecification.new("primary", config)
pool = ConnectionPool.new(spec.db_config)
role = Role.new("primary", config)
pool = ConnectionPool.new(role)
assert pool.reaper
ensure
@ -68,10 +68,10 @@ module ActiveRecord
end
def test_reaping_frequency_configuration
spec = duplicated_spec
spec.db_config.configuration_hash[:reaping_frequency] = "10.01"
role = duplicated_role
role.db_config.configuration_hash[:reaping_frequency] = "10.01"
pool = ConnectionPool.new(spec.db_config)
pool = ConnectionPool.new(role)
assert_equal 10.01, pool.reaper.frequency
ensure
@ -79,10 +79,10 @@ module ActiveRecord
end
def test_connection_pool_starts_reaper
spec = duplicated_spec
spec.db_config.configuration_hash[:reaping_frequency] = "0.0001"
role = duplicated_role
role.db_config.configuration_hash[:reaping_frequency] = "0.0001"
pool = ConnectionPool.new(spec.db_config)
pool = ConnectionPool.new(role)
conn, child = new_conn_in_thread(pool)
@ -97,11 +97,11 @@ module ActiveRecord
end
def test_reaper_works_after_pool_discard
spec = duplicated_spec
spec.db_config.configuration_hash[:reaping_frequency] = "0.0001"
role = duplicated_role
role.db_config.configuration_hash[:reaping_frequency] = "0.0001"
2.times do
pool = ConnectionPool.new(spec.db_config)
pool = ConnectionPool.new(role)
conn, child = new_conn_in_thread(pool)
@ -119,8 +119,8 @@ module ActiveRecord
# This doesn't test the reaper directly, but we want to test the action
# it would take on a discarded pool
def test_reap_flush_on_discarded_pool
spec = duplicated_spec
pool = ConnectionPool.new(spec.db_config)
role = duplicated_role
pool = ConnectionPool.new(role)
pool.discard!
pool.reap
@ -128,14 +128,14 @@ module ActiveRecord
end
def test_connection_pool_starts_reaper_in_fork
spec = duplicated_spec
spec.db_config.configuration_hash[:reaping_frequency] = "0.0001"
role = duplicated_role
role.db_config.configuration_hash[:reaping_frequency] = "0.0001"
pool = ConnectionPool.new(spec.db_config)
pool = ConnectionPool.new(role)
pool.checkout
pid = fork do
pool = ConnectionPool.new(spec.db_config)
pool = ConnectionPool.new(role)
conn, child = new_conn_in_thread(pool)
child.terminate
@ -173,10 +173,10 @@ module ActiveRecord
end
private
def duplicated_spec
def duplicated_role
old_config = ActiveRecord::Base.connection_pool.db_config.configuration_hash
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new("arunit", "primary", old_config.dup)
ConnectionSpecification.new("primary", db_config)
Role.new("primary", db_config)
end
def new_conn_in_thread(pool)