From 82455444763458d6fe6dd686bfb53e3385f78df2 Mon Sep 17 00:00:00 2001 From: Carlo Beltrame Date: Sat, 7 Aug 2021 14:55:16 +0200 Subject: [PATCH 1/4] feat: Avoid eager joining back to the just visited parent --- .../Orm/Extension/EagerLoadingExtension.php | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php b/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php index da05190e554..a951ae93982 100644 --- a/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php +++ b/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php @@ -28,6 +28,7 @@ use ApiPlatform\Exception\RuntimeException; use ApiPlatform\Metadata\ApiProperty; use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface; +use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Mapping\ClassMetadataInfo; use Doctrine\ORM\Query\Expr\Join; use Doctrine\ORM\QueryBuilder; @@ -167,7 +168,7 @@ private function apply(bool $collection, QueryBuilder $queryBuilder, QueryNameGe * * @throws RuntimeException when the max number of joins has been reached */ - private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, bool $forceEager, bool $fetchPartial, string $parentAlias, array $options = [], array $normalizationContext = [], bool $wasLeftJoin = false, int &$joinCount = 0, int $currentDepth = null) + private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, bool $forceEager, bool $fetchPartial, string $parentAlias, array $options = [], array $normalizationContext = [], bool $wasLeftJoin = false, int &$joinCount = 0, int $currentDepth = null, string $parentAssociation = null) { if ($joinCount > $this->maxJoins) { throw new RuntimeException('The total number of joined relations has exceeded the specified maximum. Raise the limit if necessary with the "api_platform.eager_loading.max_joins" configuration key (https://api-platform.com/docs/core/performance/#eager-loading), or limit the maximum serialization depth using the "enable_max_depth" option of the Symfony serializer (https://symfony.com/doc/current/components/serializer.html#handling-serialization-depth).'); @@ -232,6 +233,16 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt continue; } + // Avoid joining back to the parent that we just came from, but only on *ToOne relations + if ( + null !== $parentAssociation && + isset($mapping['inversedBy']) && + $mapping['inversedBy'] === $parentAssociation && + $mapping['type'] & ClassMetadata::TO_ONE + ) { + continue; + } + $existingJoin = QueryBuilderHelper::getExistingJoin($queryBuilder, $parentAlias, $association); if (null !== $existingJoin) { @@ -276,7 +287,7 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt } } - $this->joinRelations($queryBuilder, $queryNameGenerator, $mapping['targetEntity'], $forceEager, $fetchPartial, $associationAlias, $options, $childNormalizationContext, $isLeftJoin, $joinCount, $currentDepth); + $this->joinRelations($queryBuilder, $queryNameGenerator, $mapping['targetEntity'], $forceEager, $fetchPartial, $associationAlias, $options, $childNormalizationContext, $isLeftJoin, $joinCount, $currentDepth, $association); } } From 100388db3b22c933d65c7c63e4b981a9245132d4 Mon Sep 17 00:00:00 2001 From: Carlo Beltrame Date: Sat, 7 Aug 2021 16:23:40 +0200 Subject: [PATCH 2/4] fix: Remove unnecessary duplicate select statement This exact select statement is already added a few lines above. The only exception is when fetchPartial is active, and in that case the current implementation is wrong anyways, because it always adds the full select. When this "temporary" solution for avoiding recursion was implemented, the duplicated line above was not yet there: https://github.com/api-platform/core/blob/5ba518014e2770e1ad5686b0124b2db45245fee5/src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php#L148 But later, the duplicated statement was added, and the select statement inside the "Avoid recursion" case was made obsolete: https://github.com/api-platform/core/commit/e34427a9f3e356a2d74350a035421dff6e499634 --- .../Doctrine/Orm/Extension/EagerLoadingExtension.php | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php b/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php index a951ae93982..8fb0fbf32c7 100644 --- a/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php +++ b/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php @@ -268,13 +268,8 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt $queryBuilder->addSelect($associationAlias); } - // Avoid recursive joins + // Avoid recursive joins for self-referencing relations if ($mapping['targetEntity'] === $resourceClass) { - // Avoid joining the same association twice (see #1959) - if (!\in_array($associationAlias, $queryBuilder->getAllAliases(), true)) { - $queryBuilder->addSelect($associationAlias); - } - continue; } From 9802f4d08440ff0d2265636ce4a068b297d56e90 Mon Sep 17 00:00:00 2001 From: Carlo Beltrame Date: Sat, 7 Aug 2021 16:28:25 +0200 Subject: [PATCH 3/4] refactor(eager loading): Avoid joining unnecessary recursive relations --- features/doctrine/eager_loading.feature | 19 +++++++++++-------- .../Orm/Extension/EagerLoadingExtension.php | 8 ++++++-- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/features/doctrine/eager_loading.feature b/features/doctrine/eager_loading.feature index 06ce1baf36f..27ab9804abe 100644 --- a/features/doctrine/eager_loading.feature +++ b/features/doctrine/eager_loading.feature @@ -11,11 +11,12 @@ Feature: Eager Loading Then the response status code should be 200 And the DQL should be equal to: """ - SELECT o, thirdLevel_a1, relatedToDummyFriend_a2, dummyFriend_a3 + SELECT o, thirdLevel_a1, fourthLevel_a2, relatedToDummyFriend_a3, dummyFriend_a4 FROM ApiPlatform\Tests\Fixtures\TestBundle\Entity\RelatedDummy o LEFT JOIN o.thirdLevel thirdLevel_a1 - LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a2 - LEFT JOIN relatedToDummyFriend_a2.dummyFriend dummyFriend_a3 + LEFT JOIN thirdLevel_a1.fourthLevel fourthLevel_a2 + LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a3 + LEFT JOIN relatedToDummyFriend_a3.dummyFriend dummyFriend_a4 WHERE o.id = :id_id """ @@ -45,11 +46,12 @@ Feature: Eager Loading Then the response status code should be 200 And the DQL should be equal to: """ - SELECT o, thirdLevel_a4, relatedToDummyFriend_a1, dummyFriend_a5 + SELECT o, thirdLevel_a4, fourthLevel_a5, relatedToDummyFriend_a1, dummyFriend_a6 FROM ApiPlatform\Tests\Fixtures\TestBundle\Entity\RelatedDummy o INNER JOIN o.relatedToDummyFriend relatedToDummyFriend_a1 LEFT JOIN o.thirdLevel thirdLevel_a4 - INNER JOIN relatedToDummyFriend_a1.dummyFriend dummyFriend_a5 + LEFT JOIN thirdLevel_a4.fourthLevel fourthLevel_a5 + INNER JOIN relatedToDummyFriend_a1.dummyFriend dummyFriend_a6 WHERE o IN( SELECT o_a2 FROM ApiPlatform\Tests\Fixtures\TestBundle\Entity\RelatedDummy o_a2 @@ -81,11 +83,12 @@ Feature: Eager Loading Then the response status code should be 200 And the DQL should be equal to: """ - SELECT o, thirdLevel_a3, relatedToDummyFriend_a4, dummyFriend_a5 + SELECT o, thirdLevel_a3, fourthLevel_a4, relatedToDummyFriend_a5, dummyFriend_a6 FROM ApiPlatform\Tests\Fixtures\TestBundle\Entity\RelatedDummy o LEFT JOIN o.thirdLevel thirdLevel_a3 - LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a4 - LEFT JOIN relatedToDummyFriend_a4.dummyFriend dummyFriend_a5 + LEFT JOIN thirdLevel_a3.fourthLevel fourthLevel_a4 + LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a5 + LEFT JOIN relatedToDummyFriend_a5.dummyFriend dummyFriend_a6 WHERE o.id IN ( SELECT related_dummy_a1.id FROM ApiPlatform\Tests\Fixtures\TestBundle\Entity\RelatedDummy related_dummy_a1 diff --git a/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php b/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php index 8fb0fbf32c7..8d6fbb12504 100644 --- a/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php +++ b/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php @@ -228,8 +228,7 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt continue; } - $isNotReadableLink = false === $propertyMetadata->isReadableLink(); - if (null === $fetchEager && (false === $propertyMetadata->isReadable() || ((null === $inAttributes && $isNotReadableLink) || (false === $inAttributes)))) { + if (true !== $fetchEager && (false === $propertyMetadata->isReadable() || false === $inAttributes)) { continue; } @@ -273,6 +272,11 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt continue; } + // Only join the relation's relations recursively if it's a readableLink + if (true !== $fetchEager && (true !== $propertyMetadata->isReadableLink())) { + continue; + } + if (isset($attributesMetadata[$association])) { $maxDepth = $attributesMetadata[$association]->getMaxDepth(); From 4c6ae9eb53712bfc893fe9bd9591b65ad9d55b5c Mon Sep 17 00:00:00 2001 From: Pirmin Mattmann Date: Sat, 4 Sep 2021 13:09:09 +0200 Subject: [PATCH 4/4] fix: prevent adding same alias twice; doctrine does not like it --- .../Orm/Extension/EagerLoadingExtension.php | 16 ++++++++++++++-- .../Orm/Extension/EagerLoadingExtensionTest.php | 4 ++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php b/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php index 8d6fbb12504..9476f7b087f 100644 --- a/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php +++ b/src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php @@ -31,6 +31,7 @@ use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Mapping\ClassMetadataInfo; use Doctrine\ORM\Query\Expr\Join; +use Doctrine\ORM\Query\Expr\Select; use Doctrine\ORM\QueryBuilder; use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactoryInterface; @@ -264,7 +265,7 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt continue; } } else { - $queryBuilder->addSelect($associationAlias); + $this->addSelectOnce($queryBuilder, $associationAlias); } // Avoid recursive joins for self-referencing relations @@ -296,7 +297,7 @@ private function addSelect(QueryBuilder $queryBuilder, string $entity, string $a $entityManager = $queryBuilder->getEntityManager(); $targetClassMetadata = $entityManager->getClassMetadata($entity); if (!empty($targetClassMetadata->subClasses)) { - $queryBuilder->addSelect($associationAlias); + $this->addSelectOnce($queryBuilder, $associationAlias); return; } @@ -335,6 +336,17 @@ private function addSelect(QueryBuilder $queryBuilder, string $entity, string $a $queryBuilder->addSelect(sprintf('partial %s.{%s}', $associationAlias, implode(',', $select))); } + private function addSelectOnce(QueryBuilder $queryBuilder, string $alias) + { + $existingSelects = array_reduce($queryBuilder->getDQLPart('select') ?? [], function ($existing, $dqlSelect) { + return ($dqlSelect instanceof Select) ? array_merge($existing, $dqlSelect->getParts()) : $existing; + }, []); + + if (!\in_array($alias, $existingSelects, true)) { + $queryBuilder->addSelect($alias); + } + } + /** * Gets the serializer context. * diff --git a/tests/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php b/tests/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php index d6d9dfa152f..4326c63ab34 100644 --- a/tests/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php +++ b/tests/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php @@ -236,6 +236,7 @@ public function testApplyToItem() $queryBuilderProphecy->addSelect('partial relatedDummy4_a5.{id}')->shouldBeCalledTimes(1); $queryBuilderProphecy->addSelect('singleInheritanceRelation_a6')->shouldBeCalledTimes(1); $queryBuilderProphecy->getDQLPart('join')->willReturn([]); + $queryBuilderProphecy->getDQLPart('select')->willReturn([]); $queryBuilder = $queryBuilderProphecy->reveal(); $orderExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30, false, null, null, true); @@ -895,6 +896,7 @@ public function testApplyToCollectionNoPartial() $queryBuilderProphecy->addSelect('relatedDummy_a1')->shouldBeCalledTimes(1); $queryBuilderProphecy->addSelect('relatedDummy2_a2')->shouldBeCalledTimes(1); $queryBuilderProphecy->getDQLPart('join')->willReturn([]); + $queryBuilderProphecy->getDQLPart('select')->willReturn([]); $queryBuilder = $queryBuilderProphecy->reveal(); $eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30); @@ -960,6 +962,7 @@ private function doTestApplyToCollectionWithANonReadableButFetchEagerProperty(bo $queryBuilderProphecy->addSelect('relatedDummy_a1')->shouldBeCalledTimes(1); $queryBuilderProphecy->addSelect('relatedDummy2_a2')->shouldBeCalledTimes(1); $queryBuilderProphecy->getDQLPart('join')->willReturn([]); + $queryBuilderProphecy->getDQLPart('select')->willReturn([]); $queryBuilder = $queryBuilderProphecy->reveal(); $eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30); @@ -1004,6 +1007,7 @@ public function testApplyToCollectionWithExistingJoin(string $joinType): void new Join($joinType, 'o.relatedDummy', 'existing_join_alias'), ], ]); + $queryBuilderProphecy->getDQLPart('select')->willReturn([]); $queryBuilderProphecy->addSelect('existing_join_alias')->shouldBeCalledTimes(1); $queryBuilder = $queryBuilderProphecy->reveal();