From ccb71e4371e824198fa4a5e39fddb8b6f5812930 Mon Sep 17 00:00:00 2001 From: hamed <15238-genius.ha@users.noreply.drupalcode.org> Date: Sun, 28 Jun 2026 20:43:35 +0330 Subject: [PATCH] perf(secretary,billing): kill N+1 in secretary + claims lists (M8, M9) M8: DoctorSecretary::toArray() lazy-loaded secretary/doctor/clinic per row; fetch-join them in findByDoctorScope/findByClinic (shared listWithRelations()). M9: enrichClaims() lazy-loaded each claim's items collection and called insuranceRepo->find() per claim. Fetch-join items in findByTenant (Paginator, fetchJoinCollection) and batch-fetch insurance names once. Regressions (query count constant vs row count): SecretaryListNPlusOneTest, ClaimsListNPlusOneTest. Co-Authored-By: Claude Opus 4.8 --- docs/audit-backlog.md | 4 +- src/Billing/Controller/BillingController.php | 13 ++++- src/Billing/Repository/ClaimRepository.php | 14 +++-- .../Repository/DoctorSecretaryRepository.php | 25 ++++++--- tests/Billing/ClaimsListNPlusOneTest.php | 54 +++++++++++++++++++ tests/Secretary/SecretaryListNPlusOneTest.php | 45 ++++++++++++++++ 6 files changed, 141 insertions(+), 14 deletions(-) create mode 100644 tests/Billing/ClaimsListNPlusOneTest.php create mode 100644 tests/Secretary/SecretaryListNPlusOneTest.php diff --git a/docs/audit-backlog.md b/docs/audit-backlog.md index 2faaf672..274a8917 100644 --- a/docs/audit-backlog.md +++ b/docs/audit-backlog.md @@ -61,8 +61,8 @@ _None outstanding._ | ✅M5 | IDOR read: AppointmentSettings list endpoints leak any doctor's config — `listOverrides`, `listHolidays`, `availableLocations` (no ownership on {doctorUuid}) | src/Appointment/Controller/AppointmentSettingsController.php:150,256,349 | security-idor | **DONE** — ownership added to listOverrides/listHolidays/availableLocations. `tests/Appointment/AppointmentSettingsListOwnershipTest` | | ✅M6 | OTP send-code has no per-mobile/per-uuid cap, only per-IP (5/hr) → SMS flood from rotating IPs | src/Auth/Controller/AuthController.php:136-153 · OtpService.php:59-77 · rate_limiter.yaml:4-7 | security-ratelimit | **DONE** — per-mobile limiter (5/hr) added alongside per-IP. `tests/Auth/SendCodeMobileRateLimitTest` (6 reqs / 6 IPs → 6th 429). | | ✅M7 | Refresh token not rotated on use (same raw token 30d), never re-checks user status | src/Auth/Controller/AuthController.php:477-480 · TokenService.php:33-45 | security-auth | **DONE** — single-use rotation (revoke old + issue new) + suspended-user (status!=1) rejected. `tests/Auth/RefreshTokenRotationTest`. | -| M8 | N+1: secretary list lazy-loads secretary/doctor ManyToOne per row | src/Secretary/Controller/SecretaryController.php:192,213 | perf-nplus1 | Profiler secretary list → ~2 queries/row | -| M9 | N+1: billing claims lazy `items` + `insuranceRepo->find()` per claim in enrichClaims | src/Billing/Controller/BillingController.php:~49,68 | perf-nplus1 | GET claims → items+insurance query/claim | +| ✅M8 | N+1: secretary list lazy-loads secretary/doctor ManyToOne per row | src/Secretary/Controller/SecretaryController.php:192,213 | perf-nplus1 | **DONE** — fetch-join secretary/doctor/clinic. `tests/Secretary/SecretaryListNPlusOneTest` | +| ✅M9 | N+1: billing claims lazy `items` + `insuranceRepo->find()` per claim in enrichClaims | src/Billing/Controller/BillingController.php:~49,68 | perf-nplus1 | **DONE** — fetch-join items (Paginator) + batch insurance names. `tests/Billing/ClaimsListNPlusOneTest` | | ✅M10 | Unbounded list: `listMine` settlements `findByUser` no limit | src/Settlement/Controller/SettlementController.php:193 | perf-pagination | **DONE** — page/limit + countByUser + data.meta. `tests/Settlement/SettlementListPaginationTest` | | ✅M11 | Unbounded list: admin `pendingComments` `findPending` no limit | src/Rating/Controller/RatingController.php:350 | perf-pagination | **DONE** — findPending(limit,offset)+countPending+meta. `tests/Rating/CommentPaginationTest::testAdminPendingListPaginates` | | ✅M12 | Unbounded list: public `listComments` per-doctor no limit | src/Rating/Controller/RatingController.php:270 | perf-pagination | **DONE** — Paginator(fetchJoinCollection) page/limit + countApprovedRootsByDoctor + meta. `tests/Rating/CommentPaginationTest::testPublicListPaginates` | diff --git a/src/Billing/Controller/BillingController.php b/src/Billing/Controller/BillingController.php index c8739cd2..e229eecf 100644 --- a/src/Billing/Controller/BillingController.php +++ b/src/Billing/Controller/BillingController.php @@ -63,9 +63,18 @@ class BillingController extends BaseController $sessionDates = $this->sessionRepo->datesForIds($uniqueSessionIds); $patientInfo = $this->sessionRepo->patientInfoForIds($uniqueSessionIds); - return array_map(function (Claim $claim) use ($itemDetails, $sessionDates, $patientInfo) { + // Batch-fetch insurance names instead of one find() per claim (N+1). + $insuranceIds = array_values(array_unique(array_map(fn(Claim $c) => $c->getInsuranceId(), $claims))); + $insuranceNames = []; + if ($insuranceIds !== []) { + foreach ($this->insuranceRepo->findBy(['id' => $insuranceIds]) as $ins) { + $insuranceNames[$ins->getId()] = $ins->getName(); + } + } + + return array_map(function (Claim $claim) use ($itemDetails, $sessionDates, $patientInfo, $insuranceNames) { $data = $claim->toArray(); - $data['insurance_name'] = $this->insuranceRepo->find($claim->getInsuranceId())?->getName(); + $data['insurance_name'] = $insuranceNames[$claim->getInsuranceId()] ?? null; $patientName = null; $patientMobile = null; diff --git a/src/Billing/Repository/ClaimRepository.php b/src/Billing/Repository/ClaimRepository.php index e5db7988..5bb5c0e6 100644 --- a/src/Billing/Repository/ClaimRepository.php +++ b/src/Billing/Repository/ClaimRepository.php @@ -4,6 +4,7 @@ namespace App\Billing\Repository; use App\Billing\Entity\Claim; use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository; +use Doctrine\ORM\Tools\Pagination\Paginator; use Doctrine\Persistence\ManagerRegistry; class ClaimRepository extends ServiceEntityRepository @@ -24,12 +25,17 @@ class ClaimRepository extends ServiceEntityRepository */ public function findByTenant(string $entityType, int $entityId, array $filters = [], int $page = 1, int $limit = 50): array { - return $this->tenantQb($entityType, $entityId, $filters) + // fetch-join items so enrichClaims()/toArray() don't lazy-load the + // items collection per claim (N+1). fetchJoinCollection keeps the LIMIT + // paginating by claim. + $qb = $this->tenantQb($entityType, $entityId, $filters) + ->addSelect('i') + ->leftJoin('c.items', 'i') ->orderBy('c.id', 'DESC') ->setFirstResult(($page - 1) * $limit) - ->setMaxResults($limit) - ->getQuery() - ->getResult(); + ->setMaxResults($limit); + + return iterator_to_array(new Paginator($qb, fetchJoinCollection: true)); } public function countByTenant(string $entityType, int $entityId, array $filters = []): int diff --git a/src/Secretary/Repository/DoctorSecretaryRepository.php b/src/Secretary/Repository/DoctorSecretaryRepository.php index 3e18b690..21d2b028 100644 --- a/src/Secretary/Repository/DoctorSecretaryRepository.php +++ b/src/Secretary/Repository/DoctorSecretaryRepository.php @@ -38,13 +38,27 @@ class DoctorSecretaryRepository extends ServiceEntityRepository return $this->findBy(['doctor' => $doctor], ['createdAt' => 'DESC']); } + /** Base query with secretary/doctor/clinic fetch-joined for toArray() (no N+1). */ + private function listWithRelations(): \Doctrine\ORM\QueryBuilder + { + return $this->createQueryBuilder('s') + ->addSelect('sec', 'doc', 'cl') + ->leftJoin('s.secretary', 'sec') + ->leftJoin('s.doctor', 'doc') + ->leftJoin('s.clinic', 'cl') + ->orderBy('s.createdAt', 'DESC'); + } + /** منشی ها مطب شخصی یک دکتر (owner_type='doctor') */ public function findByDoctorScope(Doctor $doctor): array { - return $this->findBy( - ['doctor' => $doctor, 'ownerType' => DoctorSecretary::OWNER_DOCTOR], - ['createdAt' => 'DESC'] - ); + return $this->listWithRelations() + ->where('s.doctor = :doctor') + ->andWhere('s.ownerType = :type') + ->setParameter('doctor', $doctor) + ->setParameter('type', DoctorSecretary::OWNER_DOCTOR) + ->getQuery() + ->getResult(); } /** رابطه منشی در scope مطب شخصی */ @@ -111,12 +125,11 @@ class DoctorSecretaryRepository extends ServiceEntityRepository /** همه منشی ها کلینیک (owner_type='clinic') */ public function findByClinic(Clinic $clinic): array { - return $this->createQueryBuilder('s') + return $this->listWithRelations() ->where('s.clinic = :clinic') ->andWhere('s.ownerType = :type') ->setParameter('clinic', $clinic) ->setParameter('type', DoctorSecretary::OWNER_CLINIC) - ->orderBy('s.createdAt', 'DESC') ->getQuery() ->getResult(); } diff --git a/tests/Billing/ClaimsListNPlusOneTest.php b/tests/Billing/ClaimsListNPlusOneTest.php new file mode 100644 index 00000000..0a5cc35d --- /dev/null +++ b/tests/Billing/ClaimsListNPlusOneTest.php @@ -0,0 +1,54 @@ +createUser(['ROLE_DOCTOR']); + $doctor = new Doctor($owner, 'دکتر'); + $this->em->persist($doctor); + $insurance = new Insurance('بیمه', InsuranceType::Basic); + $this->em->persist($insurance); + $this->em->flush(); + + for ($i = 0; $i < $count; $i++) { + $claim = new Claim('doctor', $doctor->getId(), $insurance->getId(), 'base'); + $item = new ClaimItem($claim, 1, 100_000); + $claim->addItem($item); + $claim->submit(); + $this->em->persist($claim); + $this->em->persist($item); + } + $this->em->flush(); + + return $owner; + } + + public function testQueryCountDoesNotGrowWithClaimCount(): void + { + $this->client->disableReboot(); + + $ownerS = $this->makeTenantWithClaims(1); + $ownerL = $this->makeTenantWithClaims(5); + $this->em->clear(); + + $qSmall = $this->countQueries(fn () => $this->authJson('GET', '/api/v1/billing/claims', $ownerS)); + $qLarge = $this->countQueries(fn () => $this->authJson('GET', '/api/v1/billing/claims', $ownerL)); + + $this->assertLessThanOrEqual($qSmall + 1, $qLarge, "N+1: query count grew from $qSmall to $qLarge"); + } +} diff --git a/tests/Secretary/SecretaryListNPlusOneTest.php b/tests/Secretary/SecretaryListNPlusOneTest.php new file mode 100644 index 00000000..d858ea35 --- /dev/null +++ b/tests/Secretary/SecretaryListNPlusOneTest.php @@ -0,0 +1,45 @@ +createUser(['ROLE_DOCTOR']); + $doctor = new Doctor($owner, 'دکتر'); + $this->em->persist($doctor); + $this->em->flush(); + + for ($i = 0; $i < $count; $i++) { + $secUser = $this->createUser(['ROLE_SECRETARY']); + $this->em->persist(new DoctorSecretary($doctor, $secUser, DoctorSecretary::OWNER_DOCTOR)); + } + $this->em->flush(); + + return [$owner, $doctor]; + } + + public function testQueryCountDoesNotGrowWithSecretaryCount(): void + { + $this->client->disableReboot(); + + [$ownerS, $small] = $this->makeDoctorWithSecretaries(1); + [$ownerL, $large] = $this->makeDoctorWithSecretaries(5); + $this->em->clear(); + + $qSmall = $this->countQueries(fn () => $this->authJson('GET', '/api/v1/secretaries/' . $small->getUuid(), $ownerS)); + $qLarge = $this->countQueries(fn () => $this->authJson('GET', '/api/v1/secretaries/' . $large->getUuid(), $ownerL)); + + $this->assertLessThanOrEqual($qSmall + 1, $qLarge, "N+1: query count grew from $qSmall to $qLarge"); + } +}