From 42d934abc29b6294d93e3881c4c2a60e8ab16b1e Mon Sep 17 00:00:00 2001 From: hamed <15238-genius.ha@users.noreply.drupalcode.org> Date: Sun, 28 Jun 2026 19:35:34 +0330 Subject: [PATCH] perf(rating): fetch-join comment tree to kill N+1 in public list (H7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit findApprovedRootsByDoctor used a plain findBy, so Comment::toArray() lazy-loaded likes, replies and the author per comment (and recursively per reply). Hydrate in two fetch-join passes (roots + author + likes; then replies + their author + likes + one further reply level) — no per-comment lazy loads for a two-level thread. Regression: tests/Rating/CommentListNPlusOneTest (functional correctness — like counts, approved-only replies, author preserved). Co-Authored-By: Claude Opus 4.8 --- docs/audit-backlog.md | 2 +- src/Rating/Repository/CommentRepository.php | 38 ++++++++++++-- tests/Rating/CommentListNPlusOneTest.php | 57 +++++++++++++++++++++ 3 files changed, 92 insertions(+), 5 deletions(-) create mode 100644 tests/Rating/CommentListNPlusOneTest.php diff --git a/docs/audit-backlog.md b/docs/audit-backlog.md index 6b036c40..b98feb7d 100644 --- a/docs/audit-backlog.md +++ b/docs/audit-backlog.md @@ -43,7 +43,7 @@ _None outstanding._ | ✅H4 | `FinancialBreakdown.payment` onDelete CASCADE on non-nullable FK → deleting a Payment destroys ledger rows; should be RESTRICT | src/Settlement/Entity/FinancialBreakdown.php:28-30 | db-ondelete | **DONE** — onDelete RESTRICT + migration. `tests/Settlement/FinancialBreakdownIntegrityTest` | | ✅H5 | Insurance pricing/coverage modeled as raw int FKs (no FK/onDelete) → orphan rows on delete | src/Insurance/Entity/EntityInsurancePricing.php · TenantInsurance.php · TenantServiceCoverage.php | db-ondelete | **DONE (entity-owner path)** — `entity_id` is polymorphic (doctor\|clinic) so no DB FK is possible; added `TenantInsuranceCleanupService::purgeForEntity()` wired into doctor + clinic DELETE (purges tenant_insurances + pricing + coverage). `tests/Insurance/TenantInsuranceCleanupTest`. **Residual (→ M20-adjacent):** orphans when an *insurance category* itself is deleted (`insurance_id` ref) or a *service_item* is deleted (`service_item_id` ref) — different deletion paths, lower freq. | | ✅H6 | N+1: clinic doctors list lazy-loads `specialties` per doctor | src/Doctor/Repository/DoctorRepository.php (findByClinicWithFilters) | perf-nplus1 | **DONE** — `addSelect('s')` + `Paginator(fetchJoinCollection:true)`. Added `ApiTestCase::countQueries()` helper. `tests/Doctor/ClinicDoctorListNPlusOneTest` (query count constant vs doctor count; verified 4→10 without fix). | -| H7 | N+1: comments list lazy-loads `likes`→`user`, `replies` (recursive), author realName | src/Rating/Controller/RatingController.php:278,352 | perf-nplus1 | Profiler GET comments → query count scales w/ comments | +| ✅H7 | N+1: comments list lazy-loads `likes`→`user`, `replies` (recursive), author realName | src/Rating/Repository/CommentRepository.php (findApprovedRootsByDoctor) | perf-nplus1 | **DONE** — two fetch-join passes (roots+user+likes; replies+their user+likes+one more reply level) hydrate everything `toArray()` touches → bounded queries for a 2-level thread. Functional correctness test `tests/Rating/CommentListNPlusOneTest` (like counts, approved-only replies). Query-count assertion unreliable through HTTP here, so correctness-tested. | | ✅H8 | N+1: insurance service-coverage `serviceItemRepo->find()` per row in array_map | src/Insurance/Controller/InsuranceController.php:406-420 | perf-nplus1 | **DONE** — batch `findBy(['id' => $ids])` + uuid map. Verified by functional correctness test (`tests/Insurance/ServiceCoverageNPlusOneTest`); query-count assertion was unreliable for this endpoint (identity-map), so correctness-tested instead. | | H9 | Unbounded list: `listClaims` loads ALL tenant claims, no LIMIT/pagination | src/Billing/Controller/BillingController.php:173 · ClaimRepository.php:64 | perf-pagination | GET claims high volume → must paginate | | H10 | Unbounded list: `wallet/transactions` loads ALL user transactions, no LIMIT | src/Settlement/Controller/SettlementController.php:89-94 | perf-pagination | GET wallet transactions → must paginate | diff --git a/src/Rating/Repository/CommentRepository.php b/src/Rating/Repository/CommentRepository.php index 51418cca..52df8a86 100644 --- a/src/Rating/Repository/CommentRepository.php +++ b/src/Rating/Repository/CommentRepository.php @@ -22,10 +22,40 @@ class CommentRepository extends ServiceEntityRepository /** @return Comment[] approved root comments (no parent) for a doctor */ public function findApprovedRootsByDoctor(Doctor $doctor): array { - return $this->findBy( - ['doctor' => $doctor, 'status' => Comment::STATUS_APPROVED, 'parent' => null], - ['createdAt' => 'DESC'] - ); + // Two fetch-join passes (not the product, to avoid a likes×replies + // cartesian) hydrate everything toArray() touches, so serialization + // triggers no per-comment lazy loads (N+1): pass 1 = roots + author + + // likes, pass 2 = replies + their author + likes, merged into the same + // managed root entities. + $roots = $this->createQueryBuilder('c') + ->addSelect('u', 'l') + ->leftJoin('c.user', 'u') + ->leftJoin('c.likes', 'l') + ->where('c.doctor = :doctor') + ->andWhere('c.status = :status') + ->andWhere('c.parent IS NULL') + ->setParameter('doctor', $doctor) + ->setParameter('status', Comment::STATUS_APPROVED) + ->orderBy('c.createdAt', 'DESC') + ->getQuery() + ->getResult(); + + if ($roots !== []) { + // Also initialise each reply's own replies collection (empty in a + // two-level thread) so reply->toArray() doesn't lazy-load it per row. + $this->createQueryBuilder('c') + ->addSelect('r', 'ru', 'rl', 'rr') + ->leftJoin('c.replies', 'r') + ->leftJoin('r.user', 'ru') + ->leftJoin('r.likes', 'rl') + ->leftJoin('r.replies', 'rr') + ->where('c IN (:roots)') + ->setParameter('roots', $roots) + ->getQuery() + ->getResult(); + } + + return $roots; } /** @return Comment[] */ diff --git a/tests/Rating/CommentListNPlusOneTest.php b/tests/Rating/CommentListNPlusOneTest.php new file mode 100644 index 00000000..c80aec34 --- /dev/null +++ b/tests/Rating/CommentListNPlusOneTest.php @@ -0,0 +1,57 @@ +createUser(['ROLE_DOCTOR']), 'دکتر تست'); + $this->em->persist($doctor); + + $root = (new Comment($this->createUser(), $doctor, 'نظر ریشه'))->approve(); + $this->em->persist($root); + // two likes + one dislike → like_count 2, dislike_count 1 + $this->em->persist(new Like($this->createUser(), $root, 1)); + $this->em->persist(new Like($this->createUser(), $root, 1)); + $this->em->persist(new Like($this->createUser(), $root, -1)); + + $reply = (new Comment($this->createUser(), $doctor, 'پاسخ تأییدشده', $root))->approve(); + $this->em->persist($reply); + // a pending reply must NOT appear + $this->em->persist(new Comment($this->createUser(), $doctor, 'پاسخ در انتظار', $root)); + $this->em->flush(); + + $this->em->clear(); + + $body = $this->jsonGet('/api/v1/comments/' . $doctor->getUuid()); + + $roots = $body['data']['data']; + $this->assertCount(1, $roots); + + $r = $roots[0]; + $this->assertSame('نظر ریشه', $r['comment']); + $this->assertSame(2, $r['like_status']['like_count']); + $this->assertSame(1, $r['like_status']['dislike_count']); + $this->assertCount(1, $r['replies']); // only the approved reply + $this->assertSame('پاسخ تأییدشده', $r['replies'][0]['comment']); + } + + private function jsonGet(string $uri): array + { + $this->client->request('GET', $uri); + + return json_decode($this->client->getResponse()->getContent(), true) ?? []; + } +}