From dc899e26cf6501febdeba2be7cc48f321d709732 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sun, 13 Dec 2020 19:51:00 +0100 Subject: [PATCH 01/19] [GH-1569] Add new SUBSELECT fetch mode for OneToMany associations. Co-authored-by: Wouter M. van Vliet --- .../ORM/Mapping/ClassMetadataInfo.php | 6 + lib/Doctrine/ORM/Mapping/OneToMany.php | 2 +- lib/Doctrine/ORM/UnitOfWork.php | 80 +++++++++++- .../SubselectFetchCollectionTest.php | 115 ++++++++++++++++++ 4 files changed, 201 insertions(+), 2 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/SubselectFetchCollectionTest.php diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index 9d8f27cd1a8..6cf3c588a12 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -194,6 +194,12 @@ class ClassMetadataInfo implements ClassMetadata */ public const FETCH_EXTRA_LAZY = 4; + /** + * Specifies that all entities of a collection valued association are fetched using a single or more + * additional WHERE IN queries. + */ + const FETCH_SUBSELECT = 5; + /** * Identifies a one-to-one association. */ diff --git a/lib/Doctrine/ORM/Mapping/OneToMany.php b/lib/Doctrine/ORM/Mapping/OneToMany.php index c3ffa16da27..fb9c4c3a61d 100644 --- a/lib/Doctrine/ORM/Mapping/OneToMany.php +++ b/lib/Doctrine/ORM/Mapping/OneToMany.php @@ -39,7 +39,7 @@ final class OneToMany implements MappingAttribute * @var string * @psalm-var 'LAZY'|'EAGER'|'EXTRA_LAZY' * @readonly - * @Enum({"LAZY", "EAGER", "EXTRA_LAZY"}) + * @Enum({"LAZY", "EAGER", "EXTRA_LAZY", "SUBSELECT"}) */ public $fetch = 'LAZY'; diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index dd0150a5afd..7f9e3c274de 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -315,6 +315,9 @@ class UnitOfWork implements PropertyChangedListener */ private $eagerLoadingEntities = []; + /** @var array */ + private $subselectLoadingEntities = []; + /** @var bool */ protected $hasCache = false; @@ -3112,6 +3115,8 @@ public function createEntity($className, array $data, &$hints = []) if ($assoc['fetch'] === ClassMetadata::FETCH_EAGER) { $this->loadCollection($pColl); $pColl->takeSnapshot(); + } elseif ($assoc['fetch'] == ClassMetadata::FETCH_SUBSELECT) { + $this->scheduleCollectionForBatchLoading($pColl, $class); } $this->originalEntityData[$oid][$field] = $pColl; @@ -3128,7 +3133,7 @@ public function createEntity($className, array $data, &$hints = []) /** @return void */ public function triggerEagerLoads() { - if (! $this->eagerLoadingEntities) { + if (! $this->eagerLoadingEntities && ! $this->subselectLoadingEntities) { return; } @@ -3147,6 +3152,55 @@ public function triggerEagerLoads() array_combine($class->identifier, [array_values($ids)]) ); } + + $subselectLoadingEntities = $this->subselectLoadingEntities; // avoid recursion + $this->subselectLoadingEntities = []; + + foreach($subselectLoadingEntities as $group) { + $this->subselectLoadCollection($group['items'], $group['mapping']); + } + } + + /** + * Load all data into the given collections, according to the specified mapping + * + * @param PersistentCollection[] $collections + * @param array $mapping + */ + private function subselectLoadCollection(array $collections, array $mapping) : void + { + $targetEntity = $mapping['targetEntity']; + $class = $this->em->getClassMetadata($mapping['sourceEntity']); + $mappedBy = $mapping['mappedBy']; + + $entities = []; + $index = []; + + foreach ($collections as $idHash => $collection) { + $index[$idHash] = $collection; + $entities[] = $collection->getOwner(); + } + + $found = $this->em->getRepository($targetEntity)->findBy([ + $mappedBy => $entities + ]); + + $targetClass = $this->em->getClassMetadata($targetEntity); + $targetProperty = $targetClass->getReflectionProperty($mappedBy); + + foreach ($found as $targetValue) { + $sourceEntity = $targetProperty->getValue($targetValue); + + $id = $this->identifierFlattener->flattenIdentifier($class, $class->getIdentifierValues($sourceEntity)); + $idHash = implode(' ', $id); + + $index[$idHash]->add($targetValue); + } + + foreach ($index as $association) { + $association->setInitialized(true); + $association->takeSnapshot(); + } } /** @@ -3176,6 +3230,30 @@ public function loadCollection(PersistentCollection $collection) $collection->setInitialized(true); } + /** + * Schedule this collection for batch loading at the end of the UnitOfWork + */ + private function scheduleCollectionForBatchLoading(PersistentCollection $collection, ClassMetadata $sourceClass) : void + { + $mapping = $collection->getMapping(); + $name = $mapping['sourceEntity'] . '#' . $mapping['fieldName']; + + if (! isset($this->subselectLoadingEntities[$name])) { + $this->subselectLoadingEntities[$name] = [ + 'items' => [], + 'mapping' => $mapping, + ]; + } + + $id = $this->identifierFlattener->flattenIdentifier( + $sourceClass, + $sourceClass->getIdentifierValues($collection->getOwner()) + ); + $idHash = implode(' ', $id); + + $this->subselectLoadingEntities[$name]['items'][$idHash] = $collection; + } + /** * Gets the identity map of the UnitOfWork. * diff --git a/tests/Doctrine/Tests/ORM/Functional/SubselectFetchCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/SubselectFetchCollectionTest.php new file mode 100644 index 00000000000..30965a2e57b --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/SubselectFetchCollectionTest.php @@ -0,0 +1,115 @@ +_schemaTool->createSchema([ + $this->_em->getClassMetadata(SubselectFetchOwner::class), + $this->_em->getClassMetadata(SubselectFetchChild::class), + ]); + } catch(\Exception $e) { + } + } + + public function testSubselectFetchMode() + { + $owner = $this->createOwnerWithChildren(2); + $owner2 = $this->createOwnerWithChildren(3); + + $this->_em->flush(); + + $this->_em->clear(); + + $owner = $this->_em->find(SubselectFetchOwner::class, $owner->id); + + $afterQueryCount = $this->getCurrentQueryCount(); + $this->assertCount(2, $owner->children); + $anotherQueryCount = $this->getCurrentQueryCount(); + + $this->assertEquals($anotherQueryCount, $afterQueryCount); + + $this->assertCount(3, $this->_em->find(SubselectFetchOwner::class, $owner2->id)->children); + + $this->_em->clear(); + + $beforeQueryCount = $this->getCurrentQueryCount(); + $owners = $this->_em->getRepository(SubselectFetchOwner::class)->findAll(); + $anotherQueryCount = $this->getCurrentQueryCount(); + + // the findAll() + 1 subselect loading both collections of the two returned $owners + $this->assertEquals($beforeQueryCount + 2, $anotherQueryCount); + + $this->assertCount(2, $owners[0]->children); + $this->assertCount(3, $owners[1]->children); + + // both collections are already initialized and count'ing them does not make a difference in total query count + $this->assertEquals($anotherQueryCount, $this->getCurrentQueryCount()); + } + + /** + * @return SubselectFetchOwner + * @throws \Doctrine\ORM\ORMException + */ + protected function createOwnerWithChildren(int $children): SubselectFetchOwner + { + $owner = new SubselectFetchOwner(); + $this->_em->persist($owner); + + for ($i = 0; $i < $children; $i++) { + $child = new SubselectFetchChild(); + $child->owner = $owner; + + $owner->children->add($child); + + $this->_em->persist($child); + } + + return $owner; + } +} + +/** + * @Entity + */ +class SubselectFetchOwner +{ + /** @Id @Column(type="integer") @GeneratedValue() */ + public $id; + + /** + * @var ArrayCollection + * + * @OneToMany(targetEntity="SubselectFetchChild", mappedBy="owner", fetch="SUBSELECT") + */ + public $children; + + public function __construct() + { + $this->children = new ArrayCollection(); + } +} + +/** + * @Entity + */ +class SubselectFetchChild +{ + /** @Id @Column(type="integer") @GeneratedValue() */ + public $id; + + /** + * @ManyToOne(targetEntity="SubselectFetchOwner", inversedBy="children") + * + * @var SubselectFetchOwner + */ + public $owner; +} \ No newline at end of file From fdceb82454236908996dfa58d85fd6a734419701 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Tue, 10 Oct 2023 07:49:38 +0200 Subject: [PATCH 02/19] Disallow WITH keyword on fetch joined associatiosn via subselect. --- lib/Doctrine/ORM/Query/QueryException.php | 8 ++++ lib/Doctrine/ORM/Query/SqlWalker.php | 4 ++ .../SubselectFetchCollectionTest.php | 37 ++++++++++++------- 3 files changed, 36 insertions(+), 13 deletions(-) diff --git a/lib/Doctrine/ORM/Query/QueryException.php b/lib/Doctrine/ORM/Query/QueryException.php index be9b8ed750e..e7010a21b82 100644 --- a/lib/Doctrine/ORM/Query/QueryException.php +++ b/lib/Doctrine/ORM/Query/QueryException.php @@ -204,6 +204,14 @@ public static function iterateWithFetchJoinNotAllowed($assoc) ); } + public static function subselectFetchJoinWithNotAllowed($assoc): QueryException + { + return new self( + 'Associations with fetch-mode subselects may not be using WITH conditions in + "' . $assoc['sourceEntity'] . '#' . $assoc['fieldName'] . '".' + ); + } + public static function iterateWithMixedResultNotAllowed(): QueryException { return new self('Iterating a query with mixed results (using scalars) is not supported.'); diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index a677ca26710..a0202eaef81 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -1047,6 +1047,10 @@ public function walkJoinAssociationDeclaration($joinAssociationDeclaration, $joi } } + if ($relation['fetch'] === ClassMetadata::FETCH_SUBSELECT) { + throw QueryException::subselectFetchJoinWithNotAllowed($assoc); + } + $targetTableJoin = null; // This condition is not checking ClassMetadata::MANY_TO_ONE, because by definition it cannot diff --git a/tests/Doctrine/Tests/ORM/Functional/SubselectFetchCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/SubselectFetchCollectionTest.php index 30965a2e57b..66b5e65b161 100644 --- a/tests/Doctrine/Tests/ORM/Functional/SubselectFetchCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/SubselectFetchCollectionTest.php @@ -3,7 +3,9 @@ namespace Doctrine\Tests\ORM\Functional; use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\ORM\Query\QueryException; use Doctrine\Tests\OrmFunctionalTestCase; +use Doctrine\ORM\Mapping as ORM; class SubselectFetchCollectionTest extends OrmFunctionalTestCase { @@ -20,7 +22,7 @@ protected function setUp() : void } } - public function testSubselectFetchMode() + public function testSubselectFetchMode(): void { $owner = $this->createOwnerWithChildren(2); $owner2 = $this->createOwnerWithChildren(3); @@ -31,9 +33,9 @@ public function testSubselectFetchMode() $owner = $this->_em->find(SubselectFetchOwner::class, $owner->id); - $afterQueryCount = $this->getCurrentQueryCount(); + $afterQueryCount = count($this->getQueryLog()->queries); $this->assertCount(2, $owner->children); - $anotherQueryCount = $this->getCurrentQueryCount(); + $anotherQueryCount = count($this->getQueryLog()->queries); $this->assertEquals($anotherQueryCount, $afterQueryCount); @@ -41,9 +43,9 @@ public function testSubselectFetchMode() $this->_em->clear(); - $beforeQueryCount = $this->getCurrentQueryCount(); + $beforeQueryCount = count($this->getQueryLog()->queries); $owners = $this->_em->getRepository(SubselectFetchOwner::class)->findAll(); - $anotherQueryCount = $this->getCurrentQueryCount(); + $anotherQueryCount = count($this->getQueryLog()->queries); // the findAll() + 1 subselect loading both collections of the two returned $owners $this->assertEquals($beforeQueryCount + 2, $anotherQueryCount); @@ -52,7 +54,16 @@ public function testSubselectFetchMode() $this->assertCount(3, $owners[1]->children); // both collections are already initialized and count'ing them does not make a difference in total query count - $this->assertEquals($anotherQueryCount, $this->getCurrentQueryCount()); + $this->assertEquals($anotherQueryCount, count($this->getQueryLog()->queries)); + } + + public function testSubselectFetchJoinWithNotAllowed(): void + { + $this->expectException(QueryException::class); + $this->expectExceptionMessage('Associations with fetch-mode subselects may not be using WITH conditions'); + + $query = $this->_em->createQuery('SELECT o, c FROM ' . SubselectFetchOwner::class . ' o JOIN o.children c WITH c.id = 1'); + $query->getResult(); } /** @@ -78,17 +89,17 @@ protected function createOwnerWithChildren(int $children): SubselectFetchOwner } /** - * @Entity + * @ORM\Entity */ class SubselectFetchOwner { - /** @Id @Column(type="integer") @GeneratedValue() */ + /** @ORM\Id @ORM\Column(type="integer") @ORM\GeneratedValue() */ public $id; /** * @var ArrayCollection * - * @OneToMany(targetEntity="SubselectFetchChild", mappedBy="owner", fetch="SUBSELECT") + * @ORM\OneToMany(targetEntity="SubselectFetchChild", mappedBy="owner", fetch="SUBSELECT") */ public $children; @@ -99,17 +110,17 @@ public function __construct() } /** - * @Entity + * @ORM\Entity */ class SubselectFetchChild { - /** @Id @Column(type="integer") @GeneratedValue() */ + /** @ORM\Id @ORM\Column(type="integer") @ORM\GeneratedValue() */ public $id; /** - * @ManyToOne(targetEntity="SubselectFetchOwner", inversedBy="children") + * @ORM\ManyToOne(targetEntity="SubselectFetchOwner", inversedBy="children") * * @var SubselectFetchOwner */ public $owner; -} \ No newline at end of file +} From 47952c32281a05e20fc701c19093299bfdc76299 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Tue, 10 Oct 2023 07:51:13 +0200 Subject: [PATCH 03/19] Houskeeping: phpcs --- lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php | 2 +- lib/Doctrine/ORM/UnitOfWork.php | 18 ++++++++---------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index 6cf3c588a12..8d2200877f7 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -198,7 +198,7 @@ class ClassMetadataInfo implements ClassMetadata * Specifies that all entities of a collection valued association are fetched using a single or more * additional WHERE IN queries. */ - const FETCH_SUBSELECT = 5; + public const FETCH_SUBSELECT = 5; /** * Identifies a one-to-one association. diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 7f9e3c274de..0968dee74f5 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3115,7 +3115,7 @@ public function createEntity($className, array $data, &$hints = []) if ($assoc['fetch'] === ClassMetadata::FETCH_EAGER) { $this->loadCollection($pColl); $pColl->takeSnapshot(); - } elseif ($assoc['fetch'] == ClassMetadata::FETCH_SUBSELECT) { + } elseif ($assoc['fetch'] === ClassMetadata::FETCH_SUBSELECT) { $this->scheduleCollectionForBatchLoading($pColl, $class); } @@ -3153,10 +3153,10 @@ public function triggerEagerLoads() ); } - $subselectLoadingEntities = $this->subselectLoadingEntities; // avoid recursion + $subselectLoadingEntities = $this->subselectLoadingEntities; // avoid recursion $this->subselectLoadingEntities = []; - foreach($subselectLoadingEntities as $group) { + foreach ($subselectLoadingEntities as $group) { $this->subselectLoadCollection($group['items'], $group['mapping']); } } @@ -3165,9 +3165,9 @@ public function triggerEagerLoads() * Load all data into the given collections, according to the specified mapping * * @param PersistentCollection[] $collections - * @param array $mapping + * @param array $mapping */ - private function subselectLoadCollection(array $collections, array $mapping) : void + private function subselectLoadCollection(array $collections, array $mapping): void { $targetEntity = $mapping['targetEntity']; $class = $this->em->getClassMetadata($mapping['sourceEntity']); @@ -3181,9 +3181,7 @@ private function subselectLoadCollection(array $collections, array $mapping) : v $entities[] = $collection->getOwner(); } - $found = $this->em->getRepository($targetEntity)->findBy([ - $mappedBy => $entities - ]); + $found = $this->em->getRepository($targetEntity)->findBy([$mappedBy => $entities]); $targetClass = $this->em->getClassMetadata($targetEntity); $targetProperty = $targetClass->getReflectionProperty($mappedBy); @@ -3233,7 +3231,7 @@ public function loadCollection(PersistentCollection $collection) /** * Schedule this collection for batch loading at the end of the UnitOfWork */ - private function scheduleCollectionForBatchLoading(PersistentCollection $collection, ClassMetadata $sourceClass) : void + private function scheduleCollectionForBatchLoading(PersistentCollection $collection, ClassMetadata $sourceClass): void { $mapping = $collection->getMapping(); $name = $mapping['sourceEntity'] . '#' . $mapping['fieldName']; @@ -3245,7 +3243,7 @@ private function scheduleCollectionForBatchLoading(PersistentCollection $collect ]; } - $id = $this->identifierFlattener->flattenIdentifier( + $id = $this->identifierFlattener->flattenIdentifier( $sourceClass, $sourceClass->getIdentifierValues($collection->getOwner()) ); From b9e55bad4d132e70dae3316904c804361c79a136 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Tue, 10 Oct 2023 07:59:29 +0200 Subject: [PATCH 04/19] Introduce configuration option for subselect batch size. --- lib/Doctrine/ORM/Configuration.php | 10 +++++++++ lib/Doctrine/ORM/UnitOfWork.php | 33 ++++++++++++++++-------------- 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/lib/Doctrine/ORM/Configuration.php b/lib/Doctrine/ORM/Configuration.php index ca5063667c2..3f41a692176 100644 --- a/lib/Doctrine/ORM/Configuration.php +++ b/lib/Doctrine/ORM/Configuration.php @@ -1127,4 +1127,14 @@ public function isRejectIdCollisionInIdentityMapEnabled(): bool { return $this->_attributes['rejectIdCollisionInIdentityMap'] ?? false; } + + public function setFetchModeSubselectBatchSize(int $batchSize = 100): void + { + $this->_attributes['fetchModeSubselectBatchSize'] = $batchSize; + } + + public function getFetchModeSubselectBatchSize(): int + { + return $this->_attributes['fetchModeSubselectBatchSize'] ?? 100; + } } diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 0968dee74f5..0598829b4fe 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -52,6 +52,7 @@ use Throwable; use UnexpectedValueException; +use function array_chunk; use function array_combine; use function array_diff_key; use function array_filter; @@ -3173,29 +3174,31 @@ private function subselectLoadCollection(array $collections, array $mapping): vo $class = $this->em->getClassMetadata($mapping['sourceEntity']); $mappedBy = $mapping['mappedBy']; - $entities = []; - $index = []; + $batches = array_chunk($collections, $this->em->getConfiguration()->getFetchModeSubselectBatchSize(), true); - foreach ($collections as $idHash => $collection) { - $index[$idHash] = $collection; - $entities[] = $collection->getOwner(); - } + foreach ($batches as $collectionBatch) { + $entities = []; + + foreach ($collectionBatch as $collection) { + $entities[] = $collection->getOwner(); + } - $found = $this->em->getRepository($targetEntity)->findBy([$mappedBy => $entities]); + $found = $this->em->getRepository($targetEntity)->findBy([$mappedBy => $entities]); - $targetClass = $this->em->getClassMetadata($targetEntity); - $targetProperty = $targetClass->getReflectionProperty($mappedBy); + $targetClass = $this->em->getClassMetadata($targetEntity); + $targetProperty = $targetClass->getReflectionProperty($mappedBy); - foreach ($found as $targetValue) { - $sourceEntity = $targetProperty->getValue($targetValue); + foreach ($found as $targetValue) { + $sourceEntity = $targetProperty->getValue($targetValue); - $id = $this->identifierFlattener->flattenIdentifier($class, $class->getIdentifierValues($sourceEntity)); - $idHash = implode(' ', $id); + $id = $this->identifierFlattener->flattenIdentifier($class, $class->getIdentifierValues($sourceEntity)); + $idHash = implode(' ', $id); - $index[$idHash]->add($targetValue); + $collectionBatch[$idHash]->add($targetValue); + } } - foreach ($index as $association) { + foreach ($collections as $association) { $association->setInitialized(true); $association->takeSnapshot(); } From 41410e6be13bc1cc67b2ab1706c59e105ba8e749 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Tue, 10 Oct 2023 08:17:25 +0200 Subject: [PATCH 05/19] Go through Persister API instead of indirectly through repository. --- lib/Doctrine/ORM/UnitOfWork.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 0598829b4fe..537dd2f6c46 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3183,7 +3183,7 @@ private function subselectLoadCollection(array $collections, array $mapping): vo $entities[] = $collection->getOwner(); } - $found = $this->em->getRepository($targetEntity)->findBy([$mappedBy => $entities]); + $found = $this->getEntityPersister($targetEntity)->loadAll([$mappedBy => $entities]); $targetClass = $this->em->getClassMetadata($targetEntity); $targetProperty = $targetClass->getReflectionProperty($mappedBy); From ff28ba8080e378399c32237a41814cc2fd86d757 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Tue, 10 Oct 2023 08:25:04 +0200 Subject: [PATCH 06/19] Disallow use of fetch=SUBSELECT on to-one associations. --- lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php | 4 ++++ lib/Doctrine/ORM/Mapping/MappingException.php | 9 +++++++++ .../Tests/ORM/Mapping/ClassMetadataTest.php | 14 ++++++++++++++ 3 files changed, 27 insertions(+) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index 8d2200877f7..3379e06890e 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -1831,6 +1831,10 @@ protected function _validateAndCompleteAssociationMapping(array $mapping) $mapping['fetch'] = self::FETCH_LAZY; } + if ($mapping['fetch'] === self::FETCH_SUBSELECT && $mapping['type'] & self::TO_ONE) { + throw MappingException::illegalFetchSubselectAssociation($this->name, $mapping['fieldName']); + } + // Cascades $cascades = isset($mapping['cascade']) ? array_map('strtolower', $mapping['cascade']) : []; diff --git a/lib/Doctrine/ORM/Mapping/MappingException.php b/lib/Doctrine/ORM/Mapping/MappingException.php index c0a5d1392fe..cf69a546f51 100644 --- a/lib/Doctrine/ORM/Mapping/MappingException.php +++ b/lib/Doctrine/ORM/Mapping/MappingException.php @@ -742,6 +742,15 @@ public static function illegalToManyIdentifierAssociation($className, $field) )); } + public static function illegalFetchSubselectAssociation(string $className, string $field): MappingException + { + return new self(sprintf( + 'Cannot use fetch-mode SUBSELECT for a to-one association on entity "%s#%s".', + $className, + $field + )); + } + /** * @param string $className * diff --git a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php index 79da971300d..a4cf08f8a26 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php @@ -1353,6 +1353,20 @@ public function testRejectsEmbeddableWithoutValidClassName(): void 'columnPrefix' => false, ]); } + + public function testIllegalFetchModeSubselect(): void + { + $metadata = new ClassMetadata(TestEntity1::class); + + $this->expectException(MappingException::class); + $this->expectExceptionMessage('Cannot use fetch-mode SUBSELECT for a to-one association on entity'); + + $metadata->mapManyToOne([ + 'fieldName' => 'association', + 'targetEntity' => DDC2700MappedSuperClass::class, + 'fetch' => ClassMetadata::FETCH_SUBSELECT, + ]); + } } /** @MappedSuperclass */ From 40bfe07172f1ec33ecf28e4d8791958ff26ac621 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Tue, 10 Oct 2023 16:29:00 +0200 Subject: [PATCH 07/19] Make sure to many assocatinos are also respecting AbstractQuery::setFetchMode --- lib/Doctrine/ORM/UnitOfWork.php | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 537dd2f6c46..ed3455ecc76 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -2953,6 +2953,10 @@ public function createEntity($className, array $data, &$hints = []) continue; } + if (! isset($hints['fetchMode'][$class->name][$field])) { + $hints['fetchMode'][$class->name][$field] = $assoc['fetch']; + } + $targetClass = $this->em->getClassMetadata($assoc['targetEntity']); switch (true) { @@ -3016,10 +3020,6 @@ public function createEntity($className, array $data, &$hints = []) break; } - if (! isset($hints['fetchMode'][$class->name][$field])) { - $hints['fetchMode'][$class->name][$field] = $assoc['fetch']; - } - // Foreign key is set // Check identity map first // FIXME: Can break easily with composite keys if join column values are in @@ -3113,11 +3113,14 @@ public function createEntity($className, array $data, &$hints = []) $reflField = $class->reflFields[$field]; $reflField->setValue($entity, $pColl); - if ($assoc['fetch'] === ClassMetadata::FETCH_EAGER) { - $this->loadCollection($pColl); - $pColl->takeSnapshot(); - } elseif ($assoc['fetch'] === ClassMetadata::FETCH_SUBSELECT) { - $this->scheduleCollectionForBatchLoading($pColl, $class); + switch ($hints['fetchMode'][$class->name][$field]) { + case ClassMetadata::FETCH_EAGER: + $this->loadCollection($pColl); + $pColl->takeSnapshot(); + break; + case ClassMetadata::FETCH_SUBSELECT: + $this->scheduleCollectionForBatchLoading($pColl, $class); + break; } $this->originalEntityData[$oid][$field] = $pColl; From 76fd34f76607b1b96f381377c1c51df292c759aa Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sat, 14 Oct 2023 14:04:30 +0200 Subject: [PATCH 08/19] Avoid new fetch mode, use this strategy with fetch=EAGER for collections. --- lib/Doctrine/ORM/Configuration.php | 4 +- .../ORM/Mapping/ClassMetadataInfo.php | 4 -- lib/Doctrine/ORM/Mapping/MappingException.php | 9 --- lib/Doctrine/ORM/Mapping/OneToMany.php | 2 +- .../Entity/BasicEntityPersister.php | 2 +- lib/Doctrine/ORM/Query/QueryException.php | 4 +- lib/Doctrine/ORM/Query/SqlWalker.php | 6 +- lib/Doctrine/ORM/UnitOfWork.php | 40 +++++++------ ...nTest.php => EagerFetchCollectionTest.php} | 57 ++++++++++--------- .../ORM/Functional/Ticket/DDC440Test.php | 4 +- .../ORM/Functional/Ticket/GH10808Test.php | 3 +- 11 files changed, 62 insertions(+), 73 deletions(-) rename tests/Doctrine/Tests/ORM/Functional/{SubselectFetchCollectionTest.php => EagerFetchCollectionTest.php} (61%) diff --git a/lib/Doctrine/ORM/Configuration.php b/lib/Doctrine/ORM/Configuration.php index 3f41a692176..90ae2d95838 100644 --- a/lib/Doctrine/ORM/Configuration.php +++ b/lib/Doctrine/ORM/Configuration.php @@ -1128,12 +1128,12 @@ public function isRejectIdCollisionInIdentityMapEnabled(): bool return $this->_attributes['rejectIdCollisionInIdentityMap'] ?? false; } - public function setFetchModeSubselectBatchSize(int $batchSize = 100): void + public function setEagerFetchBatchSize(int $batchSize = 100): void { $this->_attributes['fetchModeSubselectBatchSize'] = $batchSize; } - public function getFetchModeSubselectBatchSize(): int + public function getEagerFetchBatchSize(): int { return $this->_attributes['fetchModeSubselectBatchSize'] ?? 100; } diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index 3379e06890e..8d2200877f7 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -1831,10 +1831,6 @@ protected function _validateAndCompleteAssociationMapping(array $mapping) $mapping['fetch'] = self::FETCH_LAZY; } - if ($mapping['fetch'] === self::FETCH_SUBSELECT && $mapping['type'] & self::TO_ONE) { - throw MappingException::illegalFetchSubselectAssociation($this->name, $mapping['fieldName']); - } - // Cascades $cascades = isset($mapping['cascade']) ? array_map('strtolower', $mapping['cascade']) : []; diff --git a/lib/Doctrine/ORM/Mapping/MappingException.php b/lib/Doctrine/ORM/Mapping/MappingException.php index cf69a546f51..c0a5d1392fe 100644 --- a/lib/Doctrine/ORM/Mapping/MappingException.php +++ b/lib/Doctrine/ORM/Mapping/MappingException.php @@ -742,15 +742,6 @@ public static function illegalToManyIdentifierAssociation($className, $field) )); } - public static function illegalFetchSubselectAssociation(string $className, string $field): MappingException - { - return new self(sprintf( - 'Cannot use fetch-mode SUBSELECT for a to-one association on entity "%s#%s".', - $className, - $field - )); - } - /** * @param string $className * diff --git a/lib/Doctrine/ORM/Mapping/OneToMany.php b/lib/Doctrine/ORM/Mapping/OneToMany.php index fb9c4c3a61d..c3ffa16da27 100644 --- a/lib/Doctrine/ORM/Mapping/OneToMany.php +++ b/lib/Doctrine/ORM/Mapping/OneToMany.php @@ -39,7 +39,7 @@ final class OneToMany implements MappingAttribute * @var string * @psalm-var 'LAZY'|'EAGER'|'EXTRA_LAZY' * @readonly - * @Enum({"LAZY", "EAGER", "EXTRA_LAZY", "SUBSELECT"}) + * @Enum({"LAZY", "EAGER", "EXTRA_LAZY"}) */ public $fetch = 'LAZY'; diff --git a/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php b/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php index 32ac1d4f531..f968e7b7162 100644 --- a/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php +++ b/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php @@ -1264,7 +1264,7 @@ protected function getSelectColumnsSQL() } $isAssocToOneInverseSide = $assoc['type'] & ClassMetadata::TO_ONE && ! $assoc['isOwningSide']; - $isAssocFromOneEager = $assoc['type'] !== ClassMetadata::MANY_TO_MANY && $assoc['fetch'] === ClassMetadata::FETCH_EAGER; + $isAssocFromOneEager = $assoc['type'] & ClassMetadata::TO_ONE && $assoc['fetch'] === ClassMetadata::FETCH_EAGER; if (! ($isAssocFromOneEager || $isAssocToOneInverseSide)) { continue; diff --git a/lib/Doctrine/ORM/Query/QueryException.php b/lib/Doctrine/ORM/Query/QueryException.php index e7010a21b82..f71cd7ced78 100644 --- a/lib/Doctrine/ORM/Query/QueryException.php +++ b/lib/Doctrine/ORM/Query/QueryException.php @@ -204,10 +204,10 @@ public static function iterateWithFetchJoinNotAllowed($assoc) ); } - public static function subselectFetchJoinWithNotAllowed($assoc): QueryException + public static function eagerFetchJoinWithNotAllowed($assoc): QueryException { return new self( - 'Associations with fetch-mode subselects may not be using WITH conditions in + 'Associations with fetch-mode=EAGER may not be using WITH conditions in "' . $assoc['sourceEntity'] . '#' . $assoc['fieldName'] . '".' ); } diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index a0202eaef81..6bb9caa9e26 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -1047,12 +1047,10 @@ public function walkJoinAssociationDeclaration($joinAssociationDeclaration, $joi } } - if ($relation['fetch'] === ClassMetadata::FETCH_SUBSELECT) { - throw QueryException::subselectFetchJoinWithNotAllowed($assoc); + if ($relation['fetch'] === ClassMetadata::FETCH_EAGER && $condExpr !== null) { + throw QueryException::eagerFetchJoinWithNotAllowed($assoc); } - $targetTableJoin = null; - // This condition is not checking ClassMetadata::MANY_TO_ONE, because by definition it cannot // be the owning side and previously we ensured that $assoc is always the owning side of the associations. // The owning side is necessary at this point because only it contains the JoinColumn information. diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index ed3455ecc76..d2e23ba1499 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -316,8 +316,8 @@ class UnitOfWork implements PropertyChangedListener */ private $eagerLoadingEntities = []; - /** @var array */ - private $subselectLoadingEntities = []; + /** @var array> */ + private $eagerLoadingCollections = []; /** @var bool */ protected $hasCache = false; @@ -2764,6 +2764,7 @@ public function clear($entityName = null) $this->pendingCollectionElementRemovals = $this->visitedCollections = $this->eagerLoadingEntities = + $this->eagerLoadingCollections = $this->orphanRemovals = []; } else { Deprecation::triggerIfCalledFromOutside( @@ -3115,10 +3116,6 @@ public function createEntity($className, array $data, &$hints = []) switch ($hints['fetchMode'][$class->name][$field]) { case ClassMetadata::FETCH_EAGER: - $this->loadCollection($pColl); - $pColl->takeSnapshot(); - break; - case ClassMetadata::FETCH_SUBSELECT: $this->scheduleCollectionForBatchLoading($pColl, $class); break; } @@ -3137,7 +3134,7 @@ public function createEntity($className, array $data, &$hints = []) /** @return void */ public function triggerEagerLoads() { - if (! $this->eagerLoadingEntities && ! $this->subselectLoadingEntities) { + if (! $this->eagerLoadingEntities && ! $this->eagerLoadingCollections) { return; } @@ -3157,11 +3154,11 @@ public function triggerEagerLoads() ); } - $subselectLoadingEntities = $this->subselectLoadingEntities; // avoid recursion - $this->subselectLoadingEntities = []; + $eagerLoadingCollections = $this->eagerLoadingCollections; // avoid recursion + $this->eagerLoadingCollections = []; - foreach ($subselectLoadingEntities as $group) { - $this->subselectLoadCollection($group['items'], $group['mapping']); + foreach ($eagerLoadingCollections as $group) { + $this->eagerLoadCollections($group['items'], $group['mapping']); } } @@ -3169,15 +3166,17 @@ public function triggerEagerLoads() * Load all data into the given collections, according to the specified mapping * * @param PersistentCollection[] $collections - * @param array $mapping + * @param array $mapping + * + * @psalm-var array{items: PersistentCollection[], mapping: array{targetEntity: class-string, sourceEntity: class-string, mappedBy: string, indexBy: string|null}} $mapping */ - private function subselectLoadCollection(array $collections, array $mapping): void + private function eagerLoadCollections(array $collections, array $mapping): void { $targetEntity = $mapping['targetEntity']; $class = $this->em->getClassMetadata($mapping['sourceEntity']); $mappedBy = $mapping['mappedBy']; - $batches = array_chunk($collections, $this->em->getConfiguration()->getFetchModeSubselectBatchSize(), true); + $batches = array_chunk($collections, $this->em->getConfiguration()->getEagerFetchBatchSize(), true); foreach ($batches as $collectionBatch) { $entities = []; @@ -3197,7 +3196,12 @@ private function subselectLoadCollection(array $collections, array $mapping): vo $id = $this->identifierFlattener->flattenIdentifier($class, $class->getIdentifierValues($sourceEntity)); $idHash = implode(' ', $id); - $collectionBatch[$idHash]->add($targetValue); + if ($mapping['indexBy']) { + $indexByProperty = $targetClass->getReflectionProperty($mapping['indexBy']); + $collectionBatch[$idHash]->hydrateSet($indexByProperty->getValue($targetValue), $targetValue); + } else { + $collectionBatch[$idHash]->add($targetValue); + } } } @@ -3242,8 +3246,8 @@ private function scheduleCollectionForBatchLoading(PersistentCollection $collect $mapping = $collection->getMapping(); $name = $mapping['sourceEntity'] . '#' . $mapping['fieldName']; - if (! isset($this->subselectLoadingEntities[$name])) { - $this->subselectLoadingEntities[$name] = [ + if (! isset($this->eagerLoadingCollections[$name])) { + $this->eagerLoadingCollections[$name] = [ 'items' => [], 'mapping' => $mapping, ]; @@ -3255,7 +3259,7 @@ private function scheduleCollectionForBatchLoading(PersistentCollection $collect ); $idHash = implode(' ', $id); - $this->subselectLoadingEntities[$name]['items'][$idHash] = $collection; + $this->eagerLoadingCollections[$name]['items'][$idHash] = $collection; } /** diff --git a/tests/Doctrine/Tests/ORM/Functional/SubselectFetchCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php similarity index 61% rename from tests/Doctrine/Tests/ORM/Functional/SubselectFetchCollectionTest.php rename to tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php index 66b5e65b161..02209aaa46a 100644 --- a/tests/Doctrine/Tests/ORM/Functional/SubselectFetchCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php @@ -1,50 +1,55 @@ _schemaTool->createSchema([ - $this->_em->getClassMetadata(SubselectFetchOwner::class), - $this->_em->getClassMetadata(SubselectFetchChild::class), + $this->_em->getClassMetadata(EagerFetchOwner::class), + $this->_em->getClassMetadata(EagerFetchChild::class), ]); - } catch(\Exception $e) { + } catch (Exception $e) { } } - public function testSubselectFetchMode(): void + public function testEagerFetchMode(): void { - $owner = $this->createOwnerWithChildren(2); + $owner = $this->createOwnerWithChildren(2); $owner2 = $this->createOwnerWithChildren(3); $this->_em->flush(); - $this->_em->clear(); - $owner = $this->_em->find(SubselectFetchOwner::class, $owner->id); + $owner = $this->_em->find(EagerFetchOwner::class, $owner->id); $afterQueryCount = count($this->getQueryLog()->queries); $this->assertCount(2, $owner->children); + $anotherQueryCount = count($this->getQueryLog()->queries); $this->assertEquals($anotherQueryCount, $afterQueryCount); - $this->assertCount(3, $this->_em->find(SubselectFetchOwner::class, $owner2->id)->children); + $this->assertCount(3, $this->_em->find(EagerFetchOwner::class, $owner2->id)->children); $this->_em->clear(); - $beforeQueryCount = count($this->getQueryLog()->queries); - $owners = $this->_em->getRepository(SubselectFetchOwner::class)->findAll(); + $beforeQueryCount = count($this->getQueryLog()->queries); + $owners = $this->_em->getRepository(EagerFetchOwner::class)->findAll(); $anotherQueryCount = count($this->getQueryLog()->queries); // the findAll() + 1 subselect loading both collections of the two returned $owners @@ -60,23 +65,19 @@ public function testSubselectFetchMode(): void public function testSubselectFetchJoinWithNotAllowed(): void { $this->expectException(QueryException::class); - $this->expectExceptionMessage('Associations with fetch-mode subselects may not be using WITH conditions'); + $this->expectExceptionMessage('Associations with fetch-mode=EAGER may not be using WITH conditions'); - $query = $this->_em->createQuery('SELECT o, c FROM ' . SubselectFetchOwner::class . ' o JOIN o.children c WITH c.id = 1'); + $query = $this->_em->createQuery('SELECT o, c FROM ' . EagerFetchOwner::class . ' o JOIN o.children c WITH c.id = 1'); $query->getResult(); } - /** - * @return SubselectFetchOwner - * @throws \Doctrine\ORM\ORMException - */ - protected function createOwnerWithChildren(int $children): SubselectFetchOwner + protected function createOwnerWithChildren(int $children): EagerFetchOwner { - $owner = new SubselectFetchOwner(); + $owner = new EagerFetchOwner(); $this->_em->persist($owner); for ($i = 0; $i < $children; $i++) { - $child = new SubselectFetchChild(); + $child = new EagerFetchChild(); $child->owner = $owner; $owner->children->add($child); @@ -91,15 +92,15 @@ protected function createOwnerWithChildren(int $children): SubselectFetchOwner /** * @ORM\Entity */ -class SubselectFetchOwner +class EagerFetchOwner { /** @ORM\Id @ORM\Column(type="integer") @ORM\GeneratedValue() */ public $id; /** - * @var ArrayCollection + * @ORM\OneToMany(targetEntity="EagerFetchChild", mappedBy="owner", fetch="EAGER") * - * @ORM\OneToMany(targetEntity="SubselectFetchChild", mappedBy="owner", fetch="SUBSELECT") + * @var ArrayCollection */ public $children; @@ -112,15 +113,15 @@ public function __construct() /** * @ORM\Entity */ -class SubselectFetchChild +class EagerFetchChild { /** @ORM\Id @ORM\Column(type="integer") @ORM\GeneratedValue() */ public $id; /** - * @ORM\ManyToOne(targetEntity="SubselectFetchOwner", inversedBy="children") + * @ORM\ManyToOne(targetEntity="EagerFetchOwner", inversedBy="children") * - * @var SubselectFetchOwner + * @var EagerFetchOwner */ public $owner; } diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC440Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC440Test.php index 4c9f1289db6..8101a38d418 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC440Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC440Test.php @@ -49,7 +49,7 @@ public function testOriginalEntityDataEmptyWhenProxyLoadedFromTwoAssociations(): $phone->setClient($client); $phone2 = new DDC440Phone(); - $phone->setId(2); + $phone2->setId(2); $phone2->setNumber('418 222-2222'); $phone2->setClient($client); @@ -88,10 +88,10 @@ public function testOriginalEntityDataEmptyWhenProxyLoadedFromTwoAssociations(): class DDC440Phone { /** - * @var int * @Column(name="id", type="integer") * @Id * @GeneratedValue(strategy="AUTO") + * @var int */ protected $id; diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10808Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10808Test.php index a2ff78f43a9..78966fe8073 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10808Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10808Test.php @@ -39,8 +39,7 @@ public function testDQLDeferredEagerLoad(): void $query = $this->_em->createQuery( 'SELECT appointment from Doctrine\Tests\ORM\Functional\Ticket\GH10808Appointment appointment - JOIN appointment.child appointment_child - WITH appointment_child.id = 1' + JOIN appointment.child appointment_child' ); // By default, UnitOfWork::HINT_DEFEREAGERLOAD is set to 'true' From cd54c56278f4b67bae6aef2d96b724f352bcf213 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sat, 14 Oct 2023 14:21:15 +0200 Subject: [PATCH 09/19] Directly load many to many collections, batching not supported yet. fix tests. --- lib/Doctrine/ORM/UnitOfWork.php | 11 +++++++---- .../Tests/ORM/Functional/Ticket/DDC2350Test.php | 4 ++-- .../Tests/ORM/Mapping/ClassMetadataTest.php | 14 -------------- 3 files changed, 9 insertions(+), 20 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index d2e23ba1499..3bdaf4ee40d 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3114,10 +3114,13 @@ public function createEntity($className, array $data, &$hints = []) $reflField = $class->reflFields[$field]; $reflField->setValue($entity, $pColl); - switch ($hints['fetchMode'][$class->name][$field]) { - case ClassMetadata::FETCH_EAGER: + if ($hints['fetchMode'][$class->name][$field] === ClassMetadata::FETCH_EAGER) { + if ($assoc['type'] === ClassMetadata::ONE_TO_MANY) { $this->scheduleCollectionForBatchLoading($pColl, $class); - break; + } elseif ($assoc['type'] === ClassMetadata::MANY_TO_MANY) { + $this->loadCollection($pColl); + $pColl->takeSnapshot(); + } } $this->originalEntityData[$oid][$field] = $pColl; @@ -3196,7 +3199,7 @@ private function eagerLoadCollections(array $collections, array $mapping): void $id = $this->identifierFlattener->flattenIdentifier($class, $class->getIdentifierValues($sourceEntity)); $idHash = implode(' ', $id); - if ($mapping['indexBy']) { + if (isset($mapping['indexBy'])) { $indexByProperty = $targetClass->getReflectionProperty($mapping['indexBy']); $collectionBatch[$idHash]->hydrateSet($indexByProperty->getValue($targetValue), $targetValue); } else { diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2350Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2350Test.php index 2f9e7c514bd..3fe53e6bc2f 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2350Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2350Test.php @@ -44,11 +44,11 @@ public function testEagerCollectionsAreOnlyRetrievedOnce(): void $this->getQueryLog()->reset()->enable(); $user = $this->_em->find(DDC2350User::class, $user->id); - $this->assertQueryCount(1); + $this->assertQueryCount(2); self::assertCount(2, $user->reportedBugs); - $this->assertQueryCount(1); + $this->assertQueryCount(2); } } diff --git a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php index a4cf08f8a26..79da971300d 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php @@ -1353,20 +1353,6 @@ public function testRejectsEmbeddableWithoutValidClassName(): void 'columnPrefix' => false, ]); } - - public function testIllegalFetchModeSubselect(): void - { - $metadata = new ClassMetadata(TestEntity1::class); - - $this->expectException(MappingException::class); - $this->expectExceptionMessage('Cannot use fetch-mode SUBSELECT for a to-one association on entity'); - - $metadata->mapManyToOne([ - 'fieldName' => 'association', - 'targetEntity' => DDC2700MappedSuperClass::class, - 'fetch' => ClassMetadata::FETCH_SUBSELECT, - ]); - } } /** @MappedSuperclass */ From bf74b434b8677cce42d9bc6a223c585f195fa69f Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sat, 14 Oct 2023 14:23:20 +0200 Subject: [PATCH 10/19] Housekeeping: phpcs --- .../ORM/Functional/EagerFetchCollectionTest.php | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php index 02209aaa46a..c37e9e1a41f 100644 --- a/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php @@ -94,7 +94,13 @@ protected function createOwnerWithChildren(int $children): EagerFetchOwner */ class EagerFetchOwner { - /** @ORM\Id @ORM\Column(type="integer") @ORM\GeneratedValue() */ + /** + * @ORM\Column(type="integer") + * @ORM\Id + * @ORM\GeneratedValue() + * + * @var int + */ public $id; /** @@ -115,7 +121,13 @@ public function __construct() */ class EagerFetchChild { - /** @ORM\Id @ORM\Column(type="integer") @ORM\GeneratedValue() */ + /** + * @ORM\Column(type="integer") + * @ORM\Id + * @ORM\GeneratedValue() + * + * @var int + */ public $id; /** From 8ec599bb176018ad80b12aafa408736f06eb55c4 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sat, 14 Oct 2023 15:28:57 +0200 Subject: [PATCH 11/19] Static analysis --- lib/Doctrine/ORM/Query/QueryException.php | 4 ++-- lib/Doctrine/ORM/Query/SqlWalker.php | 2 +- lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php | 2 ++ lib/Doctrine/ORM/UnitOfWork.php | 2 +- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/Doctrine/ORM/Query/QueryException.php b/lib/Doctrine/ORM/Query/QueryException.php index f71cd7ced78..9604ff5e107 100644 --- a/lib/Doctrine/ORM/Query/QueryException.php +++ b/lib/Doctrine/ORM/Query/QueryException.php @@ -204,11 +204,11 @@ public static function iterateWithFetchJoinNotAllowed($assoc) ); } - public static function eagerFetchJoinWithNotAllowed($assoc): QueryException + public static function eagerFetchJoinWithNotAllowed(string $sourceEntity, string $fieldName): QueryException { return new self( 'Associations with fetch-mode=EAGER may not be using WITH conditions in - "' . $assoc['sourceEntity'] . '#' . $assoc['fieldName'] . '".' + "' . $sourceEntity . '#' . $fieldName . '".' ); } diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 6bb9caa9e26..d3ff78a2405 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -1048,7 +1048,7 @@ public function walkJoinAssociationDeclaration($joinAssociationDeclaration, $joi } if ($relation['fetch'] === ClassMetadata::FETCH_EAGER && $condExpr !== null) { - throw QueryException::eagerFetchJoinWithNotAllowed($assoc); + throw QueryException::eagerFetchJoinWithNotAllowed($assoc['sourceEntity'], $assoc['fieldName']); } // This condition is not checking ClassMetadata::MANY_TO_ONE, because by definition it cannot diff --git a/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php b/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php index cfaa4770654..15f218199be 100644 --- a/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php +++ b/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php @@ -211,6 +211,8 @@ protected function _getFetchModeString($mode) case ClassMetadataInfo::FETCH_LAZY: return 'LAZY'; } + + return 'LAZY'; } /** diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 3bdaf4ee40d..d724427508d 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3171,7 +3171,7 @@ public function triggerEagerLoads() * @param PersistentCollection[] $collections * @param array $mapping * - * @psalm-var array{items: PersistentCollection[], mapping: array{targetEntity: class-string, sourceEntity: class-string, mappedBy: string, indexBy: string|null}} $mapping + * @psalm-param array{targetEntity: class-string, sourceEntity: class-string, mappedBy: string, indexBy: string|null} $mapping */ private function eagerLoadCollections(array $collections, array $mapping): void { From 8057b51f8591709818be4783b6ba6321040f11fe Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sat, 14 Oct 2023 15:37:53 +0200 Subject: [PATCH 12/19] last violation hopefully --- lib/Doctrine/ORM/UnitOfWork.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 4e9416de497..7faa73e70ce 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3159,7 +3159,6 @@ public function triggerEagerLoads() * * @param PersistentCollection[] $collections * @param array $mapping - * * @psalm-param array{targetEntity: class-string, sourceEntity: class-string, mappedBy: string, indexBy: string|null} $mapping */ private function eagerLoadCollections(array $collections, array $mapping): void @@ -3245,9 +3244,12 @@ private function scheduleCollectionForBatchLoading(PersistentCollection $collect ]; } + $owner = $collection->getOwner(); + assert($owner !== null); + $id = $this->identifierFlattener->flattenIdentifier( $sourceClass, - $sourceClass->getIdentifierValues($collection->getOwner()) + $sourceClass->getIdentifierValues($owner) ); $idHash = implode(' ', $id); From 3f2fa309d49db2ad40717131682bd60cbde4b35c Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sat, 14 Oct 2023 15:56:42 +0200 Subject: [PATCH 13/19] Add another testcase for DQL based fetch eager of collection. --- .../Functional/EagerFetchCollectionTest.php | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php index c37e9e1a41f..a748e2ec30d 100644 --- a/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php @@ -62,6 +62,31 @@ public function testEagerFetchMode(): void $this->assertEquals($anotherQueryCount, count($this->getQueryLog()->queries)); } + public function testEagerFetchModeWithDQL(): void + { + $owner = $this->createOwnerWithChildren(2); + $owner2 = $this->createOwnerWithChildren(3); + + $this->_em->flush(); + $this->_em->clear(); + + $query = $this->_em->createQuery('SELECT o FROM ' . EagerFetchOwner::class . ' o'); + $query->setFetchMode(EagerFetchOwner::class, 'children', ORM\ClassMetadata::FETCH_EAGER); + + $beforeQueryCount = count($this->getQueryLog()->queries); + $owners = $query->getResult(); + $afterQueryCount = count($this->getQueryLog()->queries); + + $this->assertEquals($beforeQueryCount + 2, $afterQueryCount); + + $owners[0]->children->count(); + $owners[1]->children->count(); + + $anotherQueryCount = count($this->getQueryLog()->queries); + + $this->assertEquals($anotherQueryCount, $afterQueryCount); + } + public function testSubselectFetchJoinWithNotAllowed(): void { $this->expectException(QueryException::class); From 6993ad28edfc0faaa7c02c19c18a0507438388a8 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sun, 22 Oct 2023 20:07:04 +0200 Subject: [PATCH 14/19] 1:1 and M:1 associations also use fetch batch size configuration now. --- lib/Doctrine/ORM/UnitOfWork.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 7faa73e70ce..b1fef6d2012 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3139,11 +3139,14 @@ public function triggerEagerLoads() continue; } - $class = $this->em->getClassMetadata($entityName); + $class = $this->em->getClassMetadata($entityName); + $batches = array_chunk($ids, $this->em->getConfiguration()->getEagerFetchBatchSize()); - $this->getEntityPersister($entityName)->loadAll( - array_combine($class->identifier, [array_values($ids)]) - ); + foreach ($batches as $batchedIds) { + $this->getEntityPersister($entityName)->loadAll( + array_combine($class->identifier, [$batchedIds]) + ); + } } $eagerLoadingCollections = $this->eagerLoadingCollections; // avoid recursion From d03aed1265ffdabd71ffc3ce5e38467f653f3c8c Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sun, 22 Oct 2023 20:08:06 +0200 Subject: [PATCH 15/19] Explain internals of eager loading in a bit more detail and how its configured. --- docs/en/reference/working-with-objects.rst | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/en/reference/working-with-objects.rst b/docs/en/reference/working-with-objects.rst index f10c0ab091d..ef48a9a14bc 100644 --- a/docs/en/reference/working-with-objects.rst +++ b/docs/en/reference/working-with-objects.rst @@ -782,6 +782,23 @@ and these associations are mapped as EAGER, they will automatically be loaded together with the entity being queried and is thus immediately available to your application. +Eager Loading can also be configured at runtime through +``AbstractQuery::setFetchMode`` in DQL or Native Queries. + +Eager loading for many-to-one and one-to-one associations is using either a +LEFT JOIN or a second query for fetching the related entity eagerly. + +Eager loading for many-to-one associations uses a second query to load +the collections for several entities at the same time. + +When many-to-many, one-to-one or one-to-many associations are eagerly loaded, +then the global batch size configuration is used to avoid IN(?) queries with +too many arguments. The default batch size is 100 and can be changed with +``Configuration::setEagerFetchBatchSize()``. + +For eagerly laoded Many-To-Many associations one query has to made for each +collection. + By Lazy Loading ~~~~~~~~~~~~~~~ From 609e10df36f958980be916a87fae7f3d99026368 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sun, 22 Oct 2023 20:08:18 +0200 Subject: [PATCH 16/19] Address review comments. --- .../ORM/Mapping/ClassMetadataInfo.php | 6 ----- .../Functional/EagerFetchCollectionTest.php | 24 +++++-------------- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index 8d2200877f7..9d8f27cd1a8 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -194,12 +194,6 @@ class ClassMetadataInfo implements ClassMetadata */ public const FETCH_EXTRA_LAZY = 4; - /** - * Specifies that all entities of a collection valued association are fetched using a single or more - * additional WHERE IN queries. - */ - public const FETCH_SUBSELECT = 5; - /** * Identifies a one-to-one association. */ diff --git a/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php index a748e2ec30d..90cd1793a9d 100644 --- a/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php @@ -8,7 +8,6 @@ use Doctrine\ORM\Mapping as ORM; use Doctrine\ORM\Query\QueryException; use Doctrine\Tests\OrmFunctionalTestCase; -use Exception; use function count; @@ -18,13 +17,7 @@ protected function setUp(): void { parent::setUp(); - try { - $this->_schemaTool->createSchema([ - $this->_em->getClassMetadata(EagerFetchOwner::class), - $this->_em->getClassMetadata(EagerFetchChild::class), - ]); - } catch (Exception $e) { - } + $this->createSchemaForModels(EagerFetchOwner::class, EagerFetchChild::class); } public function testEagerFetchMode(): void @@ -40,26 +33,21 @@ public function testEagerFetchMode(): void $afterQueryCount = count($this->getQueryLog()->queries); $this->assertCount(2, $owner->children); - $anotherQueryCount = count($this->getQueryLog()->queries); - - $this->assertEquals($anotherQueryCount, $afterQueryCount); + $this->assertQueryCount($afterQueryCount, 'The $owner->children collection should already be initialized by find EagerFetchOwner before.'); $this->assertCount(3, $this->_em->find(EagerFetchOwner::class, $owner2->id)->children); $this->_em->clear(); - $beforeQueryCount = count($this->getQueryLog()->queries); - $owners = $this->_em->getRepository(EagerFetchOwner::class)->findAll(); - $anotherQueryCount = count($this->getQueryLog()->queries); + $beforeQueryCount = count($this->getQueryLog()->queries); + $owners = $this->_em->getRepository(EagerFetchOwner::class)->findAll(); - // the findAll() + 1 subselect loading both collections of the two returned $owners - $this->assertEquals($beforeQueryCount + 2, $anotherQueryCount); + $this->assertQueryCount($beforeQueryCount + 2, 'the findAll() + 1 subselect loading both collections of the two returned $owners'); $this->assertCount(2, $owners[0]->children); $this->assertCount(3, $owners[1]->children); - // both collections are already initialized and count'ing them does not make a difference in total query count - $this->assertEquals($anotherQueryCount, count($this->getQueryLog()->queries)); + $this->assertQueryCount($beforeQueryCount + 2, 'both collections are already initialized and counting them does not make a difference in total query count'); } public function testEagerFetchModeWithDQL(): void From 4b2b4860fbb55c73f38fbf77a3de50e05dc4a21a Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sun, 22 Oct 2023 20:11:36 +0200 Subject: [PATCH 17/19] Housekeeping: Revert change to AbstractExporter, not needed without subselect fetch. --- lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php b/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php index 15f218199be..cfaa4770654 100644 --- a/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php +++ b/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php @@ -211,8 +211,6 @@ protected function _getFetchModeString($mode) case ClassMetadataInfo::FETCH_LAZY: return 'LAZY'; } - - return 'LAZY'; } /** From ec74c83845231fa913b387a9cc1589d5586d0e0e Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sun, 5 Nov 2023 19:26:35 +0100 Subject: [PATCH 18/19] Fix typos --- docs/en/reference/working-with-objects.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/reference/working-with-objects.rst b/docs/en/reference/working-with-objects.rst index ef48a9a14bc..d88b814e8c4 100644 --- a/docs/en/reference/working-with-objects.rst +++ b/docs/en/reference/working-with-objects.rst @@ -796,7 +796,7 @@ then the global batch size configuration is used to avoid IN(?) queries with too many arguments. The default batch size is 100 and can be changed with ``Configuration::setEagerFetchBatchSize()``. -For eagerly laoded Many-To-Many associations one query has to made for each +For eagerly loaded Many-To-Many associations one query has to be made for each collection. By Lazy Loading From 6a48b0741b1710eaf6ca806f3db9a23f49ff1588 Mon Sep 17 00:00:00 2001 From: "Alexander M. Turek" Date: Thu, 9 Nov 2023 13:14:44 +0100 Subject: [PATCH 19/19] Deprecate annotation classes for named queries --- UPGRADE.md | 9 +++++++++ lib/Doctrine/ORM/Mapping/NamedNativeQueries.php | 2 ++ lib/Doctrine/ORM/Mapping/NamedNativeQuery.php | 2 ++ lib/Doctrine/ORM/Mapping/NamedQueries.php | 2 ++ lib/Doctrine/ORM/Mapping/NamedQuery.php | 2 ++ psalm.xml | 4 ++++ 6 files changed, 21 insertions(+) diff --git a/UPGRADE.md b/UPGRADE.md index 0f5a668f4be..5ac1f28d7df 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,14 @@ # Upgrade to 2.17 +## Deprecate annotations classes for named queries + +The following classes have been deprecated: + +* `Doctrine\ORM\Mapping\NamedNativeQueries` +* `Doctrine\ORM\Mapping\NamedNativeQuery` +* `Doctrine\ORM\Mapping\NamedQueries` +* `Doctrine\ORM\Mapping\NamedQuery` + ## Deprecate `Doctrine\ORM\Query\Exec\AbstractSqlExecutor::_sqlStatements` Use `Doctrine\ORM\Query\Exec\AbstractSqlExecutor::sqlStatements` instead. diff --git a/lib/Doctrine/ORM/Mapping/NamedNativeQueries.php b/lib/Doctrine/ORM/Mapping/NamedNativeQueries.php index bd474b4c39a..02ec6100b69 100644 --- a/lib/Doctrine/ORM/Mapping/NamedNativeQueries.php +++ b/lib/Doctrine/ORM/Mapping/NamedNativeQueries.php @@ -8,6 +8,8 @@ * Is used to specify an array of native SQL named queries. * The NamedNativeQueries annotation can be applied to an entity or mapped superclass. * + * @deprecated Named queries won't be supported in ORM 3. + * * @Annotation * @Target("CLASS") */ diff --git a/lib/Doctrine/ORM/Mapping/NamedNativeQuery.php b/lib/Doctrine/ORM/Mapping/NamedNativeQuery.php index 2970b266395..159b4962965 100644 --- a/lib/Doctrine/ORM/Mapping/NamedNativeQuery.php +++ b/lib/Doctrine/ORM/Mapping/NamedNativeQuery.php @@ -8,6 +8,8 @@ * Is used to specify a native SQL named query. * The NamedNativeQuery annotation can be applied to an entity or mapped superclass. * + * @deprecated Named queries won't be supported in ORM 3. + * * @Annotation * @Target("ANNOTATION") */ diff --git a/lib/Doctrine/ORM/Mapping/NamedQueries.php b/lib/Doctrine/ORM/Mapping/NamedQueries.php index a69810639c0..d844ab325d1 100644 --- a/lib/Doctrine/ORM/Mapping/NamedQueries.php +++ b/lib/Doctrine/ORM/Mapping/NamedQueries.php @@ -5,6 +5,8 @@ namespace Doctrine\ORM\Mapping; /** + * @deprecated Named queries won't be supported in ORM 3. + * * @Annotation * @Target("CLASS") */ diff --git a/lib/Doctrine/ORM/Mapping/NamedQuery.php b/lib/Doctrine/ORM/Mapping/NamedQuery.php index f57e8e8d80e..104cd95c7f9 100644 --- a/lib/Doctrine/ORM/Mapping/NamedQuery.php +++ b/lib/Doctrine/ORM/Mapping/NamedQuery.php @@ -5,6 +5,8 @@ namespace Doctrine\ORM\Mapping; /** + * @deprecated Named queries won't be supported in ORM 3. + * * @Annotation * @Target("ANNOTATION") */ diff --git a/psalm.xml b/psalm.xml index 8905a3c4086..a8a096c3ae4 100644 --- a/psalm.xml +++ b/psalm.xml @@ -38,6 +38,10 @@ + + + +