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 <jurij.c@paztir.com>
Co-authored-by: Fran Moreno <franmomu@gmail.com>

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

src/Util/SmartPaginatorFactory.php

@@ -0,0 +1,67 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the Sonata Project package.
+ *
+ * (c) Thomas Rabaix <thomas.rabaix@sonata-project.org>
+ *
+ * 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<string, mixed> $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;
+    }
+}

tests/Util/SmartPaginatorFactoryTest.php

@@ -0,0 +1,178 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the Sonata Project package.
+ *
+ * (c) Thomas Rabaix <thomas.rabaix@sonata-project.org>
+ *
+ * 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<array{QueryBuilder, bool}>
+     */
+    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<array{QueryBuilder, bool|null}>
+     */
+    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<array{QueryBuilder, bool, bool}>
+     */
+    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,
+        ];
+    }
+}