diff --git a/.changes/unreleased/Fixed-20241206-121932.yaml b/.changes/unreleased/Fixed-20241206-121932.yaml new file mode 100644 index 000000000..55117caf6 --- /dev/null +++ b/.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: "" diff --git a/src/Bundle/ChillMainBundle/Controller/WorkflowSignatureCancelController.php b/src/Bundle/ChillMainBundle/Controller/WorkflowSignatureStateChangeController.php similarity index 94% rename from src/Bundle/ChillMainBundle/Controller/WorkflowSignatureCancelController.php rename to src/Bundle/ChillMainBundle/Controller/WorkflowSignatureStateChangeController.php index 95cd1b779..24de6f1a0 100644 --- a/src/Bundle/ChillMainBundle/Controller/WorkflowSignatureCancelController.php +++ b/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()]) diff --git a/src/Bundle/ChillMainBundle/Tests/Workflow/SignatureStepStateChangerTest.php b/src/Bundle/ChillMainBundle/Tests/Workflow/SignatureStepStateChangerTest.php index 8a239c441..0950ecba2 100644 --- a/src/Bundle/ChillMainBundle/Tests/Workflow/SignatureStepStateChangerTest.php +++ b/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); diff --git a/src/Bundle/ChillMainBundle/Workflow/Messenger/PostSignatureStateChangeHandler.php b/src/Bundle/ChillMainBundle/Workflow/Messenger/PostSignatureStateChangeHandler.php index 1a0d7bc5d..820b9698a 100644 --- a/src/Bundle/ChillMainBundle/Workflow/Messenger/PostSignatureStateChangeHandler.php +++ b/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(); } } diff --git a/src/Bundle/ChillMainBundle/Workflow/SignatureStepStateChanger.php b/src/Bundle/ChillMainBundle/Workflow/SignatureStepStateChanger.php index cccfa2ab7..7315093d7 100644 --- a/src/Bundle/ChillMainBundle/Workflow/SignatureStepStateChanger.php +++ b/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()]); diff --git a/src/Bundle/ChillPersonBundle/Tests/Controller/WorkflowSignatureCancelControllerStepTest.php b/src/Bundle/ChillPersonBundle/Tests/Controller/WorkflowSignatureCancelControllerStepTest.php index 727f525d3..704ca6ffe 100644 --- a/src/Bundle/ChillPersonBundle/Tests/Controller/WorkflowSignatureCancelControllerStepTest.php +++ b/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');