feat: fix N+1 problem by smarcet · Pull Request #505 · OpenStackweb/summit-api

smarcet · 2026-02-20T17:33:18Z

No description provided.

coderabbitai · 2026-05-23T00:03:33Z

Important

Review skipped

Draft detected.

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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2cc0a143-88bb-4670-aa0c-8be20db55b7a

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/n+1-improvements

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 and usage tips.}

github-actions · 2026-05-23T00:13:20Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-505/

This page is automatically updated on each push to this PR.

github-actions · 2026-05-23T00:23:03Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-505/

This page is automatically updated on each push to this PR.

…egorySerializer (typo '.')

…tch-load support - Add $expands parameter to DoctrineRepository::getAllByPage - Add addExpandFetchJoins call (toOne FETCH JOINs before pagination) - Add batchLoadExpandedRelations call (toMany IN-clause batching after hydration) - Add protected static getExpandFieldMap() hook (returns [] by default, subclasses override) - Use static::getExpandFieldMap() for late static binding so subclass maps resolve correctly - Make DoctrineSummitEventRepository::getExpandFieldMap() public static to allow static call from tests - Propagate array $expands = [] signature to all concrete getAllByPage overrides and interfaces

github-actions · 2026-05-23T03:19:31Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-505/

This page is automatically updated on each push to this PR.

Copilot

Pull request overview

This PR introduces expand-aware eager/batch loading for SummitEvent retrieval (including nested dot-notation expands) to reduce Doctrine N+1 query patterns, and threads the expand request parameter down into repositories so the loader can act on it.

Changes:

Add generic expand-driven fetch-join (toOne) and batch hydration (toMany) support to GraphLoaderTrait, including recursive nested expands.
Extend getAllByPage repository APIs to accept an optional $expands array and propagate expand from the Summit Events API controller/strategies.
Add unit/integration tests validating expand maps and query-count reductions for batch loading and nested expands.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/Unit/Repositories/SummitEventNestedExpandBatchLoadingTest.php	Integration test verifying nested expands reduce level-2 speaker relation queries.
tests/Unit/Repositories/SummitEventExpandMapTest.php	Unit test asserting `DoctrineSummitEventRepository` expand-name → Doctrine-field mismatch map invariants.
tests/Unit/Repositories/SummitEventExpandBatchLoadingTest.php	Integration tests measuring query reduction across expands, paging sizes, and data consistency.
tests/Unit/Repositories/GraphLoaderNestedExpandTest.php	Unit test suite for recursive nested expand resolution, overrides, and resolvers in `GraphLoaderTrait`.
app/Repositories/Summit/DoctrineSummitOrderExtraQuestionTypeRepository.php	Updates `getAllByPage` signature to accept `$expands`.
app/Repositories/Summit/DoctrineSummitEventRepository.php	Adds expand field map + implements expand-driven fetch-joins and batch/nested loading for events.
app/Repositories/Summit/DoctrineSummitAttendeeTicketRepository.php	Updates `getAllByPage` signature to accept `$expands`.
app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php	Updates `getAllByPage` signature to accept `$expands`.
app/Repositories/Summit/DoctrineSpeakerRepository.php	Updates `getAllByPage` signature to accept `$expands`.
app/Repositories/Summit/DoctrineMemberRepository.php	Updates `getAllByPage` signature to accept `$expands`.
app/Repositories/ResourceServer/DoctrineEndPointRateLimitByIPRepository.php	Updates `getAllByPage` signature to accept `$expands` (method still TODO).
app/Repositories/DoctrineRepository.php	Adds `GraphLoaderTrait` and wires expand-driven fetch-joins + batch/nested loading into the generic `getAllByPage`.
app/Models/Utils/IBaseRepository.php	Extends repository interface `getAllByPage` signature to include `$expands`.
app/Models/Foundation/Summit/Repositories/ISummitEventRepository.php	Extends summit event repository interface `getAllByPage` signature to include `$expands`.
app/Models/Foundation/Main/Repositories/IMemberRepository.php	Extends member repository interface `getAllByPage` signature to include `$expands`.
app/libs/Utils/Doctrine/GraphLoaderTrait.php	Core implementation for expand fetch-joins, batch hydration, and recursive nested expand loading.
app/Http/Controllers/Apis/Protected/Summit/Strategies/events/RetrieveSummitEventsStrategy.php	Parses `expand` string into an array and passes it into the strategy source retrieval.
app/Http/Controllers/Apis/Protected/Summit/Strategies/events/RetrieveAllSummitEventsStrategy.php	Propagates `$expands` into the repository call.
app/Http/Controllers/Apis/Protected/Summit/Strategies/events/RetrieveAllSummitEventsBySummitStrategy.php	Propagates `$expands` into the repository call.
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php	Ensures `expand` is passed to strategies (so repositories can batch-load accordingly) and reused for serialization.

Comments suppressed due to low confidence (1)

tests/Unit/Repositories/SummitEventExpandBatchLoadingTest.php:356

runFullScenario() also attaches a SQL logger and detaches it only on the happy path. If the repository call or relation touching throws, the logger remains attached and can affect subsequent tests. Use try/finally to guarantee detachQueryLogger() runs.

    private function runFullScenario(PagingInfo $paging, Filter $filter, array $expands): array
    {
        $this->clearIdentityMap();
        $debug = $this->attachQueryLogger();

        $response = $this->repository->getAllByPage($paging, $filter, null, $expands);
        $items = $response->getItems();
        $this->touchAllRelations($items);

        $count = count($debug->queries);
        $categories = $this->categorizeQueries($debug);
        $this->detachQueryLogger();

        return ['count' => $count, 'categories' => $categories, 'items' => $items];

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        $qb->shouldReceive('from')->andReturnUsing(function ($class) use ($qb, &$currentFrom) {
+            $currentFrom = $class;
+            return $qb;
+        });
+
+        $qb->shouldReceive('leftJoin')->andReturnUsing(function ($join) use ($qb, &$currentJoins) {
+            $currentJoins[] = $join;
+            return $qb;
+        });


+            // Determine child entity class and metadata
+            $firstChild = reset($childEntities);
+            $childClass = get_class($firstChild);
+
+            try {
+                $childMeta = $em->getClassMetadata($childClass);
+            } catch (\Exception $ex) {


@@ -155,54 +241,475 @@ protected function hydrateCollectionWithNestedToOne(
    }


+        $this->clearIdentityMap();
+        $debug = $this->attachQueryLogger();
+
+        $response = $this->repository->getAllByPage($paging, $filter, null, $expands);
+        $items = $response->getItems();
+        $this->touchRelations($items, $touchRelations);
+
+        $count = count($debug->queries);
+        $categories = $this->categorizeQueries($debug);
+        $this->detachQueryLogger();
+


+        $this->clearIdentityMap();
+        $debug = $this->attachQueryLogger();
+
+        $response = $this->repository->getAllByPage($paging, $filter, null, $expands);
+        $items = $response->getItems();
+        $this->touchSpeakerRelations($items, $touchNested);
+
+        $count = count($debug->queries);
+        $this->detachQueryLogger();
+
+        return ['count' => $count, 'items' => $items];


…ase timing in getAllByPage Adds ms-precision breakdown: count query, id query, hydration query, batchLoad. Was using integer time() which masks sub-second differences.

github-actions · 2026-05-23T03:42:14Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-505/

This page is automatically updated on each push to this PR.

…tAllByPage SummitEventType uses JOINED inheritance with a ClassName discriminator so Doctrine already hydrates the correct PresentationType subclass automatically via innerJoin e.type. The explicit leftJoin(PresentationType::class, et2) in the hydration query was redundant and caused Doctrine to emit PresentationType as a separate root entity in getResult(), silently dropping those events from the byId map and logging unexpected hydration row warnings. The et2 join is kept in getFastCount and getAllIdsByPage where it is needed for type_allows_attendee_vote and type_allows_custom_ordering filter predicates.

github-actions · 2026-05-23T03:53:29Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-505/

This page is automatically updated on each push to this PR.

… cache in getEvents Three causes of ~1100ms serializer overhead: 1. Presentation::getSpeakers() used matching() on an EXTRA_LAZY collection which always issues a DB query regardless of whether batchLoadExpandedRelations had already pre-loaded the assignments. Replaced with toArray() + PHP usort so that an already-initialized collection (batch-loaded) is sorted in memory with zero DB round-trips. Falls back to a single full-load query on cold collections. 2. getSpeaker() on each PresentationSpeakerAssignment triggered one lazy-load per speaker because batchLoadExpandedRelations only pre-fetched the assignments, not the speaker FK. Injecting speakers.speaker into the batch expands causes batchLoadNestedExpands to load all unique PresentationSpeaker entities in one IN-clause query, so getSpeaker() returns the already-initialized identity-map entry. 3. getEvents() did not pass use_cache=true to the serializer params, so PresentationSerializer skipped its Redis cache entirely. Adding it means warm requests serialize from cache (Redis GET + JSON decode) instead of re-running the full field-mapping and expand loop per presentation.

github-actions · 2026-05-23T04:03:36Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-505/

This page is automatically updated on each push to this PR.

…for automatic invalidation Without this, a presentation update left stale cached data for up to 20 minutes (CacheTTL=1200s). Including the last_edited Unix timestamp in the key means any update to the presentation changes the key, so the old entry is silently ignored and ages out via TTL — no explicit Cache::forget needed anywhere.

github-actions · 2026-05-23T04:06:50Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-505/

This page is automatically updated on each push to this PR.

…nse::toArray Adds: - OAuth2SummitEventsApiController::getEvents: logs total serializer ms after toArray() - PagingResponse::toArray: logs per-item entity id + ms to identify slow outliers Remove before merging to main.

github-actions · 2026-05-23T04:26:48Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-505/

This page is automatically updated on each push to this PR.

smarcet marked this pull request as draft February 20, 2026 17:33

smarcet force-pushed the main branch from 57c49e2 to 815b3ea Compare February 27, 2026 19:54

feat: fix N+1 problem

97d04c5

smarcet force-pushed the feat/n+1-improvements branch from 1e57d5a to 97d04c5 Compare May 23, 2026 00:11

feat: fix N+1 problem

e4d015c

smarcet added 2 commits May 23, 2026 00:13

fix(serializer): restore allowed_access_levels key in PresentationCat…

fd1871e

…egorySerializer (typo '.')

smarcet requested a review from Copilot May 23, 2026 03:24

Copilot started reviewing on behalf of smarcet May 23, 2026 03:25 View session

Copilot AI reviewed May 23, 2026

View reviewed changes

chore(repository): replace time() with microtime(true) and add per-ph…

fd8eaa4

…ase timing in getAllByPage Adds ms-precision breakdown: count query, id query, hydration query, batchLoad. Was using integer time() which masks sub-second differences.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fix N+1 problem#505

feat: fix N+1 problem#505
smarcet wants to merge 9 commits into
mainfrom
feat/n+1-improvements

smarcet commented Feb 20, 2026

Uh oh!

coderabbitai Bot commented May 23, 2026 •

edited

Loading

Review skipped

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -155,54 +241,475 @@ protected function hydrateCollectionWithNestedToOne(
		}

Conversation

smarcet commented Feb 20, 2026

Uh oh!

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

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented May 23, 2026 •

edited

Loading