From ce9ec0d7386d55f48365fb000f2a2202cc0d0a98 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 2 Jan 2023 19:49:54 +0100 Subject: [PATCH] Fixed ordering in tags supporting more fields --- module/Core/src/Tag/Model/OrderableField.php | 20 ++++++----- module/Core/src/Tag/Model/TagInfo.php | 4 +-- .../Core/src/Tag/Repository/TagRepository.php | 8 ++--- .../Tag/Repository/TagRepositoryTest.php | 36 ++++++++++++++----- 4 files changed, 45 insertions(+), 23 deletions(-) diff --git a/module/Core/src/Tag/Model/OrderableField.php b/module/Core/src/Tag/Model/OrderableField.php index cb398ebb..eb802a7f 100644 --- a/module/Core/src/Tag/Model/OrderableField.php +++ b/module/Core/src/Tag/Model/OrderableField.php @@ -9,22 +9,26 @@ use function Shlinkio\Shlink\Core\camelCaseToSnakeCase; enum OrderableField: string { case TAG = 'tag'; -// case SHORT_URLS = 'shortUrls'; -// case VISITS = 'visits'; -// case NON_BOT_VISITS = 'nonBotVisits'; - + case SHORT_URLS_COUNT = 'shortUrlsCount'; + case VISITS = 'visits'; + case NON_BOT_VISITS = 'nonBotVisits'; /** @deprecated Use VISITS instead */ case VISITS_COUNT = 'visitsCount'; - /** @deprecated Use SHORT_URLS instead */ - case SHORT_URLS_COUNT = 'shortUrlsCount'; public static function isAggregateField(string $field): bool { - return $field === self::SHORT_URLS_COUNT->value || $field === self::VISITS_COUNT->value; + $parsed = self::tryFrom($field); + return $parsed !== null && $parsed !== self::TAG; } public static function toSnakeCaseValidField(?string $field): string { - return camelCaseToSnakeCase($field === self::SHORT_URLS_COUNT->value ? $field : self::VISITS_COUNT->value); + $parsed = self::tryFrom($field); + $normalized = match ($parsed) { + self::VISITS_COUNT, null => self::VISITS, + default => $parsed, + }; + + return camelCaseToSnakeCase($normalized->value); } } diff --git a/module/Core/src/Tag/Model/TagInfo.php b/module/Core/src/Tag/Model/TagInfo.php index adf5d4b1..4c0018b2 100644 --- a/module/Core/src/Tag/Model/TagInfo.php +++ b/module/Core/src/Tag/Model/TagInfo.php @@ -25,8 +25,8 @@ final class TagInfo implements JsonSerializable return new self( $data['tag'], (int) $data['shortUrlsCount'], - (int) $data['visitsCount'], - isset($data['nonBotVisitsCount']) ? (int) $data['nonBotVisitsCount'] : null, + (int) $data['visits'], + isset($data['nonBotVisits']) ? (int) $data['nonBotVisits'] : null, ); } diff --git a/module/Core/src/Tag/Repository/TagRepository.php b/module/Core/src/Tag/Repository/TagRepository.php index feaaa7d5..5dd9dcd9 100644 --- a/module/Core/src/Tag/Repository/TagRepository.php +++ b/module/Core/src/Tag/Repository/TagRepository.php @@ -72,8 +72,8 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito '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', // Native queries require snake_case for cross-db compatibility - 'COUNT(DISTINCT v2.id) AS non_bot_visits_count', + 'COUNT(DISTINCT v.id) AS visits', // Native queries require snake_case for cross-db compatibility + 'COUNT(DISTINCT v2.id) AS non_bot_visits', ) ->from('(' . $subQb->getQuery()->getSQL() . ')', 't') // @phpstan-ignore-line ->leftJoin('t', 'short_urls_in_tags', 'st', $nativeQb->expr()->eq('t.id_0', 'st.tag_id')) @@ -108,8 +108,8 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $rsm = new ResultSetMappingBuilder($this->getEntityManager()); $rsm->addScalarResult('name', 'tag'); $rsm->addScalarResult('short_urls_count', 'shortUrlsCount'); - $rsm->addScalarResult('visits_count', 'visitsCount'); - $rsm->addScalarResult('non_bot_visits_count', 'nonBotVisitsCount'); + $rsm->addScalarResult('visits', 'visits'); + $rsm->addScalarResult('non_bot_visits', 'nonBotVisits'); return map( $this->getEntityManager()->createNativeQuery($nativeQb->getSQL(), $rsm)->getResult(), diff --git a/module/Core/test-db/Tag/Repository/TagRepositoryTest.php b/module/Core/test-db/Tag/Repository/TagRepositoryTest.php index fe030ca0..57b3a795 100644 --- a/module/Core/test-db/Tag/Repository/TagRepositoryTest.php +++ b/module/Core/test-db/Tag/Repository/TagRepositoryTest.php @@ -10,6 +10,7 @@ use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver; use Shlinkio\Shlink\Core\Tag\Entity\Tag; +use Shlinkio\Shlink\Core\Tag\Model\OrderableField; use Shlinkio\Shlink\Core\Tag\Model\TagsListFiltering; use Shlinkio\Shlink\Core\Tag\Repository\TagRepository; use Shlinkio\Shlink\Core\Visit\Entity\Visit; @@ -135,17 +136,21 @@ class TagRepositoryTest extends DatabaseTestCase ['baz', 1, 3, 2], ]]; yield 'ASC ordering' => [ - new TagsListFiltering(null, null, null, Ordering::fromTuple(['tag', 'ASC'])), + new TagsListFiltering(null, null, null, Ordering::fromTuple([OrderableField::TAG->value, 'ASC'])), $defaultList, ]; - yield 'DESC ordering' => [new TagsListFiltering(null, null, null, Ordering::fromTuple(['tag', 'DESC'])), [ + yield 'DESC ordering' => [new TagsListFiltering(null, null, null, Ordering::fromTuple( + [OrderableField::TAG->value, 'DESC'], + )), [ ['foo', 2, 4, 3], ['baz', 1, 3, 2], ['bar', 3, 3, 2], ['another', 0, 0, 0], ]]; yield 'short URLs count ASC ordering' => [ - new TagsListFiltering(null, null, null, Ordering::fromTuple(['shortUrlsCount', 'ASC'])), + new TagsListFiltering(null, null, null, Ordering::fromTuple( + [OrderableField::SHORT_URLS_COUNT->value, 'ASC'], + )), [ ['another', 0, 0, 0], ['baz', 1, 3, 2], @@ -154,7 +159,9 @@ class TagRepositoryTest extends DatabaseTestCase ], ]; yield 'short URLs count DESC ordering' => [ - new TagsListFiltering(null, null, null, Ordering::fromTuple(['shortUrlsCount', 'DESC'])), + new TagsListFiltering(null, null, null, Ordering::fromTuple( + [OrderableField::SHORT_URLS_COUNT->value, 'DESC'], + )), [ ['bar', 3, 3, 2], ['foo', 2, 4, 3], @@ -163,7 +170,18 @@ class TagRepositoryTest extends DatabaseTestCase ], ]; yield 'visits count ASC ordering' => [ - new TagsListFiltering(null, null, null, Ordering::fromTuple(['visitsCount', 'ASC'])), + new TagsListFiltering(null, null, null, Ordering::fromTuple([OrderableField::VISITS->value, 'ASC'])), + [ + ['another', 0, 0, 0], + ['bar', 3, 3, 2], + ['baz', 1, 3, 2], + ['foo', 2, 4, 3], + ], + ]; + yield 'non-bot visits count ASC ordering' => [ + new TagsListFiltering(null, null, null, Ordering::fromTuple( + [OrderableField::NON_BOT_VISITS->value, 'ASC'], + )), [ ['another', 0, 0, 0], ['bar', 3, 3, 2], @@ -172,7 +190,7 @@ class TagRepositoryTest extends DatabaseTestCase ], ]; yield 'visits count DESC ordering' => [ - new TagsListFiltering(null, null, null, Ordering::fromTuple(['visitsCount', 'DESC'])), + new TagsListFiltering(null, null, null, Ordering::fromTuple([OrderableField::VISITS->value, 'DESC'])), [ ['foo', 2, 4, 3], ['bar', 3, 3, 2], @@ -181,7 +199,7 @@ class TagRepositoryTest extends DatabaseTestCase ], ]; yield 'visits count DESC ordering and limit' => [ - new TagsListFiltering(2, null, null, Ordering::fromTuple(['visitsCount', 'DESC'])), + new TagsListFiltering(2, null, null, Ordering::fromTuple([OrderableField::VISITS_COUNT->value, 'DESC'])), [ ['foo', 2, 4, 3], ['bar', 3, 3, 2], @@ -195,11 +213,11 @@ class TagRepositoryTest extends DatabaseTestCase ['foo', 1, 3, 2], ]]; yield 'combined' => [new TagsListFiltering(1, null, null, Ordering::fromTuple( - ['shortUrls', 'DESC'], + [OrderableField::SHORT_URLS_COUNT->value, 'DESC'], ), ApiKey::fromMeta( ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls()), )), [ - ['foo', 1, 3, 2], + ['bar', 2, 3, 2], ]]; }