Hotfix/filter eager loading fetch eager (#1028)

soyuka · dunglas · commit 18c1a27172d7 · 2017-04-07T12:20:07.000+02:00
* Removed unneeded catch on ResourceClassNotFound

* fix FilterEagerLoading when entity has FETCH_EAGER with no force eager
diff --git a/src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php b/src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php
@@ -61,13 +61,9 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator
 
         $forceEager = $this->isForceEager($resourceClass, $options);
 
-        try {
-            $groups = $this->getSerializerGroups($resourceClass, $options, 'normalization_context');
+        $groups = $this->getSerializerGroups($resourceClass, $options, 'normalization_context');
 
-            $this->joinRelations($queryBuilder, $queryNameGenerator, $resourceClass, $forceEager, $queryBuilder->getRootAliases()[0], $groups);
-        } catch (ResourceClassNotFoundException $resourceClassNotFoundException) {
-            //ignore the not found exception
-        }
+        $this->joinRelations($queryBuilder, $queryNameGenerator, $resourceClass, $forceEager, $queryBuilder->getRootAliases()[0], $groups);
     }
 
     /**
diff --git a/src/Bridge/Doctrine/Orm/Extension/FilterEagerLoadingExtension.php b/src/Bridge/Doctrine/Orm/Extension/FilterEagerLoadingExtension.php
@@ -15,6 +15,8 @@
 
 use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface;
 use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
+use Doctrine\ORM\EntityManager;
+use Doctrine\ORM\Mapping\ClassMetadataInfo;
 use Doctrine\ORM\Query\Expr\Join;
 use Doctrine\ORM\QueryBuilder;
 
@@ -38,7 +40,10 @@ public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFa
      */
     public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, string $operationName = null)
     {
-        if (false === $this->forceEager || false === $this->isForceEager($resourceClass, ['collection_operation_name' => $operationName])) {
+        $em = $queryBuilder->getEntityManager();
+        $classMetadata = $em->getClassMetadata($resourceClass);
+
+        if (!$this->hasFetchEagerAssociation($em, $classMetadata) && (false === $this->forceEager || false === $this->isForceEager($resourceClass, ['collection_operation_name' => $operationName]))) {
             return;
         }
 
@@ -59,8 +64,6 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator
         $queryBuilderClone = clone $queryBuilder;
         $queryBuilderClone->resetDQLPart('where');
 
-        $classMetadata = $queryBuilder->getEntityManager()->getClassMetadata($resourceClass);
-
         if (!$classMetadata->isIdentifierComposite) {
             $replacementAlias = $queryNameGenerator->generateJoinAlias($originAlias);
             $in = $this->getQueryBuilderWithNewAliases($queryBuilder, $queryNameGenerator, $originAlias, $replacementAlias);
@@ -145,4 +148,27 @@ private function isForceEager(string $resourceClass, array $options): bool
 
         return is_bool($forceEager) ? $forceEager : $this->forceEager;
     }
+
+    private function hasFetchEagerAssociation(EntityManager $em, ClassMetadataInfo $classMetadata, &$checked = [])
+    {
+        $checked[] = $classMetadata->name;
+
+        foreach ($classMetadata->associationMappings as $mapping) {
+            if (ClassMetadataInfo::FETCH_EAGER === $mapping['fetch']) {
+                return true;
+            }
+
+            $related = $em->getClassMetadata($mapping['targetEntity']);
+
+            if (in_array($related->name, $checked)) {
+                continue;
+            }
+
+            if (true === $this->hasFetchEagerAssociation($em, $related, $checked)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
 }
diff --git a/tests/Bridge/Doctrine/Orm/Extension/FilterEagerLoadingExtensionTest.php b/tests/Bridge/Doctrine/Orm/Extension/FilterEagerLoadingExtensionTest.php
@@ -17,6 +17,8 @@
 use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface;
 use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
 use ApiPlatform\Core\Metadata\Resource\ResourceMetadata;
+use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeItem;
+use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeLabel;
 use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation;
 use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar;
 use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Foo;
@@ -39,8 +41,12 @@ public function testIsNoForceEagerCollectionAttributes()
             ],
         ], null));
 
+        $em = $this->prophesize(EntityManager::class);
+        $em->getClassMetadata(DummyCar::class)->shouldBeCalled()->willReturn(new ClassMetadataInfo(DummyCar::class));
+
         $qb = $this->prophesize(QueryBuilder::class);
         $qb->getDQLPart('where')->shouldNotBeCalled();
+        $qb->getEntityManager()->willReturn($em);
 
         $queryNameGenerator = $this->prophesize(QueryNameGeneratorInterface::class);
 
@@ -50,13 +56,17 @@ public function testIsNoForceEagerCollectionAttributes()
 
     public function testIsNoForceEagerResource()
     {
+        $em = $this->prophesize(EntityManager::class);
+        $em->getClassMetadata(DummyCar::class)->shouldBeCalled()->willReturn(new ClassMetadataInfo(DummyCar::class));
+
         $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
         $resourceMetadataFactoryProphecy->create(DummyCar::class)->willReturn(new ResourceMetadata(DummyCar::class, null, null, null, [
             'get' => [],
         ], ['force_eager' => false]));
 
         $qb = $this->prophesize(QueryBuilder::class);
         $qb->getDQLPart('where')->shouldNotBeCalled();
+        $qb->getEntityManager()->willReturn($em);
 
         $queryNameGenerator = $this->prophesize(QueryNameGeneratorInterface::class);
 
@@ -66,13 +76,17 @@ public function testIsNoForceEagerResource()
 
     public function testIsForceEagerConfig()
     {
+        $em = $this->prophesize(EntityManager::class);
+        $em->getClassMetadata(DummyCar::class)->shouldBeCalled()->willReturn(new ClassMetadataInfo(DummyCar::class));
+
         $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
         $resourceMetadataFactoryProphecy->create(DummyCar::class)->willReturn(new ResourceMetadata(DummyCar::class, null, null, null, [
             'get' => [],
         ]));
 
         $qb = $this->prophesize(QueryBuilder::class);
         $qb->getDQLPart('where')->shouldNotBeCalled();
+        $qb->getEntityManager()->willReturn($em);
 
         $queryNameGenerator = $this->prophesize(QueryNameGeneratorInterface::class);
 
@@ -82,11 +96,15 @@ public function testIsForceEagerConfig()
 
     public function testHasNoWherePart()
     {
+        $em = $this->prophesize(EntityManager::class);
+        $em->getClassMetadata(DummyCar::class)->shouldBeCalled()->willReturn(new ClassMetadataInfo(DummyCar::class));
+
         $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
         $resourceMetadataFactoryProphecy->create(DummyCar::class)->willReturn(new ResourceMetadata(DummyCar::class));
 
         $qb = $this->prophesize(QueryBuilder::class);
         $qb->getDQLPart('where')->shouldBeCalled()->willReturn(null);
+        $qb->getEntityManager()->willReturn($em);
 
         $queryNameGenerator = $this->prophesize(QueryNameGeneratorInterface::class);
 
@@ -96,12 +114,16 @@ public function testHasNoWherePart()
 
     public function testHasNoJoinPart()
     {
+        $em = $this->prophesize(EntityManager::class);
+        $em->getClassMetadata(DummyCar::class)->shouldBeCalled()->willReturn(new ClassMetadataInfo(DummyCar::class));
+
         $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
         $resourceMetadataFactoryProphecy->create(DummyCar::class)->willReturn(new ResourceMetadata(DummyCar::class));
 
         $qb = $this->prophesize(QueryBuilder::class);
         $qb->getDQLPart('where')->shouldBeCalled()->willReturn(new Expr\Andx());
         $qb->getDQLPart('join')->shouldBeCalled()->willReturn(null);
+        $qb->getEntityManager()->willReturn($em);
 
         $queryNameGenerator = $this->prophesize(QueryNameGeneratorInterface::class);
 
@@ -164,12 +186,12 @@ public function testHiddenOrderBy()
         $filterEagerLoadingExtension->applyToCollection($qb, $queryNameGenerator->reveal(), DummyCar::class, 'get');
 
         $expected = <<<SQL
-SELECT o, CASE WHEN o.dateCreated IS NULL THEN 0 ELSE 1 END AS HIDDEN _o_dateCreated_null_rank 
-FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o 
-LEFT JOIN o.colors colors 
+SELECT o, CASE WHEN o.dateCreated IS NULL THEN 0 ELSE 1 END AS HIDDEN _o_dateCreated_null_rank
+FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o
+LEFT JOIN o.colors colors
 WHERE o IN(
-  SELECT o_2 FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o_2 
-  LEFT JOIN o_2.colors colors_2 
+  SELECT o_2 FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o_2
+  LEFT JOIN o_2.colors colors_2
   WHERE o_2.colors = :foo
 ) ORDER BY _o_dateCreated_null_rank DESC ASC
 SQL;
@@ -204,15 +226,15 @@ public function testGroupBy()
         $filterEagerLoadingExtension->applyToCollection($qb, $queryNameGenerator->reveal(), DummyCar::class, 'get');
 
         $expected = <<<SQL
-SELECT o, count(o.id) as counter 
-FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o 
-LEFT JOIN o.colors colors WHERE o 
+SELECT o, count(o.id) as counter
+FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o
+LEFT JOIN o.colors colors WHERE o
 IN(
-  SELECT o_2 FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o_2 
-  LEFT JOIN o_2.colors colors_2 
+  SELECT o_2 FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o_2
+  LEFT JOIN o_2.colors colors_2
   WHERE o_2.colors = :foo
-) 
-GROUP BY o.colors HAVING counter > 3 
+)
+GROUP BY o.colors HAVING counter > 3
 ORDER BY o.colors ASC
 SQL;
 
@@ -250,19 +272,74 @@ public function testCompositeIdentifiers()
         $filterEagerLoadingExtension->applyToCollection($qb, $queryNameGenerator->reveal(), CompositeRelation::class, 'get');
 
         $expected = <<<SQL
-SELECT o 
-FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o 
-INNER JOIN o.compositeItem item 
-INNER JOIN o.compositeLabel label 
+SELECT o
+FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o
+INNER JOIN o.compositeItem item
+INNER JOIN o.compositeLabel label
+WHERE o.item IN(
+    SELECT IDENTITY(o_2.item) FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o_2
+    INNER JOIN o_2.compositeItem item_2
+    INNER JOIN o_2.compositeLabel label_2
+    WHERE item_2.field1 = :foo
+) AND o.label IN(
+    SELECT IDENTITY(o_2.label) FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o_2
+    INNER JOIN o_2.compositeItem item_2
+    INNER JOIN o_2.compositeLabel label_2
+    WHERE item_2.field1 = :foo
+)
+SQL;
+
+        $this->assertEquals($this->toDQLString($expected), $qb->getDQL());
+    }
+
+    public function testFetchEagerWithNoForceEager()
+    {
+        $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
+        $resourceMetadataFactoryProphecy->create(CompositeRelation::class)->willReturn(new ResourceMetadata(CompositeRelation::class));
+
+        $classMetadata = new ClassMetadataInfo(CompositeRelation::class);
+        $classMetadata->isIdentifierComposite = true;
+        $classMetadata->identifier = ['item', 'label'];
+        $classMetadata->associationMappings = [
+            'item' => ['fetch' => 3, 'joinColumns' => [['nullable' => false]], 'targetEntity' => CompositeItem::class],
+            'label' => ['fetch' => 3, 'joinColumns' => [['nullable' => false]], 'targetEntity' => CompositeLabel::class],
+        ];
+
+        $em = $this->prophesize(EntityManager::class);
+        $em->getExpressionBuilder()->shouldBeCalled()->willReturn(new Expr());
+        $em->getClassMetadata(CompositeRelation::class)->shouldBeCalled()->willReturn($classMetadata);
+
+        $qb = new QueryBuilder($em->reveal());
+
+        $qb->select('o')
+            ->from(CompositeRelation::class, 'o')
+            ->innerJoin('o.compositeItem', 'item')
+            ->innerJoin('o.compositeLabel', 'label')
+            ->where('item.field1 = :foo')
+            ->setParameter('foo', 1);
+
+        $queryNameGenerator = $this->prophesize(QueryNameGeneratorInterface::class);
+        $queryNameGenerator->generateJoinAlias('item')->shouldBeCalled()->willReturn('item_2');
+        $queryNameGenerator->generateJoinAlias('label')->shouldBeCalled()->willReturn('label_2');
+        $queryNameGenerator->generateJoinAlias('o')->shouldBeCalled()->willReturn('o_2');
+
+        $filterEagerLoadingExtension = new FilterEagerLoadingExtension($resourceMetadataFactoryProphecy->reveal(), false);
+        $filterEagerLoadingExtension->applyToCollection($qb, $queryNameGenerator->reveal(), CompositeRelation::class, 'get');
+
+        $expected = <<<SQL
+SELECT o
+FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o
+INNER JOIN o.compositeItem item
+INNER JOIN o.compositeLabel label
 WHERE o.item IN(
-    SELECT IDENTITY(o_2.item) FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o_2 
-    INNER JOIN o_2.compositeItem item_2 
-    INNER JOIN o_2.compositeLabel label_2 
+    SELECT IDENTITY(o_2.item) FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o_2
+    INNER JOIN o_2.compositeItem item_2
+    INNER JOIN o_2.compositeLabel label_2
     WHERE item_2.field1 = :foo
 ) AND o.label IN(
-    SELECT IDENTITY(o_2.label) FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o_2 
-    INNER JOIN o_2.compositeItem item_2 
-    INNER JOIN o_2.compositeLabel label_2 
+    SELECT IDENTITY(o_2.label) FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o_2
+    INNER JOIN o_2.compositeItem item_2
+    INNER JOIN o_2.compositeLabel label_2
     WHERE item_2.field1 = :foo
 )
 SQL;
@@ -272,6 +349,6 @@ public function testCompositeIdentifiers()
 
     private function toDQLString(string $dql): string
     {
-        return preg_replace('/\\r\\n|\\n/', '', str_replace('  ', '', $dql));
+        return preg_replace(['/\s+/', '/\(\s/', '/\s\)/'], [' ', '(', ')'], $dql);
     }
 }
