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

Deprecate ConnectionPool#connection #51230

Merged
merged 1 commit into from
Mar 1, 2024
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
6 changes: 3 additions & 3 deletions actioncable/test/subscription_adapter/postgresql_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def setup
ActiveRecord::Base.establish_connection database_config

begin
ActiveRecord::Base.connection.connect!
ActiveRecord::Base.lease_connection.connect!
rescue
@rx_adapter = @tx_adapter = nil
skip "Couldn't connect to PostgreSQL: #{database_config.inspect}"
Expand Down Expand Up @@ -68,7 +68,7 @@ def active?
def test_default_subscription_connection_identifier
subscribe_as_queue("channel") { }

identifiers = ActiveRecord::Base.connection.exec_query("SELECT application_name FROM pg_stat_activity").rows
identifiers = ActiveRecord::Base.lease_connection.exec_query("SELECT application_name FROM pg_stat_activity").rows
assert_includes identifiers, ["ActionCable-PID-#{$$}"]
end

Expand All @@ -81,7 +81,7 @@ def test_custom_subscription_connection_identifier

subscribe_as_queue("channel", adapter) { }

identifiers = ActiveRecord::Base.connection.exec_query("SELECT application_name FROM pg_stat_activity").rows
identifiers = ActiveRecord::Base.lease_connection.exec_query("SELECT application_name FROM pg_stat_activity").rows
assert_includes identifiers, ["hello-world-42"]
end
end
2 changes: 1 addition & 1 deletion actionmailbox/test/migrations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class ActionMailbox::MigrationsTest < ActiveSupport::TestCase
@original_verbose = ActiveRecord::Migration.verbose
ActiveRecord::Migration.verbose = false

@connection = ActiveRecord::Base.connection
@connection = ActiveRecord::Base.lease_connection
@original_options = Rails.configuration.generators.options.deep_dup
end

Expand Down
10 changes: 5 additions & 5 deletions actionview/test/active_record_unit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def setup

def reconnect
return unless able_to_connect
ActiveRecord::Base.connection.reconnect!
ActiveRecord::Base.lease_connection.reconnect!
load_schema
end

Expand All @@ -56,9 +56,9 @@ def setup_connection
options = defaults.merge adapter: adapter, timeout: 500
ActiveRecord::Base.establish_connection(options)
ActiveRecord::Base.configurations = { "sqlite3_ar_integration" => options }
ActiveRecord::Base.connection
ActiveRecord::Base.lease_connection

Object.const_set :QUOTED_TYPE, ActiveRecord::Base.connection.quote_column_name("type") unless Object.const_defined?(:QUOTED_TYPE)
Object.const_set :QUOTED_TYPE, ActiveRecord::Base.lease_connection.quote_column_name("type") unless Object.const_defined?(:QUOTED_TYPE)
else
raise "Can't setup connection since ActiveRecord isn't loaded."
end
Expand All @@ -67,7 +67,7 @@ def setup_connection
# Load actionpack sqlite3 tables
def load_schema
File.read(File.expand_path("fixtures/db_definitions/sqlite.sql", __dir__)).split(";").each do |sql|
ActiveRecord::Base.connection.execute(sql) unless sql.blank?
ActiveRecord::Base.lease_connection.execute(sql) unless sql.blank?
end
end

Expand Down Expand Up @@ -107,7 +107,7 @@ def run(*args)
end

def capture_sql
ActiveRecord::Base.connection.materialize_transactions
ActiveRecord::Base.lease_connection.materialize_transactions
SQLCounter.clear_log
yield
SQLCounter.log.dup
Expand Down
17 changes: 17 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,20 @@
* Deprecate `ActiveRecord::Base.connection` in favor of `.lease_connection`

The method has been renamed as `lease_connection` to better reflect that the returned
connection will be held for the duration of the request or job.

This deprecation is a soft deprecation, no warnings will be issued and there is no
current plan to remove the method.

*Jean Boussier*

* Deprecate `ActiveRecord::ConnectionAdapters::ConnectionPool#connection`

The method has been renamed as `lease_connection` to better reflect that the returned
connection will be held for the duration of the request or job.

*Jean Boussier*

* Expose a generic fixture accessor for fixture names that may conflict with Minitest

```ruby
Expand Down
4 changes: 2 additions & 2 deletions activerecord/examples/performance.rb
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,10 @@ def self.email
end

x.report "Model.log" do
Exhibit.connection.send(:log, "hello", "world") { }
Exhibit.lease_connection.send(:log, "hello", "world") { }
end

x.report "AR.execute(query)" do
ActiveRecord::Base.connection.execute("SELECT * FROM exhibits WHERE id = #{(rand * 1000 + 1).to_i}")
ActiveRecord::Base.lease_connection.execute("SELECT * FROM exhibits WHERE id = #{(rand * 1000 + 1).to_i}")
end
end
16 changes: 9 additions & 7 deletions activerecord/lib/active_record/associations/association.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def reset_negative_cache # :nodoc:
# Reloads the \target and returns +self+ on success.
# The QueryCache is cleared if +force+ is true.
def reload(force = false)
klass.connection.clear_query_cache if force && klass
klass.connection_pool.clear_query_cache if force && klass
reset
reset_scope
load_target
Expand Down Expand Up @@ -231,12 +231,14 @@ def find_target
end

binds = AssociationScope.get_bind_values(owner, reflection.chain)
sc.execute(binds, klass.connection) do |record|
set_inverse_instance(record)
if owner.strict_loading_n_plus_one_only? && reflection.macro == :has_many
record.strict_loading!
else
record.strict_loading!(false, mode: owner.strict_loading_mode)
klass.with_connection do |c|
sc.execute(binds, c) do |record|
set_inverse_instance(record)
if owner.strict_loading_n_plus_one_only? && reflection.macro == :has_many
record.strict_loading!
else
record.strict_loading!(false, mode: owner.strict_loading_mode)
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def strict_loading?
def append_constraints(join, constraints)
if join.is_a?(Arel::Nodes::StringJoin)
join_string = Arel::Nodes::And.new(constraints.unshift join.left)
join.left = Arel.sql(base_klass.connection.visitor.compile(join_string))
join.left = Arel.sql(base_klass.lease_connection.visitor.compile(join_string))
else
right = join.right
right.expr = Arel::Nodes::And.new(constraints.unshift right.expr)
Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ module ActiveRecord # :nodoc:
#
# Connections are usually created through
# {ActiveRecord::Base.establish_connection}[rdoc-ref:ConnectionHandling#establish_connection] and retrieved
# by ActiveRecord::Base.connection. All classes inheriting from ActiveRecord::Base will use this
# by ActiveRecord::Base.lease_connection. All classes inheriting from ActiveRecord::Base will use this
# connection. But you can also set a class-specific connection. For example, if Course is an
# ActiveRecord::Base, but resides in a different database, you can just say <tt>Course.establish_connection</tt>
# and Course and all of its subclasses will use this connection instead.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ def flush_idle_connections!(role = nil)
# for (not necessarily the current class).
def retrieve_connection(connection_name, role: ActiveRecord::Base.current_role, shard: ActiveRecord::Base.current_shard) # :nodoc:
pool = retrieve_connection_pool(connection_name, role: role, shard: shard, strict: true)
pool.connection
pool.lease_connection
end

# Returns true if a connection that's accessible to this class has
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def db_config
# Connections can be obtained and used from a connection pool in several
# ways:
#
# 1. Simply use {ActiveRecord::Base.connection}[rdoc-ref:ConnectionHandling.connection].
# 1. Simply use {ActiveRecord::Base.lease_connection}[rdoc-ref:ConnectionHandling.connection].
# When you're done with the connection(s) and wish it to be returned to the pool, you call
# {ActiveRecord::Base.connection_handler.clear_active_connections!}[rdoc-ref:ConnectionAdapters::ConnectionHandler#clear_active_connections!].
# This is the default behavior for Active Record when used in conjunction with
Expand Down Expand Up @@ -139,46 +139,46 @@ def clear(connection)
end
end

if ObjectSpace.const_defined?(:WeakKeyMap) # RUBY_VERSION >= 3.3
WeakKeyMap = ::ObjectSpace::WeakKeyMap # :nodoc:
else
class WeakKeyMap # :nodoc:
def initialize
@map = ObjectSpace::WeakMap.new
@values = nil
@size = 0
end

alias_method :clear, :initialize
class LeaseRegistry # :nodoc:
if ObjectSpace.const_defined?(:WeakKeyMap) # RUBY_VERSION >= 3.3
WeakKeyMap = ::ObjectSpace::WeakKeyMap # :nodoc:
else
class WeakKeyMap # :nodoc:
def initialize
@map = ObjectSpace::WeakMap.new
@values = nil
@size = 0
end

def [](key)
prune if @map.size != @size
@map[key]
end
alias_method :clear, :initialize

def []=(key, value)
@map[key] = value
prune if @map.size != @size
value
end
def [](key)
prune if @map.size != @size
@map[key]
end

def delete(key)
if value = self[key]
self[key] = nil
prune
def []=(key, value)
@map[key] = value
prune if @map.size != @size
value
end
value
end

private
def prune(force = false)
@values = @map.values
@size = @map.size
def delete(key)
if value = self[key]
self[key] = nil
prune
end
value
end

private
def prune(force = false)
@values = @map.values
@size = @map.size
end
end
end
end

class LeaseRegistry # :nodoc:
def initialize
@mutex = Mutex.new
@map = WeakKeyMap.new
Expand Down Expand Up @@ -293,7 +293,13 @@ def lease_connection
lease.connection ||= checkout
end

alias_method :connection, :lease_connection # TODO: deprecate
def connection
ActiveRecord.deprecator.warn(<<~MSG)
ConnectionPoool#connection is deprecated and will be removed
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

Copy link
Member

Choose a reason for hiding this comment

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

🤦 Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I wasn't even meaning this. I just noticed that Poool doesn't look very correct. My bad, should have been more specific.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I meant to fix that too, but I must be tired 😫

in Rails 7.3. Use #lease_connection instead
MSG
lease_connection
end

def pin_connection!(lock_thread) # :nodoc:
raise "There is already a pinned connection" if @pinned_connection
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,9 +296,9 @@ def truncate_tables(*table_names) # :nodoc:
# #transaction will raise exceptions when it tries to release the
# already-automatically-released savepoints:
#
# Model.connection.transaction do # BEGIN
# Model.connection.transaction(requires_new: true) do # CREATE SAVEPOINT active_record_1
# Model.connection.create_table(...)
# Model.lease_connection.transaction do # BEGIN
# Model.lease_connection.transaction(requires_new: true) do # CREATE SAVEPOINT active_record_1
# Model.lease_connection.create_table(...)
# # active_record_1 now automatically released
# end # RELEASE SAVEPOINT active_record_1 <--- BOOM! database error!
# end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ module ConnectionAdapters # :nodoc:
# and +:limit+ options, etc.
#
# All the concrete database adapters follow the interface laid down in this class.
# {ActiveRecord::Base.connection}[rdoc-ref:ConnectionHandling#connection] returns an AbstractAdapter object, which
# {ActiveRecord::Base.lease_connection}[rdoc-ref:ConnectionHandling#lease_connection] returns an AbstractAdapter object, which
# you can use.
#
# Most of the methods in the adapter are useful during migrations. Most
Expand Down
6 changes: 3 additions & 3 deletions activerecord/lib/active_record/core.rb
Original file line number Diff line number Diff line change
Expand Up @@ -377,8 +377,8 @@ def type_caster # :nodoc:
end

def cached_find_by_statement(key, &block) # :nodoc:
cache = @find_by_statement_cache[connection.prepared_statements]
cache.compute_if_absent(key) { StatementCache.create(connection, &block) }
cache = @find_by_statement_cache[lease_connection.prepared_statements]
cache.compute_if_absent(key) { StatementCache.create(lease_connection, &block) }
end

private
Expand Down Expand Up @@ -431,7 +431,7 @@ def cached_find_by(keys, values)
}

begin
statement.execute(values.flatten, connection).first
statement.execute(values.flatten, lease_connection).first
rescue TypeError
raise ActiveRecord::StatementInvalid
end
Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def initialize(message = nil, connection_pool: nil)
end

# Raised when connection to the database could not been established (for example when
# {ActiveRecord::Base.connection=}[rdoc-ref:ConnectionHandling#connection]
# {ActiveRecord::Base.lease_connection=}[rdoc-ref:ConnectionHandling#lease_connection]
# is given a +nil+ object).
class ConnectionNotEstablished < AdapterError
def initialize(message = nil, connection_pool: nil)
Expand Down
25 changes: 13 additions & 12 deletions activerecord/lib/active_record/explain.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,17 @@ def collecting_queries_for_explain # :nodoc:
# Makes the adapter execute EXPLAIN for the tuples of queries and bindings.
# Returns a formatted string ready to be logged.
def exec_explain(queries, options = []) # :nodoc:
str = queries.map do |sql, binds|
msg = +"#{build_explain_clause(options)} #{sql}"
unless binds.empty?
msg << " "
msg << binds.map { |attr| render_bind(attr) }.inspect
end
msg << "\n"
msg << connection.explain(sql, binds, options)
end.join("\n")

str = with_connection do |c|
queries.map do |sql, binds|
msg = +"#{build_explain_clause(c, options)} #{sql}"
unless binds.empty?
msg << " "
msg << binds.map { |attr| render_bind(c, attr) }.inspect
end
msg << "\n"
msg << c.explain(sql, binds, options)
end.join("\n")
end
# Overriding inspect to be more human readable, especially in the console.
def str.inspect
self
Expand All @@ -36,7 +37,7 @@ def str.inspect
end

private
def render_bind(attr)
def render_bind(connection, attr)
if ActiveModel::Attribute === attr
value = if attr.type.binary? && attr.value
"<#{attr.value_for_database.to_s.bytesize} bytes of binary data>"
Expand All @@ -51,7 +52,7 @@ def render_bind(attr)
[attr&.name, value]
end

def build_explain_clause(options = [])
def build_explain_clause(connection, options = [])
if connection.respond_to?(:build_explain_clause, true)
connection.build_explain_clause(options)
else
Expand Down
4 changes: 3 additions & 1 deletion activerecord/lib/active_record/future_result.rb
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@ def execute_or_wait
start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :float_millisecond)
@mutex.synchronize do
if pending?
execute_query(@pool.connection)
@pool.with_connection do |connection|
execute_query(connection)
end
else
@lock_wait = (Process.clock_gettime(Process::CLOCK_MONOTONIC, :float_millisecond) - start)
end
Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/insert_all.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class InsertAll # :nodoc:
attr_reader :on_duplicate, :update_only, :returning, :unique_by, :update_sql

def initialize(model, inserts, on_duplicate:, update_only: nil, returning: nil, unique_by: nil, record_timestamps: nil)
@model, @connection, @inserts = model, model.connection, inserts.map(&:stringify_keys)
@model, @connection, @inserts = model, model.lease_connection, inserts.map(&:stringify_keys)
@on_duplicate, @update_only, @returning, @unique_by = on_duplicate, update_only, returning, unique_by
@record_timestamps = record_timestamps.nil? ? model.record_timestamps : record_timestamps

Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/integration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ def collection_cache_key(collection = all, timestamp_column = :updated_at) # :no
def can_use_fast_cache_version?(timestamp)
timestamp.is_a?(String) &&
cache_timestamp_format == :usec &&
self.class.connection.default_timezone == :utc &&
self.class.lease_connection.default_timezone == :utc &&
!updated_at_came_from_user?
end

Expand Down