Fix SELECT COUNT queries when rendering ActiveRecord collections
#40870
Conversation
| @@ -144,7 +144,7 @@ def render_collection(collection, view, path, template, layout, block) | |||
| "render_collection.action_view", | |||
| identifier: identifier, | |||
| layout: layout && layout.virtual_path, | |||
| count: collection.size | |||
| count: collection.to_a.size | |||
There was a problem hiding this comment.
Would not this load the collection too early making the preload that happens inside PreloadCollectionIterator not happen anymore?
There was a problem hiding this comment.
No because the PreloadCollectionIterator has already been instantiated, and preloading has been disabled (the relation hasn't been loaded at that point). Then, to_a will load the relation without preloading. Finally, the PreloadCollectionIterator will handle the preloading of the associations.
There was a problem hiding this comment.
Great! Thank you for investigating.
There was a problem hiding this comment.
How about using collection.length instead?
| count: collection.to_a.size | |
| count: collection.length |
to_a for relation will dup extra records.
rails/activerecord/lib/active_record/relation.rb
Lines 243 to 246 in 9c120d7
There was a problem hiding this comment.
@kamipo Good suggestion, thanks. The only catch was I had to put a check for the length method because there are tests that rely on passing collections that don't respond to length, so I delegate back to size if length isn't available.
|
Is it possible to write test to make sure someone doesn't remove that |
|
I added a test. I couldn't think of a better way to test this other than asserting on the queries during a render. Kind of hard to test since it's a complete side-effect. I've never contributed to Rails before, so I'm not sure if I'm following the correct patterns or not, so let me know if I'm doing it wrong. There seems to be many different styles accumulated over the years. 🤣 |
|
|
||
| @queries = [] | ||
|
|
||
| ActiveSupport::Notifications.subscribe("sql.active_record") do |*, payload| |
There was a problem hiding this comment.
Looking at some of the other tests, you'll want to store the subscription in a variable, so you can unsubscribe at the end of the test. Right now, all subsequent tests are pushing their SQL into this array (a memory leak in the test suite).
For example:
There was a problem hiding this comment.
@natematykiewicz Thanks, I've done as you suggested.
Fixes #40837 When rendering collections, calling `size` when the collection is an ActiveRecord relation causes unwanted `SELECT COUNT(*)` queries. This change ensures the collection is an array before getting the size, and also loads the relation for any further array inspections.
Allows getting the size of a relation without duplicating records, but still loads the relation. The length method existence needs to be checked because you can pass in an `Enumerator`, which does not respond to `length`.
…40870) * Fix `SELECT COUNT` queries when rendering ActiveRecord collections Fixes #40837 When rendering collections, calling `size` when the collection is an ActiveRecord relation causes unwanted `SELECT COUNT(*)` queries. This change ensures the collection is an array before getting the size, and also loads the relation for any further array inspections. * Test queries when rendering relation collections * Add `length` support to partial collection iterator Allows getting the size of a relation without duplicating records, but still loads the relation. The length method existence needs to be checked because you can pass in an `Enumerator`, which does not respond to `length`. * Ensure unsubscribed from notifications after tests [Rafael Mendonça França + aar0nr]
Fixes #40837
When rendering collections, calling
sizewhen the collection is anActiveRecord relation causes unwanted
SELECT COUNT(*)queries. Thischange ensures the collection is loaded before getting the size.