From 7e39540de1090099d63354a8f9264a657c0a6243 Mon Sep 17 00:00:00 2001 From: alfabetagama Date: Wed, 19 May 2021 16:04:30 +0200 Subject: [PATCH] Implement sensible defaults for Paginator (#1444) * Set Paginator to not use fetchJoin if query has single primary key and no joins Set CountWalker::HINT_DISTINCT to false if there are no joins in query Set useOutputWalkers to false for simple queries without joins, having clause and single primary key * CS fixes * Use fetch join only when we have single primary key and joins Cs fixes * CS fix * CS Fix * Add SmartPaginatorFactory Co-authored-by: jure Co-authored-by: Fran Moreno --- src/Datagrid/ProxyQuery.php | 25 +--- src/Util/SmartPaginatorFactory.php | 67 +++++++++ tests/Util/SmartPaginatorFactoryTest.php | 178 +++++++++++++++++++++++ 3 files changed, 253 insertions(+), 17 deletions(-) create mode 100644 src/Util/SmartPaginatorFactory.php create mode 100644 tests/Util/SmartPaginatorFactoryTest.php diff --git a/src/Datagrid/ProxyQuery.php b/src/Datagrid/ProxyQuery.php index 3732eb04d..26b9a90be 100644 --- a/src/Datagrid/ProxyQuery.php +++ b/src/Datagrid/ProxyQuery.php @@ -18,7 +18,7 @@ use Doctrine\ORM\EntityManager; use Doctrine\ORM\Query; use Doctrine\ORM\QueryBuilder; -use Doctrine\ORM\Tools\Pagination\Paginator; +use Sonata\DoctrineORMAdminBundle\Util\SmartPaginatorFactory; /** * This class try to unify the query usage with Doctrine. @@ -200,27 +200,18 @@ public function execute(array $params = [], $hydrationMode = null) ), \E_USER_DEPRECATED); } - $query = $this->getDoctrineQuery(); - - foreach ($this->hints as $name => $value) { - $query->setHint($name, $value); - } - // NEXT_MAJOR: Remove this. if (\func_num_args() > 0) { + $query = $this->getDoctrineQuery(); + + foreach ($this->hints as $name => $value) { + $query->setHint($name, $value); + } + return $query->execute($params, $hydrationMode); } - $identifierFieldNames = $this - ->getQueryBuilder() - ->getEntityManager() - ->getMetadataFactory() - ->getMetadataFor(current($this->getQueryBuilder()->getRootEntities())) - ->getIdentifierFieldNames(); - - // Paginator with fetchJoinCollection doesn't work with composite primary keys - // https://github.com/doctrine/orm/issues/2910 - return new Paginator($query, 1 === \count($identifierFieldNames)); + return SmartPaginatorFactory::create($this, $this->hints); } /** diff --git a/src/Util/SmartPaginatorFactory.php b/src/Util/SmartPaginatorFactory.php new file mode 100644 index 000000000..71b6e7918 --- /dev/null +++ b/src/Util/SmartPaginatorFactory.php @@ -0,0 +1,67 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Sonata\DoctrineORMAdminBundle\Util; + +use Doctrine\ORM\Tools\Pagination\CountWalker; +use Doctrine\ORM\Tools\Pagination\Paginator; +use Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQuery; +use Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQueryInterface; + +/** + * @internal + */ +final class SmartPaginatorFactory +{ + /** + * NEXT_MAJOR: Replace ProxyQuery by ProxyQueryInterface. + * + * @param array $hints + */ + public static function create(ProxyQuery $proxyQuery, array $hints = []): Paginator + { + $queryBuilder = $proxyQuery->getQueryBuilder(); + + $identifierFieldNames = $queryBuilder + ->getEntityManager() + ->getClassMetadata(current($queryBuilder->getRootEntities())) + ->getIdentifierFieldNames(); + + $hasSingleIdentifierName = 1 === \count($identifierFieldNames); + $hasJoins = \count($queryBuilder->getDQLPart('join')) > 0; + + $query = $proxyQuery->getDoctrineQuery(); + + if (!$hasJoins) { + $query->setHint(CountWalker::HINT_DISTINCT, false); + } + + foreach ($hints as $name => $value) { + $query->setHint($name, $value); + } + + // Paginator with fetchJoinCollection doesn't work with composite primary keys + // https://github.com/doctrine/orm/issues/2910 + // To stay safe fetch join only when we have single primary key and joins + $paginator = new Paginator($query, $hasSingleIdentifierName && $hasJoins); + + $hasHavingPart = null !== $queryBuilder->getDQLPart('having'); + + // it is only safe to disable output walkers for really simple queries + if (!$hasHavingPart && !$hasJoins && $hasSingleIdentifierName) { + $paginator->setUseOutputWalkers(false); + } + + return $paginator; + } +} diff --git a/tests/Util/SmartPaginatorFactoryTest.php b/tests/Util/SmartPaginatorFactoryTest.php new file mode 100644 index 000000000..65ea28189 --- /dev/null +++ b/tests/Util/SmartPaginatorFactoryTest.php @@ -0,0 +1,178 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Sonata\DoctrineORMAdminBundle\Tests\Util; + +use Doctrine\ORM\QueryBuilder; +use Doctrine\ORM\Tools\Pagination\CountWalker; +use PHPUnit\Framework\TestCase; +use Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQuery; +use Sonata\DoctrineORMAdminBundle\Tests\App\Entity\Author; +use Sonata\DoctrineORMAdminBundle\Tests\App\Entity\Item; +use Sonata\DoctrineORMAdminBundle\Util\SmartPaginatorFactory; +use Symfony\Bridge\Doctrine\Test\DoctrineTestHelper; + +final class SmartPaginatorFactoryTest extends TestCase +{ + /** + * @dataProvider getQueriesForFetchJoinedCollection + */ + public function testFetchJoinedCollection(QueryBuilder $queryBuilder, bool $expected): void + { + $proxyQuery = $this->createStub(ProxyQuery::class); + $proxyQuery + ->method('getQueryBuilder') + ->willReturn($queryBuilder); + + $proxyQuery + ->method('getDoctrineQuery') + ->willReturn($queryBuilder->getQuery()); + + $paginator = SmartPaginatorFactory::create($proxyQuery); + + $this->assertSame($expected, $paginator->getFetchJoinCollection()); + } + + /** + * @phpstan-return iterable + */ + public function getQueriesForFetchJoinedCollection(): iterable + { + yield 'Without joins' => [ + DoctrineTestHelper::createTestEntityManager() + ->createQueryBuilder() + ->from(Author::class, 'author'), + false, + ]; + + yield 'With joins and simple identifier' => [ + DoctrineTestHelper::createTestEntityManager() + ->createQueryBuilder() + ->from(Author::class, 'author') + ->leftJoin('author.books', 'book'), + true, + ]; + + yield 'With joins and composite identifier' => [ + DoctrineTestHelper::createTestEntityManager() + ->createQueryBuilder() + ->from(Item::class, 'item') + ->leftJoin('item.product', 'product'), + false, + ]; + } + + /** + * @dataProvider getQueriesForOutputWalker + * + * @param bool|null $expected + */ + public function testUseOutputWalker(QueryBuilder $queryBuilder, $expected): void + { + $proxyQuery = $this->createStub(ProxyQuery::class); + $proxyQuery + ->method('getQueryBuilder') + ->willReturn($queryBuilder); + + $proxyQuery + ->method('getDoctrineQuery') + ->willReturn($queryBuilder->getQuery()); + + $paginator = SmartPaginatorFactory::create($proxyQuery); + + $this->assertSame($expected, $paginator->getUseOutputWalkers()); + } + + /** + * @phpstan-return iterable + */ + public function getQueriesForOutputWalker(): iterable + { + yield 'Simple query without joins' => [ + DoctrineTestHelper::createTestEntityManager() + ->createQueryBuilder() + ->from(Author::class, 'author'), + false, + ]; + + yield 'Simple query with having' => [ + DoctrineTestHelper::createTestEntityManager() + ->createQueryBuilder() + ->from(Author::class, 'author') + ->groupBy('author.name') + ->having('COUNT(author.id) > 0'), + null, + ]; + + yield 'With joins and simple identifier' => [ + DoctrineTestHelper::createTestEntityManager() + ->createQueryBuilder() + ->from(Author::class, 'author') + ->leftJoin('author.books', 'book'), + null, + ]; + + yield 'With joins and composite identifier' => [ + DoctrineTestHelper::createTestEntityManager() + ->createQueryBuilder() + ->from(Item::class, 'item') + ->leftJoin('item.product', 'product'), + null, + ]; + } + + /** + * @dataProvider getQueriesForCountWalkerDistinct + */ + public function testCountWalkerDistinct(QueryBuilder $queryBuilder, bool $hasHint, bool $expected): void + { + $proxyQuery = $this->createStub(ProxyQuery::class); + $proxyQuery + ->method('getQueryBuilder') + ->willReturn($queryBuilder); + + $query = $queryBuilder->getQuery(); + + $proxyQuery + ->method('getDoctrineQuery') + ->willReturn($query); + + $paginator = SmartPaginatorFactory::create($proxyQuery); + + $this->assertSame($hasHint, $query->hasHint(CountWalker::HINT_DISTINCT)); + $this->assertSame($expected, $query->getHint(CountWalker::HINT_DISTINCT)); + } + + /** + * @phpstan-return iterable + */ + public function getQueriesForCountWalkerDistinct(): iterable + { + yield 'Simple query without joins' => [ + DoctrineTestHelper::createTestEntityManager() + ->createQueryBuilder() + ->from(Author::class, 'author'), + true, + false, + ]; + + yield 'With joins and simple identifier' => [ + DoctrineTestHelper::createTestEntityManager() + ->createQueryBuilder() + ->from(Author::class, 'author') + ->leftJoin('author.books', 'book'), + false, + false, + ]; + } +}