Merge branch 'signature-state-change-wrapped-into-transaction' into 'master'

Refactor transaction handling for signature state changes, to wrap them into transactions

See merge request Chill-Projet/chill-bundles!771

.changes/unreleased/Fixed-20241206-121932.yaml

@@ -0,0 +1,5 @@
+kind: Fixed
+body: Wrap the signature's change state into a transaction, to avoid race conditions
+time: 2024-12-06T12:19:32.808137032+01:00
+custom:
+    Issue: ""

composer.json

@@ -95,6 +95,7 @@
         "phpstan/phpstan-strict-rules": "^1.0",
         "phpunit/phpunit": "^10.5.24",
         "rector/rector": "^1.1.0",
+        "symfony/amqp-messenger": "^5.4.45",
         "symfony/debug-bundle": "^5.4",
         "symfony/dotenv": "^5.4",
         "symfony/flex": "^2.4",

src/Bundle/ChillCalendarBundle/Service/ShortMessageNotification/DefaultRangeGenerator.php

@@ -18,8 +18,6 @@ declare(strict_types=1);
 
 namespace Chill\CalendarBundle\Service\ShortMessageNotification;
 
-use Monolog\DateTimeImmutable;
-
 /**
  * * Lundi => Envoi des rdv du mardi et mercredi.
  * * Mardi => Envoi des rdv du jeudi.
@@ -31,7 +29,7 @@ class DefaultRangeGenerator implements RangeGeneratorInterface
 {
     public function generateRange(\DateTimeImmutable $date): ?array
     {
-        $onMidnight = DateTimeImmutable::createFromFormat('Y-m-d H:i:s', $date->format('Y-m-d').' 00:00:00');
+        $onMidnight = \DateTimeImmutable::createFromFormat('Y-m-d H:i:s', $date->format('Y-m-d').' 00:00:00');
 
         switch ($dow = (int) $onMidnight->format('w')) {
             case 6: // Saturday

src/Bundle/ChillMainBundle/Controller/WorkflowSignatureStateChangeController.php

@@ -26,7 +26,7 @@ use Symfony\Component\Routing\Annotation\Route;
 use Symfony\Component\Security\Core\Security;
 use Twig\Environment;
 
-final readonly class WorkflowSignatureCancelController
+final readonly class WorkflowSignatureStateChangeController
 {
     public function __construct(
         private EntityManagerInterface $entityManager,
@@ -79,8 +79,9 @@ final readonly class WorkflowSignatureCancelController
         $form->handleRequest($request);
 
         if ($form->isSubmitted() && $form->isValid()) {
+            $this->entityManager->wrapInTransaction(function () use ($signature, $markSignature) {
                 $markSignature($signature);
-            $this->entityManager->flush();
+            });
 
             return new RedirectResponse(
                 $this->chillUrlGenerator->returnPathOr('chill_main_workflow_show', ['id' => $signature->getStep()->getEntityWorkflow()->getId()])

src/Bundle/ChillMainBundle/Tests/Workflow/SignatureStepStateChangerTest.php

@@ -14,10 +14,13 @@ namespace Chill\MainBundle\Tests\Workflow;
 use Chill\MainBundle\Entity\User;
 use Chill\MainBundle\Entity\Workflow\EntityWorkflow;
 use Chill\MainBundle\Entity\Workflow\EntityWorkflowSignatureStateEnum;
+use Chill\MainBundle\Entity\Workflow\EntityWorkflowStepSignature;
 use Chill\MainBundle\Workflow\EntityWorkflowMarkingStore;
 use Chill\MainBundle\Workflow\SignatureStepStateChanger;
 use Chill\MainBundle\Workflow\WorkflowTransitionContextDTO;
 use Chill\PersonBundle\Entity\Person;
+use Doctrine\DBAL\LockMode;
+use Doctrine\ORM\EntityManagerInterface;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\NullLogger;
 use Symfony\Component\Clock\MockClock;
@@ -47,7 +50,12 @@ class SignatureStepStateChangerTest extends TestCase
         $user = new User();
 
         $messengerBus = new MessageBus([]);
-        $changer = new SignatureStepStateChanger($registry, $clock, new NullLogger(), $messengerBus);
+        $entityManager = $this->createMock(EntityManagerInterface::class);
+        $entityManager->expects($this->exactly(4))->method('refresh')->with(
+            $this->isInstanceOf(EntityWorkflowStepSignature::class),
+            $this->logicalOr(LockMode::PESSIMISTIC_WRITE, LockMode::PESSIMISTIC_READ)
+        );
+        $changer = new SignatureStepStateChanger($registry, $clock, new NullLogger(), $messengerBus, $entityManager);
 
         // move it to signature
         $dto = new WorkflowTransitionContextDTO($entityWorkflow);
@@ -94,7 +102,12 @@ class SignatureStepStateChangerTest extends TestCase
         $user = new User();
 
         $messengerBus = new MessageBus([]);
-        $changer = new SignatureStepStateChanger($registry, $clock, new NullLogger(), $messengerBus);
+        $entityManager = $this->createMock(EntityManagerInterface::class);
+        $entityManager->expects($this->exactly(2))->method('refresh')->with(
+            $this->isInstanceOf(EntityWorkflowStepSignature::class),
+            $this->logicalOr(LockMode::PESSIMISTIC_WRITE, LockMode::PESSIMISTIC_READ)
+        );
+        $changer = new SignatureStepStateChanger($registry, $clock, new NullLogger(), $messengerBus, $entityManager);
 
         // move it to signature
         $dto = new WorkflowTransitionContextDTO($entityWorkflow);
@@ -126,7 +139,12 @@ class SignatureStepStateChangerTest extends TestCase
         $workflow = $registry->get($entityWorkflow, 'dummy');
         $clock = new MockClock();
         $user = new User();
-        $changer = new SignatureStepStateChanger($registry, $clock, new NullLogger(), new MessageBus([]));
+        $entityManager = $this->createMock(EntityManagerInterface::class);
+        $entityManager->expects($this->exactly(2))->method('refresh')->with(
+            $this->isInstanceOf(EntityWorkflowStepSignature::class),
+            $this->logicalOr(LockMode::PESSIMISTIC_WRITE, LockMode::PESSIMISTIC_READ)
+        );
+        $changer = new SignatureStepStateChanger($registry, $clock, new NullLogger(), new MessageBus([]), $entityManager);
 
         // move it to signature
         $dto = new WorkflowTransitionContextDTO($entityWorkflow);

src/Bundle/ChillMainBundle/Workflow/Messenger/PostSignatureStateChangeHandler.php

@@ -33,9 +33,12 @@ final readonly class PostSignatureStateChangeHandler implements MessageHandlerIn
             throw new UnrecoverableMessageHandlingException('signature not found');
         }
 
+        $this->entityManager->wrapInTransaction(function () use ($signature) {
             $this->signatureStepStateChanger->onPostMark($signature);
 
             $this->entityManager->flush();
+        });
+
         $this->entityManager->clear();
     }
 }

src/Bundle/ChillMainBundle/Workflow/SignatureStepStateChanger.php

@@ -16,11 +16,16 @@ use Chill\MainBundle\Entity\Workflow\EntityWorkflowSignatureStateEnum;
 use Chill\MainBundle\Entity\Workflow\EntityWorkflowStep;
 use Chill\MainBundle\Entity\Workflow\EntityWorkflowStepSignature;
 use Chill\MainBundle\Workflow\Messenger\PostSignatureStateChangeMessage;
+use Doctrine\DBAL\LockMode;
+use Doctrine\ORM\EntityManagerInterface;
 use Psr\Log\LoggerInterface;
 use Symfony\Component\Clock\ClockInterface;
 use Symfony\Component\Messenger\MessageBusInterface;
 use Symfony\Component\Workflow\Registry;
 
+/**
+ * Handles state changes for signature steps within a workflow.
+ */
 class SignatureStepStateChanger
 {
     private const LOG_PREFIX = '[SignatureStepStateChanger] ';
@@ -30,10 +35,26 @@ class SignatureStepStateChanger
         private readonly ClockInterface $clock,
         private readonly LoggerInterface $logger,
         private readonly MessageBusInterface $messageBus,
+        private readonly EntityManagerInterface $entityManager,
     ) {}
 
+    /**
+     * Marks a signature as signed.
+     *
+     * This method will acquire a lock on the database side, so it must be wrapped into an explicit
+     * transaction.
+     *
+     * This method updates the state of the provided signature entity, sets the signature index,
+     * and logs the action. Additionally, it dispatches a message indicating that the signature
+     * state has changed.
+     *
+     * @param EntityWorkflowStepSignature $signature the signature entity to be marked as signed
+     * @param int|null                    $atIndex   optional index position for the signature within the zone
+     */
     public function markSignatureAsSigned(EntityWorkflowStepSignature $signature, ?int $atIndex): void
     {
+        $this->entityManager->refresh($signature, LockMode::PESSIMISTIC_WRITE);
+
         $signature
             ->setState(EntityWorkflowSignatureStateEnum::SIGNED)
             ->setZoneSignatureIndex($atIndex)
@@ -42,8 +63,19 @@ class SignatureStepStateChanger
         $this->messageBus->dispatch(new PostSignatureStateChangeMessage((int) $signature->getId()));
     }
 
+    /**
+     * Marks a signature as canceled.
+     *
+     * This method will acquire a lock on the database side, so it must be wrapped into an explicit
+     * transaction.
+     *
+     * This method updates the signature state to 'canceled' and logs the action.
+     * It also dispatches a message to notify about the state change.
+     */
     public function markSignatureAsCanceled(EntityWorkflowStepSignature $signature): void
     {
+        $this->entityManager->refresh($signature, LockMode::PESSIMISTIC_WRITE);
+
         $signature
             ->setState(EntityWorkflowSignatureStateEnum::CANCELED)
             ->setStateDate($this->clock->now());
@@ -51,8 +83,21 @@ class SignatureStepStateChanger
         $this->messageBus->dispatch(new PostSignatureStateChangeMessage((int) $signature->getId()));
     }
 
+    /**
+     * Marks the given signature as rejected and updates its state and state date accordingly.
+     *
+     * This method will acquire a lock on the database side, so it must be wrapped into an explicit
+     * transaction.
+     *
+     * This method logs the rejection of the signature and dispatches a message indicating
+     * a state change has occurred.
+     *
+     * @param EntityWorkflowStepSignature $signature the signature entity to be marked as rejected
+     */
     public function markSignatureAsRejected(EntityWorkflowStepSignature $signature): void
     {
+        $this->entityManager->refresh($signature, LockMode::PESSIMISTIC_WRITE);
+
         $signature
             ->setState(EntityWorkflowSignatureStateEnum::REJECTED)
             ->setStateDate($this->clock->now());
@@ -63,10 +108,15 @@ class SignatureStepStateChanger
     /**
      * Executed after a signature has a new state.
      *
+     * This method will acquire a lock on the database side, so it must be wrapped into an explicit
+     * transaction.
+     *
      * This should be executed only by a system user (without any user registered)
      */
     public function onPostMark(EntityWorkflowStepSignature $signature): void
     {
+        $this->entityManager->refresh($signature, LockMode::PESSIMISTIC_READ);
+
         if (!EntityWorkflowStepSignature::isAllSignatureNotPendingForStep($signature->getStep())) {
             $this->logger->info(self::LOG_PREFIX.'This is not the last signature, skipping transition to another place', ['signatureId' => $signature->getId()]);
 

src/Bundle/ChillPersonBundle/Tests/Controller/WorkflowSignatureCancelControllerStepTest.php

@@ -11,7 +11,7 @@ declare(strict_types=1);
 
 namespace Chill\PersonBundle\Tests\Controller;
 
-use Chill\MainBundle\Controller\WorkflowSignatureCancelController;
+use Chill\MainBundle\Controller\WorkflowSignatureStateChangeController;
 use Chill\MainBundle\Entity\User;
 use Chill\MainBundle\Entity\Workflow\EntityWorkflow;
 use Chill\MainBundle\Routing\ChillUrlGeneratorInterface;
@@ -72,7 +72,7 @@ class WorkflowSignatureCancelControllerStepTest extends WebTestCase
         $twig->expects($this->once())->method('render')->withAnyParameters()
             ->willReturn('template');
 
-        $controller = new WorkflowSignatureCancelController($entityManager, $security, $this->formFactory, $twig, $this->signatureStepStateChanger, $this->chillUrlGenerator);
+        $controller = new WorkflowSignatureStateChangeController($entityManager, $security, $this->formFactory, $twig, $this->signatureStepStateChanger, $this->chillUrlGenerator);
 
         $request = new Request();
         $request->setMethod('GET');