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

Wrap signature state changes in transactions to prevent race conditions and ensure data integrity. Update controller and test class names to reflect broader state change capabilities. Enhance documentation with comments to clarify transaction requirements and procedure details for signature operations.

.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: ""

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()) {
-            $markSignature($signature);
-            $this->entityManager->flush();
+            $this->entityManager->wrapInTransaction(function () use ($signature, $markSignature) {
+                $markSignature($signature);
+            });
 
             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->signatureStepStateChanger->onPostMark($signature);
+        $this->entityManager->wrapInTransaction(function () use ($signature) {
+            $this->signatureStepStateChanger->onPostMark($signature);
+
+            $this->entityManager->flush();
+        });
 
-        $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');