From 652b0df054d21b9d8d1471063476df584d99769e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 24 Feb 2023 18:01:32 +0100 Subject: [PATCH 1/7] Use native query builders for all queries/sub-queries in tags list --- .../Core/src/Tag/Repository/TagRepository.php | 85 ++++++++++--------- 1 file changed, 45 insertions(+), 40 deletions(-) diff --git a/module/Core/src/Tag/Repository/TagRepository.php b/module/Core/src/Tag/Repository/TagRepository.php index 4e9eafac..83582f8b 100644 --- a/module/Core/src/Tag/Repository/TagRepository.php +++ b/module/Core/src/Tag/Repository/TagRepository.php @@ -15,7 +15,6 @@ 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\ApiKey\Spec\WithInlinedApiKeySpecsEnsuringJoin; use Shlinkio\Shlink\Rest\Entity\ApiKey; use function Functional\map; @@ -46,93 +45,99 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $orderDir = $filtering?->orderBy?->direction; $orderMainQuery = $orderField !== null && OrderableField::isAggregateField($orderField); - $subQb = $this->createQueryBuilder('t'); - $subQb->select('t.id', 't.name'); + $conn = $this->getEntityManager()->getConnection(); + $tagsSubQb = $conn->createQueryBuilder(); + $tagsSubQb->select('t.id', 't.name')->from('tags', 't'); if (! $orderMainQuery) { - $subQb->orderBy('t.name', $orderDir ?? 'ASC') - ->setMaxResults($filtering?->limit ?? PHP_INT_MAX) - ->setFirstResult($filtering?->offset ?? 0); + $tagsSubQb + ->orderBy('t.name', $orderDir ?? 'ASC') + ->setMaxResults($filtering?->limit ?? PHP_INT_MAX) + ->setFirstResult($filtering?->offset ?? 0); // TODO Check if applying limit/offset ot visits sub-queries is needed with large amounts of tags } - $conn = $this->getEntityManager()->getConnection(); - $buildVisitsSubQuery = static function (bool $excludeBots, string $aggregateAlias) use ($conn) { - $visitsSubQuery = $conn->createQueryBuilder(); - $commonJoinCondition = $visitsSubQuery->expr()->eq('v.short_url_id', 's.id'); + $buildVisitsSubQb = static function (bool $excludeBots, string $aggregateAlias) use ($conn) { + $visitsSubQb = $conn->createQueryBuilder(); + $commonJoinCondition = $visitsSubQb->expr()->eq('v.short_url_id', 's.id'); $visitsJoin = ! $excludeBots ? $commonJoinCondition - : $visitsSubQuery->expr()->and( + : $visitsSubQb->expr()->and( $commonJoinCondition, - $visitsSubQuery->expr()->eq('v.potential_bot', $conn->quote('0')), + $visitsSubQb->expr()->eq('v.potential_bot', $conn->quote('0')), ); - return $visitsSubQuery + return $visitsSubQb ->select('st.tag_id AS tag_id', 'COUNT(DISTINCT v.id) AS ' . $aggregateAlias) ->from('visits', 'v') ->join('v', 'short_urls', 's', $visitsJoin) // @phpstan-ignore-line - ->join('s', 'short_urls_in_tags', 'st', $visitsSubQuery->expr()->eq('st.short_url_id', 's.id')) + ->join('s', 'short_urls_in_tags', 'st', $visitsSubQb->expr()->eq('st.short_url_id', 's.id')) ->groupBy('st.tag_id'); }; - $allVisitsSubQuery = $buildVisitsSubQuery(false, 'visits'); - $nonBotVisitsSubQuery = $buildVisitsSubQuery(true, 'non_bot_visits'); + $allVisitsSubQb = $buildVisitsSubQb(false, 'visits'); + $nonBotVisitsSubQb = $buildVisitsSubQb(true, 'non_bot_visits'); $searchTerm = $filtering?->searchTerm; if ($searchTerm !== null) { - $subQb->andWhere($subQb->expr()->like('t.name', $conn->quote('%' . $searchTerm . '%'))); + $tagsSubQb->andWhere($tagsSubQb->expr()->like('t.name', $conn->quote('%' . $searchTerm . '%'))); // TODO Check if applying this to all sub-queries makes it faster or slower } $apiKey = $filtering?->apiKey; - $applyApiKeyToNativeQuery = static fn (?ApiKey $apiKey, NativeQueryBuilder $nativeQueryBuilder) => + $applyApiKeyToNativeQb = static fn (?ApiKey $apiKey, NativeQueryBuilder $qb) => $apiKey?->mapRoles(static fn (Role $role, array $meta) => match ($role) { - Role::DOMAIN_SPECIFIC => $nativeQueryBuilder->andWhere( - $nativeQueryBuilder->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))), + Role::DOMAIN_SPECIFIC => $qb->andWhere( + $qb->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))), ), - Role::AUTHORED_SHORT_URLS => $nativeQueryBuilder->andWhere( - $nativeQueryBuilder->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())), + Role::AUTHORED_SHORT_URLS => $qb->andWhere( + $qb->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())), ), }); // Apply API key specification to all sub-queries - $this->applySpecification($subQb, new WithInlinedApiKeySpecsEnsuringJoin($apiKey), 't'); - $applyApiKeyToNativeQuery($apiKey, $allVisitsSubQuery); - $applyApiKeyToNativeQuery($apiKey, $nonBotVisitsSubQuery); + if ($apiKey && ! $apiKey->isAdmin()) { + $tagsSubQb + ->join('t', 'short_urls_in_tags', 'st', $tagsSubQb->expr()->eq('st.tag_id', 't.id')) + ->join('st', 'short_urls', 's', $tagsSubQb->expr()->eq('st.short_url_id', 's.id')); + } + $applyApiKeyToNativeQb($apiKey, $tagsSubQb); + $applyApiKeyToNativeQb($apiKey, $allVisitsSubQb); + $applyApiKeyToNativeQb($apiKey, $nonBotVisitsSubQb); // 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 = $conn->createQueryBuilder(); - $nativeQb + $mainQb = $conn->createQueryBuilder(); + $mainQb ->select( - 't.id_0 AS id', - 't.name_1 AS name', + 't.id AS id', + 't.name AS name', 'COALESCE(v.visits, 0) AS visits', // COALESCE required for postgres to properly order 'COALESCE(v2.non_bot_visits, 0) AS non_bot_visits', // COALESCE required for postgres to properly order 'COUNT(DISTINCT s.id) AS short_urls_count', ) - ->from('(' . $subQb->getQuery()->getSQL() . ')', 't') // @phpstan-ignore-line - ->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('t', '(' . $allVisitsSubQuery->getSQL() . ')', 'v', $nativeQb->expr()->eq('t.id_0', 'v.tag_id')) - ->leftJoin('t', '(' . $nonBotVisitsSubQuery->getSQL() . ')', 'v2', $nativeQb->expr()->eq( - 't.id_0', + ->from('(' . $tagsSubQb->getSQL() . ')', 't') + ->leftJoin('t', 'short_urls_in_tags', 'st', $mainQb->expr()->eq('t.id', 'st.tag_id')) + ->leftJoin('st', 'short_urls', 's', $mainQb->expr()->eq('s.id', 'st.short_url_id')) + ->leftJoin('t', '(' . $allVisitsSubQb->getSQL() . ')', 'v', $mainQb->expr()->eq('t.id', 'v.tag_id')) + ->leftJoin('t', '(' . $nonBotVisitsSubQb->getSQL() . ')', 'v2', $mainQb->expr()->eq( + 't.id', 'v2.tag_id', )) - ->groupBy('t.id_0', 't.name_1', 'v.visits', 'v2.non_bot_visits'); + ->groupBy('t.id', 't.name', 'v.visits', 'v2.non_bot_visits'); // Apply API key role conditions to the native query too, as they will affect the amounts on the aggregates - $applyApiKeyToNativeQuery($apiKey, $nativeQb); + $applyApiKeyToNativeQb($apiKey, $mainQb); if ($orderMainQuery) { - $nativeQb + $mainQb ->orderBy(OrderableField::toSnakeCaseValidField($orderField), $orderDir ?? 'ASC') ->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); + $mainQb->addOrderBy('t.name', $orderMainQuery || $orderDir === null ? 'ASC' : $orderDir); $rsm = new ResultSetMappingBuilder($this->getEntityManager()); $rsm->addScalarResult('name', 'tag'); @@ -141,7 +146,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $rsm->addScalarResult('short_urls_count', 'shortUrlsCount'); return map( - $this->getEntityManager()->createNativeQuery($nativeQb->getSQL(), $rsm)->getResult(), + $this->getEntityManager()->createNativeQuery($mainQb->getSQL(), $rsm)->getResult(), TagInfo::fromRawData(...), ); } From f4d10df0f33440b4f4b010408877a9459b90a152 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 27 Feb 2023 09:28:27 +0100 Subject: [PATCH 2/7] Delete no longer used spec file --- .../Repository/ShortUrlRepositoryTest.php | 2 +- .../WithInlinedApiKeySpecsEnsuringJoin.php | 26 ------------------- 2 files changed, 1 insertion(+), 27 deletions(-) delete mode 100644 module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php diff --git a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php index f2cce0f7..da1755ac 100644 --- a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php @@ -395,7 +395,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase #[Test] public function importedShortUrlsAreFoundWhenExpected(): void { - $buildImported = static fn (string $shortCode, ?String $domain = null) => + $buildImported = static fn (string $shortCode, ?string $domain = null) => new ImportedShlinkUrl(ImportSource::BITLY, 'foo', [], Chronos::now(), $domain, $shortCode, null); $shortUrlWithoutDomain = ShortUrl::fromImport($buildImported('my-cool-slug'), true); diff --git a/module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php b/module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php deleted file mode 100644 index 2e84d835..00000000 --- a/module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php +++ /dev/null @@ -1,26 +0,0 @@ -apiKey === null || $this->apiKey->isAdmin() ? Spec::andX() : Spec::andX( - Spec::join($this->fieldToJoin, 's'), - $this->apiKey->inlinedSpec(), - ); - } -} From 72898339282c48df58153b5a4233cdd483afa367 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 3 Mar 2023 12:10:41 +0100 Subject: [PATCH 3/7] Move join on short URLs to tags sub-query --- .../Core/src/Tag/Repository/TagRepository.php | 24 +++++++------------ 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/module/Core/src/Tag/Repository/TagRepository.php b/module/Core/src/Tag/Repository/TagRepository.php index 83582f8b..5484ba76 100644 --- a/module/Core/src/Tag/Repository/TagRepository.php +++ b/module/Core/src/Tag/Repository/TagRepository.php @@ -47,7 +47,12 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $conn = $this->getEntityManager()->getConnection(); $tagsSubQb = $conn->createQueryBuilder(); - $tagsSubQb->select('t.id', 't.name')->from('tags', 't'); + $tagsSubQb + ->select('t.id', 't.name', 'COUNT(DISTINCT s.id) AS short_urls_count') + ->from('tags', 't') + ->leftJoin('t', 'short_urls_in_tags', 'st', $tagsSubQb->expr()->eq('st.tag_id', 't.id')) + ->leftJoin('st', 'short_urls', 's', $tagsSubQb->expr()->eq('st.short_url_id', 's.id')) + ->groupBy('t.id', 't.name'); if (! $orderMainQuery) { $tagsSubQb @@ -95,11 +100,6 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito }); // Apply API key specification to all sub-queries - if ($apiKey && ! $apiKey->isAdmin()) { - $tagsSubQb - ->join('t', 'short_urls_in_tags', 'st', $tagsSubQb->expr()->eq('st.tag_id', 't.id')) - ->join('st', 'short_urls', 's', $tagsSubQb->expr()->eq('st.short_url_id', 's.id')); - } $applyApiKeyToNativeQb($apiKey, $tagsSubQb); $applyApiKeyToNativeQb($apiKey, $allVisitsSubQb); $applyApiKeyToNativeQb($apiKey, $nonBotVisitsSubQb); @@ -113,21 +113,15 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito 't.id AS id', 't.name AS name', 'COALESCE(v.visits, 0) AS visits', // COALESCE required for postgres to properly order - 'COALESCE(v2.non_bot_visits, 0) AS non_bot_visits', // COALESCE required for postgres to properly order - 'COUNT(DISTINCT s.id) AS short_urls_count', + 'COALESCE(v2.non_bot_visits, 0) AS non_bot_visits', + 'COALESCE(t.short_urls_count, 0) AS short_urls_count', ) ->from('(' . $tagsSubQb->getSQL() . ')', 't') - ->leftJoin('t', 'short_urls_in_tags', 'st', $mainQb->expr()->eq('t.id', 'st.tag_id')) - ->leftJoin('st', 'short_urls', 's', $mainQb->expr()->eq('s.id', 'st.short_url_id')) ->leftJoin('t', '(' . $allVisitsSubQb->getSQL() . ')', 'v', $mainQb->expr()->eq('t.id', 'v.tag_id')) ->leftJoin('t', '(' . $nonBotVisitsSubQb->getSQL() . ')', 'v2', $mainQb->expr()->eq( 't.id', 'v2.tag_id', - )) - ->groupBy('t.id', 't.name', 'v.visits', 'v2.non_bot_visits'); - - // Apply API key role conditions to the native query too, as they will affect the amounts on the aggregates - $applyApiKeyToNativeQb($apiKey, $mainQb); + )); if ($orderMainQuery) { $mainQb From 1afe08caed162f0981b21879c2ca5890b94f91d9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 4 Mar 2023 09:50:38 +0100 Subject: [PATCH 4/7] Simplify how limits are applied to tags query --- module/Core/src/Tag/Model/OrderableField.php | 6 --- .../Core/src/Tag/Repository/TagRepository.php | 52 ++++++++----------- 2 files changed, 22 insertions(+), 36 deletions(-) diff --git a/module/Core/src/Tag/Model/OrderableField.php b/module/Core/src/Tag/Model/OrderableField.php index 818099de..8c6c7084 100644 --- a/module/Core/src/Tag/Model/OrderableField.php +++ b/module/Core/src/Tag/Model/OrderableField.php @@ -15,12 +15,6 @@ enum OrderableField: string /** @deprecated Use VISITS instead */ case VISITS_COUNT = 'visitsCount'; - public static function isAggregateField(string $field): bool - { - $parsed = self::tryFrom($field); - return $parsed !== null && $parsed !== self::TAG; - } - public static function toSnakeCaseValidField(?string $field): string { $parsed = $field !== null ? self::tryFrom($field) : self::VISITS; diff --git a/module/Core/src/Tag/Repository/TagRepository.php b/module/Core/src/Tag/Repository/TagRepository.php index 5484ba76..c10fce61 100644 --- a/module/Core/src/Tag/Repository/TagRepository.php +++ b/module/Core/src/Tag/Repository/TagRepository.php @@ -18,6 +18,7 @@ use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin; use Shlinkio\Shlink\Rest\Entity\ApiKey; use function Functional\map; +use function Functional\each; use const PHP_INT_MAX; @@ -43,23 +44,23 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito { $orderField = $filtering?->orderBy?->field; $orderDir = $filtering?->orderBy?->direction; - $orderMainQuery = $orderField !== null && OrderableField::isAggregateField($orderField); + $apiKey = $filtering?->apiKey; $conn = $this->getEntityManager()->getConnection(); $tagsSubQb = $conn->createQueryBuilder(); + + // For admins and when no API key is present, we'll return tags which are not linked to any short URL + $joiningMethod = $apiKey === null || $apiKey->isAdmin() ? 'leftJoin' : 'join'; $tagsSubQb ->select('t.id', 't.name', 'COUNT(DISTINCT s.id) AS short_urls_count') ->from('tags', 't') - ->leftJoin('t', 'short_urls_in_tags', 'st', $tagsSubQb->expr()->eq('st.tag_id', 't.id')) - ->leftJoin('st', 'short_urls', 's', $tagsSubQb->expr()->eq('st.short_url_id', 's.id')) + ->{$joiningMethod}('t', 'short_urls_in_tags', 'st', $tagsSubQb->expr()->eq('st.tag_id', 't.id')) + ->{$joiningMethod}('st', 'short_urls', 's', $tagsSubQb->expr()->eq('st.short_url_id', 's.id')) ->groupBy('t.id', 't.name'); - if (! $orderMainQuery) { - $tagsSubQb - ->orderBy('t.name', $orderDir ?? 'ASC') - ->setMaxResults($filtering?->limit ?? PHP_INT_MAX) - ->setFirstResult($filtering?->offset ?? 0); - // TODO Check if applying limit/offset ot visits sub-queries is needed with large amounts of tags + $searchTerm = $filtering?->searchTerm; + if ($searchTerm !== null) { + $tagsSubQb->andWhere($tagsSubQb->expr()->like('t.name', $conn->quote('%' . $searchTerm . '%'))); } $buildVisitsSubQb = static function (bool $excludeBots, string $aggregateAlias) use ($conn) { @@ -82,14 +83,8 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $allVisitsSubQb = $buildVisitsSubQb(false, 'visits'); $nonBotVisitsSubQb = $buildVisitsSubQb(true, 'non_bot_visits'); - $searchTerm = $filtering?->searchTerm; - if ($searchTerm !== null) { - $tagsSubQb->andWhere($tagsSubQb->expr()->like('t.name', $conn->quote('%' . $searchTerm . '%'))); - // TODO Check if applying this to all sub-queries makes it faster or slower - } - - $apiKey = $filtering?->apiKey; - $applyApiKeyToNativeQb = static fn (?ApiKey $apiKey, NativeQueryBuilder $qb) => + // Apply API key specification to all sub-queries + $applyApiKeyToNativeQb = static fn (NativeQueryBuilder $qb) => $apiKey?->mapRoles(static fn (Role $role, array $meta) => match ($role) { Role::DOMAIN_SPECIFIC => $qb->andWhere( $qb->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))), @@ -98,11 +93,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $qb->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())), ), }); - - // Apply API key specification to all sub-queries - $applyApiKeyToNativeQb($apiKey, $tagsSubQb); - $applyApiKeyToNativeQb($apiKey, $allVisitsSubQb); - $applyApiKeyToNativeQb($apiKey, $nonBotVisitsSubQb); + each([$tagsSubQb, $allVisitsSubQb, $nonBotVisitsSubQb], $applyApiKeyToNativeQb); // 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. @@ -110,7 +101,6 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $mainQb = $conn->createQueryBuilder(); $mainQb ->select( - 't.id AS id', 't.name AS name', 'COALESCE(v.visits, 0) AS visits', // COALESCE required for postgres to properly order 'COALESCE(v2.non_bot_visits, 0) AS non_bot_visits', @@ -121,18 +111,20 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito ->leftJoin('t', '(' . $nonBotVisitsSubQb->getSQL() . ')', 'v2', $mainQb->expr()->eq( 't.id', 'v2.tag_id', - )); + )) + ->setMaxResults($filtering?->limit ?? PHP_INT_MAX) + ->setFirstResult($filtering?->offset ?? 0); - if ($orderMainQuery) { + $orderByTag = $orderField == null || $orderField === OrderableField::TAG->value; + if ($orderByTag) { + $mainQb->orderBy('t.name', $orderDir ?? 'ASC'); + } else { $mainQb ->orderBy(OrderableField::toSnakeCaseValidField($orderField), $orderDir ?? 'ASC') - ->setMaxResults($filtering?->limit ?? PHP_INT_MAX) - ->setFirstResult($filtering?->offset ?? 0); + // Add ordering by tag name, as a fallback in case of same amount + ->addOrderBy('t.name', 'ASC'); } - // Add ordering by tag name, as a fallback in case of same amount, or as default ordering - $mainQb->addOrderBy('t.name', $orderMainQuery || $orderDir === null ? 'ASC' : $orderDir); - $rsm = new ResultSetMappingBuilder($this->getEntityManager()); $rsm->addScalarResult('name', 'tag'); $rsm->addScalarResult('visits', 'visits'); From 83c53c8b2eb9b1263c33705507ac2a92e5f12d5e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 4 Mar 2023 09:51:14 +0100 Subject: [PATCH 5/7] Add correct index on visits potential_bot column --- data/migrations/Version20230303164233.php | 28 +++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 data/migrations/Version20230303164233.php diff --git a/data/migrations/Version20230303164233.php b/data/migrations/Version20230303164233.php new file mode 100644 index 00000000..3e64c03c --- /dev/null +++ b/data/migrations/Version20230303164233.php @@ -0,0 +1,28 @@ +getTable('visits'); + $this->skipIf($visits->hasIndex(self::INDEX_NAME)); + + $visits->dropIndex('IDX_visits_potential_bot'); // Old index + $visits->addIndex(['potential_bot'], self::INDEX_NAME); + } + + public function isTransactional(): bool + { + return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform); + } +} From e51384fcc0c4356d6310b1e4228ac6ca1f220dd4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 4 Mar 2023 10:22:46 +0100 Subject: [PATCH 6/7] Reduce duplicated logic when checking if an API key is admin --- module/CLI/src/Command/Api/GenerateKeyCommand.php | 3 ++- module/CLI/src/Command/Api/ListKeysCommand.php | 2 +- module/Core/src/Tag/Repository/TagRepository.php | 10 +++++----- module/Core/src/Tag/TagService.php | 4 ++-- .../src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php | 4 ++-- module/Rest/src/Entity/ApiKey.php | 7 +++++-- 6 files changed, 17 insertions(+), 13 deletions(-) diff --git a/module/CLI/src/Command/Api/GenerateKeyCommand.php b/module/CLI/src/Command/Api/GenerateKeyCommand.php index 12adcd57..c89c4fbf 100644 --- a/module/CLI/src/Command/Api/GenerateKeyCommand.php +++ b/module/CLI/src/Command/Api/GenerateKeyCommand.php @@ -9,6 +9,7 @@ use Shlinkio\Shlink\CLI\ApiKey\RoleResolverInterface; use Shlinkio\Shlink\CLI\Util\ExitCodes; use Shlinkio\Shlink\CLI\Util\ShlinkTable; use Shlinkio\Shlink\Rest\ApiKey\Role; +use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; @@ -99,7 +100,7 @@ class GenerateKeyCommand extends Command $io = new SymfonyStyle($input, $output); $io->success(sprintf('Generated API key: "%s"', $apiKey->toString())); - if (! $apiKey->isAdmin()) { + if (! ApiKey::isAdmin($apiKey)) { ShlinkTable::default($io)->render( ['Role name', 'Role metadata'], $apiKey->mapRoles(fn (Role $role, array $meta) => [$role->value, arrayToString($meta, 0)]), diff --git a/module/CLI/src/Command/Api/ListKeysCommand.php b/module/CLI/src/Command/Api/ListKeysCommand.php index 59f5b534..c7e31819 100644 --- a/module/CLI/src/Command/Api/ListKeysCommand.php +++ b/module/CLI/src/Command/Api/ListKeysCommand.php @@ -59,7 +59,7 @@ class ListKeysCommand extends Command $rowData[] = sprintf($messagePattern, $this->getEnabledSymbol($apiKey)); } $rowData[] = $expiration?->toAtomString() ?? '-'; - $rowData[] = $apiKey->isAdmin() ? 'Admin' : implode("\n", $apiKey->mapRoles( + $rowData[] = ApiKey::isAdmin($apiKey) ? 'Admin' : implode("\n", $apiKey->mapRoles( fn (Role $role, array $meta) => empty($meta) ? $role->toFriendlyName() diff --git a/module/Core/src/Tag/Repository/TagRepository.php b/module/Core/src/Tag/Repository/TagRepository.php index c10fce61..62429d95 100644 --- a/module/Core/src/Tag/Repository/TagRepository.php +++ b/module/Core/src/Tag/Repository/TagRepository.php @@ -17,8 +17,8 @@ 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 Functional\each; +use function Functional\map; use const PHP_INT_MAX; @@ -50,13 +50,13 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $tagsSubQb = $conn->createQueryBuilder(); // For admins and when no API key is present, we'll return tags which are not linked to any short URL - $joiningMethod = $apiKey === null || $apiKey->isAdmin() ? 'leftJoin' : 'join'; + $joiningMethod = ApiKey::isAdmin($apiKey) ? 'leftJoin' : 'join'; $tagsSubQb ->select('t.id', 't.name', 'COUNT(DISTINCT s.id) AS short_urls_count') ->from('tags', 't') + ->groupBy('t.id', 't.name') ->{$joiningMethod}('t', 'short_urls_in_tags', 'st', $tagsSubQb->expr()->eq('st.tag_id', 't.id')) - ->{$joiningMethod}('st', 'short_urls', 's', $tagsSubQb->expr()->eq('st.short_url_id', 's.id')) - ->groupBy('t.id', 't.name'); + ->{$joiningMethod}('st', 'short_urls', 's', $tagsSubQb->expr()->eq('st.short_url_id', 's.id')); $searchTerm = $filtering?->searchTerm; if ($searchTerm !== null) { @@ -115,7 +115,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito ->setMaxResults($filtering?->limit ?? PHP_INT_MAX) ->setFirstResult($filtering?->offset ?? 0); - $orderByTag = $orderField == null || $orderField === OrderableField::TAG->value; + $orderByTag = $orderField === null || $orderField === OrderableField::TAG->value; if ($orderByTag) { $mainQb->orderBy('t.name', $orderDir ?? 'ASC'); } else { diff --git a/module/Core/src/Tag/TagService.php b/module/Core/src/Tag/TagService.php index d50ced75..ea9a4e8b 100644 --- a/module/Core/src/Tag/TagService.php +++ b/module/Core/src/Tag/TagService.php @@ -59,7 +59,7 @@ class TagService implements TagServiceInterface */ public function deleteTags(array $tagNames, ?ApiKey $apiKey = null): void { - if ($apiKey !== null && ! $apiKey->isAdmin()) { + if (! ApiKey::isAdmin($apiKey)) { throw ForbiddenTagOperationException::forDeletion(); } @@ -75,7 +75,7 @@ class TagService implements TagServiceInterface */ public function renameTag(TagRenaming $renaming, ?ApiKey $apiKey = null): Tag { - if ($apiKey !== null && ! $apiKey->isAdmin()) { + if (! ApiKey::isAdmin($apiKey)) { throw ForbiddenTagOperationException::forRenaming(); } diff --git a/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php b/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php index 1f8c2fd3..9a8f8056 100644 --- a/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php +++ b/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php @@ -11,14 +11,14 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; class WithApiKeySpecsEnsuringJoin extends BaseSpecification { - public function __construct(private ?ApiKey $apiKey, private string $fieldToJoin = 'shortUrls') + public function __construct(private readonly ?ApiKey $apiKey, private readonly string $fieldToJoin = 'shortUrls') { parent::__construct(); } protected function getSpec(): Specification { - return $this->apiKey === null || $this->apiKey->isAdmin() ? Spec::andX() : Spec::andX( + return $this->apiKey === null || ApiKey::isAdmin($this->apiKey) ? Spec::andX() : Spec::andX( Spec::join($this->fieldToJoin, 's'), $this->apiKey->spec($this->fieldToJoin), ); diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index 57fecdd0..88cfa27e 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -114,9 +114,12 @@ class ApiKey extends AbstractEntity return Spec::andX(...$specs); } - public function isAdmin(): bool + /** + * @return ($apiKey is null ? true : boolean) + */ + public static function isAdmin(?ApiKey $apiKey): bool { - return $this->roles->isEmpty(); + return $apiKey === null || $apiKey->roles->isEmpty(); } public function hasRole(Role $role): bool From 01bcedef7ae1410fc9a962b03a577b19a0d9e352 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 4 Mar 2023 11:54:30 +0100 Subject: [PATCH 7/7] Simplify how ordering field is resolved in tags list --- module/Core/src/Tag/Model/OrderableField.php | 10 ++-- .../Core/src/Tag/Repository/TagRepository.php | 54 +++++++++---------- 2 files changed, 27 insertions(+), 37 deletions(-) diff --git a/module/Core/src/Tag/Model/OrderableField.php b/module/Core/src/Tag/Model/OrderableField.php index 8c6c7084..b7a9509f 100644 --- a/module/Core/src/Tag/Model/OrderableField.php +++ b/module/Core/src/Tag/Model/OrderableField.php @@ -4,8 +4,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Tag\Model; -use function Shlinkio\Shlink\Core\camelCaseToSnakeCase; - enum OrderableField: string { case TAG = 'tag'; @@ -15,14 +13,12 @@ enum OrderableField: string /** @deprecated Use VISITS instead */ case VISITS_COUNT = 'visitsCount'; - public static function toSnakeCaseValidField(?string $field): string + public static function toSnakeCaseValidField(?string $field): self { - $parsed = $field !== null ? self::tryFrom($field) : self::VISITS; - $normalized = match ($parsed) { + $parsed = $field !== null ? self::tryFrom($field) : self::TAG; + return match ($parsed) { self::VISITS_COUNT, null => self::VISITS, default => $parsed, }; - - return camelCaseToSnakeCase($normalized->value); } } diff --git a/module/Core/src/Tag/Repository/TagRepository.php b/module/Core/src/Tag/Repository/TagRepository.php index 62429d95..68e5df4b 100644 --- a/module/Core/src/Tag/Repository/TagRepository.php +++ b/module/Core/src/Tag/Repository/TagRepository.php @@ -19,6 +19,7 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; use function Functional\each; use function Functional\map; +use function Shlinkio\Shlink\Core\camelCaseToSnakeCase; use const PHP_INT_MAX; @@ -42,17 +43,26 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito */ public function findTagsWithInfo(?TagsListFiltering $filtering = null): array { - $orderField = $filtering?->orderBy?->field; - $orderDir = $filtering?->orderBy?->direction; + $orderField = OrderableField::toSnakeCaseValidField($filtering?->orderBy?->field); + $orderDir = $filtering?->orderBy?->direction ?? 'ASC'; $apiKey = $filtering?->apiKey; - $conn = $this->getEntityManager()->getConnection(); - $tagsSubQb = $conn->createQueryBuilder(); + + $applyApiKeyToNativeQb = static fn (NativeQueryBuilder $qb) => + $apiKey?->mapRoles(static fn (Role $role, array $meta) => match ($role) { + Role::DOMAIN_SPECIFIC => $qb->andWhere( + $qb->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))), + ), + Role::AUTHORED_SHORT_URLS => $qb->andWhere( + $qb->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())), + ), + }); // For admins and when no API key is present, we'll return tags which are not linked to any short URL $joiningMethod = ApiKey::isAdmin($apiKey) ? 'leftJoin' : 'join'; + $tagsSubQb = $conn->createQueryBuilder(); $tagsSubQb - ->select('t.id', 't.name', 'COUNT(DISTINCT s.id) AS short_urls_count') + ->select('t.id AS tag_id', 't.name AS tag', 'COUNT(DISTINCT s.id) AS short_urls_count') ->from('tags', 't') ->groupBy('t.id', 't.name') ->{$joiningMethod}('t', 'short_urls_in_tags', 'st', $tagsSubQb->expr()->eq('st.tag_id', 't.id')) @@ -84,15 +94,6 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $nonBotVisitsSubQb = $buildVisitsSubQb(true, 'non_bot_visits'); // Apply API key specification to all sub-queries - $applyApiKeyToNativeQb = static fn (NativeQueryBuilder $qb) => - $apiKey?->mapRoles(static fn (Role $role, array $meta) => match ($role) { - Role::DOMAIN_SPECIFIC => $qb->andWhere( - $qb->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))), - ), - Role::AUTHORED_SHORT_URLS => $qb->andWhere( - $qb->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())), - ), - }); each([$tagsSubQb, $allVisitsSubQb, $nonBotVisitsSubQb], $applyApiKeyToNativeQb); // A native query builder needs to be used here, because DQL and ORM query builders do not support @@ -101,32 +102,25 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $mainQb = $conn->createQueryBuilder(); $mainQb ->select( - 't.name AS name', + 't.tag AS tag', 'COALESCE(v.visits, 0) AS visits', // COALESCE required for postgres to properly order - 'COALESCE(v2.non_bot_visits, 0) AS non_bot_visits', + 'COALESCE(b.non_bot_visits, 0) AS non_bot_visits', 'COALESCE(t.short_urls_count, 0) AS short_urls_count', ) ->from('(' . $tagsSubQb->getSQL() . ')', 't') - ->leftJoin('t', '(' . $allVisitsSubQb->getSQL() . ')', 'v', $mainQb->expr()->eq('t.id', 'v.tag_id')) - ->leftJoin('t', '(' . $nonBotVisitsSubQb->getSQL() . ')', 'v2', $mainQb->expr()->eq( - 't.id', - 'v2.tag_id', - )) + ->leftJoin('t', '(' . $allVisitsSubQb->getSQL() . ')', 'v', $mainQb->expr()->eq('t.tag_id', 'v.tag_id')) + ->leftJoin('t', '(' . $nonBotVisitsSubQb->getSQL() . ')', 'b', $mainQb->expr()->eq('t.tag_id', 'b.tag_id')) ->setMaxResults($filtering?->limit ?? PHP_INT_MAX) ->setFirstResult($filtering?->offset ?? 0); - $orderByTag = $orderField === null || $orderField === OrderableField::TAG->value; - if ($orderByTag) { - $mainQb->orderBy('t.name', $orderDir ?? 'ASC'); - } else { - $mainQb - ->orderBy(OrderableField::toSnakeCaseValidField($orderField), $orderDir ?? 'ASC') - // Add ordering by tag name, as a fallback in case of same amount - ->addOrderBy('t.name', 'ASC'); + $mainQb->orderBy(camelCaseToSnakeCase($orderField->value), $orderDir); + if ($orderField !== OrderableField::TAG) { + // Add ordering by tag name, as a fallback in case of same amounts + $mainQb->addOrderBy('tag', 'ASC'); } $rsm = new ResultSetMappingBuilder($this->getEntityManager()); - $rsm->addScalarResult('name', 'tag'); + $rsm->addScalarResult('tag', 'tag'); $rsm->addScalarResult('visits', 'visits'); $rsm->addScalarResult('non_bot_visits', 'nonBotVisits'); $rsm->addScalarResult('short_urls_count', 'shortUrlsCount');