From 71d9bc908597ada46e0a3ff15adb2a287585d11c Mon Sep 17 00:00:00 2001 From: smarcet Date: Sun, 24 May 2026 23:01:29 -0300 Subject: [PATCH 1/7] chore(profiling): enable Server-Timing + SQL pattern logger on /summits/{id}/attendees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same instrumentation pattern as /events (see adr/002): - ServerTimingDoctrine on the route (before auth.user) - timing markers in the shared ParametrizedGetAll trait - re-enabled temporary SQL pattern logger in QueryTimingCollector + 'N+1 candidate' log entries when db_count >= 20 Diagnostic-only — will be removed in cleanup once N+1s are identified and fixed. --- .../Summit/Traits/ParametrizedGetAll.php | 4 ++ .../Doctrine/QueryTimingCollector.php | 52 ++++++++++++++++++- .../Doctrine/QueryTimingMiddleware.php | 16 ++++-- app/Http/Middleware/ServerTimingDoctrine.php | 12 +++++ routes/api_v1.php | 2 +- 5 files changed, 79 insertions(+), 7 deletions(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/Traits/ParametrizedGetAll.php b/app/Http/Controllers/Apis/Protected/Summit/Traits/ParametrizedGetAll.php index 72548bbdf..ad125cf06 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/Traits/ParametrizedGetAll.php +++ b/app/Http/Controllers/Apis/Protected/Summit/Traits/ParametrizedGetAll.php @@ -85,6 +85,7 @@ public function _getAll array $serializerParams = [] ) { + Session::put('timing.controller_start', microtime(true)); return $this->processRequest(function () use ( $getFilterRules, $getFilterValidatorRules, @@ -140,6 +141,7 @@ public function _getAll ); $dbEnd = (microtime(true)-$dbStart)*1000; $transformStart = microtime(true); + Session::put('timing.serializer_start', microtime(true)); $serializerParams['filter'] = $filter; $res = $data->toArray ( @@ -149,6 +151,7 @@ public function _getAll $serializerParams, $serializerType && is_callable($serializerType) ? call_user_func($serializerType) : SerializerRegistry::SerializerType_Public ); + Session::put('timing.serializer_end', microtime(true)); $transformEnd = (microtime(true)-$transformStart)*1000; $encodeStart = microtime(true); $json_response = $this->ok($res); @@ -156,6 +159,7 @@ public function _getAll Session::put("db_time", $dbEnd ); Session::put("transform_time", $transformEnd ); Session::put("encode_time", $encodeEnd ); + Session::put('timing.controller_end', microtime(true)); Session::save(); return $json_response; }); diff --git a/app/Http/Middleware/Doctrine/QueryTimingCollector.php b/app/Http/Middleware/Doctrine/QueryTimingCollector.php index f20c70b86..2984774b9 100644 --- a/app/Http/Middleware/Doctrine/QueryTimingCollector.php +++ b/app/Http/Middleware/Doctrine/QueryTimingCollector.php @@ -15,15 +15,63 @@ class QueryTimingCollector public static float $totalMs = 0.0; public static int $count = 0; - public static function record(float $startedAt): void + /** + * Per-pattern bucket for finding N+1s during profiling. + * + * @var array + */ + public static array $patterns = []; + + public static function record(float $startedAt, ?string $sql = null): void { - self::$totalMs += (microtime(true) - $startedAt) * 1000.0; + $ms = (microtime(true) - $startedAt) * 1000.0; + self::$totalMs += $ms; self::$count++; + + if ($sql !== null) { + $pattern = self::normalize($sql); + if (!isset(self::$patterns[$pattern])) { + self::$patterns[$pattern] = ['count' => 0, 'totalMs' => 0.0, 'sample' => $sql]; + } + self::$patterns[$pattern]['count']++; + self::$patterns[$pattern]['totalMs'] += $ms; + } } public static function reset(): void { self::$totalMs = 0.0; self::$count = 0; + self::$patterns = []; + } + + /** + * Top N most-repeated patterns (count >= 2), sorted by count desc. + * + * @return array + */ + public static function topPatterns(int $limit = 10): array + { + $rows = []; + foreach (self::$patterns as $pattern => $stats) { + if ($stats['count'] < 2) continue; + $rows[] = [ + 'pattern' => $pattern, + 'count' => $stats['count'], + 'totalMs' => round($stats['totalMs'], 1), + 'sample' => $stats['sample'], + ]; + } + usort($rows, fn($a, $b) => $b['count'] <=> $a['count']); + return array_slice($rows, 0, $limit); + } + + private static function normalize(string $sql): string + { + $s = preg_replace('/\?|:[a-zA-Z_][a-zA-Z0-9_]*/', '?', $sql); + $s = preg_replace("/'[^']*'/", '?', $s); + $s = preg_replace('/\b\d+\b/', '?', $s); + $s = preg_replace('/\s+/', ' ', $s); + return trim($s); } } diff --git a/app/Http/Middleware/Doctrine/QueryTimingMiddleware.php b/app/Http/Middleware/Doctrine/QueryTimingMiddleware.php index c0581efac..101a27b31 100644 --- a/app/Http/Middleware/Doctrine/QueryTimingMiddleware.php +++ b/app/Http/Middleware/Doctrine/QueryTimingMiddleware.php @@ -43,7 +43,7 @@ public function query(string $sql): DBALResult try { return parent::query($sql); } finally { - QueryTimingCollector::record($start); + QueryTimingCollector::record($start, $sql); } } @@ -53,25 +53,33 @@ public function exec(string $sql): int try { return parent::exec($sql); } finally { - QueryTimingCollector::record($start); + QueryTimingCollector::record($start, $sql); } } public function prepare(string $sql): DBALStatement { - return new QueryTimingStatement(parent::prepare($sql)); + return new QueryTimingStatement(parent::prepare($sql), $sql); } } class QueryTimingStatement extends AbstractStatementMiddleware { + private string $sql; + + public function __construct($wrapped, string $sql) + { + parent::__construct($wrapped); + $this->sql = $sql; + } + public function execute($params = null): DBALResult { $start = microtime(true); try { return parent::execute($params); } finally { - QueryTimingCollector::record($start); + QueryTimingCollector::record($start, $this->sql); } } } diff --git a/app/Http/Middleware/ServerTimingDoctrine.php b/app/Http/Middleware/ServerTimingDoctrine.php index b22831dfc..d54cd0cde 100644 --- a/app/Http/Middleware/ServerTimingDoctrine.php +++ b/app/Http/Middleware/ServerTimingDoctrine.php @@ -3,6 +3,7 @@ namespace App\Http\Middleware; use Closure; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Session; use Symfony\Component\HttpFoundation\Response; @@ -53,6 +54,17 @@ public function handle($request, Closure $next): Response ); $response->headers->set('Timing-Allow-Origin', '*'); + // Temporary N+1 candidate logger (profiling-only — remove on cleanup). + if ($dbCount >= 20) { + foreach (\App\Http\Middleware\Doctrine\QueryTimingCollector::topPatterns(10) as $row) { + Log::warning('N+1 candidate', [ + 'count' => $row['count'], + 'totalMs' => $row['totalMs'], + 'sample' => mb_substr($row['sample'], 0, 240), + ]); + } + } + return $response; } } diff --git a/routes/api_v1.php b/routes/api_v1.php index db069b5a6..b7770be4b 100644 --- a/routes/api_v1.php +++ b/routes/api_v1.php @@ -1606,7 +1606,7 @@ Route::put('send', ['middleware' => 'auth.user', 'uses' => 'OAuth2SummitAttendeesApiController@send']); }); - Route::get('', ['middleware' => 'auth.user', 'uses' => 'OAuth2SummitAttendeesApiController@getAttendeesBySummit']); + Route::get('', ['middleware' => ['server.timing.doctrine', 'auth.user'], 'uses' => 'OAuth2SummitAttendeesApiController@getAttendeesBySummit']); Route::get('csv', ['middleware' => 'auth.user', 'uses' => 'OAuth2SummitAttendeesApiController@getAttendeesBySummitCSV']); Route::group(['prefix' => 'me'], function () { Route::get('', 'OAuth2SummitAttendeesApiController@getOwnAttendee'); From dfa430ba9f8428d1ba680cae9e70ecf91d5f6eb6 Mon Sep 17 00:00:00 2001 From: smarcet Date: Sun, 24 May 2026 23:15:17 -0300 Subject: [PATCH 2/7] =?UTF-8?q?fix(attendees):=20batch-preload=20Summit::g?= =?UTF-8?q?etSpeakerByMember=20for=20the=20page=20=E2=80=94=20eliminates?= =?UTF-8?q?=20~24=20queries?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /attendees profiling showed 3 DISTINCT PresentationSpeaker patterns firing ~8 times each per request (24 total). Traced to SummitAttendeeSerializer:133 $summit->getSpeakerByMember($member), which calls getSpeakerByMemberId() which runs THREE separate queries per call (moderator check, speaker check, assistance check). Two-part fix: 1. Summit gains a request-scoped $speakerByMemberIdCache (unannotated so Doctrine ignores it) and getSpeakerByMemberId() now checks/writes the cache at every return point. By itself this is just insurance against repeated lookups within a request. 2. New Summit::preloadSpeakersByMemberIds(array $ids) runs the same 3 lookup steps but with WHERE mb.id IN (:ids), then populates the cache for every member id (with null for the ones not found). Result: 3 batch queries instead of N×3 per-attendee queries. 3. ParametrizedGetAll trait grows an optional $afterQuery callable param that fires between the data load and the toArray() call so callers can warm caches without modifying the trait body for each endpoint. 4. OAuth2SummitAttendeesApiController::getAttendeesBySummit passes a closure that collects the page's attendee member ids and invokes $summit->preloadSpeakersByMemberIds($ids) before serialization. --- .../OAuth2SummitAttendeesApiController.php | 20 +++- .../Summit/Traits/ParametrizedGetAll.php | 14 ++- app/Models/Foundation/Summit/Summit.php | 94 ++++++++++++++++++- 3 files changed, 121 insertions(+), 7 deletions(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php index 0f4c7f3aa..24e062d64 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php @@ -701,7 +701,25 @@ function () { null, null, null, - $params + $params, + // afterQuery hook: batch-preload Summit::getSpeakerByMemberId() cache + // for every attendee's member on this page. Collapses the per-attendee + // 3-query speaker lookup (moderator/speaker/assistance) into 3 batch + // queries total — biggest /attendees N+1 (~24 queries on a 10-row page). + function ($data) use ($summit) { + $memberIds = []; + foreach ($data->getItems() as $attendee) { + if (method_exists($attendee, 'getMember')) { + $m = $attendee->getMember(); + if ($m !== null && method_exists($m, 'getId') && $m->getId()) { + $memberIds[] = $m->getId(); + } + } + } + if (!empty($memberIds)) { + $summit->preloadSpeakersByMemberIds($memberIds); + } + } ); } diff --git a/app/Http/Controllers/Apis/Protected/Summit/Traits/ParametrizedGetAll.php b/app/Http/Controllers/Apis/Protected/Summit/Traits/ParametrizedGetAll.php index ad125cf06..8e28865e5 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/Traits/ParametrizedGetAll.php +++ b/app/Http/Controllers/Apis/Protected/Summit/Traits/ParametrizedGetAll.php @@ -82,7 +82,8 @@ public function _getAll callable $defaultOrderRules = null, callable $defaultPageSize = null, callable $queryCallable = null, - array $serializerParams = [] + array $serializerParams = [], + callable $afterQuery = null ) { Session::put('timing.controller_start', microtime(true)); @@ -95,7 +96,8 @@ public function _getAll $defaultOrderRules, $defaultPageSize, $queryCallable, - $serializerParams + $serializerParams, + $afterQuery ) { $values = Request::all(); @@ -140,6 +142,14 @@ public function _getAll $applyExtraFilters ); $dbEnd = (microtime(true)-$dbStart)*1000; + + // Optional post-load hook so callers can pre-populate caches / + // batch-load related entities before serialization runs. Receives + // the PagingResponse so it can inspect items. + if (is_callable($afterQuery)) { + $afterQuery($data); + } + $transformStart = microtime(true); Session::put('timing.serializer_start', microtime(true)); $serializerParams['filter'] = $filter; diff --git a/app/Models/Foundation/Summit/Summit.php b/app/Models/Foundation/Summit/Summit.php index 0c6160e96..6efd5e4d9 100644 --- a/app/Models/Foundation/Summit/Summit.php +++ b/app/Models/Foundation/Summit/Summit.php @@ -2146,6 +2146,15 @@ public function getSpeakers() return array_merge($speakers, $moderators); } + /** + * Request-scoped cache for getSpeakerByMemberId(). + * Keyed by "{member_id}:{filter_published_events:0|1}". + * Unannotated so Doctrine ignores it; reset naturally per request. + * + * @var array + */ + private array $speakerByMemberIdCache = []; + /** * @param Member $member * @return PresentationSpeaker|null @@ -2162,6 +2171,11 @@ public function getSpeakerByMember(Member $member) */ public function getSpeakerByMemberId($member_id, $filter_published_events = true) { + $cacheKey = $member_id . ':' . ($filter_published_events ? '1' : '0'); + if (array_key_exists($cacheKey, $this->speakerByMemberIdCache)) { + return $this->speakerByMemberIdCache[$cacheKey]; + } + // moderators $moderator = $this->buildModeratorsQuery($filter_published_events) ->join('ps.member', 'mb') @@ -2169,7 +2183,7 @@ public function getSpeakerByMemberId($member_id, $filter_published_events = true ->setParameter('member_id', $member_id) ->getQuery()->getOneOrNullResult(); - if (!is_null($moderator)) return $moderator; + if (!is_null($moderator)) return $this->speakerByMemberIdCache[$cacheKey] = $moderator; // speakers $speaker = $this->buildSpeakersQuery($filter_published_events) @@ -2178,7 +2192,7 @@ public function getSpeakerByMemberId($member_id, $filter_published_events = true ->setParameter('member_id', $member_id) ->getQuery()->getOneOrNullResult(); - if (!is_null($speaker)) return $speaker; + if (!is_null($speaker)) return $this->speakerByMemberIdCache[$cacheKey] = $speaker; // assistance $speaker = $this->buildSpeakerSummitAttendanceQuery() @@ -2187,9 +2201,81 @@ public function getSpeakerByMemberId($member_id, $filter_published_events = true ->setParameter('member_id', $member_id) ->getQuery()->getOneOrNullResult(); - if (!is_null($speaker)) return $speaker; + if (!is_null($speaker)) return $this->speakerByMemberIdCache[$cacheKey] = $speaker; - return null; + return $this->speakerByMemberIdCache[$cacheKey] = null; + } + + /** + * Batch pre-populate the getSpeakerByMemberId() cache for many members in + * 3 queries instead of 3 per member. Each subsequent + * getSpeakerByMemberId($mid) call then returns from the cache with no DB. + * + * Same lookup order as the per-member method: moderator → speaker → + * assistance. Members not found in any are cached as null so callers + * don't re-query them either. + * + * @param int[] $member_ids + */ + public function preloadSpeakersByMemberIds(array $member_ids, bool $filter_published_events = true): void + { + $member_ids = array_values(array_unique(array_filter($member_ids, fn($v) => $v > 0))); + if (empty($member_ids)) return; + + $remaining = array_flip($member_ids); + $found = []; + + // Moderators first. + $moderators = $this->buildModeratorsQuery($filter_published_events) + ->join('ps.member', 'mb') + ->andWhere('mb.id IN (:ids)') + ->setParameter('ids', array_keys($remaining)) + ->getQuery()->getResult(); + foreach ($moderators as $m) { + $mb = method_exists($m, 'getMember') ? $m->getMember() : null; + if ($mb !== null) { + $found[$mb->getId()] = $m; + unset($remaining[$mb->getId()]); + } + } + + // Speakers for whoever wasn't already found as moderator. + if (!empty($remaining)) { + $speakers = $this->buildSpeakersQuery($filter_published_events) + ->join('ps.member', 'mb') + ->andWhere('mb.id IN (:ids)') + ->setParameter('ids', array_keys($remaining)) + ->getQuery()->getResult(); + foreach ($speakers as $s) { + $mb = method_exists($s, 'getMember') ? $s->getMember() : null; + if ($mb !== null) { + $found[$mb->getId()] = $s; + unset($remaining[$mb->getId()]); + } + } + } + + // Assistance for the still-remaining. + if (!empty($remaining)) { + $speakers = $this->buildSpeakerSummitAttendanceQuery() + ->join('ps.member', 'mb') + ->andWhere('mb.id IN (:ids)') + ->setParameter('ids', array_keys($remaining)) + ->getQuery()->getResult(); + foreach ($speakers as $s) { + $mb = method_exists($s, 'getMember') ? $s->getMember() : null; + if ($mb !== null) { + $found[$mb->getId()] = $s; + unset($remaining[$mb->getId()]); + } + } + } + + // Populate cache: hits for found ones, null for the rest. + $flag = $filter_published_events ? '1' : '0'; + foreach ($member_ids as $mid) { + $this->speakerByMemberIdCache[$mid . ':' . $flag] = $found[$mid] ?? null; + } } /** From a6b24db839cd5170b852a41e4c3740d7cf00d5ba Mon Sep 17 00:00:00 2001 From: smarcet Date: Sun, 24 May 2026 23:22:01 -0300 Subject: [PATCH 3/7] fix(events): correct SummitSelectedPresentation preload DQL and cache reset on auth context change - DoctrineSummitEventRepository: change 'SELECT sp, p' to 'SELECT sp ... JOIN FETCH sp.presentation p' so getResult() returns SummitSelectedPresentation[] instead of mixed arrays. The prior query caused $sp->getPresentation() to fail on every request, silently falling through the try/catch and leaving the getSelectionStatus() N+1 optimization inactive. - ResourceServerContext: reset cachedCurrentUser/cachedCurrentUserResolved in setAuthorizationContext() and updateAuthContextVar() so a context change mid-request (or between requests in tests) does not return a stale member. --- app/Models/OAuth2/ResourceServerContext.php | 4 ++++ app/Repositories/Summit/DoctrineSummitEventRepository.php | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/Models/OAuth2/ResourceServerContext.php b/app/Models/OAuth2/ResourceServerContext.php index d4ad3fd90..e291c3e0d 100644 --- a/app/Models/OAuth2/ResourceServerContext.php +++ b/app/Models/OAuth2/ResourceServerContext.php @@ -126,6 +126,8 @@ public function getCurrentUserId() public function setAuthorizationContext(array $auth_context) { $this->auth_context = $auth_context; + $this->cachedCurrentUser = null; + $this->cachedCurrentUserResolved = false; } /** @@ -156,6 +158,8 @@ private function getAuthContextVar(string $varName) public function updateAuthContextVar(string $varName, $value):void { if(isset($this->auth_context[$varName])){ $this->auth_context[$varName] = $value; + $this->cachedCurrentUser = null; + $this->cachedCurrentUserResolved = false; } } diff --git a/app/Repositories/Summit/DoctrineSummitEventRepository.php b/app/Repositories/Summit/DoctrineSummitEventRepository.php index 42cfc7097..93498f5b8 100644 --- a/app/Repositories/Summit/DoctrineSummitEventRepository.php +++ b/app/Repositories/Summit/DoctrineSummitEventRepository.php @@ -801,9 +801,9 @@ public function getAllByPage(PagingInfo $paging_info, Filter $filter = null, Ord // Filters match getSelectionStatus()'s constants exactly. try { $selections = $em->createQuery( - 'SELECT sp, p FROM ' . SummitSelectedPresentation::class . ' sp ' . + 'SELECT sp FROM ' . SummitSelectedPresentation::class . ' sp ' . + 'JOIN FETCH sp.presentation p ' . 'JOIN sp.list l ' . - 'JOIN sp.presentation p ' . 'WHERE p.id IN (:ids) ' . 'AND sp.collection = :collection ' . 'AND l.list_type = :list_type ' . From 66ed55c622443f624fdabd859072cfa49e02db4a Mon Sep 17 00:00:00 2001 From: smarcet Date: Sun, 24 May 2026 23:22:45 -0300 Subject: [PATCH 4/7] fix(attendees): batch-preload Notes / Tickets / Tags / Member in afterQuery hook Collapses 4 more per-attendee N+1 patterns into 4 batch fetch-join queries: - Notes (SummitAttendeeNote, 10 q on a 10-row page) - Tickets (SummitAttendeeTicket, 10 q) - Tags ManyToMany (10 q) - Member ManyToOne fetch-join (8 q) Each runs as 'SELECT a, X FROM SummitAttendee a LEFT JOIN a.X X WHERE a.id IN (:ids)' which fetch-joins the inverse collection / association onto each already-loaded SummitAttendee in the UnitOfWork. Subsequent serializer accesses read from memory. Expected: 63 -> ~25 queries on /summits/{id}/attendees, db time roughly halved. --- .../OAuth2SummitAttendeesApiController.php | 63 +++++++++++++++++-- 1 file changed, 58 insertions(+), 5 deletions(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php index 24e062d64..a55ae46de 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php @@ -702,13 +702,17 @@ function () { null, null, $params, - // afterQuery hook: batch-preload Summit::getSpeakerByMemberId() cache - // for every attendee's member on this page. Collapses the per-attendee - // 3-query speaker lookup (moderator/speaker/assistance) into 3 batch - // queries total — biggest /attendees N+1 (~24 queries on a 10-row page). + // afterQuery hook: batch-preload everything the serializer is about + // to touch per-attendee. Collapses ~38 lazy-load queries per request + // (Notes/Tickets/Tags/Member/Speaker lookups) into 4 batch queries. function ($data) use ($summit) { + $items = $data->getItems(); + $attendeeIds = []; $memberIds = []; - foreach ($data->getItems() as $attendee) { + foreach ($items as $attendee) { + if (method_exists($attendee, 'getId') && $attendee->getId()) { + $attendeeIds[] = $attendee->getId(); + } if (method_exists($attendee, 'getMember')) { $m = $attendee->getMember(); if ($m !== null && method_exists($m, 'getId') && $m->getId()) { @@ -716,9 +720,58 @@ function ($data) use ($summit) { } } } + + // Speaker lookup cache (moderator/speaker/assistance × N members). if (!empty($memberIds)) { $summit->preloadSpeakersByMemberIds($memberIds); } + + if (!empty($attendeeIds)) { + $em = \LaravelDoctrine\ORM\Facades\Registry::getManager( + \models\utils\SilverstripeBaseModel::EntityManager + ); + + // Notes (SummitAttendeeNote) — one batch query covers all attendees. + try { + $em->createQuery( + 'SELECT a, n FROM ' . \models\summit\SummitAttendee::class . ' a ' . + 'LEFT JOIN a.notes n WHERE a.id IN (:ids)' + )->setParameter('ids', $attendeeIds)->getResult(); + } catch (\Exception $ex) { + \Illuminate\Support\Facades\Log::warning('attendees notes preload failed', ['error' => $ex->getMessage()]); + } + + // Tickets — fetch-join collection. + try { + $em->createQuery( + 'SELECT a, t FROM ' . \models\summit\SummitAttendee::class . ' a ' . + 'LEFT JOIN a.tickets t WHERE a.id IN (:ids)' + )->setParameter('ids', $attendeeIds)->getResult(); + } catch (\Exception $ex) { + \Illuminate\Support\Facades\Log::warning('attendees tickets preload failed', ['error' => $ex->getMessage()]); + } + + // Tags ManyToMany — fetch-join populates the inverse collection. + try { + $em->createQuery( + 'SELECT a, tg FROM ' . \models\summit\SummitAttendee::class . ' a ' . + 'LEFT JOIN a.tags tg WHERE a.id IN (:ids)' + )->setParameter('ids', $attendeeIds)->getResult(); + } catch (\Exception $ex) { + \Illuminate\Support\Facades\Log::warning('attendees tags preload failed', ['error' => $ex->getMessage()]); + } + + // Member fetch-join so $attendee->getMember() returns an + // initialized entity instead of triggering a per-attendee lazy load. + try { + $em->createQuery( + 'SELECT a, m FROM ' . \models\summit\SummitAttendee::class . ' a ' . + 'LEFT JOIN a.member m WHERE a.id IN (:ids)' + )->setParameter('ids', $attendeeIds)->getResult(); + } catch (\Exception $ex) { + \Illuminate\Support\Facades\Log::warning('attendees member preload failed', ['error' => $ex->getMessage()]); + } + } } ); } From b89922552ea085df1695f5c039149107471ff86b Mon Sep 17 00:00:00 2001 From: smarcet Date: Sun, 24 May 2026 23:29:13 -0300 Subject: [PATCH 5/7] fix(attendees): fetch-join Speaker.member and Ticket.badge in preloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-up N+1s exposed by removing the upstream lazy loads: 1. Speaker member fetch — the preloadSpeakersByMemberIds queries joined ps.member mb but did not addSelect('mb'), so Doctrine used the join only for filtering and the resulting speakers had unloaded Member proxies. Add ->addSelect('mb') to all three (moderator/speaker/assistance) batch queries. Saves ~8 Member queries per request. 2. SummitAttendeeBadge — the per-ticket badge lookup (12 q on a 10-row page) fired once per ticket. Extend the tickets preload from 'SELECT a, t' to 'SELECT a, t, b' with an extra LEFT JOIN t.badge b, fetch-joining the badge alongside the ticket. Expected: another ~20 queries removed, total drops from 49 to ~29. --- .../Summit/OAuth2SummitAttendeesApiController.php | 9 ++++++--- app/Models/Foundation/Summit/Summit.php | 6 +++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php index a55ae46de..b5893e700 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php @@ -741,11 +741,14 @@ function ($data) use ($summit) { \Illuminate\Support\Facades\Log::warning('attendees notes preload failed', ['error' => $ex->getMessage()]); } - // Tickets — fetch-join collection. + // Tickets — fetch-join collection AND the badge on each ticket + // (badges fire one query per ticket otherwise — was 12 per request). try { $em->createQuery( - 'SELECT a, t FROM ' . \models\summit\SummitAttendee::class . ' a ' . - 'LEFT JOIN a.tickets t WHERE a.id IN (:ids)' + 'SELECT a, t, b FROM ' . \models\summit\SummitAttendee::class . ' a ' . + 'LEFT JOIN a.tickets t ' . + 'LEFT JOIN t.badge b ' . + 'WHERE a.id IN (:ids)' )->setParameter('ids', $attendeeIds)->getResult(); } catch (\Exception $ex) { \Illuminate\Support\Facades\Log::warning('attendees tickets preload failed', ['error' => $ex->getMessage()]); diff --git a/app/Models/Foundation/Summit/Summit.php b/app/Models/Foundation/Summit/Summit.php index 6efd5e4d9..ea4ed20cd 100644 --- a/app/Models/Foundation/Summit/Summit.php +++ b/app/Models/Foundation/Summit/Summit.php @@ -2227,7 +2227,7 @@ public function preloadSpeakersByMemberIds(array $member_ids, bool $filter_publi // Moderators first. $moderators = $this->buildModeratorsQuery($filter_published_events) - ->join('ps.member', 'mb') + ->join('ps.member', 'mb')->addSelect('mb') ->andWhere('mb.id IN (:ids)') ->setParameter('ids', array_keys($remaining)) ->getQuery()->getResult(); @@ -2242,7 +2242,7 @@ public function preloadSpeakersByMemberIds(array $member_ids, bool $filter_publi // Speakers for whoever wasn't already found as moderator. if (!empty($remaining)) { $speakers = $this->buildSpeakersQuery($filter_published_events) - ->join('ps.member', 'mb') + ->join('ps.member', 'mb')->addSelect('mb') ->andWhere('mb.id IN (:ids)') ->setParameter('ids', array_keys($remaining)) ->getQuery()->getResult(); @@ -2258,7 +2258,7 @@ public function preloadSpeakersByMemberIds(array $member_ids, bool $filter_publi // Assistance for the still-remaining. if (!empty($remaining)) { $speakers = $this->buildSpeakerSummitAttendanceQuery() - ->join('ps.member', 'mb') + ->join('ps.member', 'mb')->addSelect('mb') ->andWhere('mb.id IN (:ids)') ->setParameter('ids', array_keys($remaining)) ->getQuery()->getResult(); From 98839439e9a4a44caede30475029ddd285b92967 Mon Sep 17 00:00:00 2001 From: smarcet Date: Sun, 24 May 2026 23:31:47 -0300 Subject: [PATCH 6/7] fix(events): low-severity findings + unit tests from PR #549 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - adr/002: fix heading ADR-0001 → ADR-0002 to match filename - PresentationSpeaker: add clearPreloadedAssignmentOrder(id) and clearAllPreloadedAssignmentOrders() so write paths can invalidate the preloaded assignment-order cache - PresentationSpeakerCacheTest (8 tests, no DB): covers cache hit, null order, clear-single, clear-all, and Presentation preloaded selection-status path (memoization + reset) - ResourceServerContextTest: add setAuthorizationContextResetsUserCache asserting cachedCurrentUserResolved is cleared by setAuthorizationContext() --- ...02-events-endpoint-n-plus-1-elimination.md | 2 +- .../Summit/Speakers/PresentationSpeaker.php | 10 ++ tests/PresentationSpeakerCacheTest.php | 143 ++++++++++++++++++ tests/ResourceServerContextTest.php | 29 ++++ 4 files changed, 183 insertions(+), 1 deletion(-) create mode 100644 tests/PresentationSpeakerCacheTest.php diff --git a/adr/002-events-endpoint-n-plus-1-elimination.md b/adr/002-events-endpoint-n-plus-1-elimination.md index 6bb2938e4..82deff69e 100644 --- a/adr/002-events-endpoint-n-plus-1-elimination.md +++ b/adr/002-events-endpoint-n-plus-1-elimination.md @@ -1,4 +1,4 @@ -# ADR-0001: Eliminate N+1 Queries in `/events` Endpoint +# ADR-0002: Eliminate N+1 Queries in `/events` Endpoint - **Status:** Accepted - **Date:** 2026-05-25 diff --git a/app/Models/Foundation/Summit/Speakers/PresentationSpeaker.php b/app/Models/Foundation/Summit/Speakers/PresentationSpeaker.php index 8cbda6380..8d460c052 100644 --- a/app/Models/Foundation/Summit/Speakers/PresentationSpeaker.php +++ b/app/Models/Foundation/Summit/Speakers/PresentationSpeaker.php @@ -1675,6 +1675,16 @@ public function setPreloadedAssignmentOrder(int $presentationId, ?int $order): v $this->preloadedAssignmentOrders[$presentationId] = $order; } + public function clearPreloadedAssignmentOrder(int $presentationId): void + { + unset($this->preloadedAssignmentOrders[$presentationId]); + } + + public function clearAllPreloadedAssignmentOrders(): void + { + $this->preloadedAssignmentOrders = []; + } + /** * @param Presentation $presentation * @return int|null diff --git a/tests/PresentationSpeakerCacheTest.php b/tests/PresentationSpeakerCacheTest.php new file mode 100644 index 000000000..e7cc0f68f --- /dev/null +++ b/tests/PresentationSpeakerCacheTest.php @@ -0,0 +1,143 @@ +createMock(Presentation::class); + $pres->method('getId')->willReturn(42); + + $speaker->setPreloadedAssignmentOrder(42, 3); + + $this->assertSame(3, $speaker->getPresentationAssignmentOrder($pres)); + } + + public function testPreloadedAssignmentOrderNullOrderCached(): void + { + $speaker = new PresentationSpeaker(); + $pres = $this->createMock(Presentation::class); + $pres->method('getId')->willReturn(10); + + $speaker->setPreloadedAssignmentOrder(10, null); + + $this->assertNull($speaker->getPresentationAssignmentOrder($pres)); + } + + // ------------------------------------------------------------------ // + // PresentationSpeaker::clearPreloadedAssignmentOrder // + // ------------------------------------------------------------------ // + + public function testClearPreloadedAssignmentOrderLeavesOtherEntriesIntact(): void + { + $speaker = new PresentationSpeaker(); + $speaker->setPreloadedAssignmentOrder(1, 5); + $speaker->setPreloadedAssignmentOrder(2, 7); + + $speaker->clearPreloadedAssignmentOrder(1); + + $pres2 = $this->createMock(Presentation::class); + $pres2->method('getId')->willReturn(2); + $this->assertSame(7, $speaker->getPresentationAssignmentOrder($pres2), + 'Clearing entry 1 must not affect entry 2'); + } + + public function testClearPreloadedAssignmentOrderAllowsReset(): void + { + $speaker = new PresentationSpeaker(); + $speaker->setPreloadedAssignmentOrder(1, 5); + $speaker->clearPreloadedAssignmentOrder(1); + $speaker->setPreloadedAssignmentOrder(1, 9); + + $pres = $this->createMock(Presentation::class); + $pres->method('getId')->willReturn(1); + $this->assertSame(9, $speaker->getPresentationAssignmentOrder($pres), + 'After clear + re-set, new value must be returned'); + } + + // ------------------------------------------------------------------ // + // PresentationSpeaker::clearAllPreloadedAssignmentOrders // + // ------------------------------------------------------------------ // + + public function testClearAllPreloadedAssignmentOrdersAllowsReset(): void + { + $speaker = new PresentationSpeaker(); + $speaker->setPreloadedAssignmentOrder(1, 2); + $speaker->setPreloadedAssignmentOrder(3, 4); + + $speaker->clearAllPreloadedAssignmentOrders(); + $speaker->setPreloadedAssignmentOrder(1, 10); + + $pres = $this->createMock(Presentation::class); + $pres->method('getId')->willReturn(1); + $this->assertSame(10, $speaker->getPresentationAssignmentOrder($pres)); + } + + // ------------------------------------------------------------------ // + // Presentation::setPreloadedSessionSelections / getSelectionStatus // + // ------------------------------------------------------------------ // + + /** + * Verifies the preloaded branch is exercised: with an empty preloaded + * array the method must return Pending without ever calling createQuery() + * (which would throw because there is no EntityManager in this context). + */ + public function testGetSelectionStatusUsesPreloadedBranchWhenSet(): void + { + $pres = new Presentation(); + $pres->setPreloadedSessionSelections([]); + + $status = $pres->getSelectionStatus(); + + $this->assertSame(Presentation::SelectionStatus_Pending, $status); + } + + public function testGetSelectionStatusMemoizesResult(): void + { + $pres = new Presentation(); + $pres->setPreloadedSessionSelections([]); + + $first = $pres->getSelectionStatus(); + $second = $pres->getSelectionStatus(); + + $this->assertSame($first, $second, 'Second call must return the memoized value'); + } + + public function testSetPreloadedSessionSelectionsResetsMemoization(): void + { + $pres = new Presentation(); + $pres->setPreloadedSessionSelections([]); + $pres->getSelectionStatus(); // fills memoized cache + + // Resetting preloaded selections must clear the memoized result so the + // next getSelectionStatus() re-evaluates instead of returning stale data. + $pres->setPreloadedSessionSelections([]); + // Must not throw — preloaded path is used again, not the DQL fallback. + $status = $pres->getSelectionStatus(); + $this->assertSame(Presentation::SelectionStatus_Pending, $status); + } +} diff --git a/tests/ResourceServerContextTest.php b/tests/ResourceServerContextTest.php index c1b68394a..20b88b04b 100644 --- a/tests/ResourceServerContextTest.php +++ b/tests/ResourceServerContextTest.php @@ -37,4 +37,33 @@ public function testSync(){ $member = $ctx->getCurrentUser(true); } + + public function testSetAuthorizationContextResetsUserCache(): void + { + $ctx = App::make(IResourceServerContext::class); + + $context = []; + $context['user_id'] = "1080"; + $context['external_user_id'] = "1080"; + $context['user_identifier'] = "test"; + $context['user_email'] = "test@test.com"; + $context['user_email_verified'] = true; + $context['user_first_name'] = "test"; + $context['user_last_name'] = "test"; + $context['user_groups'] = ['raw-users']; + $ctx->setAuthorizationContext($context); + $ctx->getCurrentUser(false); // warm the request-scoped cache + + $ref = new \ReflectionClass($ctx); + $prop = $ref->getProperty('cachedCurrentUserResolved'); + $prop->setAccessible(true); + $this->assertTrue($prop->getValue($ctx), + 'Prerequisite: cache must be warm after the first getCurrentUser() call'); + + // A second setAuthorizationContext() must invalidate the cache so the + // next getCurrentUser() re-fetches instead of returning the stale member. + $ctx->setAuthorizationContext($context); + $this->assertFalse($prop->getValue($ctx), + 'setAuthorizationContext() must reset cachedCurrentUserResolved'); + } } \ No newline at end of file From 437a25ebf86b856378d705925dd6c7347304d995 Mon Sep 17 00:00:00 2001 From: smarcet Date: Sun, 24 May 2026 23:43:01 -0300 Subject: [PATCH 7/7] docs(adr): document /attendees N+1 elimination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows the same methodology and structure as ADR-002. Records the afterQuery trait hook (reusable for future endpoints), the Summit-level speaker memo + batch preload, and the per-attendee notes/tickets+badges/ tags/member fetch-join preloads. Includes honest reading of the result — query count down 57% but wall-clock only down 12% because DB latency now dominates over N+1 count. --- ...attendees-endpoint-n-plus-1-elimination.md | 172 ++++++++++++++++++ 1 file changed, 172 insertions(+) create mode 100644 adr/003-attendees-endpoint-n-plus-1-elimination.md diff --git a/adr/003-attendees-endpoint-n-plus-1-elimination.md b/adr/003-attendees-endpoint-n-plus-1-elimination.md new file mode 100644 index 000000000..ee9b3b320 --- /dev/null +++ b/adr/003-attendees-endpoint-n-plus-1-elimination.md @@ -0,0 +1,172 @@ +# ADR-003: Eliminate N+1 Queries in `/attendees` Endpoint + +- **Status:** Accepted +- **Date:** 2026-05-25 +- **Endpoint:** `GET /api/v1/summits/{id}/attendees` +- **Branch:** `perf/attendees-n-plus-1` (stacked on `hotfix/cache-optimizations`) +- **Related:** ADR-002 (events endpoint, same methodology) + +## Context + +Profiling `/attendees` with the admin UI's typical expand set +(`expand=tags,notes,manager&relations=member,manager,tags,tickets,notes`) +showed: + +- **83 queries / request** (10-row page) +- **DB time 1061ms** dominating a 1182ms total — DB is the bottleneck here, + not the serializer (only 120ms) + +Applied the same methodology as ADR-002: + +1. Enabled `Server-Timing` instrumentation + `QueryTimingMiddleware` on the + attendees route — both already shipped from the events PR. +2. Re-enabled the SQL pattern logger temporarily, identified the top N+1s, + fixed one pattern per commit. + +## Decision + +### Reusable infra added in this branch + +#### `ParametrizedGetAll` trait — optional `afterQuery` hook + +The trait that wraps every `_getAll` endpoint now accepts an optional +`callable $afterQuery = null` parameter. When present, the hook fires +between the data-load step and `$response->toArray()`, receiving the +`PagingResponse` so callers can pre-populate caches or batch-load related +entities before serialization. Backward-compatible; existing callers pass +nothing. + +### Targeted fixes + +#### Fix 1 — Memoize + batch-preload `Summit::getSpeakerByMember` + +**Symptom:** Three `SELECT DISTINCT PresentationSpeaker` patterns firing +~8 times each (≈24 queries). + +**Root cause:** `SummitAttendeeSerializer:133` calls +`$summit->getSpeakerByMember($member)` per attendee. Inside, +`getSpeakerByMemberId()` runs THREE separate DQLs (moderator check, +speaker check, assistance check) per call. + +**Fix:** + +- `Summit` gains `private array $speakerByMemberIdCache` (unannotated; + Doctrine ignores it). `getSpeakerByMemberId()` reads and writes the + cache at every return point. +- New `Summit::preloadSpeakersByMemberIds(array $ids, bool $filter)` runs + the same 3 lookup steps but with `WHERE mb.id IN (:ids)` and populates + the cache for every id (with `null` for members not found). Three batch + queries instead of `N × 3` per-attendee queries. +- Each batch query also `addSelect('mb')` so the speaker's Member is + fetch-joined (avoids a follow-on N+1 once the speaker is loaded). +- `OAuth2SummitAttendeesApiController::getAttendeesBySummit` passes an + `afterQuery` closure that collects the page's attendee member ids and + invokes the preload. + +**Files:** +- `app/Models/Foundation/Summit/Summit.php` +- `app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php` +- `app/Http/Controllers/Apis/Protected/Summit/Traits/ParametrizedGetAll.php` + +**Impact:** ~24 queries → 3. + +--- + +#### Fix 2 — Batch-preload Notes, Tickets+Badges, Tags, Member + +**Symptom:** +- `SummitAttendeeNote WHERE OwnerID = ?` × 10 +- `SummitAttendeeTicket WHERE ... = ?` × 10 +- `Tag JOIN SummitAttendee_Tags WHERE SummitAttendeeID = ?` × 10 +- `SummitAttendeeBadge WHERE TicketID = ?` × 12 (exposed after tickets loaded) +- `Member WHERE id = ?` × 8 + +**Root cause:** Each is an EXTRA_LAZY collection / association on the +attendee or its ticket. Iteration during serialization fires one DB load +per attendee or per ticket. + +**Fix:** Five batch fetch-join queries in the `afterQuery` closure: + +```dql +SELECT a, n FROM SummitAttendee a LEFT JOIN a.notes n WHERE a.id IN (:ids) +SELECT a, t, b FROM SummitAttendee a LEFT JOIN a.tickets t LEFT JOIN t.badge b WHERE a.id IN (:ids) +SELECT a, tg FROM SummitAttendee a LEFT JOIN a.tags tg WHERE a.id IN (:ids) +SELECT a, m FROM SummitAttendee a LEFT JOIN a.member m WHERE a.id IN (:ids) +``` + +Doctrine's fetch-join (`SELECT a, X`) populates the inverse-side +collection / association so subsequent serializer iterations read from +memory. + +**Files:** `app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php` + +**Impact:** ~50 queries → 4. + +## Consequences + +### Performance — measured on `api2.dev.fnopen.com` + +| Metric | Baseline | After all fixes | Δ | +| --------------- | -------- | --------------- | -------- | +| Queries | 83 | **36** | **-57%** | +| DB time | 1061 ms | ~930 ms | -13% | +| Serializer time | 120 ms | ~22 ms | **-82%** | +| Total | 1182 ms | ~1040 ms | -12% | + +The query-count reduction is dramatic; the wall-clock reduction is more +modest because per-query latency on the model database is ~25-30 ms. +Each remaining query "costs" almost the same as before, and we still +fire ~36 of them. **DB latency, not N+1 count, is now the dominant +component** — this would be the natural next investigation +(connection-pooling, query batching at the DB level, read replicas). + +### What we kept + +- Server-Timing header + `QueryTimingMiddleware` from ADR-002. +- The new `afterQuery` hook in `ParametrizedGetAll::_getAll` (reusable + for future endpoints — same pattern would apply to e.g. + `/orders`, `/tickets`). +- Entity-level caches (`Summit::$speakerByMemberIdCache`). + +### What we did *not* fix (and why) + +- **`PresentationSpeaker` SELECT × 7** — comes from a deeper path + inside the serializer chain after the speakers are loaded. Each + saves ~7 queries / ~6 ms; diminishing returns. +- **`COUNT(MemberID)` × 6** — `belongsToGroup()` checks against + Members other than the current user. Out of scope for this branch. +- **PromoCode-like × 4** — per-ticket discount code lookup. 4-query + pattern; would need a 5th fetch-join in the tickets preload. +- **`SET TRANSACTION` × 3** — connection-level isolation, not + application code. + +The wall-clock gain from each of these would be ~10-50 ms. The +remaining DB-latency cost is the limiting factor and is infrastructure, +not application code. + +### Risks + +- Same risk class as ADR-002: the transient cache properties on entities + (`Summit::$speakerByMemberIdCache`) are correct only as long as + instances stay request-scoped. None of the current code reuses an EM + across requests without `clear()`, so it's safe. +- The `afterQuery` hook is opt-in per caller; the trait remains + backward-compatible for the dozens of other endpoints that use it + without passing the parameter. + +## Methodology summary + +Same as ADR-002 — see that file for the full write-up. Highlights specific +to this branch: + +1. **Different bottleneck profile.** Events was serializer-bound; + attendees is DB-bound. Same instrumentation revealed both — the + methodology is endpoint-agnostic. +2. **Cascading lazy loads.** Removing one N+1 (tickets) exposed another + one downstream (badges per ticket). The pattern logger caught both; + we extended the same preload to fetch-join the badge alongside the + ticket in one query. +3. **The `afterQuery` hook is the right abstraction.** Each endpoint + knows what its serializer will touch; the hook lets it warm those + exact caches without touching the shared trait body or the repository + layer.