From 107c09604a699d65b4e92cbac31c6b7c5ed46a9a Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Thu, 6 Jan 2022 19:01:00 +0100
Subject: [PATCH] Fixed performance issues on list tags endpoint when
 requesting it with stats

---
 module/Core/src/Repository/TagRepository.php  | 59 ++++++++++++++-----
 .../test-db/Repository/TagRepositoryTest.php  |  4 +-
 2 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php
index 66b94a33..ab440879 100644
--- a/module/Core/src/Repository/TagRepository.php
+++ b/module/Core/src/Repository/TagRepository.php
@@ -4,6 +4,8 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Core\Repository;
 
+use Doctrine\DBAL\Types\Types;
+use Doctrine\ORM\Query\ResultSetMappingBuilder;
 use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository;
 use Happyr\DoctrineSpecification\Spec;
 use Shlinkio\Shlink\Core\Entity\Tag;
@@ -15,6 +17,8 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey;
 
 use function Functional\map;
 
+use const PHP_INT_MAX;
+
 class TagRepository extends EntitySpecificationRepository implements TagRepositoryInterface
 {
     public function deleteByName(array $names): int
@@ -35,29 +39,52 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
      */
     public function findTagsWithInfo(?TagsListFiltering $filtering = null): array
     {
-        $qb = $this->createQueryBuilder('t');
-        $qb->select('t AS tag', 'COUNT(DISTINCT s.id) AS shortUrlsCount', 'COUNT(DISTINCT v.id) AS visitsCount')
-           ->leftJoin('t.shortUrls', 's')
-           ->leftJoin('s.visits', 'v')
-           ->groupBy('t')
-           ->orderBy('t.name', 'ASC')
-           ->setMaxResults($filtering?->limit())
-           ->setFirstResult($filtering?->offset());
+        $subQb = $this->createQueryBuilder('t');
+        $subQb->select('t.id', 't.name')
+              ->orderBy('t.name', 'ASC') // TODO Make dynamic
+              ->setMaxResults($filtering?->limit() ?? PHP_INT_MAX)
+              ->setFirstResult($filtering?->offset() ?? 0);
 
         $searchTerm = $filtering?->searchTerm();
         if ($searchTerm !== null) {
-            $qb->andWhere($qb->expr()->like('t.name', ':searchPattern'))
-               ->setParameter('searchPattern', '%' . $searchTerm . '%');
+            // FIXME This value cannot be added via params, so it needs to be sanitized
+            $subQb->andWhere($subQb->expr()->like('t.name', '\'%' . $searchTerm . '%\''));
         }
 
-        $apiKey = $filtering?->apiKey();
-        if ($apiKey !== null) {
-            $this->applySpecification($qb, $apiKey->spec(false, 'shortUrls'), 't');
-        }
+        $subQuery = $subQb->getQuery()->getSQL();
+
+        // A native query builder needs to be used here because DQL and ORM query builders do not accept
+        // sub-queries at "from" and "join" level.
+        // If no sub-query is used, the whole list is loaded even with pagination, making it very inefficient.
+        $nativeQb = $this->getEntityManager()->getConnection()->createQueryBuilder();
+        $nativeQb
+            ->select(
+                't.id_0 AS id',
+                't.name_1 AS name',
+                'COUNT(DISTINCT s.id) AS short_urls_count',
+                'COUNT(DISTINCT v.id) AS visits_count',
+            )
+            ->from('(' . $subQuery . ')', 't')
+            ->leftJoin('t', 'short_urls_in_tags', 'st', $nativeQb->expr()->eq('t.id_0', 'st.tag_id'))
+            ->leftJoin('st', 'short_urls', 's', $nativeQb->expr()->eq('s.id', 'st.short_url_id'))
+            ->leftJoin('st', 'visits', 'v', $nativeQb->expr()->eq('s.id', 'v.short_url_id'))
+            ->groupBy('t.id_0', 't.name_1')
+            ->orderBy('t.name_1', 'ASC'); // TODO Make dynamic
+
+        $rsm = new ResultSetMappingBuilder($this->getEntityManager());
+        $rsm->addRootEntityFromClassMetadata(Tag::class, 't');
+        $rsm->addScalarResult('short_urls_count', 'shortUrlsCount');
+        $rsm->addScalarResult('visits_count', 'visitsCount');
+
+        // TODO Apply API key cond to main query
+//        $apiKey = $filtering?->apiKey();
+//        if ($apiKey !== null) {
+//            $this->applySpecification($nativeQb, $apiKey->spec(false, 'shortUrls'), 't');
+//        }
 
         return map(
-            $qb->getQuery()->getResult(),
-            static fn (array $row) => new TagInfo($row['tag'], (int) $row['shortUrlsCount'], (int) $row['visitsCount']),
+            $this->getEntityManager()->createNativeQuery($nativeQb->getSQL(), $rsm)->getResult(),
+            static fn (array $row) => new TagInfo($row[0], (int) $row['shortUrlsCount'], (int) $row['visitsCount']),
         );
     }
 
diff --git a/module/Core/test-db/Repository/TagRepositoryTest.php b/module/Core/test-db/Repository/TagRepositoryTest.php
index d8635b96..ab4e07ad 100644
--- a/module/Core/test-db/Repository/TagRepositoryTest.php
+++ b/module/Core/test-db/Repository/TagRepositoryTest.php
@@ -54,7 +54,7 @@ class TagRepositoryTest extends DatabaseTestCase
 
     /**
      * @test
-     * @dataProvider provideFilterings
+     * @dataProvider provideFilters
      */
     public function properTagsInfoIsReturned(?TagsListFiltering $filtering, callable $asserts): void
     {
@@ -84,7 +84,7 @@ class TagRepositoryTest extends DatabaseTestCase
         $asserts($result, $names);
     }
 
-    public function provideFilterings(): iterable
+    public function provideFilters(): iterable
     {
         $noFiltersAsserts = static function (array $result, array $tagNames): void {
             /** @var TagInfo[] $result */