Skip to content

Respect config.active_record.permanent_connection_checkout #1570

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joshuay03
Copy link

Context:

This PR addresses two things:

  1. ActiveRecord::Base.connection has been soft deprecated in favour of .lease_connection, and applications can explicitly enable the deprecation by setting config.active_record.permanent_connection_checkout to :deprecated, or disallow its usage altogether by setting it to :disallowed. In such cases the gem should not be using the deprecated method, but it would be good to avoid using it altogether so no further changes will be needed when it's hard deprecated.
  2. Applications can choose to not use permanent connection checkouts altogether (i.e. by not using .lease_connection) in favour of the ActiveRecord::ConnectionAdapters::ConnectionPool#with_connection and newly added ActiveRecord::ConnectionHandling#with_connection APIs. I think it would be good to support such applications by utilising these APIs where possible, instead of leasing a connection. I've ensured that this is true for all non-test code, but I don't know of a good way to prevent further usage (see this relevant comment). Also note that ActiveRecord::ConnectionHandling#with_connection was added in 7.2, and since this gem currently supports 7.1 I've only relied on the connection pool method.

::ActiveRecord::Base.connection.adapter_name}"
::ActiveRecord::Base.adapter_class::ADAPTER_NAME}"
Copy link
Author

Choose a reason for hiding this comment

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

I've opted to use this constant even where we could lease a connection (i.e., here and tests) for consistency.

@joshuay03 joshuay03 moved this to In Progress / Pending Review in Open Source Jun 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant