Skip to content

Preload item associations in ClosureRepository::findByNavigation - #8

Open
tannyl wants to merge 2 commits into
Setono:masterfrom
tannyl:fix/find-by-navigation-n-plus-one
Open

Preload item associations in ClosureRepository::findByNavigation#8
tannyl wants to merge 2 commits into
Setono:masterfrom
tannyl:fix/find-by-navigation-n-plus-one

Conversation

@tannyl

@tannyl tannyl commented Aug 4, 2026

Copy link
Copy Markdown

Problem

ClosureRepository::findByNavigation() joins the descendant Item purely for filtering; it does not addSelect() or fetch-join any of the item's associations. As a result:

  • GraphBuilder::build() calls $closure->getDescendant(), $closure->getAncestor(), and $descendant->hasChannel() for every closure — each triggers a lazy load.
  • Rendering templates then call $node.item.label (translation lookup: LIMIT 1 + all + COUNT) and, for TaxonItem, $node.item.taxon and $taxon.name/.slug (taxon + taxon translation) — again lazy per item.

The result is a classic N+1 that scales linearly with menu size. On a store with 732 navigation items, rendering the megamenu issued 5170 database queries per request (query time 570 ms) — verified against the real production data
set. CachedNavigationRenderer masks this in production most of the time, but the cost is paid on every cold cache (deploy, cache clear, or when the invalidation listeners fire after any admin edit) and on every dev request (where the
cache is off by default).

Fix

Preload the associations in findByNavigation():

  1. Fetch closures with descendant and ancestor Items in the main query (addSelect('item', 'ancestor') plus join('o.ancestor', 'ancestor')).
  2. Run separate preload queries for item.translations, item.channels, and TaxonItem.taxon + taxon.translations. The results attach to the already-hydrated Items via the Doctrine identity map.

Splitting into multiple queries (rather than fetch-joining everything in the main query) avoids the cartesian product of closures × item.translations × item.channels × taxon.translations that would otherwise blow up the result set.

Impact

Measured on the same 732-item menu after the patch:

Before After
Database queries 5170 130
Query time 570 ms 53 ms
Managed entities 4758 4898

Same rows/columns are retrieved end-to-end — only the round-trip count changes. A small over-fetch (~3 %) occurs because associations are now preloaded for every item in the navigation, including items that GraphBuilder later filters
out (disabled, wrong channel, or excluded ancestor). Negligible in practice.

Cold-cache first-render cost drops from ~570 ms to ~50 ms, which also makes the postUpdate/postPersist/postRemove cache invalidations from ItemBasedNavigationCacheInvalidatorListener and
TaxonBasedNavigationCacheInvalidatorListener non-painful.

Tests

Existing it_finds_closures_by_navigation continues to pass (return contract unchanged). Added it_preloads_item_associations_when_finding_by_navigation — after findByNavigation() + EntityManager::clear(), verifies that the
descendant Item, its translations collection, channels collection, TaxonItem.taxon, and taxon.translations are all initialized (no lazy proxies, PersistentCollection::isInitialized() returns true).

Local run:

  • composer check-style
  • vendor/bin/rector process --dry-run
  • composer analyse
  • composer phpunit -- --testsuite=unit — 289 tests ✓
  • composer phpunit -- --testsuite=functional — 37 tests ✓ (was 36 + 1 new)

Both GraphBuilder::build() and rendering templates iterate every closure
and touch $closure->getAncestor()/getDescendant() plus the item's
translations, channels, and (for TaxonItem) linked taxon. Because the
existing query only used the descendant join for filtering and did not
addSelect/leftJoin the associations, each per-item access triggered a
lazy load — thousands of extra queries on menus with hundreds of items.

On a real store with 732 navigation items the rendered megamenu dropped
from 5170 to 130 database queries per request (query time 570 ms → 53 ms),
verified against the production data set.

The associations are preloaded via separate queries so no single query
carries the cartesian product of multiple to-many joins; results attach
to the already-hydrated Item instances through the Doctrine identity map.

Adds a functional test that verifies translations, channels, taxon, and
taxon translations are all initialized after findByNavigation() returns.
Comment thread src/Repository/ClosureRepository.php Outdated
Comment thread src/Repository/ClosureRepository.php Outdated
Comment thread src/Repository/ClosureRepository.php Outdated
The repository referenced ItemInterface and TaxonItemInterface directly in
the DQL from() clauses, which leans on the resolve_target_entity mapping
instead of the classes actually configured for the resources. Inject the
configured model classes through the constructor instead, the same way the
form types already receive them.

This also covers the two pre-existing from(ItemInterface::class) calls in
findRootItems() and findDirectChildren(), so the whole repository follows
one convention rather than leaving a mix behind.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XfH6cmm4gJMt6Km7EeK3Uo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants