From f13b81ede7ca242cc6d887bbe73147b5cc7ab742 Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Mon, 25 Nov 2024 21:04:53 +0100 Subject: [PATCH] Query hints & persistent cache for latest approvals (#5176) --- src/Controller/TimesheetTeamController.php | 2 + src/Export/TimesheetExportRepository.php | 10 ++- src/Repository/Loader/TimesheetLoader.php | 83 ++++++++++++------- src/Repository/Query/TimesheetQuery.php | 14 ++++ src/Repository/Query/TimesheetQueryHint.php | 18 ++++ src/Repository/Result/TimesheetResult.php | 4 +- .../TimesheetInvoiceItemRepository.php | 11 ++- src/Repository/TimesheetRepository.php | 33 ++++---- src/Repository/WorkingTimeRepository.php | 14 +--- src/WorkingTime/WorkingTimeService.php | 44 ++++++---- tests/Repository/TimesheetRepositoryTest.php | 10 ++- 11 files changed, 162 insertions(+), 81 deletions(-) create mode 100644 src/Repository/Query/TimesheetQueryHint.php diff --git a/src/Controller/TimesheetTeamController.php b/src/Controller/TimesheetTeamController.php index 4b6a899d..8705b90e 100644 --- a/src/Controller/TimesheetTeamController.php +++ b/src/Controller/TimesheetTeamController.php @@ -19,6 +19,7 @@ use App\Form\Model\MultiUserTimesheet; use App\Form\TimesheetAdminEditForm; use App\Form\TimesheetMultiUserEditForm; use App\Repository\Query\TimesheetQuery; +use App\Repository\Query\TimesheetQueryHint; use App\Utils\PageSetup; use Doctrine\Common\Collections\ArrayCollection; use Symfony\Component\Form\FormInterface; @@ -180,6 +181,7 @@ final class TimesheetTeamController extends TimesheetAbstractController protected function prepareQuery(TimesheetQuery $query): void { $query->setCurrentUser($this->getUser()); + $query->addQueryHint(TimesheetQueryHint::USER_PREFERENCES); // e.g. for latest approval } protected function getCreateForm(Timesheet $entry): FormInterface diff --git a/src/Export/TimesheetExportRepository.php b/src/Export/TimesheetExportRepository.php index 11bfe4aa..17eb4c15 100644 --- a/src/Export/TimesheetExportRepository.php +++ b/src/Export/TimesheetExportRepository.php @@ -11,11 +11,12 @@ namespace App\Export; use App\Entity\Timesheet; use App\Repository\Query\ExportQuery; +use App\Repository\Query\TimesheetQueryHint; use App\Repository\TimesheetRepository; final class TimesheetExportRepository implements ExportRepositoryInterface { - public function __construct(private TimesheetRepository $repository) + public function __construct(private readonly TimesheetRepository $repository) { } @@ -41,7 +42,12 @@ final class TimesheetExportRepository implements ExportRepositoryInterface public function getExportItemsForQuery(ExportQuery $query): iterable { - return $this->repository->getTimesheetsForQuery($query, true); + $query->addQueryHint(TimesheetQueryHint::CUSTOMER_META_FIELDS); + $query->addQueryHint(TimesheetQueryHint::PROJECT_META_FIELDS); + $query->addQueryHint(TimesheetQueryHint::ACTIVITY_META_FIELDS); + $query->addQueryHint(TimesheetQueryHint::USER_PREFERENCES); + + return $this->repository->getTimesheetResult($query)->getResults(); } public function getType(): string diff --git a/src/Repository/Loader/TimesheetLoader.php b/src/Repository/Loader/TimesheetLoader.php index 32f35bc8..956203f0 100644 --- a/src/Repository/Loader/TimesheetLoader.php +++ b/src/Repository/Loader/TimesheetLoader.php @@ -13,6 +13,9 @@ use App\Entity\Activity; use App\Entity\Customer; use App\Entity\Project; use App\Entity\Timesheet; +use App\Entity\User; +use App\Repository\Query\TimesheetQuery; +use App\Repository\Query\TimesheetQueryHint; use Doctrine\ORM\EntityManagerInterface; /** @@ -23,7 +26,7 @@ final class TimesheetLoader implements LoaderInterface { public function __construct( private readonly EntityManagerInterface $entityManager, - private readonly bool $fullyHydrated = false + private readonly ?TimesheetQuery $query = null ) { } @@ -50,7 +53,7 @@ final class TimesheetLoader implements LoaderInterface return $timesheet->getProject()?->getId(); }, $results)), function ($value) { return $value !== null; }); - if ($this->fullyHydrated) { + if ($this->query !== null && $this->query->hasQueryHint(TimesheetQueryHint::PROJECT_META_FIELDS)) { $qb = $em->createQueryBuilder(); $qb->select('PARTIAL p.{id}', 'meta') ->from(Project::class, 'p') @@ -60,43 +63,67 @@ final class TimesheetLoader implements LoaderInterface ->execute(); } - $qb = $em->createQueryBuilder(); - /** @var array $projects */ - $projects = $qb->select('PARTIAL p.{id}', 'customer') - ->from(Project::class, 'p') - ->leftJoin('p.customer', 'customer') - ->andWhere($qb->expr()->in('p.id', $projectIds)) - ->getQuery() - ->execute(); - - if ($this->fullyHydrated) { - $customerIds = array_filter(array_unique(array_map(function (Project $project) { - return $project->getCustomer()?->getId(); - }, $projects)), function ($value) { return $value !== null; }); - + if (\count($projectIds) > 0) { $qb = $em->createQueryBuilder(); - $qb->select('PARTIAL c.{id}', 'meta') - ->from(Customer::class, 'c') - ->leftJoin('c.meta', 'meta') - ->andWhere($qb->expr()->in('c.id', $customerIds)) + /** @var array $projects */ + $projects = $qb->select('PARTIAL p.{id}', 'customer') + ->from(Project::class, 'p') + ->leftJoin('p.customer', 'customer') + ->andWhere($qb->expr()->in('p.id', $projectIds)) ->getQuery() ->execute(); + + if ($this->query !== null && $this->query->hasQueryHint(TimesheetQueryHint::CUSTOMER_META_FIELDS)) { + $customerIds = array_filter(array_unique(array_map(function (Project $project) { + return $project->getCustomer()?->getId(); + }, $projects)), function ($value) { return $value !== null; }); + + if (\count($customerIds) > 0) { + $qb = $em->createQueryBuilder(); + $qb->select('PARTIAL c.{id}', 'meta') + ->from(Customer::class, 'c') + ->leftJoin('c.meta', 'meta') + ->andWhere($qb->expr()->in('c.id', $customerIds)) + ->getQuery() + ->execute(); + } + } } - if ($this->fullyHydrated) { + if ($this->query !== null && $this->query->hasQueryHint(TimesheetQueryHint::ACTIVITY_META_FIELDS)) { $activityIds = array_filter(array_map(function (Timesheet $timesheet) { return $timesheet->getActivity()?->getId(); }, $results), function ($id): bool { return $id !== null; }); - $qb = $em->createQueryBuilder(); - $qb->select('PARTIAL a.{id}', 'meta') - ->from(Activity::class, 'a') - ->leftJoin('a.meta', 'meta') - ->andWhere($qb->expr()->in('a.id', $activityIds)) - ->getQuery() - ->execute(); + if (\count($activityIds) > 0) { + $qb = $em->createQueryBuilder(); + $qb->select('PARTIAL a.{id}', 'meta') + ->from(Activity::class, 'a') + ->leftJoin('a.meta', 'meta') + ->andWhere($qb->expr()->in('a.id', $activityIds)) + ->getQuery() + ->execute(); + } + } + + if ($this->query !== null && $this->query->hasQueryHint(TimesheetQueryHint::USER_PREFERENCES)) { + $userIds = array_filter(array_map(function (Timesheet $timesheet) { + return $timesheet->getUser()?->getId(); + }, $results), function ($id): bool { + return $id !== null; + }); + + if (\count($userIds) > 0) { + $qb = $em->createQueryBuilder(); + $qb->select('PARTIAL u.{id}', 'preferences') + ->from(User::class, 'u') + ->leftJoin('u.preferences', 'preferences') + ->andWhere($qb->expr()->in('u.id', $userIds)) + ->getQuery() + ->execute(); + } } $qb = $em->createQueryBuilder(); diff --git a/src/Repository/Query/TimesheetQuery.php b/src/Repository/Query/TimesheetQuery.php index 7422b73b..0d935c41 100644 --- a/src/Repository/Query/TimesheetQuery.php +++ b/src/Repository/Query/TimesheetQuery.php @@ -42,6 +42,10 @@ class TimesheetQuery extends ActivityQuery implements BillableInterface, DateRan * @var array */ private array $users = []; + /** + * @var array + */ + private array $queryHints = []; public function __construct(bool $resetTimes = true) { @@ -59,6 +63,16 @@ class TimesheetQuery extends ActivityQuery implements BillableInterface, DateRan ]); } + public function addQueryHint(TimesheetQueryHint $hint): void + { + $this->queryHints[] = $hint; + } + + public function hasQueryHint(TimesheetQueryHint $hint): bool + { + return \in_array($hint, $this->queryHints, true); + } + protected function copyFrom(BaseQuery $query): void { parent::copyFrom($query); diff --git a/src/Repository/Query/TimesheetQueryHint.php b/src/Repository/Query/TimesheetQueryHint.php new file mode 100644 index 00000000..eb727e83 --- /dev/null +++ b/src/Repository/Query/TimesheetQueryHint.php @@ -0,0 +1,18 @@ + $results */ $results = $this->query->getResult(); - $loader = new TimesheetLoader($this->entityManager, true); + $loader = new TimesheetLoader($this->entityManager, $this->timesheetQuery); $loader->loadResults($results); $this->resultCache = $results; @@ -95,7 +95,7 @@ final class TimesheetResult public function getPagerfanta(): Pagination { - $loader = new LoaderQueryPaginator(new TimesheetLoader($this->entityManager), $this->query, $this->getStatistic()->getCount()); + $loader = new LoaderQueryPaginator(new TimesheetLoader($this->entityManager, $this->timesheetQuery), $this->query, $this->getStatistic()->getCount()); $paginator = new Pagination($loader); $paginator->setMaxPerPage($this->timesheetQuery->getPageSize()); diff --git a/src/Repository/TimesheetInvoiceItemRepository.php b/src/Repository/TimesheetInvoiceItemRepository.php index 30a8d650..deb554c8 100644 --- a/src/Repository/TimesheetInvoiceItemRepository.php +++ b/src/Repository/TimesheetInvoiceItemRepository.php @@ -13,20 +13,25 @@ use App\Entity\ExportableItem; use App\Entity\Timesheet; use App\Invoice\InvoiceItemRepositoryInterface; use App\Repository\Query\InvoiceQuery; +use App\Repository\Query\TimesheetQueryHint; final class TimesheetInvoiceItemRepository implements InvoiceItemRepositoryInterface { - public function __construct(private TimesheetRepository $repository) + public function __construct(private readonly TimesheetRepository $repository) { } /** - * @param InvoiceQuery $query * @return ExportableItem[] */ public function getInvoiceItemsForQuery(InvoiceQuery $query): iterable { - return $this->repository->getTimesheetsForQuery($query, true); + $query->addQueryHint(TimesheetQueryHint::CUSTOMER_META_FIELDS); + $query->addQueryHint(TimesheetQueryHint::PROJECT_META_FIELDS); + $query->addQueryHint(TimesheetQueryHint::ACTIVITY_META_FIELDS); + $query->addQueryHint(TimesheetQueryHint::USER_PREFERENCES); + + return $this->repository->getTimesheetResult($query)->getResults(); } /** diff --git a/src/Repository/TimesheetRepository.php b/src/Repository/TimesheetRepository.php index c2a0b245..77138136 100644 --- a/src/Repository/TimesheetRepository.php +++ b/src/Repository/TimesheetRepository.php @@ -24,6 +24,7 @@ use App\Repository\Loader\TimesheetLoader; use App\Repository\Paginator\LoaderQueryPaginator; use App\Repository\Paginator\PaginatorInterface; use App\Repository\Query\TimesheetQuery; +use App\Repository\Query\TimesheetQueryHint; use App\Repository\Result\TimesheetResult; use App\Utils\Pagination; use DateInterval; @@ -472,12 +473,11 @@ class TimesheetRepository extends EntityRepository $counter = $this->countTimesheetsForQuery($timesheetQuery); $query = $this->createTimesheetQuery($timesheetQuery); - return new LoaderQueryPaginator(new TimesheetLoader($this->getEntityManager()), $query, $counter); + return new LoaderQueryPaginator(new TimesheetLoader($this->getEntityManager(), $timesheetQuery), $query, $counter); } /** - * When switching $fullyHydrated to true, the call gets even more expensive. - * You normally don't need this, unless you want to access deeply nested attributes for many entries. + * TODO @deprecated since 2.25 - use getTimesheetResult() with TimesheetQueryHint instead * * @return Timesheet[] */ @@ -485,7 +485,13 @@ class TimesheetRepository extends EntityRepository { $qb = $this->getQueryBuilderForQuery($query); - return $this->getHydratedResultsByQuery($qb, $fullyHydrated); + if ($fullyHydrated) { + $query->addQueryHint(TimesheetQueryHint::CUSTOMER_META_FIELDS); + $query->addQueryHint(TimesheetQueryHint::PROJECT_META_FIELDS); + $query->addQueryHint(TimesheetQueryHint::ACTIVITY_META_FIELDS); + } + + return $this->getHydratedResultsByQuery($qb, $query); } public function getTimesheetResult(TimesheetQuery $query): TimesheetResult @@ -501,16 +507,16 @@ class TimesheetRepository extends EntityRepository /** * @return Timesheet[] */ - private function getHydratedResultsByQuery(QueryBuilder $qb, bool $fullyHydrated = false): array + private function getHydratedResultsByQuery(QueryBuilder $qb, ?TimesheetQuery $timesheetQuery = null): array { /** @var Query $query */ $query = $qb->getQuery(); - $query = $this->prepareTimesheetQuery($query); + $query = $this->prepareTimesheetQuery($query, $timesheetQuery); /** @var array $timesheets */ $timesheets = $query->getResult(); - $loader = new TimesheetLoader($qb->getEntityManager(), $fullyHydrated); + $loader = new TimesheetLoader($qb->getEntityManager(), $timesheetQuery); $loader->loadResults($timesheets); return $timesheets; @@ -914,7 +920,7 @@ class TimesheetRepository extends EntityRepository private function createTimesheetQuery(TimesheetQuery $timesheetQuery): Query { $query = $this->getQueryBuilderForQuery($timesheetQuery)->getQuery(); - $query = $this->prepareTimesheetQuery($query); + $query = $this->prepareTimesheetQuery($query, $timesheetQuery); return $query; } @@ -923,7 +929,7 @@ class TimesheetRepository extends EntityRepository * @param Query $query * @return Query */ - public function prepareTimesheetQuery(Query $query): Query + public function prepareTimesheetQuery(Query $query, ?TimesheetQuery $timesheetQuery = null): Query { $this->getEntityManager()->getConfiguration()->setEagerFetchBatchSize(300); @@ -932,15 +938,6 @@ class TimesheetRepository extends EntityRepository $query->setFetchMode(Timesheet::class, 'project', ClassMetadata::FETCH_EAGER); $query->setFetchMode(Timesheet::class, 'user', ClassMetadata::FETCH_EAGER); - // not yet supported by Doctrine - // $query->setFetchMode(Activity::class, 'meta', ClassMetadata::FETCH_EAGER); - // $query->setFetchMode(Project::class, 'customer', ClassMetadata::FETCH_EAGER); - // $query->setFetchMode(Project::class, 'meta', ClassMetadata::FETCH_EAGER); - // $query->setFetchMode(Customer::class, 'meta', ClassMetadata::FETCH_EAGER); - - // ManyToMany not supported by Doctrine yet - // $query->setFetchMode(Timesheet::class, 'tags', ClassMetadata::FETCH_EAGER); - return $query; } } diff --git a/src/Repository/WorkingTimeRepository.php b/src/Repository/WorkingTimeRepository.php index c686c2d3..59543f2a 100644 --- a/src/Repository/WorkingTimeRepository.php +++ b/src/Repository/WorkingTimeRepository.php @@ -66,10 +66,10 @@ class WorkingTimeRepository extends EntityRepository return $qb->getQuery()->getResult(); } - public function getLatestApproval(User $user): ?WorkingTime + public function getLatestApprovalDate(User $user): ?\DateTimeInterface { $qb = $this->createQueryBuilder('w'); - $qb->select('MAX(DATE(w.date))') + $qb->select($qb->expr()->max('(DATE(w.date))')) ->where($qb->expr()->eq('w.user', ':user')) ->setParameter('user', $user->getId()) ->andWhere($qb->expr()->isNotNull('w.approvedAt')) @@ -81,14 +81,6 @@ class WorkingTimeRepository extends EntityRepository return null; } - $qb = $this->createQueryBuilder('w'); - $qb->select('w') - ->where($qb->expr()->eq('w.user', ':user')) - ->setParameter('user', $user->getId()) - ->andWhere($qb->expr()->eq('DATE(w.date)', 'DATE(:date)')) - ->setParameter('date', $date) - ; - - return $qb->getQuery()->getOneOrNullResult(); // @phpstan-ignore-line + return new \DateTimeImmutable($date . ' 00:00:00', new \DateTimeZone($user->getTimezone())); } } diff --git a/src/WorkingTime/WorkingTimeService.php b/src/WorkingTime/WorkingTimeService.php index 163fe627..c6bf8b13 100644 --- a/src/WorkingTime/WorkingTimeService.php +++ b/src/WorkingTime/WorkingTimeService.php @@ -15,6 +15,7 @@ use App\Event\WorkingTimeApproveMonthEvent; use App\Event\WorkingTimeYearEvent; use App\Event\WorkingTimeYearSummaryEvent; use App\Repository\TimesheetRepository; +use App\Repository\UserRepository; use App\Repository\WorkingTimeRepository; use App\Timesheet\DateTimeFactory; use App\WorkingTime\Mode\WorkingTimeMode; @@ -29,14 +30,15 @@ use Psr\EventDispatcher\EventDispatcherInterface; */ final class WorkingTimeService { - /** @var array */ - private array $latestApprovals = []; + private const LATEST_APPROVAL_PREF = '_latest_approval'; + private const LATEST_APPROVAL_FORMAT = 'Y-m-d H:i:s'; public function __construct( private readonly TimesheetRepository $timesheetRepository, private readonly WorkingTimeRepository $workingTimeRepository, private readonly EventDispatcherInterface $eventDispatcher, - private readonly WorkingTimeModeFactory $contractModeService + private readonly WorkingTimeModeFactory $contractModeService, + private readonly UserRepository $userRepository, ) { } @@ -56,30 +58,42 @@ final class WorkingTimeService return $yearPerUserSummary; } - public function getLatestApproval(User $user): ?WorkingTime + public function getLatestApprovalDate(User $user): ?\DateTimeInterface { if ($user->getId() === null) { return null; } - $key = 'u_' . $user->getId(); + $date = $user->getPreferenceValue(self::LATEST_APPROVAL_PREF, false); - if (!\array_key_exists($key, $this->latestApprovals)) { - $this->latestApprovals[$key] = $this->workingTimeRepository->getLatestApproval($user); + // false means = there is no setting existing: let's calculate it + // null means = there is no approval existing yet + if ($date === false) { + $date = $this->workingTimeRepository->getLatestApprovalDate($user); + + // let's store the approval always: we can later detect if an approval exists + // or not based on the existence of the preference, which saves DB queries + $value = ($date !== null) ? $date->format(self::LATEST_APPROVAL_FORMAT) : null; + $user->setPreferenceValue(self::LATEST_APPROVAL_PREF, $value); + $this->userRepository->saveUser($user); + + return $date; } - return $this->latestApprovals[$key]; + if (\is_string($date)) { + return new \DateTimeImmutable($date, new \DateTimeZone($user->getTimezone())); + } + + return null; } public function isApproved(User $user, \DateTimeInterface $dateTime): bool { - $latestApproval = $this->getLatestApproval($user); - if ($latestApproval === null) { + $latestApprovalDate = $this->getLatestApprovalDate($user); + if ($latestApprovalDate === null) { return false; } - $latestApprovalDate = $latestApproval->getDate(); - $begin = \DateTimeImmutable::createFromInterface($dateTime); $begin = $begin->setTime(0, 0, 0); @@ -170,10 +184,8 @@ final class WorkingTimeService $this->workingTimeRepository->persistScheduledWorkingTimes(); - $key = 'u_' . $user->getId(); - if (\array_key_exists($key, $this->latestApprovals)) { - unset($this->latestApprovals[$key]); - } + $user->setPreferenceValue(self::LATEST_APPROVAL_PREF, $this->workingTimeRepository->getLatestApprovalDate($user)?->format(self::LATEST_APPROVAL_FORMAT)); + $this->userRepository->saveUser($user); $this->eventDispatcher->dispatch(new WorkingTimeApproveMonthEvent($user, $month, $approvalDate, $approvedBy)); } diff --git a/tests/Repository/TimesheetRepositoryTest.php b/tests/Repository/TimesheetRepositoryTest.php index b7a213fb..e5a43876 100644 --- a/tests/Repository/TimesheetRepositoryTest.php +++ b/tests/Repository/TimesheetRepositoryTest.php @@ -17,6 +17,7 @@ use App\Entity\User; use App\Repository\ActivityRepository; use App\Repository\ProjectRepository; use App\Repository\Query\TimesheetQuery; +use App\Repository\Query\TimesheetQueryHint; use App\Repository\TimesheetRepository; use App\Utils\Pagination; @@ -36,8 +37,15 @@ class TimesheetRepositoryTest extends AbstractRepositoryTest $result = $repository->getPagerfantaForQuery($query); $this->assertInstanceOf(Pagination::class, $result); + self::assertFalse($query->hasQueryHint(TimesheetQueryHint::CUSTOMER_META_FIELDS)); + self::assertFalse($query->hasQueryHint(TimesheetQueryHint::PROJECT_META_FIELDS)); + self::assertFalse($query->hasQueryHint(TimesheetQueryHint::ACTIVITY_META_FIELDS)); - $result = $repository->getTimesheetsForQuery($query); + $result = $repository->getTimesheetsForQuery($query, true); + + self::assertTrue($query->hasQueryHint(TimesheetQueryHint::CUSTOMER_META_FIELDS)); + self::assertTrue($query->hasQueryHint(TimesheetQueryHint::PROJECT_META_FIELDS)); + self::assertTrue($query->hasQueryHint(TimesheetQueryHint::ACTIVITY_META_FIELDS)); $this->assertIsArray($result); }