fix(core/revision): duplicate ouuid put back deleted revision (#955)

EMS/core-bundle/src/Controller/ContentManagement/CrudController.php

@@ -45,20 +45,20 @@ public function createAction(?string $ouuid, string $name, Request $request): Re
                 $this->dataService->refresh($newRevision->giveContentType()->giveEnvironment());
             }
         } catch (\Exception $e) {
-            if (($e instanceof NotFoundHttpException) or ($e instanceof BadRequestHttpException)) {
+            if ($e instanceof NotFoundHttpException || $e instanceof BadRequestHttpException) {
                 throw $e;
-            } else {
-                $this->logger->error('log.crud.create_error', [
-                    EmsFields::LOG_CONTENTTYPE_FIELD => $contentType->getName(),
-                    EmsFields::LOG_ERROR_MESSAGE_FIELD => $e->getMessage(),
-                    EmsFields::LOG_EXCEPTION_FIELD => $e,
-                ]);
             }
 
+            $this->logger->error('log.crud.create_error', [
+                EmsFields::LOG_CONTENTTYPE_FIELD => $contentType->getName(),
+                EmsFields::LOG_ERROR_MESSAGE_FIELD => $e->getMessage(),
+                EmsFields::LOG_EXCEPTION_FIELD => $e,
+            ]);
+
             return $this->render("@$this->templateNamespace/ajax/notification.json.twig", [
-                    'success' => false,
-                    'ouuid' => $ouuid,
-                    'type' => $contentType->getName(),
+                'success' => false,
+                'ouuid' => $ouuid,
+                'type' => $contentType->getName(),
             ]);
         }
 

EMS/core-bundle/src/Controller/Revision/TrashController.php

@@ -81,9 +81,13 @@ public function putBack(ContentType $contentType, string $ouuid): RedirectRespon
             throw $this->createAccessDeniedException();
         }
 
-        $revisionId = $this->dataService->trashPutBack($contentType, $ouuid);
+        $restoredRevision = $this->dataService->trashPutBackAsDraft($contentType, $ouuid);
 
-        return $this->redirectToRoute(Routes::EDIT_REVISION, ['revisionId' => $revisionId]);
+        if (!$restoredRevision) {
+            throw new \RuntimeException(\sprintf('Put back failed for ouuid "%s"', $ouuid));
+        }
+
+        return $this->redirectToRoute(Routes::EDIT_REVISION, ['revisionId' => $restoredRevision->getId()]);
     }
 
     private function emptyTrashSelection(ContentType $contentType, string ...$ouuids): Response
@@ -103,10 +107,10 @@ private function putBackSelection(ContentType $contentType, string ...$ouuids):
             throw $this->createAccessDeniedException();
         }
 
-        $revisionId = $this->dataService->trashPutBack($contentType, ...$ouuids);
+        $restoredRevision = $this->dataService->trashPutBackAsDraft($contentType, ...$ouuids);
 
-        if ($revisionId) {
-            return $this->redirectToRoute(Routes::EDIT_REVISION, ['revisionId' => $revisionId]);
+        if ($restoredRevision) {
+            return $this->redirectToRoute(Routes::EDIT_REVISION, ['revisionId' => $restoredRevision->getId()]);
         }
 
         return $this->redirectToRoute(Routes::DRAFT_IN_PROGRESS, ['contentTypeId' => $contentType->getId()]);

EMS/core-bundle/src/Exception/DuplicateOuuidException.php

@@ -1,7 +1,15 @@
 <?php
 
+declare(strict_types=1);
+
 namespace EMS\CoreBundle\Exception;
 
 class DuplicateOuuidException extends ElasticmsException
 {
+    public function __construct(string $ouuid, string $contentTypeName)
+    {
+        parent::__construct(
+            message: \sprintf('Duplicate ouuid "%s" for content type "%s"', $ouuid, $contentTypeName)
+        );
+    }
 }

EMS/core-bundle/src/Repository/RevisionRepository.php

@@ -349,12 +349,15 @@ public function countByContentType(ContentType $contentType): int
 
     public function countRevisions(string $ouuid, ContentType $contentType): int
     {
-        $qb = $this->createQueryBuilder('r')
-            ->select('COUNT(r)');
-        $qb->where($qb->expr()->eq('r.ouuid', ':ouuid'));
-        $qb->andWhere($qb->expr()->eq('r.contentType', ':contentType'));
-        $qb->setParameter('ouuid', $ouuid);
-        $qb->setParameter('contentType', $contentType);
+        $qb = $this->createQueryBuilder('r');
+        $qb
+            ->select('COUNT(r.id)')
+            ->andWhere($qb->expr()->eq('r.ouuid', ':ouuid'))
+            ->andWhere($qb->expr()->eq('r.contentType', ':contentType'))
+            ->setParameters([
+                'ouuid' => $ouuid,
+                'contentType' => $contentType,
+            ]);
 
         return (int) $qb->getQuery()->getSingleScalarResult();
     }
@@ -475,20 +478,19 @@ public function finaliseRevision(ContentType $contentType, string $ouuid, \DateT
 
     public function getCurrentRevision(ContentType $contentType, string $ouuid): ?Revision
     {
-        $qb = $this->createQueryBuilder('r')->select()
-            ->where('r.contentType = ?2')
-            ->andWhere('r.ouuid = ?3')
-            ->andWhere('r.endTime is null')
-            ->setParameter(2, $contentType)
-            ->setParameter(3, $ouuid);
+        $qb = $this->createQueryBuilder('r');
+        $qb
+            ->andWhere($qb->expr()->eq('r.ouuid', ':ouuid'))
+            ->andWhere($qb->expr()->eq('r.contentType', ':contentType'))
+            ->andWhere($qb->expr()->isNull('r.endTime'))
+            ->setParameters([
+                'ouuid' => $ouuid,
+                'contentType' => $contentType,
+            ]);
 
-        /** @var Revision[] $currentRevision */
-        $currentRevision = $qb->getQuery()->execute();
-        if (isset($currentRevision[0])) {
-            return $currentRevision[0];
-        } else {
-            return null;
-        }
+        $revision = $qb->getQuery()->getOneOrNullResult();
+
+        return $revision instanceof Revision ? $revision : null;
     }
 
     public function publishRevision(Revision $revision, bool $draft = false): int
@@ -833,6 +835,7 @@ public function findTrashRevisions(ContentType $contentType, string ...$ouuids):
             ->andWhere($qb->expr()->eq('c.id', ':content_type_id'))
             ->andWhere($qb->expr()->in('r.ouuid', ':ouuids'))
             ->andWhere($qb->expr()->eq('r.deleted', $qb->expr()->literal(true)))
+            ->orderBy('r.startTime', 'DESC')
             ->setParameter('content_type_id', $contentType->getId())
             ->setParameter('ouuids', $ouuids, ArrayParameterType::STRING)
         ;

EMS/core-bundle/src/Service/DataService.php

@@ -49,7 +49,6 @@
 use Symfony\Component\Form\FormInterface;
 use Symfony\Component\Form\FormRegistryInterface;
 use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
-use Symfony\Component\HttpKernel\Exception\ConflictHttpException;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
@@ -443,36 +442,14 @@ public function createData(?string $ouuid, array $rawdata, ContentType $contentT
         $newRevision->setEndTime(null);
         $newRevision->setDeleted(false);
         $newRevision->setDraft(true);
-        if ($byARealUser) {
-            $token = $this->tokenStorage->getToken();
-            if (null === $token) {
-                throw new \RuntimeException('Unexpected null token');
-            }
-            $newRevision->setLockBy($token->getUserIdentifier());
-        } else {
-            $newRevision->setLockBy('DATA_SERVICE');
-        }
-        $newRevision->setLockUntil($until);
-        $newRevision->setRawData($rawdata);
 
-        $em = $this->doctrine->getManager();
-        if (!empty($ouuid)) {
-            $revisionRepository = $em->getRepository(Revision::class);
-            $anotherObject = $revisionRepository->findOneBy([
-                    'contentType' => $contentType,
-                    'ouuid' => $newRevision->getOuuid(),
-                    'endTime' => null,
-            ]);
+        $lockBy = $byARealUser ? $this->userService->getCurrentUser()->getUserIdentifier() : 'DATA_SERVICE';
+        $newRevision->setLockBy($lockBy);
 
-            if (!empty($anotherObject)) {
-                throw new ConflictHttpException('Duplicate OUUID '.$ouuid.' for this content type');
-            }
-        }
-
-        $em->persist($newRevision);
-        $em->flush();
+        $newRevision->setLockUntil($until);
+        $newRevision->setRawData($rawdata);
 
-        return $newRevision;
+        return $this->createNewRevision($newRevision);
     }
 
     /**
@@ -934,15 +911,9 @@ public function getNewestRevision(string $type, string $ouuid): Revision
     public function newDocument(ContentType $contentType, ?string $ouuid = null, ?array $rawData = null, ?string $username = null): Revision
     {
         $this->hasCreateRights($contentType);
-        /** @var RevisionRepository $revisionRepository */
-        $revisionRepository = $this->em->getRepository(Revision::class);
 
         $revision = new Revision();
 
-        if (null !== $ouuid && $revisionRepository->countRevisions($ouuid, $contentType)) {
-            throw new DuplicateOuuidException();
-        }
-
         if (!empty($contentType->getDefaultValue())) {
             try {
                 $template = $this->twig->createTemplate($contentType->getDefaultValue());
@@ -1026,10 +997,7 @@ public function newDocument(ContentType $contentType, ?string $ouuid = null, ?ar
         }
         $this->setMetaFields($revision);
 
-        $this->em->persist($revision);
-        $this->em->flush();
-
-        return $revision;
+        return $this->createNewRevision($revision);
     }
 
     /**
@@ -1277,25 +1245,23 @@ public function trashEmpty(ContentType $contentType, string ...$ouuids): void
         $this->em->flush();
     }
 
-    public function trashPutBack(ContentType $contentType, string ...$ouuids): null|int
+    public function trashPutBackAsDraft(ContentType $contentType, string ...$ouuids): null|Revision
     {
-        $revisionIds = [];
         $revisions = $this->revRepository->findTrashRevisions($contentType, ...$ouuids);
 
         foreach ($revisions as $revision) {
-            $this->lockRevision($revision);
             $revision->setDeleted(false);
             $revision->setDeletedBy(null);
             if (null === $revision->getEndTime()) {
+                $this->lockRevision($revision);
                 $revision->setDraft(true);
-                $revisionIds[] = $revision->getId();
                 $this->auditLogger->notice('log.revision.restored', LogRevisionContext::update($revision));
             }
             $this->em->persist($revision);
         }
         $this->em->flush();
 
-        return 1 === \count($ouuids) ? \array_shift($revisionIds) : null;
+        return 1 === \count($ouuids) ? \array_shift($revisions) : null;
     }
 
     /**
@@ -1985,4 +1951,36 @@ public function getAllDrafts(): array
     {
         return $this->revRepository->findAllDrafts();
     }
+
+    private function createNewRevision(Revision $revision): Revision
+    {
+        $ouuid = $revision->getOuuid();
+        $contentType = $revision->giveContentType();
+
+        $currentRevision = $ouuid ? $this->revRepository->getCurrentRevision($contentType, $ouuid) : null;
+        if ($ouuid && $currentRevision && !$currentRevision->getDeleted()) {
+            throw new DuplicateOuuidException($ouuid, $contentType->getName());
+        }
+
+        $this->em->getConnection()->beginTransaction();
+
+        try {
+            $restoredDraft = $currentRevision ? $this->trashPutBackAsDraft($contentType, $currentRevision->giveOuuid()) : null;
+
+            if ($restoredDraft) {
+                $restoredDraft->setDraft(false);
+                $restoredDraft->setEndTime($revision->getStartTime());
+                $this->unlockRevision($restoredDraft);
+            }
+
+            $this->em->persist($revision);
+            $this->em->flush();
+            $this->em->getConnection()->commit();
+        } catch (\Throwable $e) {
+            $this->em->getConnection()->rollBack();
+            throw $e;
+        }
+
+        return $revision;
+    }
 }