From 2d861b40773cbfcacb43f5133f60a018557a2058 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 8 Jan 2022 17:25:09 +0100 Subject: [PATCH] Improved performance when loading paginated tags, by using an ugly compound of native queries and DQL --- CHANGELOG.md | 4 +- module/Core/src/Repository/TagRepository.php | 58 ++++++++++++++----- .../Core/src/Repository/VisitRepository.php | 2 +- 3 files changed, 48 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fb1e3a2..7ba90574 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index ab440879..5793de67 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/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']), diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 9978dbc0..3a8440a7 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/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();