Skip to content

Improved recursion handling in EagerLoadingExtension #4377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 29, 2021
Merged

Improved recursion handling in EagerLoadingExtension #4377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8245544
feat: Avoid eager joining back to the just visited parent
carlobeltrame Aug 7, 2021
100388d
fix: Remove unnecessary duplicate select statement
carlobeltrame Aug 7, 2021
9802f4d
refactor(eager loading): Avoid joining unnecessary recursive relations
carlobeltrame Aug 7, 2021
4c6ae9e
fix: prevent adding same alias twice; doctrine does not like it
pmattmann Sep 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions features/doctrine/eager_loading.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
"""

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
44 changes: 33 additions & 11 deletions src/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@
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\Query\Expr\Select;
use Doctrine\ORM\QueryBuilder;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactoryInterface;
Expand Down Expand Up @@ -167,7 +169,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).');
Expand Down Expand Up @@ -227,8 +229,17 @@ 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;
}

// 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;
}

Expand All @@ -254,16 +265,16 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt
continue;
}
} else {
$queryBuilder->addSelect($associationAlias);
$this->addSelectOnce($queryBuilder, $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;
}

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

Expand All @@ -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);
}
}

Expand All @@ -286,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;
}
Expand Down Expand Up @@ -325,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.
*
Expand Down
4 changes: 4 additions & 0 deletions tests/Core/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down