From 59fddbaa9e9bfbe8bb946c7179f18bf4d43b0cee Mon Sep 17 00:00:00 2001 From: Diego Rin Date: Wed, 24 Feb 2021 22:32:19 +0100 Subject: [PATCH] Changed lock function to compare timestamps instead of DateTimeInterface objects directly. When using optimistic lock with DateTimeInterface based version field a bug appears due to the use of the === operator for comparing the lock version and the entity version. This comparison always resolves to false because the === operator when comparing objects is only true when both sides are the exact same instance of the object. To fix the issue I have decided to compare timestamps instead the DateTimeInterface based objects directly, calling getTimestamp() method and doing a strict comparison. Modified OptimisticLockException to use DateTimeInterface instead of DateTime class. Fixes #8499 --- lib/Doctrine/ORM/OptimisticLockException.php | 6 +- lib/Doctrine/ORM/UnitOfWork.php | 7 +- .../ORM/Functional/Ticket/GH8499Test.php | 176 ++++++++++++++++++ 3 files changed, 185 insertions(+), 4 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/GH8499Test.php diff --git a/lib/Doctrine/ORM/OptimisticLockException.php b/lib/Doctrine/ORM/OptimisticLockException.php index cd00ef4d34e..a5d47bfefe5 100644 --- a/lib/Doctrine/ORM/OptimisticLockException.php +++ b/lib/Doctrine/ORM/OptimisticLockException.php @@ -20,7 +20,7 @@ namespace Doctrine\ORM; -use DateTime; +use DateTimeInterface; /** * An OptimisticLockException is thrown when a version check on an object @@ -70,8 +70,8 @@ public static function lockFailed($entity) */ public static function lockFailedVersionMismatch($entity, $expectedLockVersion, $actualLockVersion) { - $expectedLockVersion = $expectedLockVersion instanceof DateTime ? $expectedLockVersion->getTimestamp() : $expectedLockVersion; - $actualLockVersion = $actualLockVersion instanceof DateTime ? $actualLockVersion->getTimestamp() : $actualLockVersion; + $expectedLockVersion = $expectedLockVersion instanceof DateTimeInterface ? $expectedLockVersion->getTimestamp() : $expectedLockVersion; + $actualLockVersion = $actualLockVersion instanceof DateTimeInterface ? $actualLockVersion->getTimestamp() : $actualLockVersion; return new self('The optimistic lock failed, version ' . $expectedLockVersion . ' was expected, but is actually ' . $actualLockVersion, $entity); } diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 7151cab8356..5bd9bc96a49 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -20,6 +20,7 @@ namespace Doctrine\ORM; +use DateTimeInterface; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; use Doctrine\Common\EventManager; @@ -2494,7 +2495,11 @@ public function lock($entity, $lockMode, $lockVersion = null) $entityVersion = $class->reflFields[$class->versionField]->getValue($entity); - if ($entityVersion !== $lockVersion) { + if ($entityVersion instanceof DateTimeInterface && $lockVersion instanceof DateTimeInterface) { + if ($entityVersion->getTimestamp() !== $lockVersion->getTimestamp()) { + throw OptimisticLockException::lockFailedVersionMismatch($entity, $lockVersion, $entityVersion); + } + } elseif ($entityVersion !== $lockVersion) { throw OptimisticLockException::lockFailedVersionMismatch($entity, $lockVersion, $entityVersion); } diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH8499Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH8499Test.php new file mode 100644 index 00000000000..dec6c96714f --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH8499Test.php @@ -0,0 +1,176 @@ +setUpEntitySchema( + [GH8499VersionableEntity::class] + ); + } catch (SchemaException $e) { + } + } + + /** + * @group GH-8499 + */ + public function testOptimisticLockInitializesVersionOnEntityInsert(): void + { + $entity = new GH8499VersionableEntity(); + $entity->setName('Test Entity'); + $entity->setDescription('Entity to test optimistic lock fix with DateTimeInterface objects'); + $this->_em->persist($entity); + $this->_em->flush(); + $this->_em->refresh($entity); + + $this->assertInstanceOf(DateTimeInterface::class, $entity->getRevision(), 'Version field not set to DateTimeInterface'); + } + + /** + * @group GH-8499 + */ + public function testOptimisticLockUpdatesVersionOnEntityUpdate(): void + { + $entity = new GH8499VersionableEntity(); + $entity->setName('Test Entity'); + $entity->setDescription('Entity to test optimistic lock fix with DateTimeInterface objects'); + $this->_em->persist($entity); + $this->_em->flush(); + $this->_em->refresh($entity); + $firstVersion = $entity->getRevision(); + sleep(1); + + $entity->setName('Test Entity Updated'); + $this->_em->persist($entity); + $this->_em->flush(); + $this->_em->refresh($entity); + $lastVersion = $entity->getRevision(); + + $this->assertNotEquals($firstVersion, $lastVersion, 'Version field value not updated on persist'); + } + + /** + * @group GH-8499 + */ + public function testOptimisticLockWithDateTimeForVersion(): void + { + $entity = new GH8499VersionableEntity(); + $entity->setName('Test Entity'); + $entity->setDescription('Entity to test optimistic lock fix with DateTimeInterface objects'); + $this->_em->persist($entity); + $this->_em->flush(); + + $firstVersion = $entity->getRevision(); + + $this->_em->lock($entity, LockMode::OPTIMISTIC, $firstVersion); + sleep(1); + + $entity->setName('Test Entity Locked'); + $this->_em->persist($entity); + $this->_em->flush(); + + $this->assertNotEquals($firstVersion, $entity->getRevision(), 'Version field value not updated on persist'); + } + + /** + * @group GH-8499 + */ + public function testOptimisticLockWithDateTimeForVersionThrowsException(): void + { + $this->expectException(OptimisticLockException::class); + + $entity = new GH8499VersionableEntity(); + $entity->setName('Test Entity'); + $entity->setDescription('Entity to test optimistic lock fix with DateTimeInterface objects'); + $this->_em->persist($entity); + $this->_em->flush(); + + $this->_em->lock($entity, LockMode::OPTIMISTIC, new DateTime('2020-07-15 18:04:00')); + } +} + +/** + * @Entity + * @Table + */ +class GH8499VersionableEntity +{ + /** + * @Id + * @Column(type="integer") + * @GeneratedValue + * @var int + */ + public $id; + + /** + * @Column(type="string") + * @var string + */ + public $name; + + /** + * @Column(type="string") + * @var string + */ + public $description; + + /** + * @Version + * @Column(type="datetime") + * @var DateTimeInterface + */ + public $revision; + + public function getId(): int + { + return $this->id; + } + + public function getName(): string + { + return $this->name; + } + + public function setName(string $name): void + { + $this->name = $name; + } + + public function getDescription(): string + { + return $this->description; + } + + public function setDescription(string $description): void + { + $this->description = $description; + } + + public function getRevision(): DateTimeInterface + { + return $this->revision; + } +}