diff --git a/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php b/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php index 0665df3ce..e1051c5ff 100644 --- a/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php +++ b/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php @@ -31,6 +31,7 @@ use Doctrine\ORM\PersistentCollection, * * @author Roman Borschel * @since 2.0 + * @internal Highly performance-sensitive code. */ class ObjectHydrator extends AbstractHydrator { @@ -49,7 +50,10 @@ class ObjectHydrator extends AbstractHydrator private $_fetchedAssociations; private $_rootAliases = array(); private $_initializedCollections = array(); + private $_existingCollections = array(); private $_proxyFactory; + //private $_createdEntities; + /** @override */ protected function _prepare() @@ -57,9 +61,7 @@ class ObjectHydrator extends AbstractHydrator $this->_allowPartialObjects = $this->_em->getConfiguration()->getAllowPartialObjects() || isset($this->_hints[Query::HINT_FORCE_PARTIAL_LOAD]); - if ( ! $this->_allowPartialObjects) { - $this->_proxyFactory = $this->_em->getProxyFactory(); - } + $this->_proxyFactory = $this->_em->getProxyFactory(); $this->_identifierMap = $this->_resultPointers = @@ -81,7 +83,7 @@ class ObjectHydrator extends AbstractHydrator // collection stubs or proxies and where not. if (isset($this->_rsm->relationMap[$dqlAlias])) { $targetClassName = $this->_rsm->aliasMap[$this->_rsm->parentAliasMap[$dqlAlias]]; - $targetClass = $this->_em->getClassMetadata($targetClassName); + $targetClass = $this->_getClassMetadata($targetClassName); $this->_ce[$targetClassName] = $targetClass; $assoc = $targetClass->associationMappings[$this->_rsm->relationMap[$dqlAlias]]; $this->_fetchedAssociations[$assoc->sourceEntityName][$assoc->sourceFieldName] = true; @@ -99,22 +101,20 @@ class ObjectHydrator extends AbstractHydrator /** * {@inheritdoc} - * - * @override */ - protected function _cleanup() + /*@override*/ protected function _cleanup() { parent::_cleanup(); $this->_identifierMap = + $this->_initializedCollections = + $this->_existingCollections = $this->_resultPointers = array(); } /** * {@inheritdoc} - * - * @override */ - protected function _hydrateAll() + /*@override*/ protected function _hydrateAll() { $result = array(); $cache = array(); @@ -122,11 +122,10 @@ class ObjectHydrator extends AbstractHydrator $this->_hydrateRow($data, $cache, $result); } - // Take snapshots from all initialized collections + // Take snapshots from all newly initialized collections foreach ($this->_initializedCollections as $coll) { $coll->takeSnapshot(); } - $this->_initializedCollections = array(); return $result; } @@ -155,17 +154,20 @@ class ObjectHydrator extends AbstractHydrator $value ); $value->setOwner($entity, $relation); - } else { - // Is already PersistentCollection. + $class->reflFields[$name]->setValue($entity, $value); + $this->_uow->setOriginalEntityProperty($oid, $name, $value); + $this->_initializedCollections[$oid . $name] = $value; + } else if (isset($this->_hints[Query::HINT_REFRESH])) { + // Is already PersistentCollection, but REFRESH $value->clear(); $value->setDirty(false); $value->setInitialized(true); + $this->_initializedCollections[$oid . $name] = $value; + } else { + // Is already PersistentCollection, and DONT REFRESH + $this->_existingCollections[$oid . $name] = $value; } - $class->reflFields[$name]->setValue($entity, $value); - $this->_uow->setOriginalEntityProperty($oid, $name, $value); - $this->_initializedCollections[$oid . $name] = $value; - return $value; } @@ -187,6 +189,7 @@ class ObjectHydrator extends AbstractHydrator $entity = $this->_uow->createEntity($className, $data, $this->_hints); + //FIXME: If $entity comes from the identity map there is no need to do this! // Properly initialize any unfetched associations, if partial objects are not allowed. if ( ! $this->_allowPartialObjects) { $oid = spl_object_hash($entity); @@ -232,6 +235,23 @@ class ObjectHydrator extends AbstractHydrator return $entity; } + private function _getEntityFromIdentityMap($className, array $data) + { + $class = $this->_ce[$className]; + + if ($class->isIdentifierComposite) { + $idHash = ''; + foreach ($class->identifier as $fieldName) { + $idHash .= $data[$fieldName] . ' '; + } + $idHash = rtrim($idHash); + } else { + $idHash = $data[$class->identifier[0]]; + } + + return $this->_uow->tryGetByIdHash($idHash, $class->rootEntityName); + } + /** * Gets a ClassMetadata instance from the local cache. * If the instance is not yet in the local cache, it is loaded into the @@ -250,10 +270,8 @@ class ObjectHydrator extends AbstractHydrator /** * {@inheritdoc} - * - * @override */ - protected function _hydrateRow(array &$data, array &$cache, array &$result) + /*@override*/ protected function _hydrateRow(array &$data, array &$cache, array &$result) { // Initialize $id = $this->_idTemplate; // initialize the id-memory @@ -266,7 +284,7 @@ class ObjectHydrator extends AbstractHydrator unset($rowData['scalars']); } - // Hydrate the entity data found in the current row. + // Hydrate the data found in the current row. foreach ($rowData as $dqlAlias => $data) { $index = false; $entityName = $this->_rsm->aliasMap[$dqlAlias]; @@ -274,31 +292,34 @@ class ObjectHydrator extends AbstractHydrator if (isset($this->_rsm->parentAliasMap[$dqlAlias])) { // It's a joined result - $parent = $this->_rsm->parentAliasMap[$dqlAlias]; + $parentAlias = $this->_rsm->parentAliasMap[$dqlAlias]; - // Get a reference to the right object to which the joined result belongs. - if ($this->_rsm->isMixed && isset($this->_rootAliases[$parent])) { + // Get a reference to the parent object to which the joined element belongs. + if ($this->_rsm->isMixed && isset($this->_rootAliases[$parentAlias])) { $first = reset($this->_resultPointers); - $baseElement = $this->_resultPointers[$parent][key($first)]; - } else if (isset($this->_resultPointers[$parent])) { - $baseElement = $this->_resultPointers[$parent]; + $parentObject = $this->_resultPointers[$parentAlias][key($first)]; + } else if (isset($this->_resultPointers[$parentAlias])) { + $parentObject = $this->_resultPointers[$parentAlias]; } else { - throw HydrationException::parentObjectOfRelationNotFound($dqlAlias, $parent); + // Parent object of relation not found, so skip it. + continue; } - $parentClass = get_class($baseElement); - $oid = spl_object_hash($baseElement); + $parentClass = get_class($parentObject); + $oid = spl_object_hash($parentObject); $relationField = $this->_rsm->relationMap[$dqlAlias]; $relation = $this->_ce[$parentClass]->associationMappings[$relationField]; $reflField = $this->_ce[$parentClass]->reflFields[$relationField]; - $reflFieldValue = $reflField->getValue($baseElement); // Check the type of the relation (many or single-valued) if ( ! $relation->isOneToOne()) { // Collection-valued association if (isset($nonemptyComponents[$dqlAlias])) { - if ( ! isset($this->_initializedCollections[$oid . $relationField])) { - $reflFieldValue = $this->_initRelatedCollection($baseElement, $relationField); + $collKey = $oid . $relationField; + if (isset($this->_initializedCollections[$collKey])) { + $reflFieldValue = $this->_initializedCollections[$collKey]; + } else if ( ! isset($this->_existingCollections[$collKey])) { + $reflFieldValue = $this->_initRelatedCollection($parentObject, $relationField); } $indexExists = isset($this->_identifierMap[$dqlAlias][$id[$dqlAlias]]); @@ -306,66 +327,78 @@ class ObjectHydrator extends AbstractHydrator $indexIsValid = $index !== false ? isset($reflFieldValue[$index]) : false; if ( ! $indexExists || ! $indexIsValid) { - $element = $this->_getEntity($data, $dqlAlias); - - // If it's a bi-directional many-to-many, also initialize the reverse collection. - if ($relation->isManyToMany()) { - if ($relation->isOwningSide && isset($this->_ce[$entityName]->inverseMappings[$relation->sourceEntityName][$relationField])) { - $inverseFieldName = $this->_ce[$entityName]->inverseMappings[$relation->sourceEntityName][$relationField]->sourceFieldName; - // Only initialize reverse collection if it is not yet initialized. - if ( ! isset($this->_initializedCollections[spl_object_hash($element) . $inverseFieldName])) { - $this->_initRelatedCollection($element, $inverseFieldName); - } - } else if ($relation->mappedByFieldName) { - // Only initialize reverse collection if it is not yet initialized. - if ( ! isset($this->_initializedCollections[spl_object_hash($element) . $relation->mappedByFieldName])) { - $this->_initRelatedCollection($element, $relation->mappedByFieldName); + if (isset($this->_existingCollections[$collKey])) { + // Collection exists, only look for $element in identity map. + if ($element = $this->_getEntityFromIdentityMap($entityName, $data)) { + $this->_resultPointers[$dqlAlias] = $element; + } else { + unset($this->_resultPointers[$dqlAlias]); + } + } else { + $element = $this->_getEntity($data, $dqlAlias); + + // If it's a bi-directional many-to-many, also initialize the reverse collection. + if ($relation->isManyToMany()) { + if ($relation->isOwningSide && isset($this->_ce[$entityName]->inverseMappings[$relation->sourceEntityName][$relationField])) { + $inverseFieldName = $this->_ce[$entityName]->inverseMappings[$relation->sourceEntityName][$relationField]->sourceFieldName; + // Only initialize reverse collection if it is not yet initialized. + if ( ! isset($this->_initializedCollections[spl_object_hash($element) . $inverseFieldName])) { + $this->_initRelatedCollection($element, $inverseFieldName); + } + } else if ($relation->mappedByFieldName) { + // Only initialize reverse collection if it is not yet initialized. + if ( ! isset($this->_initializedCollections[spl_object_hash($element) . $relation->mappedByFieldName])) { + $this->_initRelatedCollection($element, $relation->mappedByFieldName); + } } } + + if (isset($this->_rsm->indexByMap[$dqlAlias])) { + $field = $this->_rsm->indexByMap[$dqlAlias]; + $indexValue = $this->_ce[$entityName]->reflFields[$field]->getValue($element); + $reflFieldValue->hydrateSet($indexValue, $element); + $this->_identifierMap[$dqlAlias][$id[$dqlAlias]] = $indexValue; + } else { + $reflFieldValue->hydrateAdd($element); + $reflFieldValue->last(); + $this->_identifierMap[$dqlAlias][$id[$dqlAlias]] = $reflFieldValue->key(); + } + // Update result pointer + $this->_resultPointers[$dqlAlias] = $element; } - - if (isset($this->_rsm->indexByMap[$dqlAlias])) { - $field = $this->_rsm->indexByMap[$dqlAlias]; - $indexValue = $this->_ce[$entityName]->reflFields[$field]->getValue($element); - $reflFieldValue->hydrateSet($indexValue, $element); - $this->_identifierMap[$dqlAlias][$id[$dqlAlias]] = $indexValue; - } else { - $reflFieldValue->hydrateAdd($element); - $reflFieldValue->last(); - $this->_identifierMap[$dqlAlias][$id[$dqlAlias]] = $reflFieldValue->key(); - } + } else { + // Update result pointer + $this->_resultPointers[$dqlAlias] = $reflFieldValue[$index]; } - - // Update result pointer - $this->_resultPointers[$dqlAlias] = $index === false ? $element : $reflFieldValue[$index]; - - } else if ( ! $reflFieldValue) { + } else if ( ! $reflField->getValue($parentObject)) { $coll = new PersistentCollection($this->_em, $this->_ce[$entityName], new ArrayCollection); - $reflField->setValue($baseElement, $coll); + $reflField->setValue($parentObject, $coll); $this->_uow->setOriginalEntityProperty($oid, $relationField, $coll); } } else { // Single-valued association - $reflFieldValue = $reflField->getValue($baseElement); - if ( ! $reflFieldValue /* || doctrine.refresh hint set */) { + $reflFieldValue = $reflField->getValue($parentObject); + if ( ! $reflFieldValue || isset($this->_hints[Query::HINT_REFRESH])) { if (isset($nonemptyComponents[$dqlAlias])) { $element = $this->_getEntity($data, $dqlAlias); - $reflField->setValue($baseElement, $element); + $reflField->setValue($parentObject, $element); $this->_uow->setOriginalEntityProperty($oid, $relationField, $element); $targetClass = $this->_ce[$relation->targetEntityName]; if ($relation->isOwningSide) { // If there is an inverse mapping on the target class its bidirectional if (isset($targetClass->inverseMappings[$relation->sourceEntityName][$relationField])) { $sourceProp = $targetClass->inverseMappings[$relation->sourceEntityName][$relationField]->sourceFieldName; - $targetClass->reflFields[$sourceProp]->setValue($element, $baseElement); + $targetClass->reflFields[$sourceProp]->setValue($element, $parentObject); } else if ($this->_ce[$parentClass] === $targetClass && $relation->mappedByFieldName) { // Special case: bi-directional self-referencing one-one on the same class - $targetClass->reflFields[$relationField]->setValue($element, $baseElement); + $targetClass->reflFields[$relationField]->setValue($element, $parentObject); } } else { // For sure bidirectional, as there is no inverse side in unidirectional mappings - $targetClass->reflFields[$relation->mappedByFieldName]->setValue($element, $baseElement); + $targetClass->reflFields[$relation->mappedByFieldName]->setValue($element, $parentObject); } + // Update result pointer + $this->_resultPointers[$dqlAlias] = $element; } // else leave $reflFieldValue null for single-valued associations } else { diff --git a/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php b/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php index bd1b484cf..6b0820080 100644 --- a/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php +++ b/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php @@ -273,7 +273,7 @@ class AnnotationDriver implements Driver $mapping['mappedBy'] = $manyToManyAnnot->mappedBy; $mapping['cascade'] = $manyToManyAnnot->cascade; $metadata->mapManyToMany($mapping); - } + } } // Evaluate HasLifecycleCallbacks annotation diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index eaa9d5028..803ac488d 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1080,10 +1080,8 @@ class UnitOfWork implements PropertyChangedListener */ public function tryGetByIdHash($idHash, $rootClassName) { - if ($this->containsIdHash($idHash, $rootClassName)) { - return $this->getByIdHash($idHash, $rootClassName); - } - return false; + return isset($this->_identityMap[$rootClassName][$idHash]) ? + $this->_identityMap[$rootClassName][$idHash] : false; } /** @@ -1656,8 +1654,7 @@ class UnitOfWork implements PropertyChangedListener * @param string $className The name of the entity class. * @param array $data The data for the entity. * @return object The created entity instance. - * @internal Highly performance-sensitive method. Run the performance test suites when - * making modifications. + * @internal Highly performance-sensitive method. */ public function createEntity($className, array $data, $hints = array()) { @@ -1703,6 +1700,7 @@ class UnitOfWork implements PropertyChangedListener } } + //TODO: These should be invoked later, because associations are not yet loaded here. if (isset($class->lifecycleCallbacks[Events::postLoad])) { $class->invokeLifecycleCallbacks(Events::postLoad, $entity); } diff --git a/tests/Doctrine/Tests/ORM/Functional/AllTests.php b/tests/Doctrine/Tests/ORM/Functional/AllTests.php index ea10b6b99..9a0923670 100644 --- a/tests/Doctrine/Tests/ORM/Functional/AllTests.php +++ b/tests/Doctrine/Tests/ORM/Functional/AllTests.php @@ -40,6 +40,7 @@ class AllTests $suite->addTestSuite('Doctrine\Tests\ORM\Functional\StandardEntityPersisterTest'); $suite->addTestSuite('Doctrine\Tests\ORM\Functional\MappedSuperclassTest'); $suite->addTestSuite('Doctrine\Tests\ORM\Functional\EntityRepositoryTest'); + $suite->addTestSuite('Doctrine\Tests\ORM\Functional\IdentityMapTest'); $suite->addTest(Locking\AllTests::suite()); $suite->addTest(SchemaTool\AllTests::suite()); diff --git a/tests/Doctrine/Tests/ORM/Functional/IdentityMapTest.php b/tests/Doctrine/Tests/ORM/Functional/IdentityMapTest.php new file mode 100644 index 000000000..7a4f23808 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/IdentityMapTest.php @@ -0,0 +1,137 @@ + + */ +class IdentityMapTest extends \Doctrine\Tests\OrmFunctionalTestCase +{ + protected function setUp() { + $this->useModelSet('cms'); + parent::setUp(); + } + + public function testSingleValuedAssociationIdentityMapBehavior() + { + $address = new CmsAddress; + $address->country = 'de'; + $address->zip = '12345'; + $address->city = 'Berlin'; + + $user1 = new CmsUser; + $user1->status = 'dev'; + $user1->username = 'romanb'; + $user1->name = 'Roman B.'; + + $user2 = new CmsUser; + $user2->status = 'dev'; + $user2->username = 'gblanco'; + $user2->name = 'Guilherme Blanco'; + + $address->setUser($user1); + + $this->_em->persist($address); + $this->_em->persist($user1); + $this->_em->persist($user2); + $this->_em->flush(); + + + $this->assertSame($user1, $address->user); + + //external update to CmsAddress + $this->_em->getConnection()->executeUpdate('update cms_addresses set user_id = ?', array($user2->getId())); + + //select + $q = $this->_em->createQuery('select a, u from Doctrine\Tests\Models\CMS\CmsAddress a join a.user u'); + $address2 = $q->getSingleResult(); + + $this->assertSame($address, $address2); + + // Should still be $user1 + $this->assertSame($user1, $address2->user); + $this->assertTrue($user2->address === null); + + // But we want to have this external change! + // Solution 1: refresh(), broken atm! + //$this->_em->refresh($address2); + // Solution 2: Alternatively, a refresh query should work + $q = $this->_em->createQuery('select a, u from Doctrine\Tests\Models\CMS\CmsAddress a join a.user u'); + $q->setHint(Query::HINT_REFRESH, true); + $address3 = $q->getSingleResult(); + + $this->assertSame($address, $address3); // should still be the same, always from identity map + + // Now the association should be "correct", referencing $user2 + $this->assertSame($user2, $address2->user); + $this->assertSame($user2->address, $address2); // check back reference also + + // Attention! refreshes can result in broken bidirectional associations! this is currently expected! + // $user1 still points to $address2! + $this->assertSame($user1->address, $address2); + } + + public function testCollectionValuedAssociationIdentityMapBehavior() + { + $user = new CmsUser; + $user->status = 'dev'; + $user->username = 'romanb'; + $user->name = 'Roman B.'; + + $phone1 = new CmsPhonenumber; + $phone1->phonenumber = 123; + + $phone2 = new CmsPhonenumber; + $phone2->phonenumber = 234; + + $phone3 = new CmsPhonenumber; + $phone3->phonenumber = 345; + + $user->addPhonenumber($phone1); + $user->addPhonenumber($phone2); + $user->addPhonenumber($phone3); + + $this->_em->persist($user); // cascaded to phone numbers + $this->_em->flush(); + + $this->assertEquals(3, count($user->getPhonenumbers())); + + //external update to CmsAddress + $this->_em->getConnection()->executeUpdate('insert into cms_phonenumbers (phonenumber, user_id) VALUES (?,?)', array(999, $user->getId())); + + //select + $q = $this->_em->createQuery('select u, p from Doctrine\Tests\Models\CMS\CmsUser u join u.phonenumbers p'); + $user2 = $q->getSingleResult(); + + $this->assertSame($user, $user2); + + // Should still be the same 3 phonenumbers + $this->assertEquals(3, count($user2->getPhonenumbers())); + + // But we want to have this external change! + // Solution 1: refresh(). + //$this->_em->refresh($user2); broken atm! + // Solution 2: Alternatively, a refresh query should work + $q = $this->_em->createQuery('select u, p from Doctrine\Tests\Models\CMS\CmsUser u join u.phonenumbers p'); + $q->setHint(Query::HINT_REFRESH, true); + $user3 = $q->getSingleResult(); + + $this->assertSame($user, $user3); // should still be the same, always from identity map + + // Now the collection should be refreshed with correct count + $this->assertEquals(4, count($user3->getPhonenumbers())); + } +} + diff --git a/tests/Doctrine/Tests/ORM/Functional/StandardEntityPersisterTest.php b/tests/Doctrine/Tests/ORM/Functional/StandardEntityPersisterTest.php index d3979d4c3..747545bd9 100644 --- a/tests/Doctrine/Tests/ORM/Functional/StandardEntityPersisterTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/StandardEntityPersisterTest.php @@ -88,7 +88,7 @@ class StandardEntityPersisterTest extends \Doctrine\Tests\OrmFunctionalTestCase // (change from ArrayCollection to PersistentCollection) $f3 = new ECommerceFeature(); $f3->setDescription('XVID'); - $p->addfeature($f3); + $p->addFeature($f3); // Now we persist the Feature #3 $this->_em->persist($p);