From a198484ab6a548fb03340d36672f9cf30acb61df Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 21 Jan 2022 19:21:30 +0100 Subject: [PATCH 01/10] Updated test utils lib --- composer.json | 2 +- module/Core/src/Repository/TagRepository.php | 2 +- module/Core/test-db/Domain/Repository/DomainRepositoryTest.php | 2 +- module/Core/test-db/Repository/ShortUrlRepositoryTest.php | 2 +- module/Core/test-db/Repository/TagRepositoryTest.php | 2 +- module/Core/test-db/Repository/VisitRepositoryTest.php | 2 +- .../test-db/Tag/Paginator/Adapter/TagsPaginatorAdapterTest.php | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/composer.json b/composer.json index 41389109..89d9e14d 100644 --- a/composer.json +++ b/composer.json @@ -74,7 +74,7 @@ "phpunit/phpunit": "^9.5", "roave/security-advisories": "dev-master", "shlinkio/php-coding-standard": "~2.2.0", - "shlinkio/shlink-test-utils": "^2.5", + "shlinkio/shlink-test-utils": "^3.0", "symfony/var-dumper": "^6.0", "veewee/composer-run-parallel": "^1.1" }, diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index 8e6e2080..cc952796 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -53,7 +53,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito } $apiKey = $filtering?->apiKey(); - $this->applySpecification($subQb, new WithInlinedApiKeySpecsEnsuringJoin($apiKey, 'shortUrls'), 't'); + $this->applySpecification($subQb, new WithInlinedApiKeySpecsEnsuringJoin($apiKey), 't'); $subQuery = $subQb->getQuery(); $subQuerySql = $subQuery->getSQL(); diff --git a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php index b5ca98ea..3f69e7d9 100644 --- a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php +++ b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php @@ -21,7 +21,7 @@ class DomainRepositoryTest extends DatabaseTestCase { private DomainRepository $repo; - protected function beforeEach(): void + protected function setUp(): void { $this->repo = $this->getEntityManager()->getRepository(Domain::class); } diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index 30b95774..4ad89629 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -33,7 +33,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase private ShortUrlRepository $repo; private PersistenceShortUrlRelationResolver $relationResolver; - public function beforeEach(): void + protected function setUp(): void { $this->repo = $this->getEntityManager()->getRepository(ShortUrl::class); $this->relationResolver = new PersistenceShortUrlRelationResolver($this->getEntityManager()); diff --git a/module/Core/test-db/Repository/TagRepositoryTest.php b/module/Core/test-db/Repository/TagRepositoryTest.php index d030bd03..9bcfaf2b 100644 --- a/module/Core/test-db/Repository/TagRepositoryTest.php +++ b/module/Core/test-db/Repository/TagRepositoryTest.php @@ -27,7 +27,7 @@ class TagRepositoryTest extends DatabaseTestCase private TagRepository $repo; private PersistenceShortUrlRelationResolver $relationResolver; - protected function beforeEach(): void + protected function setUp(): void { $this->repo = $this->getEntityManager()->getRepository(Tag::class); $this->relationResolver = new PersistenceShortUrlRelationResolver($this->getEntityManager()); diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php index 950bfc8a..c23bd8aa 100644 --- a/module/Core/test-db/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Repository/VisitRepositoryTest.php @@ -38,7 +38,7 @@ class VisitRepositoryTest extends DatabaseTestCase private VisitRepository $repo; private PersistenceShortUrlRelationResolver $relationResolver; - protected function beforeEach(): void + protected function setUp(): void { $this->repo = $this->getEntityManager()->getRepository(Visit::class); $this->relationResolver = new PersistenceShortUrlRelationResolver($this->getEntityManager()); diff --git a/module/Core/test-db/Tag/Paginator/Adapter/TagsPaginatorAdapterTest.php b/module/Core/test-db/Tag/Paginator/Adapter/TagsPaginatorAdapterTest.php index d906f80c..7e75aa22 100644 --- a/module/Core/test-db/Tag/Paginator/Adapter/TagsPaginatorAdapterTest.php +++ b/module/Core/test-db/Tag/Paginator/Adapter/TagsPaginatorAdapterTest.php @@ -16,7 +16,7 @@ class TagsPaginatorAdapterTest extends DatabaseTestCase { private TagRepository $repo; - protected function beforeEach(): void + protected function setUp(): void { $this->repo = $this->getEntityManager()->getRepository(Tag::class); } From 33a6c9fda70ebd9c2386f73b8769216622504ec5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 21 Jan 2022 19:52:25 +0100 Subject: [PATCH 02/10] Added support to order tags with stats by short URLs or visits count. In a non-performant way --- module/Core/src/Repository/TagRepository.php | 28 ++++- .../test-db/Repository/TagRepositoryTest.php | 107 +++++++++++++++++- 2 files changed, 125 insertions(+), 10 deletions(-) diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index cc952796..7e69dcc9 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -16,6 +16,7 @@ use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin; use Shlinkio\Shlink\Rest\ApiKey\Spec\WithInlinedApiKeySpecsEnsuringJoin; use Shlinkio\Shlink\Rest\Entity\ApiKey; +use function Functional\contains; use function Functional\map; use const PHP_INT_MAX; @@ -40,12 +41,19 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito */ public function findTagsWithInfo(?TagsListFiltering $filtering = null): array { + $orderBy = $filtering?->orderBy(); + $orderField = $orderBy?->orderField(); + $orderMainQuery = contains(['shortUrlsCount', 'visitsCount'], $orderField); + $conn = $this->getEntityManager()->getConnection(); $subQb = $this->createQueryBuilder('t'); - $subQb->select('t.id', 't.name') - ->orderBy('t.name', $filtering?->orderBy()?->orderDirection() ?? 'ASC') // TODO Make filed dynamic - ->setMaxResults($filtering?->limit() ?? PHP_INT_MAX) - ->setFirstResult($filtering?->offset() ?? 0); + $subQb->select('t.id', 't.name'); + + if (! $orderMainQuery) { + $subQb->orderBy('t.name', $orderBy?->orderDirection() ?? 'ASC') + ->setMaxResults($filtering?->limit() ?? PHP_INT_MAX) + ->setFirstResult($filtering?->offset() ?? 0); + } $searchTerm = $filtering?->searchTerm(); if ($searchTerm !== null) { @@ -74,7 +82,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito ->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', $filtering?->orderBy()?->orderDirection() ?? 'ASC'); // TODO Make field dynamic + ->orderBy('t.name_1', $orderBy?->orderDirection() ?? 'ASC'); // TODO Make field 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) { @@ -87,6 +95,16 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito default => $nativeQb, }); + if ($orderMainQuery) { + $nativeQb + ->orderBy( + $orderField === 'shortUrlsCount' ? 'short_urls_count' : 'visits_count', + $orderBy?->orderDirection() ?? 'ASC', + ) + ->setMaxResults($filtering?->limit() ?? PHP_INT_MAX) + ->setFirstResult($filtering?->offset() ?? 0); + } + $rsm = new ResultSetMappingBuilder($this->getEntityManager()); $rsm->addRootEntityFromClassMetadata(Tag::class, 't'); $rsm->addScalarResult('short_urls_count', 'shortUrlsCount'); diff --git a/module/Core/test-db/Repository/TagRepositoryTest.php b/module/Core/test-db/Repository/TagRepositoryTest.php index 9bcfaf2b..0ddd04ab 100644 --- a/module/Core/test-db/Repository/TagRepositoryTest.php +++ b/module/Core/test-db/Repository/TagRepositoryTest.php @@ -86,6 +86,15 @@ class TagRepositoryTest extends DatabaseTestCase $shortUrl2 = ShortUrl::fromMeta($metaWithTags($secondUrlTags, null), $this->relationResolver); $this->getEntityManager()->persist($shortUrl2); $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl2, Visitor::emptyInstance())); + + // One of the tags has two extra short URLs, but with no visits + $this->getEntityManager()->persist( + ShortUrl::fromMeta($metaWithTags(['bar'], null), $this->relationResolver), + ); + $this->getEntityManager()->persist( + ShortUrl::fromMeta($metaWithTags(['bar'], null), $this->relationResolver), + ); + $this->getEntityManager()->flush(); $result = $this->repo->findTagsWithInfo($filtering); @@ -102,7 +111,7 @@ class TagRepositoryTest extends DatabaseTestCase self::assertEquals(0, $result[0]->visitsCount()); self::assertEquals($tagNames[3], $result[0]->tag()->__toString()); - self::assertEquals(1, $result[1]->shortUrlsCount()); + self::assertEquals(3, $result[1]->shortUrlsCount()); self::assertEquals(3, $result[1]->visitsCount()); self::assertEquals($tagNames[1], $result[1]->tag()->__toString()); @@ -124,7 +133,7 @@ class TagRepositoryTest extends DatabaseTestCase self::assertEquals(0, $result[0]->visitsCount()); self::assertEquals($tagNames[3], $result[0]->tag()->__toString()); - self::assertEquals(1, $result[1]->shortUrlsCount()); + self::assertEquals(3, $result[1]->shortUrlsCount()); self::assertEquals(3, $result[1]->visitsCount()); self::assertEquals($tagNames[1], $result[1]->tag()->__toString()); }]; @@ -140,7 +149,7 @@ class TagRepositoryTest extends DatabaseTestCase static function (array $result, array $tagNames): void { /** @var TagInfo[] $result */ self::assertCount(2, $result); - self::assertEquals(1, $result[0]->shortUrlsCount()); + self::assertEquals(3, $result[0]->shortUrlsCount()); self::assertEquals(3, $result[0]->visitsCount()); self::assertEquals($tagNames[1], $result[0]->tag()->__toString()); @@ -154,7 +163,7 @@ class TagRepositoryTest extends DatabaseTestCase static function (array $result, array $tagNames): void { /** @var TagInfo[] $result */ self::assertCount(2, $result); - self::assertEquals(1, $result[0]->shortUrlsCount()); + self::assertEquals(3, $result[0]->shortUrlsCount()); self::assertEquals(3, $result[0]->visitsCount()); self::assertEquals($tagNames[1], $result[0]->tag()->__toString()); @@ -176,7 +185,7 @@ class TagRepositoryTest extends DatabaseTestCase self::assertEquals(0, $result[3]->visitsCount()); self::assertEquals($tagNames[3], $result[3]->tag()->__toString()); - self::assertEquals(1, $result[2]->shortUrlsCount()); + self::assertEquals(3, $result[2]->shortUrlsCount()); self::assertEquals(3, $result[2]->visitsCount()); self::assertEquals($tagNames[1], $result[2]->tag()->__toString()); @@ -189,6 +198,94 @@ class TagRepositoryTest extends DatabaseTestCase self::assertEquals($tagNames[0], $result[0]->tag()->__toString()); }, ]; + yield 'short URLs count ASC ordering' => [ + new TagsListFiltering(null, null, null, Ordering::fromTuple(['shortUrlsCount', 'ASC'])), + static function (array $result, array $tagNames): void { + /** @var TagInfo[] $result */ + self::assertCount(4, $result); + self::assertEquals(0, $result[0]->shortUrlsCount()); + self::assertEquals(0, $result[0]->visitsCount()); + self::assertEquals($tagNames[3], $result[0]->tag()->__toString()); + + self::assertEquals(1, $result[1]->shortUrlsCount()); + self::assertEquals(3, $result[1]->visitsCount()); + self::assertEquals($tagNames[2], $result[1]->tag()->__toString()); + + self::assertEquals(2, $result[2]->shortUrlsCount()); + self::assertEquals(4, $result[2]->visitsCount()); + self::assertEquals($tagNames[0], $result[2]->tag()->__toString()); + + self::assertEquals(3, $result[3]->shortUrlsCount()); + self::assertEquals(3, $result[3]->visitsCount()); + self::assertEquals($tagNames[1], $result[3]->tag()->__toString()); + }, + ]; + yield 'short URLs count DESC ordering' => [ + new TagsListFiltering(null, null, null, Ordering::fromTuple(['shortUrlsCount', 'DESC'])), + static function (array $result, array $tagNames): void { + /** @var TagInfo[] $result */ + self::assertCount(4, $result); + self::assertEquals(3, $result[0]->shortUrlsCount()); + self::assertEquals(3, $result[0]->visitsCount()); + self::assertEquals($tagNames[1], $result[0]->tag()->__toString()); + + self::assertEquals(2, $result[1]->shortUrlsCount()); + self::assertEquals(4, $result[1]->visitsCount()); + self::assertEquals($tagNames[0], $result[1]->tag()->__toString()); + + self::assertEquals(1, $result[2]->shortUrlsCount()); + self::assertEquals(3, $result[2]->visitsCount()); + self::assertEquals($tagNames[2], $result[2]->tag()->__toString()); + + self::assertEquals(0, $result[3]->shortUrlsCount()); + self::assertEquals(0, $result[3]->visitsCount()); + self::assertEquals($tagNames[3], $result[3]->tag()->__toString()); + }, + ]; + yield 'visits count ASC ordering' => [ + new TagsListFiltering(null, null, null, Ordering::fromTuple(['visitsCount', 'ASC'])), + static function (array $result, array $tagNames): void { + /** @var TagInfo[] $result */ + self::assertCount(4, $result); + self::assertEquals(0, $result[0]->shortUrlsCount()); + self::assertEquals(0, $result[0]->visitsCount()); + self::assertEquals($tagNames[3], $result[0]->tag()->__toString()); + + self::assertEquals(3, $result[1]->shortUrlsCount()); + self::assertEquals(3, $result[1]->visitsCount()); + self::assertEquals($tagNames[1], $result[1]->tag()->__toString()); + + self::assertEquals(1, $result[2]->shortUrlsCount()); + self::assertEquals(3, $result[2]->visitsCount()); + self::assertEquals($tagNames[2], $result[2]->tag()->__toString()); + + self::assertEquals(2, $result[3]->shortUrlsCount()); + self::assertEquals(4, $result[3]->visitsCount()); + self::assertEquals($tagNames[0], $result[3]->tag()->__toString()); + }, + ]; + yield 'visits count DESC ordering' => [ + new TagsListFiltering(null, null, null, Ordering::fromTuple(['visitsCount', 'DESC'])), + static function (array $result, array $tagNames): void { + /** @var TagInfo[] $result */ + self::assertCount(4, $result); + self::assertEquals(2, $result[0]->shortUrlsCount()); + self::assertEquals(4, $result[0]->visitsCount()); + self::assertEquals($tagNames[0], $result[0]->tag()->__toString()); + + self::assertEquals(3, $result[1]->shortUrlsCount()); + self::assertEquals(3, $result[1]->visitsCount()); + self::assertEquals($tagNames[1], $result[1]->tag()->__toString()); + + self::assertEquals(1, $result[2]->shortUrlsCount()); + self::assertEquals(3, $result[2]->visitsCount()); + self::assertEquals($tagNames[2], $result[2]->tag()->__toString()); + + self::assertEquals(0, $result[3]->shortUrlsCount()); + self::assertEquals(0, $result[3]->visitsCount()); + self::assertEquals($tagNames[3], $result[3]->tag()->__toString()); + }, + ]; yield 'api key' => [new TagsListFiltering(null, null, null, null, ApiKey::fromMeta( ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls()), )), static function (array $result, array $tagNames): void { From afca66d6557dcbe6b73eb29b9c44be6a4a6d7498 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 21 Jan 2022 19:58:56 +0100 Subject: [PATCH 03/10] Added tests covering tags info with counted ordering and limit --- docker-compose.yml | 2 +- .../Core/test-db/Repository/TagRepositoryTest.php | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/docker-compose.yml b/docker-compose.yml index 5e2a3bd6..739c0079 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -100,7 +100,7 @@ services: shlink_db_maria: container_name: shlink_db_maria - image: mariadb:10.5 + image: mariadb:10.7 ports: - "3308:3306" volumes: diff --git a/module/Core/test-db/Repository/TagRepositoryTest.php b/module/Core/test-db/Repository/TagRepositoryTest.php index 0ddd04ab..30cb6256 100644 --- a/module/Core/test-db/Repository/TagRepositoryTest.php +++ b/module/Core/test-db/Repository/TagRepositoryTest.php @@ -286,6 +286,20 @@ class TagRepositoryTest extends DatabaseTestCase self::assertEquals($tagNames[3], $result[3]->tag()->__toString()); }, ]; + yield 'visits count DESC ordering and limit' => [ + new TagsListFiltering(2, null, null, Ordering::fromTuple(['visitsCount', 'DESC'])), + static function (array $result, array $tagNames): void { + /** @var TagInfo[] $result */ + self::assertCount(2, $result); + self::assertEquals(2, $result[0]->shortUrlsCount()); + self::assertEquals(4, $result[0]->visitsCount()); + self::assertEquals($tagNames[0], $result[0]->tag()->__toString()); + + self::assertEquals(3, $result[1]->shortUrlsCount()); + self::assertEquals(3, $result[1]->visitsCount()); + self::assertEquals($tagNames[1], $result[1]->tag()->__toString()); + }, + ]; yield 'api key' => [new TagsListFiltering(null, null, null, null, ApiKey::fromMeta( ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls()), )), static function (array $result, array $tagNames): void { From d5606114cd2b321bd7ce0875ea5503fe77415313 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 21 Jan 2022 20:02:52 +0100 Subject: [PATCH 04/10] Documented new ordering fields supported on tags list --- docs/swagger/paths/v2_tags_stats.json | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/swagger/paths/v2_tags_stats.json b/docs/swagger/paths/v2_tags_stats.json index bd745fd0..48e5690f 100644 --- a/docs/swagger/paths/v2_tags_stats.json +++ b/docs/swagger/paths/v2_tags_stats.json @@ -51,7 +51,11 @@ "type": "string", "enum": [ "tag-ASC", - "tag-DESC" + "tag-DESC", + "shortUrlsCount-ASC", + "shortUrlsCount-DESC", + "visitsCount-ASC", + "visitsCount-DESC" ] } } From 361e864415a0926f96a24be07c8c5fee8d0e7625 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 21 Jan 2022 20:12:16 +0100 Subject: [PATCH 05/10] Added fallback ordering to tags list --- module/Core/src/Repository/TagRepository.php | 1 + 1 file changed, 1 insertion(+) diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index 7e69dcc9..c0ab0d2e 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -101,6 +101,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $orderField === 'shortUrlsCount' ? 'short_urls_count' : 'visits_count', $orderBy?->orderDirection() ?? 'ASC', ) + ->addOrderBy('t.name_1', 'ASC') // In case of same amount, order by tag too ->setMaxResults($filtering?->limit() ?? PHP_INT_MAX) ->setFirstResult($filtering?->offset() ?? 0); } From 6b409b06cc52ee9bc32a9dae754c82b97ea5e230 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 21 Jan 2022 22:04:53 +0100 Subject: [PATCH 06/10] Simplified TagRepository test for tags info list, making it more predictable --- .../test-db/Repository/TagRepositoryTest.php | 273 +++++------------- 1 file changed, 78 insertions(+), 195 deletions(-) diff --git a/module/Core/test-db/Repository/TagRepositoryTest.php b/module/Core/test-db/Repository/TagRepositoryTest.php index 30cb6256..50129e54 100644 --- a/module/Core/test-db/Repository/TagRepositoryTest.php +++ b/module/Core/test-db/Repository/TagRepositoryTest.php @@ -13,7 +13,6 @@ use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Repository\TagRepository; use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver; -use Shlinkio\Shlink\Core\Tag\Model\TagInfo; use Shlinkio\Shlink\Core\Tag\Model\TagsListFiltering; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; @@ -21,6 +20,7 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase; use function array_chunk; +use function count; class TagRepositoryTest extends DatabaseTestCase { @@ -57,7 +57,7 @@ class TagRepositoryTest extends DatabaseTestCase * @test * @dataProvider provideFilters */ - public function properTagsInfoIsReturned(?TagsListFiltering $filtering, callable $asserts): void + public function properTagsInfoIsReturned(?TagsListFiltering $filtering, array $expectedList): void { $names = ['foo', 'bar', 'baz', 'another']; foreach ($names as $name) { @@ -92,231 +92,114 @@ class TagRepositoryTest extends DatabaseTestCase ShortUrl::fromMeta($metaWithTags(['bar'], null), $this->relationResolver), ); $this->getEntityManager()->persist( - ShortUrl::fromMeta($metaWithTags(['bar'], null), $this->relationResolver), + ShortUrl::fromMeta($metaWithTags(['bar'], $apiKey), $this->relationResolver), ); $this->getEntityManager()->flush(); $result = $this->repo->findTagsWithInfo($filtering); - $asserts($result, $names); + self::assertCount(count($expectedList), $result); + foreach ($expectedList as $index => [$tag, $shortUrlsCount, $visitsCount]) { + self::assertEquals($shortUrlsCount, $result[$index]->shortUrlsCount()); + self::assertEquals($visitsCount, $result[$index]->visitsCount()); + self::assertEquals($tag, $result[$index]->tag()->__toString()); + } } public function provideFilters(): iterable { - $defaultAsserts = static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(4, $result); - self::assertEquals(0, $result[0]->shortUrlsCount()); - self::assertEquals(0, $result[0]->visitsCount()); - self::assertEquals($tagNames[3], $result[0]->tag()->__toString()); - - self::assertEquals(3, $result[1]->shortUrlsCount()); - self::assertEquals(3, $result[1]->visitsCount()); - self::assertEquals($tagNames[1], $result[1]->tag()->__toString()); - - self::assertEquals(1, $result[2]->shortUrlsCount()); - self::assertEquals(3, $result[2]->visitsCount()); - self::assertEquals($tagNames[2], $result[2]->tag()->__toString()); - - self::assertEquals(2, $result[3]->shortUrlsCount()); - self::assertEquals(4, $result[3]->visitsCount()); - self::assertEquals($tagNames[0], $result[3]->tag()->__toString()); - }; - - yield 'no filter' => [null, $defaultAsserts]; - yield 'empty filter' => [new TagsListFiltering(), $defaultAsserts]; - yield 'limit' => [new TagsListFiltering(2), static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(2, $result); - self::assertEquals(0, $result[0]->shortUrlsCount()); - self::assertEquals(0, $result[0]->visitsCount()); - self::assertEquals($tagNames[3], $result[0]->tag()->__toString()); - - self::assertEquals(3, $result[1]->shortUrlsCount()); - self::assertEquals(3, $result[1]->visitsCount()); - self::assertEquals($tagNames[1], $result[1]->tag()->__toString()); - }]; - yield 'offset' => [new TagsListFiltering(null, 3), static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(1, $result); - self::assertEquals(2, $result[0]->shortUrlsCount()); - self::assertEquals(4, $result[0]->visitsCount()); - self::assertEquals($tagNames[0], $result[0]->tag()->__toString()); - }]; - yield 'limit and offset' => [ - new TagsListFiltering(2, 1), - static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(2, $result); - self::assertEquals(3, $result[0]->shortUrlsCount()); - self::assertEquals(3, $result[0]->visitsCount()); - self::assertEquals($tagNames[1], $result[0]->tag()->__toString()); - - self::assertEquals(1, $result[1]->shortUrlsCount()); - self::assertEquals(3, $result[1]->visitsCount()); - self::assertEquals($tagNames[2], $result[1]->tag()->__toString()); - }, + $defaultList = [ + ['another', 0, 0], + ['bar', 3, 3], + ['baz', 1, 3], + ['foo', 2, 4], ]; - yield 'search term' => [ - new TagsListFiltering(null, null, 'ba'), - static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(2, $result); - self::assertEquals(3, $result[0]->shortUrlsCount()); - self::assertEquals(3, $result[0]->visitsCount()); - self::assertEquals($tagNames[1], $result[0]->tag()->__toString()); - self::assertEquals(1, $result[1]->shortUrlsCount()); - self::assertEquals(3, $result[1]->visitsCount()); - self::assertEquals($tagNames[2], $result[1]->tag()->__toString()); - }, - ]; + yield 'no filter' => [null, $defaultList]; + yield 'empty filter' => [new TagsListFiltering(), $defaultList]; + yield 'limit' => [new TagsListFiltering(2), [ + ['another', 0, 0], + ['bar', 3, 3], + ]]; + yield 'offset' => [new TagsListFiltering(null, 3), [ + ['foo', 2, 4], + ]]; + yield 'limit and offset' => [new TagsListFiltering(2, 1), [ + ['bar', 3, 3], + ['baz', 1, 3], + ]]; + yield 'search term' => [new TagsListFiltering(null, null, 'ba'), [ + ['bar', 3, 3], + ['baz', 1, 3], + ]]; yield 'ASC ordering' => [ new TagsListFiltering(null, null, null, Ordering::fromTuple(['tag', 'ASC'])), - $defaultAsserts, - ]; - yield 'DESC ordering' => [ - new TagsListFiltering(null, null, null, Ordering::fromTuple(['tag', 'DESC'])), - static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(4, $result); - self::assertEquals(0, $result[3]->shortUrlsCount()); - self::assertEquals(0, $result[3]->visitsCount()); - self::assertEquals($tagNames[3], $result[3]->tag()->__toString()); - - self::assertEquals(3, $result[2]->shortUrlsCount()); - self::assertEquals(3, $result[2]->visitsCount()); - self::assertEquals($tagNames[1], $result[2]->tag()->__toString()); - - self::assertEquals(1, $result[1]->shortUrlsCount()); - self::assertEquals(3, $result[1]->visitsCount()); - self::assertEquals($tagNames[2], $result[1]->tag()->__toString()); - - self::assertEquals(2, $result[0]->shortUrlsCount()); - self::assertEquals(4, $result[0]->visitsCount()); - self::assertEquals($tagNames[0], $result[0]->tag()->__toString()); - }, + $defaultList, ]; + yield 'DESC ordering' => [new TagsListFiltering(null, null, null, Ordering::fromTuple(['tag', 'DESC'])), [ + ['foo', 2, 4], + ['baz', 1, 3], + ['bar', 3, 3], + ['another', 0, 0], + ]]; yield 'short URLs count ASC ordering' => [ new TagsListFiltering(null, null, null, Ordering::fromTuple(['shortUrlsCount', 'ASC'])), - static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(4, $result); - self::assertEquals(0, $result[0]->shortUrlsCount()); - self::assertEquals(0, $result[0]->visitsCount()); - self::assertEquals($tagNames[3], $result[0]->tag()->__toString()); - - self::assertEquals(1, $result[1]->shortUrlsCount()); - self::assertEquals(3, $result[1]->visitsCount()); - self::assertEquals($tagNames[2], $result[1]->tag()->__toString()); - - self::assertEquals(2, $result[2]->shortUrlsCount()); - self::assertEquals(4, $result[2]->visitsCount()); - self::assertEquals($tagNames[0], $result[2]->tag()->__toString()); - - self::assertEquals(3, $result[3]->shortUrlsCount()); - self::assertEquals(3, $result[3]->visitsCount()); - self::assertEquals($tagNames[1], $result[3]->tag()->__toString()); - }, + [ + ['another', 0, 0], + ['baz', 1, 3], + ['foo', 2, 4], + ['bar', 3, 3], + ], ]; yield 'short URLs count DESC ordering' => [ new TagsListFiltering(null, null, null, Ordering::fromTuple(['shortUrlsCount', 'DESC'])), - static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(4, $result); - self::assertEquals(3, $result[0]->shortUrlsCount()); - self::assertEquals(3, $result[0]->visitsCount()); - self::assertEquals($tagNames[1], $result[0]->tag()->__toString()); - - self::assertEquals(2, $result[1]->shortUrlsCount()); - self::assertEquals(4, $result[1]->visitsCount()); - self::assertEquals($tagNames[0], $result[1]->tag()->__toString()); - - self::assertEquals(1, $result[2]->shortUrlsCount()); - self::assertEquals(3, $result[2]->visitsCount()); - self::assertEquals($tagNames[2], $result[2]->tag()->__toString()); - - self::assertEquals(0, $result[3]->shortUrlsCount()); - self::assertEquals(0, $result[3]->visitsCount()); - self::assertEquals($tagNames[3], $result[3]->tag()->__toString()); - }, + [ + ['bar', 3, 3], + ['foo', 2, 4], + ['baz', 1, 3], + ['another', 0, 0], + ], ]; yield 'visits count ASC ordering' => [ new TagsListFiltering(null, null, null, Ordering::fromTuple(['visitsCount', 'ASC'])), - static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(4, $result); - self::assertEquals(0, $result[0]->shortUrlsCount()); - self::assertEquals(0, $result[0]->visitsCount()); - self::assertEquals($tagNames[3], $result[0]->tag()->__toString()); - - self::assertEquals(3, $result[1]->shortUrlsCount()); - self::assertEquals(3, $result[1]->visitsCount()); - self::assertEquals($tagNames[1], $result[1]->tag()->__toString()); - - self::assertEquals(1, $result[2]->shortUrlsCount()); - self::assertEquals(3, $result[2]->visitsCount()); - self::assertEquals($tagNames[2], $result[2]->tag()->__toString()); - - self::assertEquals(2, $result[3]->shortUrlsCount()); - self::assertEquals(4, $result[3]->visitsCount()); - self::assertEquals($tagNames[0], $result[3]->tag()->__toString()); - }, + [ + ['another', 0, 0], + ['bar', 3, 3], + ['baz', 1, 3], + ['foo', 2, 4], + ], ]; yield 'visits count DESC ordering' => [ new TagsListFiltering(null, null, null, Ordering::fromTuple(['visitsCount', 'DESC'])), - static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(4, $result); - self::assertEquals(2, $result[0]->shortUrlsCount()); - self::assertEquals(4, $result[0]->visitsCount()); - self::assertEquals($tagNames[0], $result[0]->tag()->__toString()); - - self::assertEquals(3, $result[1]->shortUrlsCount()); - self::assertEquals(3, $result[1]->visitsCount()); - self::assertEquals($tagNames[1], $result[1]->tag()->__toString()); - - self::assertEquals(1, $result[2]->shortUrlsCount()); - self::assertEquals(3, $result[2]->visitsCount()); - self::assertEquals($tagNames[2], $result[2]->tag()->__toString()); - - self::assertEquals(0, $result[3]->shortUrlsCount()); - self::assertEquals(0, $result[3]->visitsCount()); - self::assertEquals($tagNames[3], $result[3]->tag()->__toString()); - }, + [ + ['foo', 2, 4], + ['bar', 3, 3], + ['baz', 1, 3], + ['another', 0, 0], + ], ]; yield 'visits count DESC ordering and limit' => [ new TagsListFiltering(2, null, null, Ordering::fromTuple(['visitsCount', 'DESC'])), - static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(2, $result); - self::assertEquals(2, $result[0]->shortUrlsCount()); - self::assertEquals(4, $result[0]->visitsCount()); - self::assertEquals($tagNames[0], $result[0]->tag()->__toString()); - - self::assertEquals(3, $result[1]->shortUrlsCount()); - self::assertEquals(3, $result[1]->visitsCount()); - self::assertEquals($tagNames[1], $result[1]->tag()->__toString()); - }, + [ + ['foo', 2, 4], + ['bar', 3, 3], + ], ]; yield 'api key' => [new TagsListFiltering(null, null, null, null, ApiKey::fromMeta( ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls()), - )), static function (array $result, array $tagNames): void { - /** @var TagInfo[] $result */ - self::assertCount(3, $result); - self::assertEquals(1, $result[0]->shortUrlsCount()); - self::assertEquals(3, $result[0]->visitsCount()); - self::assertEquals($tagNames[1], $result[0]->tag()->__toString()); - - self::assertEquals(1, $result[1]->shortUrlsCount()); - self::assertEquals(3, $result[1]->visitsCount()); - self::assertEquals($tagNames[2], $result[1]->tag()->__toString()); - - self::assertEquals(1, $result[2]->shortUrlsCount()); - self::assertEquals(3, $result[2]->visitsCount()); - self::assertEquals($tagNames[0], $result[2]->tag()->__toString()); - }]; + )), [ + ['bar', 2, 3], + ['baz', 1, 3], + ['foo', 1, 3], + ]]; + yield 'combined' => [new TagsListFiltering(1, null, null, Ordering::fromTuple( + ['shortUrls', 'DESC'], + ), ApiKey::fromMeta( + ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls()), + )), [ + ['foo', 1, 3], + ]]; } /** @test */ From 1c9ce0ede0265338f4045dacf9d46bb61407fd87 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 21 Jan 2022 22:22:55 +0100 Subject: [PATCH 07/10] Fixed default/fallback tags with stats ordering --- module/Core/src/Repository/TagRepository.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index c0ab0d2e..ab9f1738 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -43,6 +43,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito { $orderBy = $filtering?->orderBy(); $orderField = $orderBy?->orderField(); + $orderDir = $orderBy?->orderDirection(); $orderMainQuery = contains(['shortUrlsCount', 'visitsCount'], $orderField); $conn = $this->getEntityManager()->getConnection(); @@ -50,7 +51,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $subQb->select('t.id', 't.name'); if (! $orderMainQuery) { - $subQb->orderBy('t.name', $orderBy?->orderDirection() ?? 'ASC') + $subQb->orderBy('t.name', $orderDir ?? 'ASC') ->setMaxResults($filtering?->limit() ?? PHP_INT_MAX) ->setFirstResult($filtering?->offset() ?? 0); } @@ -81,8 +82,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito ->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', $orderBy?->orderDirection() ?? 'ASC'); // TODO Make field dynamic + ->groupBy('t.id_0', 't.name_1'); // 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) { @@ -99,13 +99,15 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $nativeQb ->orderBy( $orderField === 'shortUrlsCount' ? 'short_urls_count' : 'visits_count', - $orderBy?->orderDirection() ?? 'ASC', + $orderDir ?? 'ASC', ) - ->addOrderBy('t.name_1', 'ASC') // In case of same amount, order by tag too ->setMaxResults($filtering?->limit() ?? PHP_INT_MAX) ->setFirstResult($filtering?->offset() ?? 0); } + // Add ordering by tag name, as a fallback in case of same amount, or as default ordering + $nativeQb->addOrderBy('t.name_1', $orderMainQuery || $orderDir === null ? 'ASC' : $orderDir); + $rsm = new ResultSetMappingBuilder($this->getEntityManager()); $rsm->addRootEntityFromClassMetadata(Tag::class, 't'); $rsm->addScalarResult('short_urls_count', 'shortUrlsCount'); From dd6bcd68ccfad7620f0d01dbb1eb831d51c1472d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 22 Jan 2022 20:36:50 +0100 Subject: [PATCH 08/10] Removed not-needed extra line --- module/Core/src/Repository/TagRepository.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index ab9f1738..dc3b2e65 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -41,9 +41,8 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito */ public function findTagsWithInfo(?TagsListFiltering $filtering = null): array { - $orderBy = $filtering?->orderBy(); - $orderField = $orderBy?->orderField(); - $orderDir = $orderBy?->orderDirection(); + $orderField = $filtering?->orderBy()?->orderField(); + $orderDir = $filtering?->orderBy()?->orderDirection(); $orderMainQuery = contains(['shortUrlsCount', 'visitsCount'], $orderField); $conn = $this->getEntityManager()->getConnection(); From 8adb6596fb48d2ff99311d3c0259798c755b53af Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 23 Jan 2022 09:37:02 +0100 Subject: [PATCH 09/10] Refactored TagInfo to wrap the raw tag name instead of a Tag entity --- module/CLI/src/Command/Tag/ListTagsCommand.php | 3 +-- module/CLI/test/Command/Tag/ListTagsCommandTest.php | 5 ++--- module/Core/src/Repository/TagRepository.php | 4 ++-- module/Core/src/Tag/Model/TagInfo.php | 5 ++--- module/Core/test-db/Repository/TagRepositoryTest.php | 2 +- module/Core/test/Tag/TagServiceTest.php | 2 +- module/Rest/src/Action/Tag/ListTagsAction.php | 2 +- module/Rest/test/Action/Tag/ListTagsActionTest.php | 4 ++-- module/Rest/test/Action/Tag/TagsStatsActionTest.php | 5 ++--- 9 files changed, 14 insertions(+), 18 deletions(-) diff --git a/module/CLI/src/Command/Tag/ListTagsCommand.php b/module/CLI/src/Command/Tag/ListTagsCommand.php index 7d21613d..9c7269fa 100644 --- a/module/CLI/src/Command/Tag/ListTagsCommand.php +++ b/module/CLI/src/Command/Tag/ListTagsCommand.php @@ -46,8 +46,7 @@ class ListTagsCommand extends Command return map( $tags, - static fn (TagInfo $tagInfo) => - [$tagInfo->tag()->__toString(), $tagInfo->shortUrlsCount(), $tagInfo->visitsCount()], + static fn (TagInfo $tagInfo) => [$tagInfo->tag(), $tagInfo->shortUrlsCount(), $tagInfo->visitsCount()], ); } } diff --git a/module/CLI/test/Command/Tag/ListTagsCommandTest.php b/module/CLI/test/Command/Tag/ListTagsCommandTest.php index 879b2eb7..499442d0 100644 --- a/module/CLI/test/Command/Tag/ListTagsCommandTest.php +++ b/module/CLI/test/Command/Tag/ListTagsCommandTest.php @@ -10,7 +10,6 @@ use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\Tag\ListTagsCommand; use Shlinkio\Shlink\Common\Paginator\Paginator; -use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Tag\Model\TagInfo; use Shlinkio\Shlink\Core\Tag\TagServiceInterface; use ShlinkioTest\Shlink\CLI\CliTestUtilsTrait; @@ -45,8 +44,8 @@ class ListTagsCommandTest extends TestCase public function listOfTagsIsPrinted(): void { $tagsInfo = $this->tagService->tagsInfo(Argument::any())->willReturn(new Paginator(new ArrayAdapter([ - new TagInfo(new Tag('foo'), 10, 2), - new TagInfo(new Tag('bar'), 7, 32), + new TagInfo('foo', 10, 2), + new TagInfo('bar', 7, 32), ]))); $this->commandTester->execute([]); diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index dc3b2e65..66aebae3 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -108,13 +108,13 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $nativeQb->addOrderBy('t.name_1', $orderMainQuery || $orderDir === null ? 'ASC' : $orderDir); $rsm = new ResultSetMappingBuilder($this->getEntityManager()); - $rsm->addRootEntityFromClassMetadata(Tag::class, 't'); + $rsm->addScalarResult('name', 'tag'); $rsm->addScalarResult('short_urls_count', 'shortUrlsCount'); $rsm->addScalarResult('visits_count', 'visitsCount'); return map( $this->getEntityManager()->createNativeQuery($nativeQb->getSQL(), $rsm)->getResult(), - static fn (array $row) => new TagInfo($row[0], (int) $row['shortUrlsCount'], (int) $row['visitsCount']), + static fn (array $row) => new TagInfo($row['tag'], (int) $row['shortUrlsCount'], (int) $row['visitsCount']), ); } diff --git a/module/Core/src/Tag/Model/TagInfo.php b/module/Core/src/Tag/Model/TagInfo.php index 1a436cd4..6e917399 100644 --- a/module/Core/src/Tag/Model/TagInfo.php +++ b/module/Core/src/Tag/Model/TagInfo.php @@ -5,15 +5,14 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Tag\Model; use JsonSerializable; -use Shlinkio\Shlink\Core\Entity\Tag; final class TagInfo implements JsonSerializable { - public function __construct(private Tag $tag, private int $shortUrlsCount, private int $visitsCount) + public function __construct(private string $tag, private int $shortUrlsCount, private int $visitsCount) { } - public function tag(): Tag + public function tag(): string { return $this->tag; } diff --git a/module/Core/test-db/Repository/TagRepositoryTest.php b/module/Core/test-db/Repository/TagRepositoryTest.php index 50129e54..fe544376 100644 --- a/module/Core/test-db/Repository/TagRepositoryTest.php +++ b/module/Core/test-db/Repository/TagRepositoryTest.php @@ -103,7 +103,7 @@ class TagRepositoryTest extends DatabaseTestCase foreach ($expectedList as $index => [$tag, $shortUrlsCount, $visitsCount]) { self::assertEquals($shortUrlsCount, $result[$index]->shortUrlsCount()); self::assertEquals($visitsCount, $result[$index]->visitsCount()); - self::assertEquals($tag, $result[$index]->tag()->__toString()); + self::assertEquals($tag, $result[$index]->tag()); } } diff --git a/module/Core/test/Tag/TagServiceTest.php b/module/Core/test/Tag/TagServiceTest.php index c3efc4b5..8c301f0f 100644 --- a/module/Core/test/Tag/TagServiceTest.php +++ b/module/Core/test/Tag/TagServiceTest.php @@ -67,7 +67,7 @@ class TagServiceTest extends TestCase TagsListFiltering $expectedFiltering, int $countCalls, ): void { - $expected = [new TagInfo(new Tag('foo'), 1, 1), new TagInfo(new Tag('bar'), 3, 10)]; + $expected = [new TagInfo('foo', 1, 1), new TagInfo('bar', 3, 10)]; $find = $this->repo->findTagsWithInfo($expectedFiltering)->willReturn($expected); $count = $this->repo->matchSingleScalarResult(Argument::cetera())->willReturn(2); diff --git a/module/Rest/src/Action/Tag/ListTagsAction.php b/module/Rest/src/Action/Tag/ListTagsAction.php index ba25ffe5..ab81400c 100644 --- a/module/Rest/src/Action/Tag/ListTagsAction.php +++ b/module/Rest/src/Action/Tag/ListTagsAction.php @@ -41,7 +41,7 @@ class ListTagsAction extends AbstractRestAction // This part is deprecated. To get tags with stats, the /tags/stats endpoint should be used instead $tagsInfo = $this->tagService->tagsInfo($params, $apiKey); $rawTags = $this->serializePaginator($tagsInfo, null, 'stats'); - $rawTags['data'] = map($tagsInfo, static fn (TagInfo $info) => $info->tag()->__toString()); + $rawTags['data'] = map($tagsInfo, static fn (TagInfo $info) => $info->tag()); return new JsonResponse(['tags' => $rawTags]); } diff --git a/module/Rest/test/Action/Tag/ListTagsActionTest.php b/module/Rest/test/Action/Tag/ListTagsActionTest.php index 504f7b4f..123e4945 100644 --- a/module/Rest/test/Action/Tag/ListTagsActionTest.php +++ b/module/Rest/test/Action/Tag/ListTagsActionTest.php @@ -76,8 +76,8 @@ class ListTagsActionTest extends TestCase public function returnsStatsWhenRequested(): void { $stats = [ - new TagInfo(new Tag('foo'), 1, 1), - new TagInfo(new Tag('bar'), 3, 10), + new TagInfo('foo', 1, 1), + new TagInfo('bar', 3, 10), ]; $itemsCount = count($stats); $tagsInfo = $this->tagService->tagsInfo(Argument::any(), Argument::type(ApiKey::class))->willReturn( diff --git a/module/Rest/test/Action/Tag/TagsStatsActionTest.php b/module/Rest/test/Action/Tag/TagsStatsActionTest.php index 3f98b64e..2cb3ad64 100644 --- a/module/Rest/test/Action/Tag/TagsStatsActionTest.php +++ b/module/Rest/test/Action/Tag/TagsStatsActionTest.php @@ -13,7 +13,6 @@ use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Common\Paginator\Paginator; -use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Tag\Model\TagInfo; use Shlinkio\Shlink\Core\Tag\TagServiceInterface; use Shlinkio\Shlink\Rest\Action\Tag\TagsStatsAction; @@ -38,8 +37,8 @@ class TagsStatsActionTest extends TestCase public function returnsTagsStatsWhenRequested(): void { $stats = [ - new TagInfo(new Tag('foo'), 1, 1), - new TagInfo(new Tag('bar'), 3, 10), + new TagInfo('foo', 1, 1), + new TagInfo('bar', 3, 10), ]; $itemsCount = count($stats); $tagsInfo = $this->tagService->tagsInfo(Argument::any(), Argument::type(ApiKey::class))->willReturn( From cdb18a5baf6ee9efa043f904e19ed350dc27e85f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 23 Jan 2022 10:48:38 +0100 Subject: [PATCH 10/10] Documented performance issue when sorting by visits or short URLs count --- docs/swagger/paths/v2_tags_stats.json | 2 +- module/Core/src/Model/Ordering.php | 3 +++ module/Core/src/Repository/TagRepository.php | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/swagger/paths/v2_tags_stats.json b/docs/swagger/paths/v2_tags_stats.json index 48e5690f..91771335 100644 --- a/docs/swagger/paths/v2_tags_stats.json +++ b/docs/swagger/paths/v2_tags_stats.json @@ -45,7 +45,7 @@ { "name": "orderBy", "in": "query", - "description": "To determine how to order the results.", + "description": "To determine how to order the results.

**Important!** Ordering by `shortUrlsCount` or `visitsCount` has a [known performance issue](https://github.com/shlinkio/shlink/issues/1346) which makes loading a subset of the list take as much as loading the whole list.
If you plan to order by any of these fields, it's worth loading the whole list with no pagination.", "required": false, "schema": { "type": "string", diff --git a/module/Core/src/Model/Ordering.php b/module/Core/src/Model/Ordering.php index 6112dde7..bd648227 100644 --- a/module/Core/src/Model/Ordering.php +++ b/module/Core/src/Model/Ordering.php @@ -12,6 +12,9 @@ final class Ordering { } + /** + * @param array{string|null, string|null} $props + */ public static function fromTuple(array $props): self { [$field, $dir] = $props; diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index 66aebae3..1ee5404b 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -84,7 +84,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito ->groupBy('t.id_0', 't.name_1'); // 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) { + $apiKey?->mapRoles(static fn (string $roleName, array $meta) => match ($roleName) { Role::DOMAIN_SPECIFIC => $nativeQb->andWhere( $nativeQb->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))), ),