Proposal: Prevent activerecord access

rubyonrails-core
1

Having recently refactored some methods full of N+1 queries by prefetching data, I’m interested in preventing some methods from growing new N+1 queries. For this reason, I imagined a new ActiveRecord API: A block which could be used to prevent all database access through ActiveRecord inside the block.

Inspired by @eileencodes and this PR Part 7: Multi db improvements, Add ability to block writes to a database by eileencodes · Pull Request #34505 · rails/rails · GitHub, I imagine an API like:

def my_method_with_potential_n_plus_ones
  ActiveRecord::Base.connection.while_preventing_access do
    # do my complex stuff and be 100% certain that no new n+1 queries emerge
  end
end

I am willing to make the PR if there is interest. Please chime in with your thoughts!

1 Like
2

Did you try the strict loading feature? https://www.bigbinary.com/blog/rails-6-1-adds-strict_loading-to-warn-lazy-loading-associations

Rafael França Rails Core Team Member

1 Like
3

That looks like a fabulous feature! I will definitely try it out. It should solve my problems to the extent that queries are being lazily generated. But I do want something even stronger because not all of the queries I’m looking at are lazily generated.

I have some methods that I know are being called in loops and these methods will become N+1 DB query situations if a database query is generated in any way. What I’m hoping for is some sort of runtime policy that says “no ActiveRecord access is allowed in here, you must load everything you need first”

Here’s another way to put it. If I have some business logic that combines data loading with some kind of pure calculation, it would be nice to have the power, when it is useful, to strongly separate these concerns and be sure the separation remains.

This power could be useful for any situation where a hot code path must remain free of queries. Perhaps it could be used to implement a policy to prevent database access during the render phase of an HTTP request.

4

I think it would be interesting to see how a PR would look here, if you’re up for it.

To me it feels like a very heavy hammer to apply in the middle of application code, but at GitHub we’ve certainly had a desire for this sort of strict limitation come up in some tests.

I always feel bad encouraging a PR when we’re not sure whether we actually want the feature, but it really does so often come down to “how much change does this force onto the implementation?” and “how does the resulting API feel?”, and both of those are much easier to discuss in the concrete.

This basically comes down to “PDI, but don’t sink too much time into it if it proves challenging”, I suppose? (At a glance I imagine the enforcement would be an extension [and rename] of check_if_write_query? It’ll be worth exploring whether that connection-specific context makes the most sense for the likely use cases in a multi-DB environment :thinking:)

5

@matthewd Here’s more or less what I imagined in PR form:

6

Could this feature be accomplished via Instrumentation? Specifically sql.activerecord.

I have multiple times done something like this for testing. Basically have the instrumentation keep a count of the number of SQL queries. Then use change matcher in a test to see if the number of SQL queries changed in a block.

Seems your while_preventing_access block could enable a thread-safe variable. When enabled the instrumentation throws an error if called. After the block the thread-safe variable is disabled. Here is a stupid impl that doesn’t integrate at all with Rails but shows the idea:

module PreventDBAccess
  ViolationError = Class.new StandardError

  thread_mattr_accessor :enabled, instance_accessor: false, default: false

  def self.while_preventing_access
    old = enabled
    self.enabled = true
    yield
  ensure
    self.enabled = old
  end
end

ActiveSupport::Notifications.subscribe "sql.active_record" do |event|
  raise PreventDBAccess::ViolationError, "Database access not allowed" if PreventDBAccess.enabled
end

With this in place if I call Wdiget.count my answer is returned. If I wrap it in the method through I get my exception. Could also take the Rails env into account so in production it is just inefficient. In testing env and staging environments it actually blows up.

7

The idea of making a system to do this that is separate from all the db connection handling seems like a good idea. It could be built on notifications, but I’m also thinking about something more direct. @eileencodes had some similar feedback in the PR.

I’m not a regular Rails contributor so I’m not familiar with where to discuss. Should we continue discussing here? or in the PR at this point?