From 527cf23d4fd716670c88067ef5ec739d8fbb3beb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Mon, 21 Oct 2024 17:39:31 +0200 Subject: [PATCH] Fix Canceling of stale workflow cronjob Refactor workflow cancellation logic to encapsulate transition checks in a dedicated method, and update CronJob handling to use entity workflows instead of IDs. Enhance test coverage to ensure proper handling and instantiate mocks for EntityManagerInterface. --- .../Workflow/EntityWorkflowRepository.php | 8 +++--- .../Workflow/CancelStaleWorkflowCronJob.php | 16 ++++++++---- .../Workflow/CancelStaleWorkflowHandler.php | 19 +++++++++++--- .../EntityWorkflowRepositoryTest.php | 9 +++++-- .../CancelStaleWorkflowCronJobTest.php | 26 ++++++++++++++++--- .../CancelStaleWorkflowHandlerTest.php | 24 ++++++++++------- 6 files changed, 73 insertions(+), 29 deletions(-) diff --git a/src/Bundle/ChillMainBundle/Repository/Workflow/EntityWorkflowRepository.php b/src/Bundle/ChillMainBundle/Repository/Workflow/EntityWorkflowRepository.php index cd39b6e28..a3f210ed2 100644 --- a/src/Bundle/ChillMainBundle/Repository/Workflow/EntityWorkflowRepository.php +++ b/src/Bundle/ChillMainBundle/Repository/Workflow/EntityWorkflowRepository.php @@ -205,13 +205,13 @@ class EntityWorkflowRepository implements ObjectRepository * * @param \DateTimeImmutable $olderThanDate the date to compare against * - * @return list the list of workflow IDs that meet the criteria + * @return iterable */ - public function findWorkflowsWithoutFinalStepAndOlderThan(\DateTimeImmutable $olderThanDate): array + public function findWorkflowsWithoutFinalStepAndOlderThan(\DateTimeImmutable $olderThanDate): iterable { $qb = $this->repository->createQueryBuilder('sw'); - $qb->select('sw.id') + $qb->select('sw') // only the workflow which are not finalized ->where(sprintf('NOT EXISTS (SELECT 1 FROM %s ews WHERE ews.isFinal = TRUE AND ews.entityWorkflow = sw.id)', EntityWorkflowStep::class)) ->andWhere( @@ -227,7 +227,7 @@ class EntityWorkflowRepository implements ObjectRepository ->setParameter('initial', 'initial') ; - return $qb->getQuery()->getResult(); + return $qb->getQuery()->toIterable(); } public function getClassName(): string diff --git a/src/Bundle/ChillMainBundle/Service/Workflow/CancelStaleWorkflowCronJob.php b/src/Bundle/ChillMainBundle/Service/Workflow/CancelStaleWorkflowCronJob.php index be3dd2a5e..e18f660c9 100644 --- a/src/Bundle/ChillMainBundle/Service/Workflow/CancelStaleWorkflowCronJob.php +++ b/src/Bundle/ChillMainBundle/Service/Workflow/CancelStaleWorkflowCronJob.php @@ -14,6 +14,7 @@ namespace Chill\MainBundle\Service\Workflow; use Chill\MainBundle\Cron\CronJobInterface; use Chill\MainBundle\Entity\CronJobExecution; use Chill\MainBundle\Repository\Workflow\EntityWorkflowRepository; +use Doctrine\ORM\EntityManagerInterface; use Psr\Log\LoggerInterface; use Symfony\Component\Clock\ClockInterface; use Symfony\Component\Messenger\MessageBusInterface; @@ -31,6 +32,7 @@ class CancelStaleWorkflowCronJob implements CronJobInterface private readonly ClockInterface $clock, private readonly MessageBusInterface $messageBus, private readonly LoggerInterface $logger, + private readonly EntityManagerInterface $entityManager, ) {} public function canRun(?CronJobExecution $cronJobExecution): bool @@ -48,19 +50,23 @@ class CancelStaleWorkflowCronJob implements CronJobInterface $this->logger->info('Cronjob started: Canceling stale workflows.'); $olderThanDate = $this->clock->now()->sub(new \DateInterval(self::KEEP_INTERVAL)); - $staleWorkflowIds = $this->workflowRepository->findWorkflowsWithoutFinalStepAndOlderThan($olderThanDate); + $staleEntityWorkflows = $this->workflowRepository->findWorkflowsWithoutFinalStepAndOlderThan($olderThanDate); $lastCanceled = $lastExecutionData[self::LAST_CANCELED_WORKFLOW] ?? 0; $processedCount = 0; - foreach ($staleWorkflowIds as $wId) { + foreach ($staleEntityWorkflows as $staleEntityWorkflow) { try { - $this->messageBus->dispatch(new CancelStaleWorkflowMessage($wId)); - $lastCanceled = max($wId, $lastCanceled); + $this->messageBus->dispatch(new CancelStaleWorkflowMessage($staleEntityWorkflow->getId())); + $lastCanceled = max($staleEntityWorkflow->getId(), $lastCanceled); ++$processedCount; } catch (\Exception $e) { - $this->logger->error("Failed to dispatch CancelStaleWorkflow for ID {$wId}", ['exception' => $e]); + $this->logger->error('Failed to dispatch CancelStaleWorkflow', ['exception' => $e, 'entityWorkflowId' => $staleEntityWorkflow->getId()]); continue; } + + if (0 === $processedCount % 10) { + $this->entityManager->clear(); + } } $this->logger->info("Cronjob completed: {$processedCount} workflows processed."); diff --git a/src/Bundle/ChillMainBundle/Service/Workflow/CancelStaleWorkflowHandler.php b/src/Bundle/ChillMainBundle/Service/Workflow/CancelStaleWorkflowHandler.php index 54485dad5..d8f0863fc 100644 --- a/src/Bundle/ChillMainBundle/Service/Workflow/CancelStaleWorkflowHandler.php +++ b/src/Bundle/ChillMainBundle/Service/Workflow/CancelStaleWorkflowHandler.php @@ -18,7 +18,9 @@ use Psr\Log\LoggerInterface; use Symfony\Component\Clock\ClockInterface; use Symfony\Component\Messenger\Attribute\AsMessageHandler; use Symfony\Component\Messenger\Exception\UnrecoverableMessageHandlingException; +use Symfony\Component\Workflow\Metadata\MetadataStoreInterface; use Symfony\Component\Workflow\Registry; +use Symfony\Component\Workflow\Transition; #[AsMessageHandler] final readonly class CancelStaleWorkflowHandler @@ -57,10 +59,7 @@ final readonly class CancelStaleWorkflowHandler $wasInInitialPosition = 'initial' === $workflow->getStep(); foreach ($transitions as $transition) { - $isFinal = $metadataStore->getMetadata('isFinal', $transition); - $isFinalPositive = $metadataStore->getMetadata('isFinalPositive', $transition); - - if ($isFinal && !$isFinalPositive) { + if ($this->willTransitionLeadToFinalNegative($transition, $metadataStore)) { $dto = new WorkflowTransitionContextDTO($workflow); $workflowComponent->apply($workflow, $transition->getName(), [ 'context' => $dto, @@ -85,4 +84,16 @@ final readonly class CancelStaleWorkflowHandler $this->em->flush(); } + + private function willTransitionLeadToFinalNegative(Transition $transition, MetadataStoreInterface $metadataStore): bool + { + foreach ($transition->getTos() as $place) { + $metadata = $metadataStore->getPlaceMetadata($place); + if (($metadata['isFinal'] ?? true) && false === ($metadata['isFinalPositive'] ?? true)) { + return true; + } + } + + return false; + } } diff --git a/src/Bundle/ChillMainBundle/Tests/Repository/EntityWorkflowRepositoryTest.php b/src/Bundle/ChillMainBundle/Tests/Repository/EntityWorkflowRepositoryTest.php index a26b29709..6e11a7e9c 100644 --- a/src/Bundle/ChillMainBundle/Tests/Repository/EntityWorkflowRepositoryTest.php +++ b/src/Bundle/ChillMainBundle/Tests/Repository/EntityWorkflowRepositoryTest.php @@ -12,6 +12,7 @@ declare(strict_types=1); namespace ChillMainBundle\Tests\Repository; use Chill\MainBundle\Entity\User; +use Chill\MainBundle\Entity\Workflow\EntityWorkflow; use Chill\MainBundle\Repository\Workflow\EntityWorkflowRepository; use Doctrine\ORM\EntityManagerInterface; use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase; @@ -40,9 +41,13 @@ class EntityWorkflowRepositoryTest extends KernelTestCase { $repository = new EntityWorkflowRepository($this->em); - $actual = $repository->findWorkflowsWithoutFinalStepAndOlderThan(new \DateTimeImmutable('10 years ago')); + $actual = $repository->findWorkflowsWithoutFinalStepAndOlderThan((new \DateTimeImmutable('now'))->add(new \DateInterval('P10Y'))); - self::assertIsArray($actual, 'check that the query is successful'); + self::assertIsIterable($actual, 'check that the query is successful'); + + foreach ($actual as $entityWorkflow) { + self::assertInstanceOf(EntityWorkflow::class, $entityWorkflow); + } } public function testCountQueryByDest(): void diff --git a/src/Bundle/ChillMainBundle/Tests/Services/Workflow/CancelStaleWorkflowCronJobTest.php b/src/Bundle/ChillMainBundle/Tests/Services/Workflow/CancelStaleWorkflowCronJobTest.php index 200b92099..e560e3871 100644 --- a/src/Bundle/ChillMainBundle/Tests/Services/Workflow/CancelStaleWorkflowCronJobTest.php +++ b/src/Bundle/ChillMainBundle/Tests/Services/Workflow/CancelStaleWorkflowCronJobTest.php @@ -9,12 +9,14 @@ declare(strict_types=1); * the LICENSE file that was distributed with this source code. */ -namespace Services\Workflow; +namespace ChillMainBundle\Tests\Services\Workflow; use Chill\MainBundle\Entity\CronJobExecution; +use Chill\MainBundle\Entity\Workflow\EntityWorkflow; use Chill\MainBundle\Repository\Workflow\EntityWorkflowRepository; use Chill\MainBundle\Service\Workflow\CancelStaleWorkflowCronJob; use Chill\MainBundle\Service\Workflow\CancelStaleWorkflowMessage; +use Doctrine\ORM\EntityManagerInterface; use PHPUnit\Framework\MockObject\Exception; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; @@ -39,8 +41,9 @@ class CancelStaleWorkflowCronJobTest extends TestCase { $clock = new MockClock(new \DateTimeImmutable('2024-01-01 00:00:00', new \DateTimeZone('+00:00'))); $logger = $this->createMock(LoggerInterface::class); + $entityManager = $this->createMock(EntityManagerInterface::class); - $cronJob = new CancelStaleWorkflowCronJob($this->createMock(EntityWorkflowRepository::class), $clock, $this->buildMessageBus(), $logger); + $cronJob = new CancelStaleWorkflowCronJob($this->createMock(EntityWorkflowRepository::class), $clock, $this->buildMessageBus(), $logger, $entityManager); self::assertEquals($expected, $cronJob->canRun($cronJobExecution)); } @@ -54,11 +57,16 @@ class CancelStaleWorkflowCronJobTest extends TestCase { $clock = new MockClock((new \DateTimeImmutable('now', new \DateTimeZone('+00:00')))->add(new \DateInterval('P120D'))); $workflowRepository = $this->createMock(EntityWorkflowRepository::class); + $entityManager = $this->createMock(EntityManagerInterface::class); - $workflowRepository->method('findWorkflowsWithoutFinalStepAndOlderThan')->willReturn([1, 3, 2]); + $workflowRepository->method('findWorkflowsWithoutFinalStepAndOlderThan')->willReturn([ + $this->buildEntityWorkflow(1), + $this->buildEntityWorkflow(3), + $this->buildEntityWorkflow(2), + ]); $messageBus = $this->buildMessageBus(true); - $cronJob = new CancelStaleWorkflowCronJob($workflowRepository, $clock, $messageBus, new NullLogger()); + $cronJob = new CancelStaleWorkflowCronJob($workflowRepository, $clock, $messageBus, new NullLogger(), $entityManager); $results = $cronJob->run([]); @@ -67,6 +75,16 @@ class CancelStaleWorkflowCronJobTest extends TestCase self::assertEquals(3, $results['last-canceled-workflow-id']); } + private function buildEntityWorkflow(int $id): EntityWorkflow + { + $entityWorkflow = new EntityWorkflow(); + $reflectionClass = new \ReflectionClass($entityWorkflow); + $idProperty = $reflectionClass->getProperty('id'); + $idProperty->setValue($entityWorkflow, $id); + + return $entityWorkflow; + } + /** * @throws \Exception */ diff --git a/src/Bundle/ChillMainBundle/Tests/Services/Workflow/CancelStaleWorkflowHandlerTest.php b/src/Bundle/ChillMainBundle/Tests/Services/Workflow/CancelStaleWorkflowHandlerTest.php index b6cac68de..275adfe75 100644 --- a/src/Bundle/ChillMainBundle/Tests/Services/Workflow/CancelStaleWorkflowHandlerTest.php +++ b/src/Bundle/ChillMainBundle/Tests/Services/Workflow/CancelStaleWorkflowHandlerTest.php @@ -131,18 +131,22 @@ class CancelStaleWorkflowHandlerTest extends TestCase ->setInitialPlaces('initial') ->addPlaces(['initial', 'step1', 'canceled', 'final']) ->addTransition(new Transition('to_step1', 'initial', 'step1')) - ->addTransition($cancelInit = new Transition('cancel', 'initial', 'canceled')) - ->addTransition($finalizeInit = new Transition('finalize', 'initial', 'final')) - ->addTransition($cancelStep1 = new Transition('cancel', 'step1', 'canceled')) - ->addTransition($finalizeStep1 = new Transition('finalize', 'step1', 'final')); + ->addTransition(new Transition('cancel', 'initial', 'canceled')) + ->addTransition(new Transition('finalize', 'initial', 'final')) + ->addTransition(new Transition('cancel', 'step1', 'canceled')) + ->addTransition(new Transition('finalize', 'step1', 'final')); - $transitionStorage = new \SplObjectStorage(); - $transitionStorage->attach($finalizeInit, ['isFinal' => true, 'isFinalPositive' => true]); - $transitionStorage->attach($cancelInit, ['isFinal' => true, 'isFinalPositive' => false]); - $transitionStorage->attach($finalizeStep1, ['isFinal' => true, 'isFinalPositive' => true]); - $transitionStorage->attach($cancelStep1, ['isFinal' => true, 'isFinalPositive' => false]); + $definitionBuilder->setMetadataStore(new InMemoryMetadataStore(placesMetadata: [ + 'canceled' => [ + 'isFinal' => true, + 'isFinalPositive' => false, + ], + 'final' => [ + 'isFinal' => true, + 'isFinalPositive', true, + ], + ])); - $definitionBuilder->setMetadataStore(new InMemoryMetadataStore([], [], $transitionStorage)); $workflow = new Workflow($definitionBuilder->build(), new EntityWorkflowMarkingStore(), null, 'dummy_workflow'); $supports = new class () implements WorkflowSupportStrategyInterface {