Use ActiveRecord::Base .with_connection rather than .connection

[davidrunger]

Per comment here, calling ActiveRecord::Base.connection is soft-deprecated, so this change uses with_connection, instead.

[davidrunger]

See relevant Rails PR https://github.com/rails/rails/pull/51349, where an active_record.permanent_connection_checkout Rails config option was added to allow flagging (either via a printed warning or via a raised error) any usages of ActiveRecord::Base.connection.

I tried setting that config to :disallowed in my app, and errors were raised for ActiveAdmin index pages at the code location that I'm modifying in this PR to use the suggested with_connection, rather than connection.

[tagliala]

Thanks for this PR, is there any deprecation warning in the logs that we missed?

I was expecting to see a deprecation warning somewhere here: https://github.com/activeadmin/activeadmin/actions/runs/10706856019/job/29685533496

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.11%. Comparing base (1477e90) to head (95cb6ad). Report is 32 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #8474   +/-   ##
=======================================
  Coverage   99.11%   99.11%           
=======================================
  Files         141      141           
  Lines        4069     4072    +3     
=======================================
+ Hits         4033     4036    +3     
  Misses         36       36           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[davidrunger]

Thanks for this PR, is there any deprecation warning in the logs that we missed?

I was expecting to see a deprecation warning somewhere here: https://github.com/activeadmin/activeadmin/actions/runs/10706856019/job/29685533496

@tagliala : No, there is no deprecation warning in the logs that you have missed. ActiveRecord::Base.connection is only "soft deprecated", which (in this case, at least) seems to mean that users must opt in, in order to receive any deprecation warnings. By default, Rails will not print any deprecation warnings about this, since ActiveRecord::Base.connection is not (yet?) being "fully deprecated".

I guess that deprecation warnings would show up somewhere in the logs, if the ActiveAdmin test/sample app were to opt in to the deprecation warnings by adding config.active_record.permanent_connection_checkout = :deprecated to its config/application.rb file. (Alternatively, set it to :disallowed in the test/sample app, in order to get outright errors / test failures, rather than just printed deprecation warnings.) However, since the config.active_record.permanent_connection_checkout config option was first introduced in Rails 7.2, this could only be done in the Rails 7.2 (or higher) matrix builds.

[tagliala]

@davidrunger ah sorry, I did not pay enough attention and I was confused by this line including ActiveRecord.deprecator.warn:

https://github.com/rails/rails/blob/a11f0a63673d274c59c69c2688c63ba303b86193/activerecord/lib/active_record/connection_handling.rb#L265

[davidrunger]

I guess that deprecation warnings would show up somewhere in the logs, if the ActiveAdmin test/sample app were to opt in to the deprecation warnings by adding config.active_record.permanent_connection_checkout = :deprecated to its config/application.rb file. (Alternatively, set it to :disallowed in the test/sample app, in order to get outright errors / test failures, rather than just printed deprecation warnings.)

Actually, unfortunately, I think that this is not true. Due to ... something -- sorry, I am not clear on the details ... I guess something having to do with tests being wrapped within a test-specific database transaction for transactional fixtures, which seems to affect the way that ActiveRecord connections get checked out from the connection pool ... unfortunately, even if we do set config.active_record.permanent_connection_checkout = :disallowed for the test app, then the tests still won't raise errors / fail. The concrete reason is that, when calling ActiveRecord::Base.connection in specs, we end up going into this else branch, rather than going into this if branch that actually checks against the config.active_record.permanent_connection_checkout setting.

So, in summary, unfortunately, I'm not sure that it's really reasonably possible to have specs cover this change / be sensitive to this soft deprecation.

In my own app, I actually ended up monkeypatching in tests the ActiveRecord::Base.connection method so that it will actually check against the config.active_record.permanent_connection_checkout setting, but I mostly doubt that we'd want to try to include such a monkeypatch into the test app in the activeadmin test suite.

[mgrunberg]

@davidrunger thanks for opening the PR! This is really interesting. I investigated a little bit.

Tests ignores config.active_record.permanent_connection_checkout = :deprecated because we have set config.use_transactional_fixtures = true (here). By doing that Rails establishes a connection, leases a connection from the pool, and opens a transaction before running each spec. That's why our calls ActiverRecord::Base.connection inside the test goes through the pool.permanent_lease? else branch.

I don't think we can turn off use_transactional_fixtures. But I could turn it off for a particular spec using uses_transaction.

With all this, I opened a PR ( https://github.com/davidrunger/activeadmin/pull/4) with my suggestions to add some specs about this change.

Please let me know what you think.

[davidrunger]

@mgrunberg Thank you so much for digging into this, for uncovering more clearly why the config.active_record.permanent_connection_checkout setting was being ignored in specs, and for figuring out a way to work around that!

I was not familiar with the uses_transaction method. It seems to work great, in order to be able to test this change, and I think it should be fine to skip using transactional fixtures for the two tests where you did that, since I think those tests don't modify the database state in any way. I merged your PR (https://github.com/davidrunger/activeadmin/pull/4) into this branch. Thank you!

That being said, unfortunately, due to effectively needing to explicitly "opt in" to enforcing the config.active_record.permanent_connection_checkout setting in each relevant spec via uses_transaction, I guess that this still will not really provide ActiveAdmin with generalized/global protection against direct usage of ActiveRecord::Base.connection. I guess that other code could be added to ActiveAdmin that does use ActiveRecord::Base.connection, and, unless that code is exercised in a test for which we have declared uses_transaction, then that usage will go undetected (i.e. it won't cause any specs to fail). In light of this, it perhaps could even be argued that adding config.active_record.permanent_connection_checkout = :disallowed for the Rails 7.2 test app is somewhat of a downside, since perhaps it gives "a false sense of security", since (at first glance / naively) I think it probably suggests to most readers of that code that the Rails 7.2 ActiveAdmin test suite will respect that setting in general, whereas the reality is that the setting will only be enforced for specs that explicitly opt in to the protection via uses_transaction.

Still, despite that potential downside, I do like having test coverage for this change, which is why I was happy to merge your PR into mine, even if it will not be globally effective, and even if it risks being somewhat misleading.

[davidrunger]

[...] it perhaps could even be argued that adding config.active_record.permanent_connection_checkout = :disallowed for the Rails 7.2 test app is somewhat of a downside, since perhaps it gives "a false sense of security", since (at first glance / naively) I think it probably suggests to most readers of that code that the Rails 7.2 ActiveAdmin test suite will respect that setting in general, whereas the reality is that the setting will only be enforced for specs that explicitly opt in to the protection via uses_transaction.

To try to lessen this risk, I added 2f776801bacf40603302e814d01c7e03145636bb, which adds a comment in spec/support/rails_template.rb about the need to opt in to respecting that configuration setting in specs via uses_transaction.

[davidrunger]

In light of this, it perhaps could even be argued that adding config.active_record.permanent_connection_checkout = :disallowed for the Rails 7.2 test app is somewhat of a downside since perhaps it gives "a false sense of security", since (at first glance / naively) I think it probably suggests to most readers of that code that the Rails 7.2 ActiveAdmin test suite will respect that setting in general, whereas the reality is that the setting will only be enforced for specs that explicitly opt in to the protection via uses_transaction.

This is not entirely true. You are ignoring the fact that the template app is also used for development. Turning config.active_record.permanent_connection_checkout = :disallowed for Rails 7.2 template increases the chance of catching problems in new code by maintainers during development or manual tests of new branches.

@mgrunberg This is a really great point that I had not thought about. Thank you for mentioning it!

It's great that adding config.active_record.permanent_connection_checkout = :disallowed for the Rails 7.2 sample app will raise errors when contributors are poking around the app manually, in development mode, where, unlike in specs, that config setting will be always in effect.

On the other hand, this important point actually makes me realize that this PR should not currently be merged! :exclamation: , because the latest released version of ransack (which ActiveAdmin depends upon, as you know) still has a few usages of ActiveRecord::Base.connection, and so those ransack usages will cause exceptions when attempting to go through those ransack code paths in development mode in the ActiveAdmin sample app (if/when we have config.active_record.permanent_connection_checkout = :disallowed).

For example, on the latest version of this branch, after freshly generating the development example app, if I log in to the example admin app, go to the categories index page, and search for categories with a name that contains "Rock"...

... then I get this exception:

... which is triggered by ransack using ActiveRecord::Base.connection (rather than with_connection) here. There are a few other places in ransack where ActiveRecord::Base.connection is used, all of which I guess would need to be updated, before we could safely merge this PR adding config.active_record.permanent_connection_checkout = :disallowed for the Rails 7.2 sample app. Otherwise, we will cause ActiveAdmin contributors poking around the example app in development mode to experience exceptions like the one seen above.

[davidrunger]

I have converted this PR to "draft" status, to indicate that it should not currently be merged, as I elaborated on in my comment just above.

I don't have a ton of appetite for submitting a similar change to ransack (i.e. converting usage of ActiveRecord::Base.connection to instead use ActiveRecord::Base.with_connection), which I think is a prerequisite for merging this PR, at least if we are going to continue to try to add config.active_record.permanent_connection_checkout = :disallowed for the Rails 7.2 example app.

One option would be to not make that addition to the example application configuration, and instead just make the change here in activeadmin from using ActiveRecord::Base.connection to instead use ActiveRecord::Base.with_connection but leave that change essentially untested and unenforced in development mode. As far as activeadmin is concerned, it should be safe to merge such a change, even if ransack continues to use ActiveRecord::Base.connection. The downside of this approach is that there would be no protection against inadvertently adding calls elsewhere in ActiveAdmin to ActiveRecord::Base.connection, thereby reintroducing the issue that this PR aims to avoid.

Another option is to leave this PR as-is in draft mode, and wait/hope for someone to make a similar change to ransack and for that change to be released. Then, it should be safe to reconsider this PR (including setting config.active_record.permanent_connection_checkout = :disallowed for the Rails 7.2 example app). Maybe someone following/participating in this thread will have interest in doing that. Maybe even I might feel more motivated to submit such a change to ransack, at some point in the future.

Or, rather than leaving this PR open in draft mode indefinitely, I'm also open to simply closing it, if that's what maintainers think is best / would prefer. But I guess that this is something that probably should be dealt with in ActiveAdmin sooner or later, as I imagine that more and more apps will probably start to enable this config.active_record.permanent_connection_checkout setting over time, and also because it sounds like there are tentative/long-term plans to eventually (in a matter of years) fully deprecate ActiveRecord::Base.connection in Rails.

[mgrunberg]

@davidrunger I see your point.

I may have the time to work on a ransack PR with a similar change this week. Until then, I'm ok with leaving this as a draft.

If I can't find the time to do that, or if it proves hard, another alternative is to find a way to set config.active_record.permanent_connection_checkout = :disallowed just in the two specs we are adding and, optionally, use :deprecated in the template app. Let's see how it goes.

[javierjulio]

The deprecation is unlikely to happen anytime soon. We'll also have issues with other dependencies needing to update. We will reopen the PR if that area makes progress. Just let us know. Thank you.

[henrikbjorn]

@javierjulio Can we reopen this?

It is now deprecated, and if one is not using Puma but maybe Falcon as the web server then AA is broken as a deprecation is raised.

[tagliala]

@henrikbjorn any chance to replicate this?

Maybe starting with https://github.com/activeadmin/demo.activeadmin.info, which is based on Ruby 3.4.7 and Rails 8.1.1

I don't know how to configure falcon

[henrikbjorn]

@tagliala https://github.com/activeadmin/demo.activeadmin.info/compare/main...henrikbjorn:demo.activeadmin.info:connection-checkout-disallow?expand=1

It is when config.active_record.permanent_connection_checkout = :disallowed config is used. Which is a good idea when using fibers, as there can be many more fibers than threads and then dry up the db pool

Try and load the admin users page and see the exception.

[tagliala]

@henrikbjorn excellent, thanks

I was able to replicate with puma at http://localhost:3000/admin/comments with just that single line change

diff --git a/config/application.rb b/config/application.rb
index 262d716..97caab5 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -35,5 +35,6 @@ module ActiveAdminDemo
     #
     # config.time_zone = "Central Time (US & Canada)"
     # config.eager_load_paths << Rails.root.join("extras")
+    config.active_record.permanent_connection_checkout = :disallowed
   end
 end

So I guess this is an existing issue, affecting both v3 and v4, starting from version 7.2 and above.

It appears that I cannot reopen this PR, because the upstream is gone, let me check what we can do