Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Dec 20, 2018
Merged
Diff settings

Always

Just for now

@@ -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');
@@ -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));
@@ -144,12 +144,12 @@ abstract class Gateway
* Returns an array with the relevant data.
*
* @param mixed $contentId
* @param mixed $version
* @param mixed|null $version Current version on null value.
This conversation was marked as resolved by andrerom

This comment has been minimized.

Copy link
@alongosz

alongosz Dec 17, 2018

Member

Good opportunity to make it more clear what we expect here (I'm fine with changing whole docblock of this method for consistency):

Suggested change
* @param mixed|null $version Current version on null value.
* @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.
@@ -818,35 +818,16 @@ 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 $this->internalLoadContent([$contentId], $version, $translations);
/*if (empty($list)) {
return [];
}
return $statement->fetchAll(\PDO::FETCH_ASSOC);
return $list[0];*/

This comment has been minimized.

Copy link
@alongosz

alongosz Dec 17, 2018

Member

Dead code detected 😛

This comment has been minimized.

Copy link
@andrerom

andrerom Dec 17, 2018

Author Member

Fixed :)

}
/**
@@ -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);
@@ -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];
@@ -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())
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.