From 7bf619aeca5c551202418142f4ce83ce88aad286 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Wed, 4 Mar 2026 15:09:32 +0100 Subject: [PATCH] Refactor transaction logic in `AuditDumpRequestHandler` and optimize audit dumping process - Removed unused `StoredObjectRepository` dependency and improved transaction management by replacing `beganTx` flag with `isTransactionActive` checks. - Adjusted entity locking logic to separate `find()` and `lock()` calls for `StoredObject`. - Simplified `AuditEventDumper` by removing redundant assertions and unused `AuditTrail` import. - Standardized subject display configuration in `AuditEventDumper` and ensured consistent data formatting. --- .../Audit/AuditEventDumper.php | 17 ++++----- .../Messenger/AuditDumpRequestHandler.php | 17 +++++---- .../Messenger/AuditDumpRequestHandlerTest.php | 36 +++++++++++++------ 3 files changed, 40 insertions(+), 30 deletions(-) diff --git a/src/Bundle/ChillMainBundle/Audit/AuditEventDumper.php b/src/Bundle/ChillMainBundle/Audit/AuditEventDumper.php index 0001ec8b7..95bb818db 100644 --- a/src/Bundle/ChillMainBundle/Audit/AuditEventDumper.php +++ b/src/Bundle/ChillMainBundle/Audit/AuditEventDumper.php @@ -16,7 +16,6 @@ use Chill\DocStoreBundle\Service\StoredObjectManagerInterface; use Chill\MainBundle\Audit\Exception\AuditDumpAlreadyGeneratedException; use Chill\MainBundle\Audit\Exception\AuditDumpTooMuchLines; use Chill\MainBundle\Repository\AuditTrailRepository; -use Chill\MainBundle\Entity\AuditTrail; use Chill\MainBundle\Templating\Entity\UserRender; use Doctrine\ORM\EntityManagerInterface; use Symfony\Contracts\Translation\TranslatorInterface; @@ -91,20 +90,16 @@ class AuditEventDumper $i = 0; foreach ($this->auditTrailRepository->findByCriteriaIterator($criteria) as $audit) { - \assert($audit instanceof AuditTrail); - - $occurredAt = $formatter?->format($audit->getOccurredAt()) ?: $audit->getOccurredAt()->format('c'); + $occurredAt = $formatter->format($audit->getOccurredAt()); $user = $audit->getUser(); $userId = $user?->getId(); - $userLabel = $user ? $this->userRender->renderString($user, ['at_date' => $audit->getOccurredAt()]) : ''; + $userLabel = null !== $user ? $this->userRender->renderString($user, ['at_date' => $audit->getOccurredAt()]) : ''; // Displayable name of the audit (main subject) $mainSubjectDisplay = ''; $mainSubject = $audit->getMainSubject(); - if (null !== $mainSubject) { - $mainSubjectDisplay = $this->converterManager->display(Subject::fromArray($mainSubject), 'string'); - } + $mainSubjectDisplay = $this->converterManager->display(Subject::fromArray($mainSubject), 'string'); // Translated action $action = $this->translator->trans('audit_trail.action.'.$audit->getAction(), [], null, $lang); @@ -115,9 +110,9 @@ class AuditEventDumper // Associated subjects, comma separated $subjectsDisplay = ''; $subjects = $audit->getSubjects(); - if (!empty($subjects)) { + if ([] !== $subjects) { $subjectsDisplay = implode(', ', array_map(function (array $s): string { - return $this->converterManager->display(Subject::fromArray($s), 'fr'); + return $this->converterManager->display(Subject::fromArray($s), 'string'); }, $subjects)); } @@ -145,7 +140,7 @@ class AuditEventDumper $storedObject->setStatus(StoredObject::STATUS_READY); $this->entityManager->flush(); - $content = file_get_contents($tmpPath) ?: ''; + $content = file_get_contents($tmpPath); $this->storedObjectManager->write($storedObject, $content, 'text/csv'); // Remove temp file from disk diff --git a/src/Bundle/ChillMainBundle/Audit/Messenger/AuditDumpRequestHandler.php b/src/Bundle/ChillMainBundle/Audit/Messenger/AuditDumpRequestHandler.php index d68dee24f..240c9b75d 100644 --- a/src/Bundle/ChillMainBundle/Audit/Messenger/AuditDumpRequestHandler.php +++ b/src/Bundle/ChillMainBundle/Audit/Messenger/AuditDumpRequestHandler.php @@ -12,7 +12,6 @@ declare(strict_types=1); namespace Chill\MainBundle\Audit\Messenger; use Chill\DocStoreBundle\Entity\StoredObject; -use Chill\DocStoreBundle\Repository\StoredObjectRepositoryInterface; use Chill\MainBundle\Audit\AuditEventDumper; use Chill\MainBundle\Audit\Exception\AuditDumpAlreadyGeneratedException; use Chill\MainBundle\Audit\Exception\AuditDumpTooMuchLines; @@ -27,7 +26,6 @@ final readonly class AuditDumpRequestHandler implements MessageHandlerInterface { public function __construct( private AuditEventDumper $auditEventDumper, - private StoredObjectRepositoryInterface $storedObjectRepository, private EntityManagerInterface $entityManager, private UserRepositoryInterface $userRepository, ) {} @@ -35,24 +33,25 @@ final readonly class AuditDumpRequestHandler implements MessageHandlerInterface public function __invoke(AuditDumpRequestMessage $message): void { $conn = $this->entityManager->getConnection(); - $beganTx = false; if (!$conn->isTransactionActive()) { $conn->beginTransaction(); - $beganTx = true; } try { // Lock the StoredObject until the end of the process - $storedObject = $this->entityManager->find(StoredObject::class, $message->storedObjectId, LockMode::PESSIMISTIC_WRITE); + $storedObject = $this->entityManager->find(StoredObject::class, $message->storedObjectId); + if (!$storedObject instanceof StoredObject) { // Nothing to do if stored object does not exist anymore - if ($beganTx) { + if ($conn->isTransactionActive()) { $conn->commit(); } return; } + $this->entityManager->lock($storedObject, LockMode::PESSIMISTIC_WRITE); + // Build criteria expected by AuditTrailRepository $criteria = []; if (null !== $message->from) { @@ -83,18 +82,18 @@ final readonly class AuditDumpRequestHandler implements MessageHandlerInterface $this->auditEventDumper->dump($criteria, $message->lang, $storedObject); $this->entityManager->flush(); - if ($beganTx) { + if ($conn->isTransactionActive()) { $conn->commit(); } } catch (AuditDumpTooMuchLines|AuditDumpAlreadyGeneratedException $e) { - if ($beganTx) { + if ($conn->isTransactionActive()) { $this->entityManager->flush(); $conn->commit(); } throw new UnrecoverableMessageHandlingException(previous: $e); } catch (\Throwable $e) { - if ($beganTx && $conn->isTransactionActive()) { + if ($conn->isTransactionActive()) { $conn->rollBack(); } throw $e; diff --git a/src/Bundle/ChillMainBundle/Tests/Audit/Messenger/AuditDumpRequestHandlerTest.php b/src/Bundle/ChillMainBundle/Tests/Audit/Messenger/AuditDumpRequestHandlerTest.php index 2c6e1184e..3b8b45b90 100644 --- a/src/Bundle/ChillMainBundle/Tests/Audit/Messenger/AuditDumpRequestHandlerTest.php +++ b/src/Bundle/ChillMainBundle/Tests/Audit/Messenger/AuditDumpRequestHandlerTest.php @@ -57,7 +57,6 @@ class AuditDumpRequestHandlerTest extends TestCase $this->handler = new AuditDumpRequestHandler( $this->auditEventDumper->reveal(), - $this->storedObjectRepository->reveal(), $this->entityManager->reveal(), $this->userRepository->reveal() ); @@ -77,11 +76,17 @@ class AuditDumpRequestHandlerTest extends TestCase $storedObject = new StoredObject(); $user = $this->prophesize(User::class)->reveal(); - $this->connection->isTransactionActive()->willReturn(false); - $this->connection->beginTransaction()->shouldBeCalled(); + $beganTx = false; + $this->connection->isTransactionActive()->will(function ($args) use (&$beganTx) { + return $beganTx; + }); + $this->connection->beginTransaction()->will(function ($args) use (&$beganTx) { + $beganTx = true; + })->shouldBeCalled(); - $this->entityManager->find(StoredObject::class, 123, LockMode::PESSIMISTIC_WRITE) + $this->entityManager->find(StoredObject::class, 123) ->willReturn($storedObject); + $this->entityManager->lock($storedObject, LockMode::PESSIMISTIC_WRITE)->shouldBeCalled(); $this->userRepository->find(1)->willReturn($user); @@ -119,11 +124,17 @@ class AuditDumpRequestHandlerTest extends TestCase $message = new AuditDumpRequestMessage('fr', 123); $storedObject = new StoredObject(); - $this->connection->isTransactionActive()->willReturn(false, true); - $this->connection->beginTransaction()->shouldBeCalled(); + $beganTx = false; + $this->connection->isTransactionActive()->will(function ($args) use (&$beganTx) { + return $beganTx; + }); + $this->connection->beginTransaction()->will(function ($args) use (&$beganTx) { + $beganTx = true; + })->shouldBeCalled(); - $this->entityManager->find(StoredObject::class, 123, LockMode::PESSIMISTIC_WRITE) + $this->entityManager->find(StoredObject::class, 123) ->willReturn($storedObject); + $this->entityManager->lock($storedObject, LockMode::PESSIMISTIC_WRITE)->shouldBeCalled(); $this->auditEventDumper->dump(Argument::any(), 'fr', $storedObject) ->willThrow($exception); @@ -153,10 +164,15 @@ class AuditDumpRequestHandlerTest extends TestCase { $message = new AuditDumpRequestMessage('fr', 123); - $this->connection->isTransactionActive()->willReturn(false); - $this->connection->beginTransaction()->shouldBeCalled(); + $beganTx = false; + $this->connection->isTransactionActive()->will(function ($args) use (&$beganTx) { + return $beganTx; + }); + $this->connection->beginTransaction()->will(function ($args) use (&$beganTx) { + $beganTx = true; + })->shouldBeCalled(); - $this->entityManager->find(StoredObject::class, 123, LockMode::PESSIMISTIC_WRITE) + $this->entityManager->find(StoredObject::class, 123) ->willReturn(null); $this->connection->commit()->shouldBeCalled();