From 3af7824d0162e18f02711de7607250cc7b5a597e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Fri, 6 Dec 2024 12:20:42 +0100 Subject: [PATCH] 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. --- .../unreleased/Fixed-20241206-121932.yaml | 5 ++ ...orkflowSignatureStateChangeController.php} | 7 +-- .../SignatureStepStateChangerTest.php | 24 +++++++-- .../PostSignatureStateChangeHandler.php | 7 ++- .../Workflow/SignatureStepStateChanger.php | 50 +++++++++++++++++++ ...kflowSignatureCancelControllerStepTest.php | 4 +- 6 files changed, 87 insertions(+), 10 deletions(-) create mode 100644 .changes/unreleased/Fixed-20241206-121932.yaml rename src/Bundle/ChillMainBundle/Controller/{WorkflowSignatureCancelController.php => WorkflowSignatureStateChangeController.php} (94%) 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');