Last Updated: 2024-11-21

In a controller, I saw:

def load_sidebar_taxons
  @sidebar_taxons = Rails.cache.fetch("taxons_db_data_#{current_store}") do
    Taxonomy.for_store(current_store)
end

The view looped through these taxonomies and looped on the contained Taxon models. I immediately thought "major N+1" issue and spent 20 minutes rewriting.

But, in fact, the for_store method already anticipated this by having includes(:taxons) chained. I would have known this if I had either - a. checked the code in full by looking into the guts of a method that I assumed was broken when it wasn't - b. (better) verified in my logs that the database was actually hit N+1 times

Lesson

Only refactor after verifying something is actually an issue. This has the advantage of letting you verify that your solution actually made an improvement.

Another thing that cropped up here: it would have been good to establish expectations about where the includes for avoiding N+1 go - is it at the controller or model level? My inconsistency about this lead to the issue in some sense.