Skip to content

Commit 2625c81

Browse files
carlobeltramepmattmann
carlobeltrame
and
pmattmann
authored
Improved recursion handling in EagerLoadingExtension (api-platform#4377)
* feat: Avoid eager joining back to the just visited parent * 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: api-platform@e34427a * refactor(eager loading): Avoid joining unnecessary recursive relations * fix: prevent adding same alias twice; doctrine does not like it Co-authored-by: Pirmin Mattmann <[email protected]>
1 parent 6b64fc2 commit 2625c81

File tree

3 files changed

+48
-19
lines changed

3 files changed

+48
-19
lines changed

‎features/doctrine/eager_loading.feature‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,12 @@ Feature: Eager Loading
1111
Then the response status code should be 200
1212
And the DQL should be equal to:
1313
"""
14-
SELECT o, thirdLevel_a1, relatedToDummyFriend_a2, dummyFriend_a3
14+
SELECT o, thirdLevel_a1, fourthLevel_a2, relatedToDummyFriend_a3, dummyFriend_a4
1515
FROM ApiPlatform\Tests\Fixtures\TestBundle\Entity\RelatedDummy o
1616
LEFT JOIN o.thirdLevel thirdLevel_a1
17-
LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a2
18-
LEFT JOIN relatedToDummyFriend_a2.dummyFriend dummyFriend_a3
17+
LEFT JOIN thirdLevel_a1.fourthLevel fourthLevel_a2
18+
LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a3
19+
LEFT JOIN relatedToDummyFriend_a3.dummyFriend dummyFriend_a4
1920
WHERE o.id = :id_id
2021
"""
2122

@@ -45,11 +46,12 @@ Feature: Eager Loading
4546
Then the response status code should be 200
4647
And the DQL should be equal to:
4748
"""
48-
SELECT o, thirdLevel_a4, relatedToDummyFriend_a1, dummyFriend_a5
49+
SELECT o, thirdLevel_a4, fourthLevel_a5, relatedToDummyFriend_a1, dummyFriend_a6
4950
FROM ApiPlatform\Tests\Fixtures\TestBundle\Entity\RelatedDummy o
5051
INNER JOIN o.relatedToDummyFriend relatedToDummyFriend_a1
5152
LEFT JOIN o.thirdLevel thirdLevel_a4
52-
INNER JOIN relatedToDummyFriend_a1.dummyFriend dummyFriend_a5
53+
LEFT JOIN thirdLevel_a4.fourthLevel fourthLevel_a5
54+
INNER JOIN relatedToDummyFriend_a1.dummyFriend dummyFriend_a6
5355
WHERE o IN(
5456
SELECT o_a2
5557
FROM ApiPlatform\Tests\Fixtures\TestBundle\Entity\RelatedDummy o_a2
@@ -81,11 +83,12 @@ Feature: Eager Loading
8183
Then the response status code should be 200
8284
And the DQL should be equal to:
8385
"""
84-
SELECT o, thirdLevel_a3, relatedToDummyFriend_a4, dummyFriend_a5
86+
SELECT o, thirdLevel_a3, fourthLevel_a4, relatedToDummyFriend_a5, dummyFriend_a6
8587
FROM ApiPlatform\Tests\Fixtures\TestBundle\Entity\RelatedDummy o
8688
LEFT JOIN o.thirdLevel thirdLevel_a3
87-
LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a4
88-
LEFT JOIN relatedToDummyFriend_a4.dummyFriend dummyFriend_a5
89+
LEFT JOIN thirdLevel_a3.fourthLevel fourthLevel_a4
90+
LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a5
91+
LEFT JOIN relatedToDummyFriend_a5.dummyFriend dummyFriend_a6
8992
WHERE o.id IN (
9093
SELECT related_dummy_a1.id
9194
FROM ApiPlatform\Tests\Fixtures\TestBundle\Entity\RelatedDummy related_dummy_a1

‎src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php‎

Lines changed: 33 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,10 @@
2828
use ApiPlatform\Exception\RuntimeException;
2929
use ApiPlatform\Metadata\ApiProperty;
3030
use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface;
31+
use Doctrine\ORM\Mapping\ClassMetadata;
3132
use Doctrine\ORM\Mapping\ClassMetadataInfo;
3233
use Doctrine\ORM\Query\Expr\Join;
34+
use Doctrine\ORM\Query\Expr\Select;
3335
use Doctrine\ORM\QueryBuilder;
3436
use Symfony\Component\HttpFoundation\RequestStack;
3537
use Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactoryInterface;
@@ -169,7 +171,7 @@ private function apply(bool $collection, QueryBuilder $queryBuilder, QueryNameGe
169171
*
170172
* @throws RuntimeException when the max number of joins has been reached
171173
*/
172-
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)
174+
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)
173175
{
174176
if ($joinCount > $this->maxJoins) {
175177
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).');
@@ -229,8 +231,17 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt
229231
continue;
230232
}
231233

232-
$isNotReadableLink = false === $propertyMetadata->isReadableLink();
233-
if (null === $fetchEager && (false === $propertyMetadata->isReadable() || ((null === $inAttributes && $isNotReadableLink) || (false === $inAttributes)))) {
234+
if (true !== $fetchEager && (false === $propertyMetadata->isReadable() || false === $inAttributes)) {
235+
continue;
236+
}
237+
238+
// Avoid joining back to the parent that we just came from, but only on *ToOne relations
239+
if (
240+
null !== $parentAssociation &&
241+
isset($mapping['inversedBy']) &&
242+
$mapping['inversedBy'] === $parentAssociation &&
243+
$mapping['type'] & ClassMetadata::TO_ONE
244+
) {
234245
continue;
235246
}
236247

@@ -256,16 +267,16 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt
256267
continue;
257268
}
258269
} else {
259-
$queryBuilder->addSelect($associationAlias);
270+
$this->addSelectOnce($queryBuilder, $associationAlias);
260271
}
261272

262-
// Avoid recursive joins
273+
// Avoid recursive joins for self-referencing relations
263274
if ($mapping['targetEntity'] === $resourceClass) {
264-
// Avoid joining the same association twice (see #1959)
265-
if (!\in_array($associationAlias, $queryBuilder->getAllAliases(), true)) {
266-
$queryBuilder->addSelect($associationAlias);
267-
}
275+
continue;
276+
}
268277

278+
// Only join the relation's relations recursively if it's a readableLink
279+
if (true !== $fetchEager && (true !== $propertyMetadata->isReadableLink())) {
269280
continue;
270281
}
271282

@@ -278,7 +289,7 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt
278289
}
279290
}
280291

281-
$this->joinRelations($queryBuilder, $queryNameGenerator, $mapping['targetEntity'], $forceEager, $fetchPartial, $associationAlias, $options, $childNormalizationContext, $isLeftJoin, $joinCount, $currentDepth);
292+
$this->joinRelations($queryBuilder, $queryNameGenerator, $mapping['targetEntity'], $forceEager, $fetchPartial, $associationAlias, $options, $childNormalizationContext, $isLeftJoin, $joinCount, $currentDepth, $association);
282293
}
283294
}
284295

@@ -288,7 +299,7 @@ private function addSelect(QueryBuilder $queryBuilder, string $entity, string $a
288299
$entityManager = $queryBuilder->getEntityManager();
289300
$targetClassMetadata = $entityManager->getClassMetadata($entity);
290301
if (!empty($targetClassMetadata->subClasses)) {
291-
$queryBuilder->addSelect($associationAlias);
302+
$this->addSelectOnce($queryBuilder, $associationAlias);
292303

293304
return;
294305
}
@@ -327,6 +338,17 @@ private function addSelect(QueryBuilder $queryBuilder, string $entity, string $a
327338
$queryBuilder->addSelect(sprintf('partial %s.{%s}', $associationAlias, implode(',', $select)));
328339
}
329340

341+
private function addSelectOnce(QueryBuilder $queryBuilder, string $alias)
342+
{
343+
$existingSelects = array_reduce($queryBuilder->getDQLPart('select') ?? [], function ($existing, $dqlSelect) {
344+
return ($dqlSelect instanceof Select) ? array_merge($existing, $dqlSelect->getParts()) : $existing;
345+
}, []);
346+
347+
if (!\in_array($alias, $existingSelects, true)) {
348+
$queryBuilder->addSelect($alias);
349+
}
350+
}
351+
330352
/**
331353
* Gets the serializer context.
332354
*

‎tests/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,7 @@ public function testApplyToItem()
236236
$queryBuilderProphecy->addSelect('partial relatedDummy4_a5.{id}')->shouldBeCalledTimes(1);
237237
$queryBuilderProphecy->addSelect('singleInheritanceRelation_a6')->shouldBeCalledTimes(1);
238238
$queryBuilderProphecy->getDQLPart('join')->willReturn([]);
239+
$queryBuilderProphecy->getDQLPart('select')->willReturn([]);
239240

240241
$queryBuilder = $queryBuilderProphecy->reveal();
241242
$orderExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30, false, null, null, true);
@@ -895,6 +896,7 @@ public function testApplyToCollectionNoPartial()
895896
$queryBuilderProphecy->addSelect('relatedDummy_a1')->shouldBeCalledTimes(1);
896897
$queryBuilderProphecy->addSelect('relatedDummy2_a2')->shouldBeCalledTimes(1);
897898
$queryBuilderProphecy->getDQLPart('join')->willReturn([]);
899+
$queryBuilderProphecy->getDQLPart('select')->willReturn([]);
898900

899901
$queryBuilder = $queryBuilderProphecy->reveal();
900902
$eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30);
@@ -960,6 +962,7 @@ private function doTestApplyToCollectionWithANonReadableButFetchEagerProperty(bo
960962
$queryBuilderProphecy->addSelect('relatedDummy_a1')->shouldBeCalledTimes(1);
961963
$queryBuilderProphecy->addSelect('relatedDummy2_a2')->shouldBeCalledTimes(1);
962964
$queryBuilderProphecy->getDQLPart('join')->willReturn([]);
965+
$queryBuilderProphecy->getDQLPart('select')->willReturn([]);
963966

964967
$queryBuilder = $queryBuilderProphecy->reveal();
965968
$eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30);
@@ -1004,6 +1007,7 @@ public function testApplyToCollectionWithExistingJoin(string $joinType): void
10041007
new Join($joinType, 'o.relatedDummy', 'existing_join_alias'),
10051008
],
10061009
]);
1010+
$queryBuilderProphecy->getDQLPart('select')->willReturn([]);
10071011
$queryBuilderProphecy->addSelect('existing_join_alias')->shouldBeCalledTimes(1);
10081012

10091013
$queryBuilder = $queryBuilderProphecy->reveal();

0 commit comments

Comments
 (0)