Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix preventing writes for ApplicationRecord #41030

Merged
merged 1 commit into from Jan 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -24,7 +24,7 @@ class NullPool # :nodoc:

attr_accessor :schema_cache

def owner_name
def connection_klass
nil
end
end
Expand Down Expand Up @@ -360,7 +360,7 @@ def run
include ConnectionAdapters::AbstractPool

attr_accessor :automatic_reconnect, :checkout_timeout
attr_reader :db_config, :size, :reaper, :pool_config, :owner_name
attr_reader :db_config, :size, :reaper, :pool_config, :connection_klass

delegate :schema_cache, :schema_cache=, to: :pool_config

Expand All @@ -375,7 +375,7 @@ def initialize(pool_config)

@pool_config = pool_config
@db_config = pool_config.db_config
@owner_name = pool_config.connection_specification_name
@connection_klass = pool_config.connection_klass

@checkout_timeout = db_config.checkout_timeout
@idle_timeout = db_config.idle_timeout
Expand Down Expand Up @@ -1040,7 +1040,7 @@ def connection_pool_list(role = ActiveRecord::Base.current_role)
end
alias :connection_pools :connection_pool_list

def establish_connection(config, owner_name: Base.name, role: ActiveRecord::Base.current_role, shard: Base.current_shard)
def establish_connection(config, owner_name: Base, role: ActiveRecord::Base.current_role, shard: Base.current_shard)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a slight behavior change in this public API that now accepts the class instead of a string. We could defensively handle this but applications really should be using establish_connection on the handler for multiple databases and this API is new in 6.1 so I think it's ok to change it. Happy to handle a string if we disagree.

owner_name = config.to_s if config.is_a?(Symbol)

pool_config = resolve_pool_config(config, owner_name)
Expand Down
Expand Up @@ -111,7 +111,7 @@ def use_metadata_table?
@config.fetch(:use_metadata_table, true)
end

# Determines whether writes are currently being prevents.
# Determines whether writes are currently being prevented.
#
# Returns true if the connection is a replica.
#
Expand All @@ -123,10 +123,9 @@ def use_metadata_table?
def preventing_writes?
return true if replica?
return ActiveRecord::Base.connection_handler.prevent_writes if ActiveRecord::Base.legacy_connection_handling
return false if owner_name.nil?
return false if connection_klass.nil?

klass = self.owner_name.safe_constantize
klass&.current_preventing_writes
connection_klass.current_preventing_writes
end

def migrations_paths # :nodoc:
Expand Down Expand Up @@ -202,8 +201,8 @@ def lease
@owner = Thread.current
end

def owner_name # :nodoc:
@pool.owner_name
def connection_klass # :nodoc:
@pool.connection_klass
end

def schema_cache
Expand Down
16 changes: 13 additions & 3 deletions activerecord/lib/active_record/connection_adapters/pool_config.rb
Expand Up @@ -5,7 +5,7 @@ module ConnectionAdapters
class PoolConfig # :nodoc:
include Mutex_m

attr_reader :db_config, :connection_specification_name
attr_reader :db_config, :connection_klass
attr_accessor :schema_cache

INSTANCES = ObjectSpace::WeakMap.new
Expand All @@ -17,14 +17,24 @@ def discard_pools!
end
end

def initialize(connection_specification_name, db_config)
def initialize(connection_klass, db_config)
super()
@connection_specification_name = connection_specification_name
@connection_klass = connection_klass
@db_config = db_config
@pool = nil
INSTANCES[self] = self
end

def connection_specification_name
if connection_klass.is_a?(String)
connection_klass
elsif connection_klass.primary_class?
"ActiveRecord::Base"
else
connection_klass.name
end
end

def disconnect!
ActiveSupport::ForkTracker.check!

Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/connection_handling.rb
Expand Up @@ -341,7 +341,7 @@ def resolve_config_for_connection(config_or_env)
self.connection_specification_name = owner_name

db_config = Base.configurations.resolve(config_or_env)
[db_config, owner_name]
[db_config, self]
end

def with_handler(handler_key, &blk)
Expand Down
Expand Up @@ -40,7 +40,7 @@ def test_expire_mutates_in_use

def test_close
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new("test", "primary", {})
pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new("primary", db_config)
pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new(ActiveRecord::Base, db_config)
pool = Pool.new(pool_config)
pool.insert_connection_for_test! @adapter
@adapter.pool = pool
Expand Down
Expand Up @@ -414,6 +414,27 @@ def test_prevent_writes_can_be_changed_granularly
ActiveRecord::Base.establish_connection(:arunit)
ENV["RAILS_ENV"] = previous_env
end

class ApplicationRecord < ActiveRecord::Base
self.abstract_class = true
end

def test_application_record_prevent_writes_can_be_changed
Object.const_set(:ApplicationRecord, ApplicationRecord)

ApplicationRecord.connects_to(database: { writing: :arunit, reading: :arunit })

# Switch everything to writing
ActiveRecord::Base.connected_to(role: :writing) do
assert_not_predicate ApplicationRecord.connection, :preventing_writes?

ApplicationRecord.connected_to(role: :reading) do
assert_predicate ApplicationRecord.connection, :preventing_writes?
end
end
ensure
Object.send(:remove_const, :ApplicationRecord)
end
end
end
end
Expand Down
8 changes: 4 additions & 4 deletions activerecord/test/cases/connection_pool_test.rb
Expand Up @@ -13,7 +13,7 @@ def setup

# Keep a duplicate pool so we do not bother others
@db_config = ActiveRecord::Base.connection_pool.db_config
@pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new("primary", @db_config)
@pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new(ActiveRecord::Base, @db_config)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should have always been ActiveRecord::Base - using primary was deprecated some time ago.

@pool = ConnectionPool.new(@pool_config)

if in_memory_db?
Expand Down Expand Up @@ -204,7 +204,7 @@ def test_idle_timeout_configuration
config = @db_config.configuration_hash.merge(idle_timeout: "0.02")
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new(@db_config.env_name, @db_config.name, config)

pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new("primary", db_config)
pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new(ActiveRecord::Base, db_config)
@pool = ConnectionPool.new(pool_config)
idle_conn = @pool.checkout
@pool.checkin(idle_conn)
Expand All @@ -231,7 +231,7 @@ def test_disable_flush

config = @db_config.configuration_hash.merge(idle_timeout: -5)
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new(@db_config.env_name, @db_config.name, config)
pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new("primary", db_config)
pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new(ActiveRecord::Base, db_config)
@pool = ConnectionPool.new(pool_config)
idle_conn = @pool.checkout
@pool.checkin(idle_conn)
Expand Down Expand Up @@ -743,7 +743,7 @@ def test_public_connections_access_threadsafe
def with_single_connection_pool
config = @db_config.configuration_hash.merge(pool: 1)
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new("arunit", "primary", config)
pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new("primary", db_config)
pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new(ActiveRecord::Base, db_config)

yield(pool = ConnectionPool.new(pool_config))
ensure
Expand Down
4 changes: 2 additions & 2 deletions activerecord/test/cases/reaper_test.rb
Expand Up @@ -59,7 +59,7 @@ def test_some_time

def test_pool_has_reaper
config = ActiveRecord::Base.configurations.configs_for(env_name: "arunit", name: "primary")
pool_config = PoolConfig.new("primary", config)
pool_config = PoolConfig.new(ActiveRecord::Base, config)
pool = ConnectionPool.new(pool_config)

assert pool.reaper
Expand Down Expand Up @@ -171,7 +171,7 @@ def test_reaper_does_not_reap_discarded_connection_pools
def duplicated_pool_config(merge_config_options = {})
old_config = ActiveRecord::Base.connection_pool.db_config.configuration_hash.merge(merge_config_options)
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new("arunit", "primary", old_config.dup)
PoolConfig.new("primary", db_config)
PoolConfig.new(ActiveRecord::Base, db_config)
end

def new_conn_in_thread(pool)
Expand Down