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(database_tasks): ConnectionPool deprecation warning in Rails 7.2 #341

Conversation

martingregoire
Copy link
Contributor

Fixes this deprecation warning raised in Rails 7.2:

DEPRECATION WARNING: ActiveRecord::ConnectionAdapters::ConnectionPool#connection is deprecated
and will be removed in Rails 8.0. Use #lease_connection instead.

The changed code is simply copied from the Rails repository, please see https://github.com/rails/rails/blob/97169912f197eee6e76fafb091113bddf624aa67/activerecord/lib/active_record/tasks/database_tasks.rb#L520 and
https://github.com/rails/rails/blob/97169912f197eee6e76fafb091113bddf624aa67/activerecord/lib/active_record/tasks/database_tasks.rb#L550.

see #340

Fixes this deprecation warning raised in Rails 7.2:

```
DEPRECATION WARNING: ActiveRecord::ConnectionAdapters::ConnectionPool#connection is deprecated
and will be removed in Rails 8.0. Use #lease_connection instead.
```

The changed code is simply copied from the Rails repository, please see
https://github.com/rails/rails/blob/97169912f197eee6e76fafb091113bddf624aa67/activerecord/lib/active_record/tasks/database_tasks.rb#L520
and
https://github.com/rails/rails/blob/97169912f197eee6e76fafb091113bddf624aa67/activerecord/lib/active_record/tasks/database_tasks.rb#L550
def with_temporary_connection(db_config) # :nodoc:
with_temporary_pool(db_config) do |pool|
yield pool.connection
def with_temporary_connection(db_config, clobber: false, &block) # :nodoc:
Copy link
Collaborator

Choose a reason for hiding this comment

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

changing the method definition would cause issues for older rails versions (tested against 7.0, 7.1 already supports the clobber keyword)

Would you be able to patch that for rails 7.0?

@vprigent
Copy link
Collaborator

vprigent commented Oct 5, 2024

Hi @martingregoire

I had a more complete look at the PR above.

rails/rails@a918394#diff-4a42b5efdab725af0936f94b4a84dcf6a8be8738e28ed265e0a3e74f275f5048R189
has introduced a few changes, one of which breaks the guard clause for adding these methods to our implementation of the DatabaseTask.

I'm looking at simply changing the behavior here, from with_temporary_connection_for_each to with_temporary_pool_for_each and seeing where this leads.

Also coincidentally fixes the rails 7.2 connection pool deprecation warning
@vprigent
Copy link
Collaborator

vprigent commented Oct 6, 2024

Hi @martingregoire I hope you won't mind, I amended your PR a bit, would you be able to give this a go and tell me if it is working for you?
Thanks!

@vprigent vprigent merged commit b24256b into ilyakatz:main Oct 8, 2024
15 checks passed
@martingregoire
Copy link
Contributor Author

Hi @vprigent, thanks a lot! It works perfectly for us. 👍

@martingregoire martingregoire deleted the fix/rails-7.2-connection-pool-deprecation-warning branch October 8, 2024 10:09
@vprigent
Copy link
Collaborator

vprigent commented Oct 8, 2024

Brilliant! I'll cut a new version out today!

Thanks for helping out with this resolution

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.

2 participants