From d00a56bec094ecee6a7686b1ed26c21f9a815ef2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 6 Jan 2022 12:22:05 +0100 Subject: [PATCH 1/4] Fixed query to count tags when a search term is present --- .../Adapter/AbstractTagsPaginatorAdapter.php | 15 ++++++--- .../Adapter/TagsPaginatorAdapterTest.php | 31 +++++++++++-------- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/module/Core/src/Tag/Paginator/Adapter/AbstractTagsPaginatorAdapter.php b/module/Core/src/Tag/Paginator/Adapter/AbstractTagsPaginatorAdapter.php index f6331f5e..ba6bc78d 100644 --- a/module/Core/src/Tag/Paginator/Adapter/AbstractTagsPaginatorAdapter.php +++ b/module/Core/src/Tag/Paginator/Adapter/AbstractTagsPaginatorAdapter.php @@ -23,11 +23,18 @@ abstract class AbstractTagsPaginatorAdapter implements AdapterInterface public function getNbResults(): int { - return (int) $this->repo->matchSingleScalarResult(Spec::andX( - // FIXME I don't think using Spec::selectNew is the correct thing here, ideally it should be Spec::select, - // but seems to be the only way to use Spec::COUNT(...) + $conditions = [ + // FIXME I don't think using Spec::selectNew is the correct thing in this context. + // Ideally it should be Spec::select, but seems to be the only way to use Spec::COUNT(...). Spec::selectNew(Tag::class, Spec::COUNT('id', true)), new WithApiKeySpecsEnsuringJoin($this->apiKey), - )); + ]; + + $searchTerm = $this->params->searchTerm(); + if ($searchTerm !== null) { + $conditions[] = Spec::like('name', $searchTerm); + } + + return (int) $this->repo->matchSingleScalarResult(Spec::andX(...$conditions)); } } diff --git a/module/Core/test-db/Tag/Paginator/Adapter/TagsPaginatorAdapterTest.php b/module/Core/test-db/Tag/Paginator/Adapter/TagsPaginatorAdapterTest.php index 62d515ab..db3e444a 100644 --- a/module/Core/test-db/Tag/Paginator/Adapter/TagsPaginatorAdapterTest.php +++ b/module/Core/test-db/Tag/Paginator/Adapter/TagsPaginatorAdapterTest.php @@ -23,8 +23,13 @@ class TagsPaginatorAdapterTest extends DatabaseTestCase * @test * @dataProvider provideFilters */ - public function expectedListOfTagsIsReturned(?string $searchTerm, int $offset, int $length, int $expected): void - { + public function expectedListOfTagsIsReturned( + ?string $searchTerm, + int $offset, + int $length, + int $expectedSliceSize, + int $expectedTotalCount, + ): void { $names = ['foo', 'bar', 'baz', 'another']; foreach ($names as $name) { $this->getEntityManager()->persist(new Tag($name)); @@ -33,20 +38,20 @@ class TagsPaginatorAdapterTest extends DatabaseTestCase $adapter = new TagsPaginatorAdapter($this->repo, TagsParams::fromRawData(['searchTerm' => $searchTerm]), null); - self::assertCount($expected, $adapter->getSlice($offset, $length)); - self::assertEquals(4, $adapter->getNbResults()); + self::assertCount($expectedSliceSize, $adapter->getSlice($offset, $length)); + self::assertEquals($expectedTotalCount, $adapter->getNbResults()); } public function provideFilters(): iterable { - yield [null, 0, 10, 4]; - yield [null, 2, 10, 2]; - yield [null, 1, 3, 3]; - yield [null, 3, 3, 1]; - yield [null, 0, 2, 2]; - yield ['ba', 0, 10, 2]; - yield ['ba', 0, 1, 1]; - yield ['foo', 0, 10, 1]; - yield ['a', 0, 10, 3]; + yield [null, 0, 10, 4, 4]; + yield [null, 2, 10, 2, 4]; + yield [null, 1, 3, 3, 4]; + yield [null, 3, 3, 1, 4]; + yield [null, 0, 2, 2, 4]; + yield ['ba', 0, 10, 2, 2]; + yield ['ba', 0, 1, 1, 2]; + yield ['foo', 0, 10, 1, 1]; + yield ['a', 0, 10, 3, 3]; } } From 2b0567b368f00c749b40ee177eaf44b16f3e8f34 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 6 Jan 2022 18:35:50 +0100 Subject: [PATCH 2/4] Fixed typo --- module/Core/src/Repository/VisitRepository.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 5c39c21e..9978dbc0 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -204,7 +204,7 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo $qb->select('v.id') ->orderBy('v.id', 'DESC') - // Falling back to values that will behave as no limit/offset, but will workaround MS SQL not allowing + // Falling back to values that will behave as no limit/offset, but will work around MS SQL not allowing // order on sub-queries without offset ->setMaxResults($limit ?? PHP_INT_MAX) ->setFirstResult($offset ?? 0); From 107c09604a699d65b4e92cbac31c6b7c5ed46a9a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 6 Jan 2022 19:01:00 +0100 Subject: [PATCH 3/4] 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 */ From 2d861b40773cbfcacb43f5133f60a018557a2058 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 8 Jan 2022 17:25:09 +0100 Subject: [PATCH 4/4] 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();