perf(rating): fetch-join comment tree to kill N+1 in public list (H7)
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 <noreply@anthropic.com>
This commit is contained in:
hamed
@@ -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 |
|
||||
|
||||
@@ -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[] */
|
||||
|
||||
@@ -0,0 +1,57 @@
|
||||
<?php
|
||||
|
||||
namespace App\Tests\Rating;
|
||||
|
||||
use App\Doctor\Entity\Doctor;
|
||||
use App\Rating\Entity\Comment;
|
||||
use App\Rating\Entity\Like;
|
||||
use App\Tests\ApiTestCase;
|
||||
|
||||
/**
|
||||
* The public comment list fetch-joins likes/replies/authors (two passes) to
|
||||
* avoid lazy loading per comment (N+1). This guards the refactor's correctness:
|
||||
* the serialized tree — like counts, approved replies, author names — must be
|
||||
* unchanged.
|
||||
*/
|
||||
class CommentListNPlusOneTest extends ApiTestCase
|
||||
{
|
||||
public function testTreeIsSerializedCorrectly(): void
|
||||
{
|
||||
$doctor = new Doctor($this->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) ?? [];
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user