From 1806c484c9e47eb14f757622d2970792aa8306fe Mon Sep 17 00:00:00 2001 From: soyuka Date: Wed, 13 Mar 2019 12:05:19 +0100 Subject: [PATCH] Do not use DISTINCT where there are no joins Fix https://github.com/api-platform/core/issues/2552 --- .../Orm/Extension/PaginationExtension.php | 10 ++- .../Orm/Extension/PaginationExtensionTest.php | 61 +++++++++++++++---- 2 files changed, 57 insertions(+), 14 deletions(-) diff --git a/src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php b/src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php index 43af45e3780..7647d07a6e0 100644 --- a/src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php +++ b/src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php @@ -23,6 +23,7 @@ use ApiPlatform\Core\Metadata\Resource\ResourceMetadata; use Doctrine\Common\Persistence\ManagerRegistry; use Doctrine\ORM\QueryBuilder; +use Doctrine\ORM\Tools\Pagination\CountWalker; use Doctrine\ORM\Tools\Pagination\Paginator as DoctrineOrmPaginator; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\RequestStack; @@ -143,7 +144,14 @@ public function supportsResult(string $resourceClass, string $operationName = nu */ public function getResult(QueryBuilder $queryBuilder, string $resourceClass = null, string $operationName = null, array $context = []) { - $doctrineOrmPaginator = new DoctrineOrmPaginator($queryBuilder, $this->useFetchJoinCollection($queryBuilder, $resourceClass, $operationName)); + $query = $queryBuilder->getQuery(); + + // Only one alias, without joins, disable the DISTINCT on the COUNT + if (1 === \count($queryBuilder->getAllAliases())) { + $query->setHint(CountWalker::HINT_DISTINCT, false); + } + + $doctrineOrmPaginator = new DoctrineOrmPaginator($query, $this->useFetchJoinCollection($queryBuilder, $resourceClass, $operationName)); $doctrineOrmPaginator->setUseOutputWalkers($this->useOutputWalkers($queryBuilder)); if (null === $this->requestStack) { diff --git a/tests/Bridge/Doctrine/Orm/Extension/PaginationExtensionTest.php b/tests/Bridge/Doctrine/Orm/Extension/PaginationExtensionTest.php index a39e6f9ee6e..3dbc0cc7807 100644 --- a/tests/Bridge/Doctrine/Orm/Extension/PaginationExtensionTest.php +++ b/tests/Bridge/Doctrine/Orm/Extension/PaginationExtensionTest.php @@ -27,6 +27,7 @@ use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\Query; use Doctrine\ORM\QueryBuilder; +use Doctrine\ORM\Tools\Pagination\CountWalker; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Symfony\Component\HttpFoundation\Request; @@ -887,6 +888,35 @@ public function testGetResult() $this->assertInstanceOf(PaginatorInterface::class, $result); } + public function testGetResultWithoutDistinct() + { + $configuration = new Configuration(); + + $entityManagerProphecy = $this->prophesize(EntityManagerInterface::class); + $entityManagerProphecy->getConfiguration()->willReturn($configuration)->shouldBeCalled(); + + $query = new Query($entityManagerProphecy->reveal()); + $query->setFirstResult(0); + $query->setMaxResults(42); + + $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + $queryBuilderProphecy->getRootEntities()->willReturn([])->shouldBeCalled(); + $queryBuilderProphecy->getAllAliases()->willReturn(['o'])->shouldBeCalled(); + $queryBuilderProphecy->getQuery()->willReturn($query)->shouldBeCalled(); + $queryBuilderProphecy->getDQLPart(Argument::that(function ($arg) { + return \in_array($arg, ['having', 'orderBy', 'join'], true); + }))->willReturn('')->shouldBeCalled(); + $queryBuilderProphecy->getMaxResults()->willReturn(42)->shouldBeCalled(); + $queryBuilder = $queryBuilderProphecy->reveal(); + + $result = $this->getPaginationExtensionResult(false, false, true, $queryBuilder); + + $this->assertInstanceOf(PartialPaginatorInterface::class, $result); + $this->assertInstanceOf(PaginatorInterface::class, $result); + + $this->assertFalse($query->getHint(CountWalker::HINT_DISTINCT)); + } + /** * @group legacy * @expectedDeprecation Passing an instance of "Symfony\Component\HttpFoundation\RequestStack" as second argument of "ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\PaginationExtension" is deprecated since API Platform 2.4 and will not be possible anymore in API Platform 3. Pass an instance of "ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface" instead. @@ -963,7 +993,7 @@ public function testLegacySimpleGetResult() $this->assertInstanceOf(PaginatorInterface::class, $result); } - private function getPaginationExtensionResult(bool $partial = false, bool $legacy = false, bool $fetchJoinCollection = true) + private function getPaginationExtensionResult(bool $partial = false, bool $legacy = false, bool $fetchJoinCollection = true, QueryBuilder $queryBuilder = null) { $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); $resourceMetadataFactory = $resourceMetadataFactoryProphecy->reveal(); @@ -979,17 +1009,21 @@ private function getPaginationExtensionResult(bool $partial = false, bool $legac $entityManagerProphecy = $this->prophesize(EntityManagerInterface::class); $entityManagerProphecy->getConfiguration()->willReturn($configuration); - $query = new Query($entityManagerProphecy->reveal()); - $query->setFirstResult(0); - $query->setMaxResults(42); - - $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); - $queryBuilderProphecy->getRootEntities()->willReturn([]); - $queryBuilderProphecy->getQuery()->willReturn($query); - $queryBuilderProphecy->getDQLPart(Argument::that(function ($arg) { - return \in_array($arg, ['having', 'orderBy', 'join'], true); - }))->willReturn(''); - $queryBuilderProphecy->getMaxResults()->willReturn(42); + if (null === $queryBuilder) { + $query = new Query($entityManagerProphecy->reveal()); + $query->setFirstResult(0); + $query->setMaxResults(42); + + $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + $queryBuilderProphecy->getRootEntities()->willReturn([])->shouldBeCalled(); + $queryBuilderProphecy->getAllAliases()->willReturn([])->shouldBeCalled(); + $queryBuilderProphecy->getQuery()->willReturn($query)->shouldBeCalled(); + $queryBuilderProphecy->getDQLPart(Argument::that(function ($arg) { + return \in_array($arg, ['having', 'orderBy', 'join'], true); + }))->willReturn('')->shouldBeCalled(); + $queryBuilderProphecy->getMaxResults()->willReturn(42)->shouldBeCalled(); + $queryBuilder = $queryBuilderProphecy->reveal(); + } $paginationExtension = new PaginationExtension( $this->prophesize(ManagerRegistry::class)->reveal(), @@ -997,7 +1031,7 @@ private function getPaginationExtensionResult(bool $partial = false, bool $legac $pagination ); - $args = [$queryBuilderProphecy->reveal(), null, null, ['filters' => ['partial' => $partial]]]; + $args = [$queryBuilder, null, null, ['filters' => ['partial' => $partial]]]; if (!$legacy) { $args[1] = 'Foo'; @@ -1029,6 +1063,7 @@ private function getLegacyPaginationExtensionResult(bool $partial = false, bool $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); $queryBuilderProphecy->getRootEntities()->willReturn([])->shouldBeCalled(); + $queryBuilderProphecy->getAllAliases()->willReturn([])->shouldBeCalled(); $queryBuilderProphecy->getQuery()->willReturn($query)->shouldBeCalled(); $queryBuilderProphecy->getDQLPart(Argument::that(function ($arg) { return \in_array($arg, ['having', 'orderBy', 'join'], true);