Fix duplicated eager loading joins (api-platform#3525)

soyuka · Mar 9, 2021 · 414351b · 414351b
1 parent 1e55e85
commit 414351b
Show file tree

Hide file tree

Showing 11 changed files with 313 additions and 14 deletions.
diff --git a/features/doctrine/eager_loading.feature b/features/doctrine/eager_loading.feature
@@ -0,0 +1,80 @@
+@!mongodb
+Feature: Eager Loading
+  In order to have better performance
+  As a client software developer
+  The eager loading should be enabled
+
+  @createSchema
+  Scenario: Eager loading for a relation
+    Given there is a RelatedDummy with 2 friends
+    When I send a "GET" request to "/related_dummies/1"
+    And the response status code should be 200
+    And the DQL should be equal to:
+    """
+    SELECT o, thirdLevel_a1, relatedToDummyFriend_a2, dummyFriend_a3
+    FROM ApiPlatform\Core\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
+    WHERE o.id = :id_id
+    """
+
+  Scenario: Eager loading for the search filter
+    Given there is a dummy object with a fourth level relation
+    When I send a "GET" request to "/dummies?relatedDummy.thirdLevel.level=3"
+    Then the response status code should be 200
+    And the DQL should be equal to:
+    """
+    SELECT o
+    FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy o
+        INNER JOIN o.relatedDummy relatedDummy_a1
+        INNER JOIN relatedDummy_a1.thirdLevel thirdLevel_a2
+    WHERE o IN(
+            SELECT o_a3
+            FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy o_a3
+                INNER JOIN o_a3.relatedDummy relatedDummy_a4
+                INNER JOIN relatedDummy_a4.thirdLevel thirdLevel_a5
+            WHERE thirdLevel_a5.level = :level_p1
+        )
+    ORDER BY o.id ASC
+    """
+
+  Scenario: Eager loading for a relation and a search filter
+    Given there is a RelatedDummy with 2 friends
+    When I send a "GET" request to "/related_dummies?relatedToDummyFriend.dummyFriend=2"
+    And the response status code should be 200
+    And the DQL should be equal to:
+    """
+    SELECT o, thirdLevel_a4, relatedToDummyFriend_a1, dummyFriend_a5
+    FROM ApiPlatform\Core\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
+    WHERE o IN(
+            SELECT o_a2
+            FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy o_a2
+                INNER JOIN o_a2.relatedToDummyFriend relatedToDummyFriend_a3
+            WHERE relatedToDummyFriend_a3.dummyFriend = :dummyFriend_p1
+        )
+    ORDER BY o.id ASC
+    """
+
+  Scenario: Eager loading for a relation with complex sub-query filter
+    Given there is a RelatedDummy with 2 friends
+    When I send a "GET" request to "/related_dummies?complex_sub_query_filter=1"
+    Then the response status code should be 200
+    And the DQL should be equal to:
+    """
+    SELECT o, thirdLevel_a3, relatedToDummyFriend_a4, dummyFriend_a5
+    FROM ApiPlatform\Core\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
+    WHERE o.id IN (
+            SELECT related_dummy_a1.id
+            FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy related_dummy_a1
+            INNER JOIN related_dummy_a1.relatedToDummyFriend related_to_dummy_friend_a2
+            WITH related_to_dummy_friend_a2.name = :name_p1
+        )
+    ORDER BY o.id ASC
+    """
diff --git a/src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php b/src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php
@@ -14,6 +14,7 @@
 namespace ApiPlatform\Core\Bridge\Doctrine\Orm\Extension;
 
 use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\EagerLoadingTrait;
+use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryBuilderHelper;
 use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface;
 use ApiPlatform\Core\Exception\InvalidArgumentException;
 use ApiPlatform\Core\Exception\PropertyNotFoundException;
@@ -24,6 +25,7 @@
 use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
 use ApiPlatform\Core\Serializer\SerializerContextBuilderInterface;
 use Doctrine\ORM\Mapping\ClassMetadataInfo;
+use Doctrine\ORM\Query\Expr\Join;
 use Doctrine\ORM\QueryBuilder;
 use Symfony\Component\HttpFoundation\RequestStack;
 use Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactoryInterface;
@@ -190,16 +192,20 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt
                 continue;
             }
 
-            $isNullable = $mapping['joinColumns'][0]['nullable'] ?? true;
-            if (false !== $wasLeftJoin || true === $isNullable) {
-                $method = 'leftJoin';
+            $existingJoin = QueryBuilderHelper::getExistingJoin($queryBuilder, $parentAlias, $association);
+
+            if (null !== $existingJoin) {
+                $associationAlias = $existingJoin->getAlias();
+                $isLeftJoin = Join::LEFT_JOIN === $existingJoin->getJoinType();
             } else {
-                $method = 'innerJoin';
-            }
+                $isNullable = $mapping['joinColumns'][0]['nullable'] ?? true;
+                $isLeftJoin = false !== $wasLeftJoin || true === $isNullable;
+                $method = $isLeftJoin ? 'leftJoin' : 'innerJoin';
 
-            $associationAlias = $queryNameGenerator->generateJoinAlias($association);
-            $queryBuilder->{$method}(sprintf('%s.%s', $parentAlias, $association), $associationAlias);
-            ++$joinCount;
+                $associationAlias = $queryNameGenerator->generateJoinAlias($association);
+                $queryBuilder->{$method}(sprintf('%s.%s', $parentAlias, $association), $associationAlias);
+                ++$joinCount;
+            }
 
             if (true === $fetchPartial) {
                 try {
@@ -230,7 +236,7 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt
                 }
             }
 
-            $this->joinRelations($queryBuilder, $queryNameGenerator, $mapping['targetEntity'], $forceEager, $fetchPartial, $associationAlias, $options, $normalizationContext, 'leftJoin' === $method, $joinCount, $currentDepth);
+            $this->joinRelations($queryBuilder, $queryNameGenerator, $mapping['targetEntity'], $forceEager, $fetchPartial, $associationAlias, $options, $normalizationContext, $isLeftJoin, $joinCount, $currentDepth);
         }
     }
 

diff --git a/src/Bridge/Doctrine/Orm/Util/EagerLoadingTrait.php b/src/Bridge/Doctrine/Orm/Util/EagerLoadingTrait.php
@@ -13,7 +13,7 @@
 
 namespace ApiPlatform\Core\Bridge\Doctrine\Orm\Util;
 
-use Doctrine\ORM\EntityManager;
+use Doctrine\ORM\EntityManagerInterface;
 use Doctrine\ORM\Mapping\ClassMetadataInfo;
 
 /**
@@ -66,7 +66,7 @@ private function getBooleanOperationAttribute(string $resourceClass, array $opti
      *
      * @param array $checked array cache of tested metadata classes
      */
-    private function hasFetchEagerAssociation(EntityManager $em, ClassMetadataInfo $classMetadata, array &$checked = []): bool
+    private function hasFetchEagerAssociation(EntityManagerInterface $em, ClassMetadataInfo $classMetadata, array &$checked = []): bool
     {
         $checked[] = $classMetadata->name;
 

diff --git a/src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php b/src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php
@@ -160,7 +160,7 @@ public static function traverseJoins(string $alias, QueryBuilder $queryBuilder,
     /**
      * Gets the existing join from QueryBuilder DQL parts.
      */
-    private static function getExistingJoin(QueryBuilder $queryBuilder, string $alias, string $association, string $originAlias = null): ?Join
+    public static function getExistingJoin(QueryBuilder $queryBuilder, string $alias, string $association, string $originAlias = null): ?Join
     {
         $parts = $queryBuilder->getDQLPart('join');
         $rootAlias = $originAlias ?? $queryBuilder->getRootAliases()[0];

diff --git a/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml b/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml
@@ -131,7 +131,8 @@
             <argument type="service" id="serializer.mapping.class_metadata_factory" />
 
             <tag name="api_platform.doctrine.orm.query_extension.item" priority="-8" />
-            <tag name="api_platform.doctrine.orm.query_extension.collection" priority="-8" />
+            <!-- After filter_eager_loading -->
+            <tag name="api_platform.doctrine.orm.query_extension.collection" priority="-18" />
         </service>
         <service id="ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\EagerLoadingExtension" alias="api_platform.doctrine.orm.query_extension.eager_loading" />
 

diff --git a/tests/Behat/DoctrineContext.php b/tests/Behat/DoctrineContext.php
@@ -13,6 +13,7 @@
 
 namespace ApiPlatform\Core\Tests\Behat;
 
+use ApiPlatform\Core\Tests\Fixtures\TestBundle\Doctrine\Orm\EntityManager;
 use ApiPlatform\Core\Tests\Fixtures\TestBundle\Document\AbsoluteUrlDummy as AbsoluteUrlDummyDocument;
 use ApiPlatform\Core\Tests\Fixtures\TestBundle\Document\AbsoluteUrlRelationDummy as AbsoluteUrlRelationDummyDocument;
 use ApiPlatform\Core\Tests\Fixtures\TestBundle\Document\Address as AddressDocument;
@@ -214,6 +215,25 @@ public function createDatabase()
         $this->doctrine->getManager()->clear();
     }
 
+    /**
+     * @Then the DQL should be equal to:
+     */
+    public function theDqlShouldBeEqualTo(PyStringNode $dql)
+    {
+        /** @var EntityManager $manager */
+        $manager = $this->doctrine->getManager();
+
+        $actualDql = $manager::$dql;
+
+        $expectedDql = preg_replace('/\(\R */', '(', (string) $dql);
+        $expectedDql = preg_replace('/\R *\)/', ')', $expectedDql);
+        $expectedDql = preg_replace('/\R */', ' ', $expectedDql);
+
+        if ($expectedDql !== $actualDql) {
+            throw new \RuntimeException("The DQL:\n'$actualDql' is not equal to:\n'$expectedDql'");
+        }
+    }
+
     /**
      * @Given there are :nb dummy objects
      */

diff --git a/tests/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php b/tests/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php
@@ -35,6 +35,7 @@
 use Doctrine\ORM\EntityManager;
 use Doctrine\ORM\Mapping\ClassMetadata;
 use Doctrine\ORM\Mapping\ClassMetadataInfo;
+use Doctrine\ORM\Query\Expr\Join;
 use Doctrine\ORM\QueryBuilder;
 use PHPUnit\Framework\TestCase;
 use Prophecy\Argument;
@@ -124,6 +125,7 @@ public function testApplyToCollection()
         $queryBuilderProphecy->innerJoin('o.relatedDummy2', 'relatedDummy2_a2')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('partial relatedDummy_a1.{id,name,embeddedDummy.name}')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('partial relatedDummy2_a2.{id,name,embeddedDummy.name}')->shouldBeCalledTimes(1);
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([]);
 
         $queryBuilder = $queryBuilderProphecy->reveal();
         $eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30, false, null, null, true);
@@ -230,6 +232,7 @@ public function testApplyToItem()
         $queryBuilderProphecy->addSelect('partial relatedDummy3_a4.{id}')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('partial relatedDummy4_a5.{id}')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('singleInheritanceRelation_a6')->shouldBeCalledTimes(1);
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([]);
 
         $queryBuilder = $queryBuilderProphecy->reveal();
         $orderExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30, false, null, null, true);
@@ -377,6 +380,7 @@ public function testMaxJoinsReached()
 
         $queryBuilderProphecy->innerJoin(Argument::type('string'), Argument::type('string'))->shouldBeCalled();
         $queryBuilderProphecy->addSelect(Argument::type('string'))->shouldBeCalled();
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([]);
 
         $eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30, false, null, null, true);
         $eagerExtensionTest->applyToCollection($queryBuilderProphecy->reveal(), new QueryNameGenerator(), Dummy::class, null, ['groups' => ['foo']]);
@@ -445,6 +449,7 @@ public function testMaxDepth()
 
         $queryBuilderProphecy->innerJoin(Argument::type('string'), Argument::type('string'))->shouldBeCalledTimes(2);
         $queryBuilderProphecy->addSelect(Argument::type('string'))->shouldBeCalled();
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([]);
 
         $eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30, false, null, null, true, $classMetadataFactoryProphecy->reveal());
         $eagerExtensionTest->applyToCollection($queryBuilderProphecy->reveal(), new QueryNameGenerator(), Dummy::class);
@@ -486,6 +491,7 @@ public function testForceEager()
 
         $queryBuilderProphecy->innerJoin('o.relation', 'relation_a1')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('partial relation_a1.{id}')->shouldBeCalledTimes(1);
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([]);
 
         $queryBuilderProphecy->getRootAliases()->willReturn(['o']);
         $queryBuilderProphecy->getEntityManager()->willReturn($emProphecy);
@@ -602,6 +608,7 @@ public function testResourceClassNotFoundExceptionPropertyNameCollection()
         $queryBuilderProphecy->getRootAliases()->willReturn(['o']);
         $queryBuilderProphecy->getEntityManager()->willReturn($emProphecy);
         $queryBuilderProphecy->innerJoin('o.relation', 'relation_a1')->shouldBeCalledTimes(1);
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([]);
 
         $orderExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30, true, null, null, true);
         $orderExtensionTest->applyToItem($queryBuilderProphecy->reveal(), new QueryNameGenerator(), Dummy::class, []);
@@ -657,6 +664,7 @@ public function testApplyToCollectionWithSerializerContextBuilder()
 
         $queryBuilderProphecy->leftJoin('o.relatedDummy', 'relatedDummy_a1')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('partial relatedDummy_a1.{id,name}')->shouldBeCalledTimes(1);
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([]);
 
         $request = Request::create('/api/dummies', 'GET', []);
 
@@ -721,6 +729,7 @@ public function testAttributes()
 
         $queryBuilderProphecy->leftJoin('o.relatedDummy', 'relatedDummy_a1')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('partial relatedDummy_a1.{id,name}')->shouldBeCalledTimes(1);
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([]);
 
         $request = Request::create('/api/dummies', 'GET', []);
 
@@ -828,6 +837,7 @@ public function testOnlyOneRelationNotInAttributes()
 
         $queryBuilderProphecy->leftJoin('o.relatedDummy', 'relatedDummy_a1')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('partial relatedDummy_a1.{id,name}')->shouldBeCalledTimes(1);
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([]);
 
         $request = Request::create('/api/dummies', 'GET', []);
 
@@ -877,6 +887,7 @@ public function testApplyToCollectionNoPartial()
         $queryBuilderProphecy->innerJoin('o.relatedDummy2', 'relatedDummy2_a2')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('relatedDummy_a1')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('relatedDummy2_a2')->shouldBeCalledTimes(1);
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([]);
 
         $queryBuilder = $queryBuilderProphecy->reveal();
         $eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30);
@@ -933,12 +944,64 @@ private function doTestApplyToCollectionWithANonReadableButFetchEagerProperty(bo
         $queryBuilderProphecy->innerJoin('o.relatedDummy2', 'relatedDummy2_a2')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('relatedDummy_a1')->shouldBeCalledTimes(1);
         $queryBuilderProphecy->addSelect('relatedDummy2_a2')->shouldBeCalledTimes(1);
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([]);
 
         $queryBuilder = $queryBuilderProphecy->reveal();
         $eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30);
         $eagerExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), Dummy::class);
     }
 
+    /**
+     * @dataProvider provideExistingJoinCases
+     */
+    public function testApplyToCollectionWithExistingJoin(string $joinType): void
+    {
+        $context = ['groups' => ['foo']];
+        $callContext = ['serializer_groups' => ['foo']];
+        $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
+        $resourceMetadataFactoryProphecy->create(Dummy::class)->willReturn(new ResourceMetadata());
+
+        $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class);
+
+        $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class);
+        $relationPropertyMetadata = new PropertyMetadata();
+        $relationPropertyMetadata = $relationPropertyMetadata->withReadableLink(true);
+
+        $propertyMetadataFactoryProphecy->create(Dummy::class, 'relatedDummy', $callContext)->willReturn($relationPropertyMetadata)->shouldBeCalled();
+
+        $queryBuilderProphecy = $this->prophesize(QueryBuilder::class);
+
+        $classMetadataProphecy = $this->prophesize(ClassMetadata::class);
+        $classMetadataProphecy->associationMappings = [
+            'relatedDummy' => ['fetch' => ClassMetadataInfo::FETCH_EAGER, 'joinColumns' => [['nullable' => true]], 'targetEntity' => RelatedDummy::class],
+        ];
+
+        $relatedClassMetadataProphecy = $this->prophesize(ClassMetadata::class);
+
+        $emProphecy = $this->prophesize(EntityManager::class);
+        $emProphecy->getClassMetadata(Dummy::class)->shouldBeCalled()->willReturn($classMetadataProphecy->reveal());
+        $emProphecy->getClassMetadata(RelatedDummy::class)->shouldBeCalled()->willReturn($relatedClassMetadataProphecy->reveal());
+
+        $queryBuilderProphecy->getRootAliases()->willReturn(['o']);
+        $queryBuilderProphecy->getEntityManager()->willReturn($emProphecy);
+        $queryBuilderProphecy->getDQLPart('join')->willReturn([
+            'o' => [
+                new Join($joinType, 'o.relatedDummy', 'existing_join_alias'),
+            ],
+        ]);
+        $queryBuilderProphecy->addSelect('existing_join_alias')->shouldBeCalledTimes(1);
+
+        $queryBuilder = $queryBuilderProphecy->reveal();
+        $eagerExtensionTest = new EagerLoadingExtension($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $resourceMetadataFactoryProphecy->reveal(), 30, false);
+        $eagerExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), Dummy::class, null, $context);
+    }
+
+    public function provideExistingJoinCases(): iterable
+    {
+        yield [Join::LEFT_JOIN];
+        yield [Join::INNER_JOIN];
+    }
+
     public function testApplyToCollectionWithAReadableButNotFetchEagerProperty()
     {
         $this->doTestApplyToCollectionWithAReadableButNotFetchEagerProperty(false);