Improved performance when loading paginated tags, by using an ugly compound of native queries and DQL

CHANGELOG.md

@@ -12,11 +12,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 
     The `short-urls:list` command now accepts a `-i`/`--including-all-tags` flag which behaves the same.
 
-* [#1273](https://github.com/shlinkio/shlink/issues/1273) Added support for pagination in tags lists.
+* [#1273](https://github.com/shlinkio/shlink/issues/1273) Added support for pagination in tags lists, allowing to improve performance by loading subsets of tags.
 
     For backwards compatibility, lists continue returning all items by default, but the `GET /tags` endpoint now supports `page` and `itemsPerPage` query params, to make sure only a subset of the tags is returned.
 
-    This is supported both when invoking the endpoint with and without `withStats=true`.
+    This is supported both when invoking the endpoint with and without `withStats=true` query param.
 
     Additionally, the endpoint also supports filtering by `searchTerm` query param. When provided, only tags matching it will be returned.
 

module/Core/src/Repository/TagRepository.php

@@ -4,7 +4,6 @@ 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;
@@ -12,15 +11,24 @@ use Shlinkio\Shlink\Core\Entity\Tag;
 use Shlinkio\Shlink\Core\Tag\Model\TagInfo;
 use Shlinkio\Shlink\Core\Tag\Model\TagsListFiltering;
 use Shlinkio\Shlink\Core\Tag\Spec\CountTagsWithName;
+use Shlinkio\Shlink\Rest\ApiKey\Role;
 use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
 
 use function Functional\map;
+use function is_object;
+use function method_exists;
+use function sprintf;
+use function strlen;
+use function strpos;
+use function substr_replace;
 
 use const PHP_INT_MAX;
 
 class TagRepository extends EntitySpecificationRepository implements TagRepositoryInterface
 {
+    private const PARAM_PLACEHOLDER = '?';
+
     public function deleteByName(array $names): int
     {
         if (empty($names)) {
@@ -39,6 +47,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
      */
     public function findTagsWithInfo(?TagsListFiltering $filtering = null): array
     {
+        $conn = $this->getEntityManager()->getConnection();
         $subQb = $this->createQueryBuilder('t');
         $subQb->select('t.id', 't.name')
               ->orderBy('t.name', 'ASC') // TODO Make dynamic
@@ -47,16 +56,34 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
 
         $searchTerm = $filtering?->searchTerm();
         if ($searchTerm !== null) {
-            // FIXME This value cannot be added via params, so it needs to be sanitized
-            $subQb->andWhere($subQb->expr()->like('t.name', '\'%' . $searchTerm . '%\''));
+            $subQb->andWhere($subQb->expr()->like('t.name', $conn->quote('%' . $searchTerm . '%')));
         }
 
-        $subQuery = $subQb->getQuery()->getSQL();
+        $apiKey = $filtering?->apiKey();
+        if ($apiKey !== null) {
+            $this->applySpecification($subQb, $apiKey->spec(false, 'shortUrls'), 't');
+        }
 
-        // A native query builder needs to be used here because DQL and ORM query builders do not accept
+        $subQuery = $subQb->getQuery();
+        $subQuerySql = $subQuery->getSQL();
+
+        // Sadly, we need to manually interpolate the params in the query replacing the placeholders, as this is going
+        // to be used as a sub-query in a native query. There's no need to sanitize, though.
+        foreach ($subQuery->getParameters() as $param) {
+            $value = $param->getValue();
+            $pos = strpos($subQuerySql, self::PARAM_PLACEHOLDER);
+            $subQuerySql = substr_replace(
+                $subQuerySql,
+                sprintf('\'%s\'', is_object($value) && method_exists($value, 'getId') ? $value->getId() : $value),
+                $pos === false ? -1 : $pos,
+                strlen(self::PARAM_PLACEHOLDER),
+            );
+        }
+
+        // A native query builder needs to be used here, because DQL and ORM query builders do not support
         // 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 = $conn->createQueryBuilder();
         $nativeQb
             ->select(
                 't.id_0 AS id',
@@ -64,24 +91,29 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
                 'COUNT(DISTINCT s.id) AS short_urls_count',
                 'COUNT(DISTINCT v.id) AS visits_count',
             )
-            ->from('(' . $subQuery . ')', 't')
+            ->from('(' . $subQuerySql . ')', '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
 
+        // Apply API key role conditions to the native query too, as they will affect the amounts on the aggregates
+        $apiKey?->mapRoles(fn (string $roleName, array $meta) => match ($roleName) {
+            Role::DOMAIN_SPECIFIC => $nativeQb->andWhere(
+                $nativeQb->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))),
+            ),
+            Role::AUTHORED_SHORT_URLS => $nativeQb->andWhere(
+                $nativeQb->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())),
+            ),
+            default => $nativeQb,
+        });
+
         $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(
             $this->getEntityManager()->createNativeQuery($nativeQb->getSQL(), $rsm)->getResult(),
             static fn (array $row) => new TagInfo($row[0], (int) $row['shortUrlsCount'], (int) $row['visitsCount']),

module/Core/src/Repository/VisitRepository.php

@@ -210,7 +210,7 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo
            ->setFirstResult($offset ?? 0);
         $subQuery = $qb->getQuery()->getSQL();
 
-        // A native query builder needs to be used here because DQL and ORM query builders do not accept
+        // A native query builder needs to be used here, because DQL and ORM query builders do not support
         // sub-queries at "from" and "join" level.
         // If no sub-query is used, then performance drops dramatically while the "offset" grows.
         $nativeQb = $this->getEntityManager()->getConnection()->createQueryBuilder();