Add ability to prevent access to a database

[stevecrozz]

Motivation / Background

From discussion: https://discuss.rubyonrails.org/t/proposal-prevent-activerecord-access/85322

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.

N+1 queries can be a real performance concern. It isn't always immediately obvious what the impacts could be of adding a new query. I wish engineers would be constantly watching the query log as they work. But things can get out of hand quickly, and even if you are watching the wall of queries being logged, N+1s can sometimes disappear into the noise.

Also, there are times when I want a pure in-memory algorithm. An API like this would allow me to both express and enforce the intention to not be hitting the database in a branch of code.

Just today, I was working with some code that broke a test that asserts for a given controller action, exactly 118 queries are dispatched. This is a terrible test to have, but we have it because we have had real problems with query explosions emerging unexpectedly. I believe strict loading could help, but it would not have prevented all of the problems that led to write and maintain this test.

This is indeed a heavy hammer to use. But I don't see an alternative that can achieve the same level of assurance, and the implementation was pretty easy thanks to the prior art of while_preventing_writes.

Detail

This Pull Request adds while_preventing_access to the public API of ActiveRecord::Base much like was done for while_preventing_writes in #34505

Additional information

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[eileencodes]

Thanks for the PR @stevecrozz!

I personally don't love how close yet different prevent_writes and prevent_access are. It feels like a confusing API and it would let an application set both when that just doesn't really make sense. It only makes sense to use one or the other.

I would rather rewrite the prevent_writes API than have two arguments that are this similar. I think it would be better to do the following:

• Deprecate prevent_writes and friends
• Add a new argument called something like prevent_queries. This would take a symbol of :writes and :all...maybe also :reads (I'm on the fence whether we need reads).
• Raise the appropriate error based on the symbol set by prevent_queries.

In use the API would look like this:

ActiveRecord::Base.connected_to(role: :writing, prevent_queries: :all) do
...
end

ActiveRecord::Base.while_preventing_queries(:writes) do
...
end

This would make current_preventing_access more complicated but it's better for Rails to have clean APIs and messy internals than an API that doesn't feel right.

An alternative option is to not lean on or change the connected_to API and make a new API for preventing access. I feel like preventing access is somewhat of a separate concern from multi-db connection management that requires prevent_writes to be a feature (we use it for connections to act like they are on a replica for dev/test). So I could see this being an entirely separate API.

Let me know if I can clarify anything.

[stevecrozz]

An alternative option is to not lean on or change the connected_to API and make a new API for preventing access. I feel like preventing access is somewhat of a separate concern from multi-db connection management that requires prevent_writes to be a feature (we use it for connections to act like they are on a replica for dev/test). So I could see this being an entirely separate API.

This was bugging me too and I think it's worth exploring. Currently connected_to calls with_role_and_shard, and then append_to_connected_to_stack and all of these seem to be tied into the multi-db connection connection management concern which does seem separate from preventing access. The connected_to_stack itself also seems tied to these concepts.

Ultimately, what do you think should be appended to that stack when we're preventing access?

[stevecrozz]

@eileencodes I have prepared a new implementation of the same feature. The new implementation is now separate from the multi-connection handling concerns. It stores a value on the current thread as opposed to a specific connection using thread_mattr_accessor (thank you for this suggestion e_a).