Add lazy-load guard to detect N+1 relation queries by KalimeroMK · Pull Request #591 · yiisoft/active-record

KalimeroMK · 2026-09-20T21:54:29Z

Closes #590

What — LazyLoadGuardTrait overrides retrieveRelation() and reports every lazy load to LazyLoadGuard, which has Log (default) / Strict modes and is configured statically: LazyLoadGuard::set(LazyLoadGuardMode::Strict) or LazyLoadGuard::set(LazyLoadGuardMode::Log, $logger). No events and no dispatcher configuration; models without the trait are not affected. Meant for development and testing.

How — retrieveRelation() is the single lazy-load path; with() populates $related up front so relation() returns before reaching it. Works together with EventsTrait, which doesn't override retrieveRelation(). psr/log added to require-dev + suggest and registered in composer-dependency-analyser.php alongside the other opt-in packages.

Tests — tests/LazyLoadGuardTest.php, 9 cases across all five drivers: counting lazy loads, not counting after with(), each mode, combination with EventsTrait, and reset().

Sqlite suite green (467 tests, 2022 assertions). Psalm, Rector, PHP CS Fixer and composer-dependency-analyser clean.

coderabbitai · 2026-09-20T21:54:35Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: a294c025-7e60-427e-a6cb-22c2f5957b9a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-09-20T21:58:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.70%. Comparing base (00729d8) to head (4e3d408).

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #591      +/-   ##
============================================
+ Coverage     99.69%   99.70%   +0.01%     
- Complexity      662      686      +24     
============================================
  Files            43       47       +4     
  Lines          1622     1693      +71     
============================================
+ Hits           1617     1688      +71     
  Misses            5        5

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Tigrov

Two questions:

Is this only for development or debugging / profiling?
Why is LazyLoadGuard not designed as a class attribute #[Attribute(Attribute::TARGET_CLASS)]? It will not require properties $only and $except if apply the attribute to a specified AR class.

KalimeroMK · 2026-09-23T07:49:14Z

Dev/test only — Strict in the suite, Log on staging.
The mode is an environment decision, not a model one, and a logger can't go through attribute arguments.

only/except/Off dropped in 174a71d.

Tigrov

The solution looks a bit complicated. Perhaps better to move it to a separate trait where the counter will be enabled inside the method retrieveRelation() without using events. Then it will allow to use only this trait without difficult configurations.

KalimeroMK · 2026-09-23T09:27:59Z

Reworked into a separate LazyLoadGuardTrait that overrides retrieveRelation() and counts directly — no events. Configured statically, like ConnectionProvider. Docs in docs/traits/lazy-load-guard.md.

samdark · 2026-10-11T10:24:45Z

findByPk(1)->getOrders()
findByPk(2)->getOrders()

Would that trigger false positive?

samdark · 2026-10-11T10:25:57Z

Docs mention "every lazy load" but it looks like it's 2nd lazy load now.

samdark · 2026-10-11T10:32:38Z

+    /**
+     * Sets the mode of the guard and the logger used in the {@see LazyLoadGuardMode::Log} mode.
+     */
+    public static function set(LazyLoadGuardMode $mode, ?LoggerInterface $logger = null): void


Logger isn't used in strict mode.

samdark · 2026-10-11T10:34:30Z

-        if (!isset(self::$dispatchers[$targetClass])) {
-            self::$dispatchers[$targetClass] = new Dispatcher(new Provider(self::getListenersFromAttributes($targetClass)));
-        }
+        self::$dispatchers[$targetClass] ??= new Dispatcher(new Provider(self::getListenersFromAttributes($targetClass)));


Not related?

KalimeroMK · 2026-10-11T10:43:00Z

Yes. Fixed in 4a715b6 — counted per result set now, grouped in ActiveQuery::populate(), which both all() and findByPk() go through.

foreach (Customer::query()->all() as $c) { $c->getOrders(); }  // reported
Customer::query()->findByPk(1)->getOrders();                   // not reported
Customer::query()->findByPk(2)->getOrders();

Docs wording fixed in b12bf63 — LazyLoadGuardMode said "every lazy load" too.

KalimeroMK · 2026-10-11T11:04:04Z

@samdark

Result set of more than 1 row — 4a715b6
Reset between unit tests — 7425406
LazyLoadGuardMode phpdoc — b12bf63
Count and the with() call in the message — ab9e220
MagicPropertiesTrait covered — 2a8dd34
Logger in Strict — docblock now says it's Log only. Can log before throwing instead, say which.
EventDispatcherProvider — the rector job rewrites it; revert it and it comes back.

FriendlyException skipped: not in composer.json, only transitive. Add it?

samdark · 2026-10-11T17:36:31Z

FriendlyException is a very light dependency, so yes, no problem with it.

KalimeroMK · 2026-10-11T18:15:18Z

Added in 8e97cf1.

samdark · 2026-10-11T21:34:31Z

+     */
+    public static function reset(): void
+    {
+        self::$mode = LazyLoadGuardMode::Log;


Docs suggest calling reset() after each request in long-running workers, but it drops the mode and the logger too, so they have to be configured again every time. I'd either make reset() clear counters only, or add a separate method for that.

samdark · 2026-10-11T21:34:32Z

+use Yiisoft\Test\Support\EventDispatcher\SimpleEventDispatcher;
+use Yiisoft\Test\Support\Log\SimpleLogger;
+
+abstract class LazyLoadGuardTest extends TestCase


Missing cases:

A record re-loading a relation after refresh() / resetRelation().

asArray() result sets.

Records eager loaded with with() that then lazy load a nested relation, e.g. Customer::query()->with('orders')->all() and then $order->getItems() for each order.

…rectly A record re-reading a relation after refresh() or resetRelation() was counted as another record of the result set, and the per-result-set counters were never freed. Both are gone with the counts held by a result set object in the WeakMap. class_uses() only returns the traits a class uses directly, so a model using the guard trait through a trait of its own was never guarded, and only the first model of a result set was checked. reset() dropped the mode and the logger too, which made it unusable between the requests of a worker; resetCounters() keeps them.

The suggested with() call named Customer whatever the model was, and offered a mode the enum doesn't have.

A WeakMap held as the value of a WeakMap entry keyed by the same record segfaults PHP when the records are freed. The result set only outlives its own records, so their ids are enough to count them.

KalimeroMK · 2026-10-11T22:16:43Z

@samdark

Records of a result set are counted instead of lazy loads, so a record reading the relation again after refresh() or resetRelation() is counted once, and the counts are freed with the result set — 862e620, 64fec4e
The trait is detected through parent classes and other traits, and every model of a set is checked — 862e620
resetCounters() keeps the mode and the logger, reset() still restores everything — 862e620
The solution names the model and drops the Disabled mode, Strict phpdoc applied — 98c15b1
Cases for a re-loaded relation, asArray() sets, a nested lazy load after with() and the indirect trait — a891589, 2b0d82f
EventDispatcherProvider and MagicRelationsTrait are what rector-cs.yml rewrites repo-wide; it commits them itself if the branch doesn't carry them.

Tigrov · 2026-10-12T02:45:20Z

+            if (!$model instanceof ActiveRecordInterface || !self::isGuarded($model::class)) {
+                continue;
+            }


Can it be checked one time out of the loop?

I'd like to, but that's the version @samdark asked me to change: it checked only the first model, and a set isn't guaranteed to be of one class, since createModels() returns whatever a resultCallback hands back. So it's one cached lookup per row now. If you both read a result set as always homogeneous, I'll happily move it out of the loop.

I don't think it can be moved out of the loop without false positives.

Tigrov · 2026-10-12T02:49:36Z

+        $resultSet = $resultSets[$model] ?? null;
+
+        if ($resultSet === null) {
+            /** A record populated outside a result set is a result set of its own. */
+            $resultSet = new LazyLoadResultSet();
+            $resultSets[$model] = $resultSet;
+        }


Suggested change

$resultSet = $resultSets[$model] ?? null;

if ($resultSet === null) {

/** A record populated outside a result set is a result set of its own. */

$resultSet = new LazyLoadResultSet();

$resultSets[$model] = $resultSet;

}

$resultSets[$model] ??= new LazyLoadResultSet();

$resultSet = $resultSets[$model];

Psalm at level 1 rejects it: PossiblyNullArgument on WeakMap::offsetSet() and PossiblyNullReference on the registerLazyLoad() call.

Static analysis should not compromise code quality by necessitating increased complexity.
Either needs to find the appropriate annotations for static analysis or suspend it.

Tigrov · 2026-10-12T03:02:25Z

+        $resultSets = self::$resultSets ?? new WeakMap();
+
+        return self::$resultSets = $resultSets;


Suggested change

$resultSets = self::$resultSets ?? new WeakMap();

return self::$resultSets = $resultSets;

return self::$resultSets ??= new WeakMap();

Psalm at level 1 infers new WeakMap() as WeakMap<object, mixed>, so it reports PropertyTypeCoercion, LessSpecificReturnStatement and MoreSpecificReturnType for it.

Static analysis should not compromise code quality by necessitating increased complexity.
Either needs to find the appropriate annotations for static analysis or suspend it.

The inline @psalm-var is exactly the "appropriate annotation" option — no suppression needed, and the one-liner fails Psalm level 1 with PropertyTypeCoercion / LessSpecificReturnStatement / MoreSpecificReturnType, since new WeakMap() infers as WeakMap<object, mixed>. Keeping it as is unless you prefer a @psalm-suppress instead.

Tigrov · 2026-10-12T03:04:21Z

+                continue;
+            }
+
+            $resultSet ??= new LazyLoadResultSet();


Why not to move this outside?

Tigrov · 2026-10-12T03:07:06Z

+        if (count($models) > 1) {
+            LazyLoadGuard::registerResultSet($models);
+        }


It is concerning that this code will always execute, even when N+1 query detection is not needed.

Why is it needed if you always create new LazyLoadResultSet() there https://github.com/yiisoft/active-record/pull/591/changes#diff-d6bdeea9079931e23a1a13d2c4a1974b57a13a8f0ec17e84fea80aa6c33be4dfR93

Fair point, I measured it to be sure: registering 10,000 unguarded records takes 0.93 ms against 45.71 ms of hydrating them, so it's ~2% in the worst case I could build. If that's still too much, the cheapest fix is an explicit switch checked first thing here. @samdark had me drop the Off mode in 174a71d, so I'd rather not bring one back on my own — what do you two prefer, always on and cheap, or a switch again?

But that's not consuming anything if not registered, right? So not an issue in production and very little resource consumption in dev/staging. That's acceptable to me.

registering 10,000 unguarded records takes 0.93 ms against 45.71 ms

For debugging / profiling it is okay

Given this https://github.com/yiisoft/active-record/pull/591/changes#r4244295174, it's better to remove this in production.

Tigrov · 2026-10-12T03:10:45Z

+            }
+
+            $resultSet ??= new LazyLoadResultSet();
+            self::resultSets()[$model] = $resultSet;


Call of self::resultSets() also possible to move outside:
For example self::$resultSets ??= new WeakMap();

zoranbogoevskimkd · 2026-10-12T07:19:33Z

@Tigrov

The map and the result set are taken once per set — 4e3d408
Both ??= forms fail Psalm at level 1: $resultSets[$model] ??= new LazyLoadResultSet() reports PossiblyNullArgument and PossiblyNullReference on WeakMap::offsetGet(), and self::$resultSets ??= new WeakMap() reports PropertyTypeCoercion because new WeakMap() is inferred as WeakMap<object, mixed>.
The trait check stays per model: createModels() returns whatever a resultCallback hands back, so a result set isn't always of one class, and after the first call per class isGuarded() is an array lookup. Registering 10,000 unguarded records takes 0.93 ms against 45.71 ms of hydrating them.
registerResultSet() is what groups the records of one query. The result set created in check() covers a single record, so without the grouping a relation read by every record of a query would never be reported.

KalimeroMK · 2026-10-12T11:45:35Z

@Tigrov Both ??= forms are confirmed locally against psalm.xml at level 1, the errors are quoted in the two threads above.

KalimeroMK · 2026-10-18T21:02:26Z

Merged earlier than intended, remaining points tracked in #595 and #594. Happy to revert if you prefer.

reset() dropping the mode and the logger — resetCounters(), 862e620
Logger unused in strict mode — ab9e220
Missing test cases — a891589
resultSets() inside the loop — 4e3d408

Add BeforeLazyRelationLoad event and LazyLoadGuard

27007b4

KalimeroMK added 3 commits September 21, 2026 00:02

Cover prevented BeforeLazyRelationLoad event

c1b49e7

Apply Rector suggestion in EventDispatcherProvider

0a27a4f

Fix double event dispatch in tests, count in strict mode, document event

37fc942

KalimeroMK marked this pull request as ready for review September 21, 2026 05:23

Tigrov reviewed Sep 23, 2026

View reviewed changes

Drop Off mode and only/except from LazyLoadGuard

174a71d

Tigrov reviewed Sep 23, 2026

View reviewed changes

Comment thread src/Trait/EventsTrait.php Outdated

Comment thread docs/traits/events.md Outdated

Move lazy load guard to a separate trait without events

39f3a9d

Tigrov reviewed Sep 23, 2026

View reviewed changes

Comment thread src/LazyLoadGuard.php

Tigrov requested a review from a team September 23, 2026 11:26

KalimeroMK added 3 commits September 23, 2026 15:01

Report lazy load starting from the second load of the same relation

c06c21e

Merge branch 'master' into lazy-load-guard

c0a004d

Apply Rector suggestion in MagicRelationsTrait

21b0bf5

samdark requested changes Oct 11, 2026

View reviewed changes

KalimeroMK added 2 commits October 11, 2026 12:38

Count lazy loads per result set

4a715b6

Clarify when a lazy load is reported

b12bf63

KalimeroMK added 3 commits October 11, 2026 13:02

Report the count and the relation to eager load

ab9e220

Cover MagicPropertiesTrait usage

2a8dd34

Document the report and resetting the guard between tests

7425406

Throw a friendly exception when the guard reports in strict mode

8e97cf1

samdark reviewed Oct 11, 2026

View reviewed changes

Comment thread src/LazyLoadException.php Outdated

samdark reviewed Oct 11, 2026

View reviewed changes

KalimeroMK added 5 commits October 12, 2026 00:08

Name the model in the exception solution

98c15b1

The suggested with() call named Customer whatever the model was, and offered a mode the enum doesn't have.

Cover re-loaded relations, asArray sets, nested and indirect guards

a891589

Document record counting, indirect trait use and resetCounters()

2b0d82f

Track the records of a result set by id, not in a WeakMap

64fec4e

A WeakMap held as the value of a WeakMap entry keyed by the same record segfaults PHP when the records are freed. The result set only outlives its own records, so their ids are enough to count them.

Tigrov reviewed Oct 12, 2026

View reviewed changes

Take the result set and the map once per registered set

4e3d408

KalimeroMK merged commit a4d53b2 into yiisoft:master Oct 18, 2026
47 checks passed

zoranbogoevskimkd mentioned this pull request Oct 18, 2026

Follow-up on the lazy-load guard (#591) #593

Closed

KalimeroMK mentioned this pull request Oct 18, 2026

Follow-up on the lazy-load guard (#591) #595

Closed

		$resultSets = self::$resultSets ?? new WeakMap();

		return self::$resultSets = $resultSets;

Conversation

KalimeroMK commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

codecov Bot commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Tigrov left a comment

Choose a reason for hiding this comment

Uh oh!

KalimeroMK commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tigrov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KalimeroMK commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

samdark commented Oct 11, 2026

Uh oh!

samdark commented Oct 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KalimeroMK commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KalimeroMK commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samdark commented Oct 11, 2026

Uh oh!

KalimeroMK commented Oct 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KalimeroMK commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KalimeroMK Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KalimeroMK Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

KalimeroMK commented Sep 20, 2026 •

edited

Loading

coderabbitai Bot commented Sep 20, 2026 •

edited

Loading

codecov Bot commented Sep 20, 2026 •

edited

Loading

KalimeroMK commented Sep 23, 2026 •

edited

Loading

KalimeroMK commented Sep 23, 2026 •

edited

Loading

KalimeroMK commented Oct 11, 2026 •

edited

Loading

KalimeroMK commented Oct 11, 2026 •

edited

Loading

KalimeroMK commented Oct 11, 2026 •

edited

Loading

KalimeroMK Oct 12, 2026 •

edited

Loading

KalimeroMK Oct 12, 2026 •

edited

Loading

KalimeroMK commented Oct 18, 2026 •

edited

Loading