EZP-29899: Content loading can end up loading wrong version under concurrency

eZ/Publish/Core/Persistence/Cache/ContentHandler.php

@@ -23,6 +23,7 @@
 class ContentHandler extends AbstractHandler implements ContentHandlerInterface
 {
     const ALL_TRANSLATIONS_KEY = '0';
+    const PUBLISHED_VERSION = 0;
 
     /**
      * @see \eZ\Publish\SPI\Persistence\Content\Handler::create
@@ -62,10 +63,10 @@ public function copy($contentId, $versionNo = null, $newOwnerId = null)
     /**
      * @see \eZ\Publish\SPI\Persistence\Content\Handler::load
      */
-    public function load($contentId, $version, array $translations = null)
+    public function load($contentId, $version = null, array $translations = null)
     {
         $translationsKey = empty($translations) ? self::ALL_TRANSLATIONS_KEY : implode('|', $translations);
-        $cache = $this->cache->getItem('content', $contentId, $version, $translationsKey);
+        $cache = $this->cache->getItem('content', $contentId, $version ?: self::PUBLISHED_VERSION, $translationsKey);
         $content = $cache->get();
         if ($cache->isMiss()) {
             $this->logger->logCall(__METHOD__, array('content' => $contentId, 'version' => $version, 'translations' => $translations));
@@ -152,6 +153,7 @@ public function setStatus($contentId, $status, $version)
         $return = $this->persistenceHandler->contentHandler()->setStatus($contentId, $status, $version);
 
         $this->cache->clear('content', $contentId, $version);
+        $this->cache->clear('content', $contentId, self::PUBLISHED_VERSION);
         if ($status === VersionInfo::STATUS_PUBLISHED) {
             $this->cache->clear('content', 'info', $contentId);
             $this->cache->clear('content', 'info', 'remoteId');
@@ -172,6 +174,7 @@ public function updateMetadata($contentId, MetadataUpdateStruct $struct)
         $contentInfo = $this->persistenceHandler->contentHandler()->updateMetadata($contentId, $struct);
 
         $this->cache->clear('content', $contentId, $contentInfo->currentVersionNo);
+        $this->cache->clear('content', $contentId, self::PUBLISHED_VERSION);
         $this->cache->clear('content', 'info', $contentId);
 
         if ($struct->remoteId) {
@@ -192,6 +195,7 @@ public function updateContent($contentId, $versionNo, UpdateStruct $struct)
         $this->logger->logCall(__METHOD__, array('content' => $contentId, 'version' => $versionNo, 'struct' => $struct));
         $content = $this->persistenceHandler->contentHandler()->updateContent($contentId, $versionNo, $struct);
         $this->cache->clear('content', $contentId, $versionNo);
+        $this->cache->clear('content', $contentId, self::PUBLISHED_VERSION);
         $this->cache->clear('content', 'info', $contentId, 'versioninfo', $versionNo);
 
         return $content;
@@ -239,6 +243,7 @@ public function deleteVersion($contentId, $versionNo)
         $return = $this->persistenceHandler->contentHandler()->deleteVersion($contentId, $versionNo);
 
         $this->cache->clear('content', $contentId, $versionNo);
+        $this->cache->clear('content', $contentId, self::PUBLISHED_VERSION);
         $this->cache->clear('content', 'info', $contentId);
         $this->cache->clear('content', 'info', 'remoteId');
         $this->cache->clear('location', 'subtree');

eZ/Publish/Core/Persistence/Cache/Tests/ContentHandlerTest.php

@@ -416,6 +416,12 @@ public function testSetStatus()
         $this->cacheMock
             ->expects($this->at(1))
             ->method('clear')
+            ->with('content', 2, ContentHandler::PUBLISHED_VERSION)
+            ->will($this->returnValue(null));
+
+        $this->cacheMock
+            ->expects($this->at(2))
+            ->method('clear')
             ->with('content', 'info', 2, 'versioninfo', 1)
             ->will($this->returnValue(null));
 
@@ -451,6 +457,12 @@ public function testSetStatusPublished()
         $this->cacheMock
             ->expects($this->at(1))
             ->method('clear')
+            ->with('content', 2, ContentHandler::PUBLISHED_VERSION)
+            ->will($this->returnValue(null));
+
+        $this->cacheMock
+            ->expects($this->at(2))
+            ->method('clear')
             ->with('content', 'info', 2)
             ->will($this->returnValue(null));
 
@@ -486,11 +498,17 @@ public function testUpdateMetadata()
         $this->cacheMock
             ->expects($this->at(1))
             ->method('clear')
+            ->with('content', 2, ContentHandler::PUBLISHED_VERSION)
+            ->will($this->returnValue(null));
+
+        $this->cacheMock
+            ->expects($this->at(2))
+            ->method('clear')
             ->with('content', 'info', 2)
             ->willReturn(null);
 
         $this->cacheMock
-            ->expects($this->at(2))
+            ->expects($this->at(3))
             ->method('clear')
             ->with('content', 'info', 'remoteId', 'o34')
             ->willReturn(null);
@@ -541,6 +559,12 @@ public function testUpdateContent()
         $this->cacheMock
             ->expects($this->at(1))
             ->method('clear')
+            ->with('content', 2, ContentHandler::PUBLISHED_VERSION)
+            ->will($this->returnValue(null));
+
+        $this->cacheMock
+            ->expects($this->at(2))
+            ->method('clear')
             ->with('content', 'info', 2, 'versioninfo', 1)
             ->will($this->returnValue(null));
 
@@ -659,18 +683,24 @@ public function testDeleteVersion()
         $this->cacheMock
             ->expects($this->at(1))
             ->method('clear')
-            ->with('content', 'info', 2)
+            ->with('content', 2, ContentHandler::PUBLISHED_VERSION)
             ->will($this->returnValue(null));
 
         $this->cacheMock
             ->expects($this->at(2))
             ->method('clear')
-            ->with('content', 'info', 'remoteId')
+            ->with('content', 'info', 2)
             ->will($this->returnValue(null));
 
         $this->cacheMock
             ->expects($this->at(3))
             ->method('clear')
+            ->with('content', 'info', 'remoteId')
+            ->will($this->returnValue(null));
+
+        $this->cacheMock
+            ->expects($this->at(4))
+            ->method('clear')
             ->with('location', 'subtree')
             ->will($this->returnValue(null));
 

eZ/Publish/Core/Persistence/Legacy/Content/Gateway.php

@@ -144,12 +144,12 @@ abstract public function updateNonTranslatableField(
      * Returns an array with the relevant data.
      *
      * @param mixed $contentId
-     * @param mixed $version
+     * @param int|null $version Current version on null value.
      * @param string[] $translations
      *
      * @return array
      */
-    abstract public function load($contentId, $version, array $translations = null);
+    abstract public function load($contentId, $version = null, array $translations = null);
 
     /**
      * Loads current version for a list of content objects.

eZ/Publish/Core/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php

@@ -818,35 +818,11 @@ public function updateNonTranslatableField(
     }
 
     /**
-     * Loads data for a content object.
-     *
-     * Returns an array with the relevant data.
-     *
-     * @param mixed $contentId
-     * @param mixed $version
-     * @param string[] $translations
-     *
-     * @return array
+     * {@inheritdoc}
      */
-    public function load($contentId, $version, array $translations = null)
+    public function load($contentId, $version = null, array $translations = null)
     {
-        $query = $this->queryBuilder->createFindQuery($translations);
-        $query->where(
-            $query->expr->lAnd(
-                $query->expr->eq(
-                    $this->dbHandler->quoteColumn('id', 'ezcontentobject'),
-                    $query->bindValue($contentId)
-                ),
-                $query->expr->eq(
-                    $this->dbHandler->quoteColumn('version', 'ezcontentobject_version'),
-                    $query->bindValue($version)
-                )
-            )
-        );
-        $statement = $query->prepare();
-        $statement->execute();
-
-        return $statement->fetchAll(\PDO::FETCH_ASSOC);
+        return $this->internalLoadContent([$contentId], $version, $translations);
     }
 
     /**
@@ -861,7 +837,7 @@ public function loadContentList(array $contentIds, array $translations = null)
      * @see loadContentList()
      *
      * @param array $contentIds
-     * @param int $version
+     * @param int|null $version
      * @param string[]|null $translations
      *
      * @return array

eZ/Publish/Core/Persistence/Legacy/Content/Gateway/ExceptionConversion.php

@@ -255,17 +255,9 @@ public function updateNonTranslatableField(
     }
 
     /**
-     * Loads data for a content object.
-     *
-     * Returns an array with the relevant data.
-     *
-     * @param mixed $contentId
-     * @param mixed $version
-     * @param string[] $translations
-     *
-     * @return array
+     * {@inheritdoc}
      */
-    public function load($contentId, $version, array $translations = null)
+    public function load($contentId, $version = null, array $translations = null)
     {
         try {
             return $this->innerGateway->load($contentId, $version, $translations);

eZ/Publish/Core/Persistence/Legacy/Content/Handler.php

@@ -303,23 +303,9 @@ public function createDraftFromVersion($contentId, $srcVersion, $userId)
     }
 
     /**
-     * Returns the raw data of a content object identified by $id, in a struct.
-     *
-     * A version to load must be specified. If you want to load the current
-     * version of a content object use SearchHandler::findSingle() with the
-     * ContentId criterion.
-     *
-     * Optionally a translation filter may be specified. If specified only the
-     * translations with the listed language codes will be retrieved. If not,
-     * all translations will be retrieved.
-     *
-     * @param int|string $id
-     * @param int|string $version
-     * @param string[] $translations
-     *
-     * @return \eZ\Publish\SPI\Persistence\Content Content value object
+     * {@inheritdoc}
      */
-    public function load($id, $version, array $translations = null)
+    public function load($id, $version = null, array $translations = null)
     {
         $rows = $this->contentGateway->load($id, $version, $translations);
 
@@ -329,7 +315,10 @@ public function load($id, $version, array $translations = null)
 
         $contentObjects = $this->mapper->extractContentFromRows(
             $rows,
-            $this->contentGateway->loadVersionedNameData(array(array('id' => $id, 'version' => $version)))
+            $this->contentGateway->loadVersionedNameData([[
+                'id' => $id,
+                'version' => $rows[0]['ezcontentobject_version_version'],
+            ]])
         );
         $content = $contentObjects[0];
 

eZ/Publish/Core/Persistence/Legacy/Tests/Content/ContentHandlerTest.php

@@ -246,15 +246,15 @@ public function testPublishFirstVersion()
                 )
             );
 
+        $contentRows = [['ezcontentobject_version_version' => 1]];
+
         $gatewayMock->expects($this->once())
             ->method('load')
             ->with(
                 $this->equalTo(23),
                 $this->equalTo(1),
                 $this->equalTo(null)
-            )->will(
-                $this->returnValue(array(42))
-            );
+            )->willReturn($contentRows);
 
         $gatewayMock->expects($this->once())
             ->method('loadVersionedNameData')
@@ -266,7 +266,7 @@ public function testPublishFirstVersion()
 
         $mapperMock->expects($this->once())
             ->method('extractContentFromRows')
-            ->with($this->equalTo(array(42)), $this->equalTo(array(22)))
+            ->with($this->equalTo($contentRows), $this->equalTo(array(22)))
             ->will($this->returnValue(array($this->getContentFixtureForDraft())));
 
         $fieldHandlerMock->expects($this->once())
@@ -323,14 +323,16 @@ public function testPublish()
             ->method('setStatus')
             ->with(23, VersionInfo::STATUS_ARCHIVED, 1);
 
+        $contentRows = [['ezcontentobject_version_version' => 2]];
+
         $gatewayMock->expects($this->once())
             ->method('load')
             ->with(
                 $this->equalTo(23),
                 $this->equalTo(2),
                 $this->equalTo(null)
             )
-            ->will($this->returnValue(array(42)));
+            ->willReturn($contentRows);
 
         $gatewayMock->expects($this->once())
             ->method('loadVersionedNameData')
@@ -342,7 +344,7 @@ public function testPublish()
 
         $mapperMock->expects($this->once())
             ->method('extractContentFromRows')
-            ->with($this->equalTo(array(42)), $this->equalTo(array(22)))
+            ->with($this->equalTo($contentRows), $this->equalTo(array(22)))
             ->will($this->returnValue(array($this->getContentFixtureForDraft())));
 
         $fieldHandlerMock->expects($this->once())
@@ -474,14 +476,16 @@ public function testLoad()
         $mapperMock = $this->getMapperMock();
         $fieldHandlerMock = $this->getFieldHandlerMock();
 
+        $contentRows = [['ezcontentobject_version_version' => 2]];
+
         $gatewayMock->expects($this->once())
             ->method('load')
             ->with(
                 $this->equalTo(23),
                 $this->equalTo(2),
                 $this->equalTo(array('eng-GB'))
             )->will(
-                $this->returnValue(array(42))
+                $this->returnValue($contentRows)
             );
 
         $gatewayMock->expects($this->once())
@@ -494,7 +498,7 @@ public function testLoad()
 
         $mapperMock->expects($this->once())
             ->method('extractContentFromRows')
-            ->with($this->equalTo(array(42)), $this->equalTo(array(22)))
+            ->with($this->equalTo($contentRows), $this->equalTo(array(22)))
             ->will($this->returnValue(array($this->getContentFixtureForDraft())));
 
         $fieldHandlerMock->expects($this->once())