From 88b081353617b36b1b3f5450b62c4a67c39dcd2e Mon Sep 17 00:00:00 2001 From: "Roman S. Borschel" Date: Sun, 4 Jul 2010 18:55:49 +0200 Subject: [PATCH] Improved entity state detection. --- lib/Doctrine/ORM/Mapping/ClassMetadata.php | 6 +- lib/Doctrine/ORM/PersistentCollection.php | 3 +- .../ORM/Persisters/BasicEntityPersister.php | 24 ++- lib/Doctrine/ORM/Query/SqlWalker.php | 1 - lib/Doctrine/ORM/UnitOfWork.php | 196 +++++++++++------- .../Tests/Mocks/EntityPersisterMock.php | 12 ++ .../ORM/Functional/DetachedEntityTest.php | 14 ++ .../ManyToManyBasicAssociationTest.php | 4 +- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 87 ++++---- 9 files changed, 224 insertions(+), 123 deletions(-) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadata.php b/lib/Doctrine/ORM/Mapping/ClassMetadata.php index 540162c76..64be31b5b 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadata.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadata.php @@ -143,7 +143,11 @@ class ClassMetadata extends ClassMetadataInfo } return $id; } else { - return array($this->identifier[0] => $this->reflFields[$this->identifier[0]]->getValue($entity)); + $value = $this->reflFields[$this->identifier[0]]->getValue($entity); + if ($value !== null) { + return array($this->identifier[0] => $value); + } + return array(); } } diff --git a/lib/Doctrine/ORM/PersistentCollection.php b/lib/Doctrine/ORM/PersistentCollection.php index 6eb6d1088..85accfdb0 100644 --- a/lib/Doctrine/ORM/PersistentCollection.php +++ b/lib/Doctrine/ORM/PersistentCollection.php @@ -32,12 +32,11 @@ use Doctrine\ORM\Mapping\AssociationMapping, * Similarly, if you remove entities from a collection that is part of a one-many * mapping this will only result in the nulling out of the foreign keys on flush. * - * @license http://www.opensource.org/licenses/lgpl-license.php LGPL * @since 2.0 - * @version $Revision: 4930 $ * @author Konsta Vesterinen * @author Roman Borschel * @author Giorgio Sironi + * @todo Design for inheritance to allow custom implementations? */ final class PersistentCollection implements Collection { diff --git a/lib/Doctrine/ORM/Persisters/BasicEntityPersister.php b/lib/Doctrine/ORM/Persisters/BasicEntityPersister.php index 33066017f..9526210b0 100644 --- a/lib/Doctrine/ORM/Persisters/BasicEntityPersister.php +++ b/lib/Doctrine/ORM/Persisters/BasicEntityPersister.php @@ -57,9 +57,9 @@ use PDO, * * - {@link load} : Loads (the state of) a single, managed entity. * - {@link loadAll} : Loads multiple, managed entities. - * - {@link loadOneToOneEntity} : Loads a one/many-to-one association (lazy-loading). - * - {@link loadOneToManyCollection} : Loads a one-to-many association (lazy-loading). - * - {@link loadManyToManyCollection} : Loads a many-to-many association (lazy-loading). + * - {@link loadOneToOneEntity} : Loads a one/many-to-one entity association (lazy-loading). + * - {@link loadOneToManyCollection} : Loads a one-to-many entity association (lazy-loading). + * - {@link loadManyToManyCollection} : Loads a many-to-many entity association (lazy-loading). * * The BasicEntityPersister implementation provides the default behavior for * persisting and querying entities that are mapped to a single database table. @@ -712,7 +712,7 @@ class BasicEntityPersister * Creates or fills a single entity object from an SQL result. * * @param $result The SQL result. - * @param object $entity The entity object to fill. + * @param object $entity The entity object to fill, if any. * @param array $hints Hints for entity creation. * @return object The filled and managed entity object or NULL, if the SQL result is empty. */ @@ -1119,6 +1119,22 @@ class BasicEntityPersister $stmt->closeCursor(); } + /** + * Checks whether the given managed entity exists in the database. + * + * @param object $entity + * @return boolean TRUE if the entity exists in the database, FALSE otherwise. + */ + public function exists($entity) + { + $criteria = $this->_class->getIdentifierValues($entity); + $sql = 'SELECT 1 FROM ' . $this->_class->getQuotedTableName($this->_platform) + . ' ' . $this->_getSQLTableAlias($this->_class->name) + . ' WHERE ' . $this->_getSelectConditionSQL($criteria); + + return (bool) $this->_conn->fetchColumn($sql, array_values($criteria)); + } + //TODO /*protected function _getOneToOneEagerFetchSQL() { diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 323599927..89634ecb4 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -497,7 +497,6 @@ class SqlWalker implements TreeWalker $sql .= reset($assoc->targetToSourceKeyColumns); } else { - // 2- Inverse side: NOT (YET?) SUPPORTED throw QueryException::associationPathInverseSideNotSupported(); } break; diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index fb41899e0..526e1effa 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -556,7 +556,7 @@ class UnitOfWork implements PropertyChangedListener return; // "Persistence by reachability" only if persist cascade specified } - // Look through the entities, and in any of their associations, for transient + // Look through the entities, and in any of their associations, for transient (new) // enities, recursively. ("Persistence by reachability") if ($assoc->isOneToOne()) { if ($value instanceof Proxy && ! $value->__isInitialized__) { @@ -569,42 +569,45 @@ class UnitOfWork implements PropertyChangedListener $targetClass = $this->_em->getClassMetadata($assoc->targetEntityName); foreach ($value as $entry) { - $state = $this->getEntityState($entry, self::STATE_NEW); + $state = $this->getEntityState($entry); $oid = spl_object_hash($entry); if ($state == self::STATE_NEW) { - if (isset($targetClass->lifecycleCallbacks[Events::prePersist])) { - $targetClass->invokeLifecycleCallbacks(Events::prePersist, $entry); - } - if ($this->_evm->hasListeners(Events::prePersist)) { - $this->_evm->dispatchEvent(Events::prePersist, new LifecycleEventArgs($entry, $this->_em)); - } - - // Get identifier, if possible (not post-insert) - $idGen = $targetClass->idGenerator; - if ( ! $idGen->isPostInsertGenerator()) { - $idValue = $idGen->generate($this->_em, $entry); - if ( ! $idGen instanceof \Doctrine\ORM\Id\AssignedGenerator) { - $this->_entityIdentifiers[$oid] = array($targetClass->identifier[0] => $idValue); - $targetClass->getSingleIdReflectionProperty()->setValue($entry, $idValue); - } else { - $this->_entityIdentifiers[$oid] = $idValue; - } - $this->addToIdentityMap($entry); - } - $this->_entityStates[$oid] = self::STATE_MANAGED; - - // NEW entities are INSERTed within the current unit of work. - $this->_entityInsertions[$oid] = $entry; - + $this->persistNew($targetClass, $entry); $this->computeChangeSet($targetClass, $entry); - } else if ($state == self::STATE_REMOVED) { - throw ORMException::removedEntityInCollectionDetected($entity, $assoc); + throw ORMException::removedEntityInCollectionDetected($entry, $assoc); + } else if ($state == self::STATE_DETACHED) { + throw new \InvalidArgumentException("Detached entity in association during cascading a persist operation."); } // MANAGED associated entities are already taken into account // during changeset calculation anyway, since they are in the identity map. } } + + private function persistNew($class, $entity) + { + $oid = spl_object_hash($entity); + if (isset($class->lifecycleCallbacks[Events::prePersist])) { + $class->invokeLifecycleCallbacks(Events::prePersist, $entity); + } + if ($this->_evm->hasListeners(Events::prePersist)) { + $this->_evm->dispatchEvent(Events::prePersist, new LifecycleEventArgs($entity, $this->_em)); + } + + $idGen = $class->idGenerator; + if ( ! $idGen->isPostInsertGenerator()) { + $idValue = $idGen->generate($this->_em, $entity); + if ( ! $idGen instanceof \Doctrine\ORM\Id\AssignedGenerator) { + $this->_entityIdentifiers[$oid] = array($class->identifier[0] => $idValue); + $class->setIdentifierValues($entity, $idValue); + } else { + $this->_entityIdentifiers[$oid] = $idValue; + } + } + $this->_entityStates[$oid] = self::STATE_MANAGED; + + $this->scheduleForInsert($entity); + } /** * INTERNAL: @@ -1031,35 +1034,54 @@ class UnitOfWork implements PropertyChangedListener } /** - * Gets the state of an entity within the current unit of work. - * - * NOTE: This method sees entities that are not MANAGED or REMOVED and have a - * populated identifier, whether it is generated or manually assigned, as - * DETACHED. This can be incorrect for manually assigned identifiers. + * Gets the state of an entity with regard to the current unit of work. * * @param object $entity - * @param integer $assume The state to assume if the state is not yet known. This is usually - * used to avoid costly state lookups, in the worst case with a database - * lookup. + * @param integer $assume The state to assume if the state is not yet known (not MANAGED or REMOVED). + * This parameter can be set to improve performance of entity state detection + * by potentially avoiding a database lookup if the distinction between NEW and DETACHED + * is either known or does not matter for the caller of the method. * @return int The entity state. */ public function getEntityState($entity, $assume = null) { $oid = spl_object_hash($entity); if ( ! isset($this->_entityStates[$oid])) { - // State can only be NEW or DETACHED, because MANAGED/REMOVED states are immediately - // set by the UnitOfWork directly. We treat all entities that have a populated - // identifier as DETACHED and all others as NEW. This is not really correct for - // manually assigned identifiers but in that case we would need to hit the database - // and we would like to avoid that. + // State can only be NEW or DETACHED, because MANAGED/REMOVED states are known. + // Note that you can not remember the NEW or DETACHED state in _entityStates since + // the UoW does not hold references to such objects and the object hash can be reused. + // More generally because the state may "change" between NEW/DETACHED without the UoW being aware of it. if ($assume === null) { - if ($this->_em->getClassMetadata(get_class($entity))->getIdentifierValues($entity)) { - $this->_entityStates[$oid] = self::STATE_DETACHED; + $class = $this->_em->getClassMetadata(get_class($entity)); + $id = $class->getIdentifierValues($entity); + if ( ! $id) { + return self::STATE_NEW; + } else if ($class->isIdentifierNatural()) { + // Check for a version field, if available, to avoid a db lookup. + if ($class->isVersioned) { + if ($class->getFieldValue($entity, $class->versionField)) { + return self::STATE_DETACHED; + } else { + return self::STATE_NEW; + } + } else { + // Last try before db lookup: check the identity map. + if ($this->tryGetById($id, $class->rootEntityName)) { + return self::STATE_DETACHED; + } else { + // db lookup + if ($this->getEntityPersister(get_class($entity))->exists($entity)) { + return self::STATE_DETACHED; + } else { + return self::STATE_NEW; + } + } + } } else { - $this->_entityStates[$oid] = self::STATE_NEW; + return self::STATE_DETACHED; } } else { - $this->_entityStates[$oid] = $assume; + return $assume; } } return $this->_entityStates[$oid]; @@ -1169,12 +1191,10 @@ class UnitOfWork implements PropertyChangedListener } /** - * Saves an entity as part of the current unit of work. - * This method is internally called during save() cascades as it tracks - * the already visited entities to prevent infinite recursions. + * Persists an entity as part of the current unit of work. * - * NOTE: This method always considers entities that are not yet known to - * this UnitOfWork as NEW. + * This method is internally called during persist() cascades as it tracks + * the already visited entities to prevent infinite recursions. * * @param object $entity The entity to persist. * @param array $visited The already visited entities. @@ -1189,8 +1209,8 @@ class UnitOfWork implements PropertyChangedListener $visited[$oid] = $entity; // Mark visited $class = $this->_em->getClassMetadata(get_class($entity)); - $entityState = $this->getEntityState($entity, self::STATE_NEW); - + $entityState = $this->getEntityState($entity); + switch ($entityState) { case self::STATE_MANAGED: // Nothing to do, except if policy is "deferred explicit" @@ -1199,30 +1219,10 @@ class UnitOfWork implements PropertyChangedListener } break; case self::STATE_NEW: - if (isset($class->lifecycleCallbacks[Events::prePersist])) { - $class->invokeLifecycleCallbacks(Events::prePersist, $entity); - } - if ($this->_evm->hasListeners(Events::prePersist)) { - $this->_evm->dispatchEvent(Events::prePersist, new LifecycleEventArgs($entity, $this->_em)); - } - - $idGen = $class->idGenerator; - if ( ! $idGen->isPostInsertGenerator()) { - $idValue = $idGen->generate($this->_em, $entity); - if ( ! $idGen instanceof \Doctrine\ORM\Id\AssignedGenerator) { - $this->_entityIdentifiers[$oid] = array($class->identifier[0] => $idValue); - $class->setIdentifierValues($entity, $idValue); - } else { - $this->_entityIdentifiers[$oid] = $idValue; - } - } - $this->_entityStates[$oid] = self::STATE_MANAGED; - - $this->scheduleForInsert($entity); + $this->persistNew($class, $entity); break; case self::STATE_DETACHED: - throw new \InvalidArgumentException( - "Behavior of persist() for a detached entity is not yet defined."); + throw new \InvalidArgumentException("Detached entity passed to persist()."); case self::STATE_REMOVED: // Entity becomes managed again if ($this->isScheduledForDelete($entity)) { @@ -1235,7 +1235,7 @@ class UnitOfWork implements PropertyChangedListener default: throw ORMException::invalidEntityState($entityState); } - + $this->_cascadePersist($entity, $visited); } @@ -1440,7 +1440,6 @@ class UnitOfWork implements PropertyChangedListener * * @param object $entity * @param array $visited - * @internal This method always considers entities with an assigned identifier as DETACHED. */ private function _doDetach($entity, array &$visited) { @@ -1657,7 +1656,7 @@ class UnitOfWork implements PropertyChangedListener */ public function lock($entity, $lockMode, $lockVersion = null) { - if ($this->getEntityState($entity) != self::STATE_MANAGED) { + if ($this->getEntityState($entity, self::STATE_NEW) != self::STATE_MANAGED) { throw new \InvalidArgumentException("Entity is not MANAGED."); } @@ -2183,4 +2182,47 @@ class UnitOfWork implements PropertyChangedListener { return $this->_collectionUpdates; } + + /** + * Specifically tells this UnitOfWork that the given entity should be treated + * as NEW. This can be useful in the case of manually assigned identifiers to + * avoid a database lookup that is used to tell NEW and DETACHED entities apart. + * Use setNew($entity) prior to persist($entity) to avoid the database lookup. + * + * Note that the UnitOfWork uses the spl_object_hash() of the object to associate the + * state with the object. Thus the UnitOfWork does not prevent the object from being + * garbage collected which can result in the object hash being reused for other objects. + * + * @param object $entity The entity to mark as NEW. + * @throws InvalidArgumentException If the entity is already known to this UnitOfWork. + */ + public function setNew($entity) + { + $oid = spl_object_hash($entity); + if (isset($this->_entityStates[$oid])) { + throw new \InvalidArgumentException("The passed entity must not be already known to this UnitOfWork."); + } + $this->_entityStates[$oid] = self::STATE_NEW; + } + + /** + * Specifically tells this UnitOfWork that the given entity should be treated + * as DETACHED. This can be useful in the case of manually assigned identifiers to + * avoid a database lookup that is used to tell NEW and DETACHED entities apart. + * + * Note that the UnitOfWork uses the spl_object_hash() of the object to associate the + * state with the object. Thus the UnitOfWork does not prevent the object from being + * garbage collected which can result in the object hash being reused for other objects. + * + * @param object $entity The entity to mark as DETACHED. + * @throws InvalidArgumentException If the entity is already known to this UnitOfWork. + */ + public function setDetached($entity) + { + $oid = spl_object_hash($entity); + if (isset($this->_entityStates[$oid])) { + throw new \InvalidArgumentException("The passed entity must not be already known to this UnitOfWork."); + } + $this->_entityStates[$oid] = self::STATE_DETACHED; + } } diff --git a/tests/Doctrine/Tests/Mocks/EntityPersisterMock.php b/tests/Doctrine/Tests/Mocks/EntityPersisterMock.php index aa43081a8..61d4d855e 100644 --- a/tests/Doctrine/Tests/Mocks/EntityPersisterMock.php +++ b/tests/Doctrine/Tests/Mocks/EntityPersisterMock.php @@ -13,6 +13,7 @@ class EntityPersisterMock extends \Doctrine\ORM\Persisters\BasicEntityPersister private $_identityColumnValueCounter = 0; private $_mockIdGeneratorType; private $_postInsertIds = array(); + private $existsCalled = false; /** * @param $entity @@ -57,6 +58,11 @@ class EntityPersisterMock extends \Doctrine\ORM\Persisters\BasicEntityPersister { $this->_updates[] = $entity; } + + public function exists($entity) + { + $this->existsCalled = true; + } public function delete($entity) { @@ -80,9 +86,15 @@ class EntityPersisterMock extends \Doctrine\ORM\Persisters\BasicEntityPersister public function reset() { + $this->existsCalled = false; $this->_identityColumnValueCounter = 0; $this->_inserts = array(); $this->_updates = array(); $this->_deletes = array(); } + + public function isExistsCalled() + { + return $this->existsCalled; + } } \ No newline at end of file diff --git a/tests/Doctrine/Tests/ORM/Functional/DetachedEntityTest.php b/tests/Doctrine/Tests/ORM/Functional/DetachedEntityTest.php index ddb3c7fec..1ad60ad4a 100644 --- a/tests/Doctrine/Tests/ORM/Functional/DetachedEntityTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/DetachedEntityTest.php @@ -86,6 +86,20 @@ class DetachedEntityTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->assertTrue($this->_em->contains($phonenumbers[1])); } + /** + * @expectedException InvalidArgumentException + * @group DDC-203 + */ + public function testDetachedEntityWithAssignedIdentityThrowsExceptionOnPersist() + { + $ph = new CmsPhonenumber(); + $ph->phonenumber = '12345'; + $this->_em->persist($ph); + $this->_em->flush(); + $this->_em->clear(); + $this->_em->persist($ph); + } + /** * @group DDC-518 */ diff --git a/tests/Doctrine/Tests/ORM/Functional/ManyToManyBasicAssociationTest.php b/tests/Doctrine/Tests/ORM/Functional/ManyToManyBasicAssociationTest.php index a12862227..752d11fb8 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ManyToManyBasicAssociationTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/ManyToManyBasicAssociationTest.php @@ -185,7 +185,9 @@ class ManyToManyBasicAssociationTest extends \Doctrine\Tests\OrmFunctionalTestCa /* @var $freshUser CmsUser */ $freshUser = $this->_em->find('Doctrine\Tests\Models\CMS\CmsUser', $user->getId()); - $freshUser->addGroup($group); + $newGroup = new CmsGroup(); + $newGroup->setName('12Monkeys'); + $freshUser->addGroup($newGroup); $this->assertFalse($freshUser->groups->isInitialized(), "CmsUser::groups Collection has to be uninitialized for this test."); diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index 12b9c1ea0..f816add25 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -2,6 +2,7 @@ namespace Doctrine\Tests\ORM; +use Doctrine\ORM\UnitOfWork; use Doctrine\Tests\Mocks\ConnectionMock; use Doctrine\Tests\Mocks\EntityManagerMock; use Doctrine\Tests\Mocks\UnitOfWorkMock; @@ -147,47 +148,46 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase $this->assertEquals(array('data' => array('thedata', 'newdata')), $this->_unitOfWork->getEntityChangeSet($entity)); } - /* - public function testSavingSingleEntityWithSequenceIdGeneratorSchedulesInsert() + public function testGetEntityStateOnVersionedEntityWithAssignedIdentifier() { - //... + $persister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\ORM\VersionedAssignedIdentifierEntity")); + $this->_unitOfWork->setEntityPersister('Doctrine\Tests\ORM\VersionedAssignedIdentifierEntity', $persister); + + $e = new VersionedAssignedIdentifierEntity(); + $e->id = 42; + $this->assertEquals(UnitOfWork::STATE_NEW, $this->_unitOfWork->getEntityState($e)); + $this->assertFalse($persister->isExistsCalled()); } - - public function testSavingSingleEntityWithTableIdGeneratorSchedulesInsert() + + public function testGetEntityStateWithAssignedIdentity() { - //... + $persister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\Models\CMS\CmsPhonenumber")); + $this->_unitOfWork->setEntityPersister('Doctrine\Tests\Models\CMS\CmsPhonenumber', $persister); + + $ph = new \Doctrine\Tests\Models\CMS\CmsPhonenumber(); + $ph->phonenumber = '12345'; + + $this->assertEquals(UnitOfWork::STATE_NEW, $this->_unitOfWork->getEntityState($ph)); + $this->assertTrue($persister->isExistsCalled()); + + $persister->reset(); + + // setNew should avoid exists() check + $this->_unitOfWork->setNew($ph); + $this->assertEquals(UnitOfWork::STATE_NEW, $this->_unitOfWork->getEntityState($ph)); + $this->assertFalse($persister->isExistsCalled()); + + $persister->reset(); + + // if the entity is already managed the exists() check should also be skipped + $this->_unitOfWork->registerManaged($ph, array('phonenumber' => '12345'), array()); + $this->assertEquals(UnitOfWork::STATE_MANAGED, $this->_unitOfWork->getEntityState($ph)); + $this->assertFalse($persister->isExistsCalled()); + $ph2 = new \Doctrine\Tests\Models\CMS\CmsPhonenumber(); + $ph2->phonenumber = '12345'; + $this->assertEquals(UnitOfWork::STATE_DETACHED, $this->_unitOfWork->getEntityState($ph2)); + $this->assertFalse($persister->isExistsCalled()); } - - public function testSavingSingleEntityWithSingleNaturalIdForcesInsert() - { - //... - } - - public function testSavingSingleEntityWithCompositeIdForcesInsert() - { - //... - } - - public function testSavingEntityGraphWithIdentityColumnsForcesInserts() - { - //... - } - - public function testSavingEntityGraphWithSequencesDelaysInserts() - { - //... - } - - public function testSavingEntityGraphWithNaturalIdsForcesInserts() - { - //... - } - - public function testSavingEntityGraphWithMixedIdGenerationStrategies() - { - //... - } - */ } /** @@ -243,4 +243,17 @@ class NotifyChangedEntity implements \Doctrine\Common\NotifyPropertyChanged } } } +} + +/** @Entity */ +class VersionedAssignedIdentifierEntity +{ + /** + * @Id @Column(type="integer") + */ + public $id; + /** + * @Version @Column(type="integer") + */ + public $version; } \ No newline at end of file