From f678873e9fc8e5b7646a83ee6ccce276a8a5d358 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 21 Mar 2024 08:47:39 +0100 Subject: [PATCH] Use pre-calculated visits counts when listing short URLs --- .../Command/ShortUrl/ListShortUrlsCommand.php | 9 ++-- module/Core/src/ShortUrl/Entity/ShortUrl.php | 4 +- .../Model/ShortUrlWithVisitsSummary.php | 31 +++++++++++++ .../Adapter/ShortUrlRepositoryAdapter.php | 10 ++--- .../Repository/ShortUrlListRepository.php | 43 ++++++++----------- .../ShortUrlListRepositoryInterface.php | 4 +- .../Core/src/ShortUrl/ShortUrlListService.php | 10 ++--- .../Transformer/ShortUrlDataTransformer.php | 11 +++-- 8 files changed, 75 insertions(+), 47 deletions(-) create mode 100644 module/Core/src/ShortUrl/Model/ShortUrlWithVisitsSummary.php diff --git a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php index c4346f14..b03e0312 100644 --- a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php +++ b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php @@ -13,6 +13,7 @@ use Shlinkio\Shlink\Common\Paginator\Util\PagerfantaUtilsTrait; use Shlinkio\Shlink\Common\Rest\DataTransformerInterface; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlsParams; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; use Shlinkio\Shlink\Core\ShortUrl\Model\TagsMode; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlsParamsInputFilter; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlListServiceInterface; @@ -23,10 +24,10 @@ use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; use function array_keys; -use function array_map; use function array_pad; use function explode; use function implode; +use function Shlinkio\Shlink\Core\ArrayUtils\map; use function sprintf; class ListShortUrlsCommand extends Command @@ -184,10 +185,10 @@ class ListShortUrlsCommand extends Command ): Paginator { $shortUrls = $this->shortUrlService->listShortUrls($params); - $rows = array_map(function (ShortUrl $shortUrl) use ($columnsMap) { + $rows = map([...$shortUrls], function (ShortUrlWithVisitsSummary $shortUrl) use ($columnsMap) { $rawShortUrl = $this->transformer->transform($shortUrl); - return array_map(fn (callable $call) => $call($rawShortUrl, $shortUrl), $columnsMap); - }, [...$shortUrls]); + return map($columnsMap, fn (callable $call) => $call($rawShortUrl, $shortUrl)); + }); ShlinkTable::default($output)->render( array_keys($columnsMap), diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index cc7ebc85..ac21b34b 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -256,7 +256,7 @@ class ShortUrl extends AbstractEntity return true; } - public function toArray(): array + public function toArray(?VisitsSummary $precalculatedSummary = null): array { return [ 'shortCode' => $this->shortCode, @@ -272,7 +272,7 @@ class ShortUrl extends AbstractEntity 'title' => $this->title, 'crawlable' => $this->crawlable, 'forwardQuery' => $this->forwardQuery, - 'visitsSummary' => VisitsSummary::fromTotalAndNonBots( + 'visitsSummary' => $precalculatedSummary ?? VisitsSummary::fromTotalAndNonBots( count($this->visits), count($this->visits->matching( Criteria::create()->where(Criteria::expr()->eq('potentialBot', false)), diff --git a/module/Core/src/ShortUrl/Model/ShortUrlWithVisitsSummary.php b/module/Core/src/ShortUrl/Model/ShortUrlWithVisitsSummary.php new file mode 100644 index 00000000..8244cde4 --- /dev/null +++ b/module/Core/src/ShortUrl/Model/ShortUrlWithVisitsSummary.php @@ -0,0 +1,31 @@ +shortUrl->toArray($this->visitsSummary); + } +} diff --git a/module/Core/src/ShortUrl/Paginator/Adapter/ShortUrlRepositoryAdapter.php b/module/Core/src/ShortUrl/Paginator/Adapter/ShortUrlRepositoryAdapter.php index 83ce8bd9..56f8e5a5 100644 --- a/module/Core/src/ShortUrl/Paginator/Adapter/ShortUrlRepositoryAdapter.php +++ b/module/Core/src/ShortUrl/Paginator/Adapter/ShortUrlRepositoryAdapter.php @@ -11,13 +11,13 @@ use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsListFiltering; use Shlinkio\Shlink\Core\ShortUrl\Repository\ShortUrlListRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; -class ShortUrlRepositoryAdapter implements AdapterInterface +readonly class ShortUrlRepositoryAdapter implements AdapterInterface { public function __construct( - private readonly ShortUrlListRepositoryInterface $repository, - private readonly ShortUrlsParams $params, - private readonly ?ApiKey $apiKey, - private readonly string $defaultDomain, + private ShortUrlListRepositoryInterface $repository, + private ShortUrlsParams $params, + private ?ApiKey $apiKey, + private string $defaultDomain, ) { } diff --git a/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php b/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php index 2f638c73..0bcf7974 100644 --- a/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php +++ b/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php @@ -11,34 +11,39 @@ use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\OrderableField; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; use Shlinkio\Shlink\Core\ShortUrl\Model\TagsMode; use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsCountFiltering; use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsListFiltering; use Shlinkio\Shlink\Core\Visit\Entity\Visit; -use function array_column; +use function Shlinkio\Shlink\Core\ArrayUtils\map; use function sprintf; class ShortUrlListRepository extends EntitySpecificationRepository implements ShortUrlListRepositoryInterface { /** - * @return ShortUrl[] + * @return ShortUrlWithVisitsSummary[] */ public function findList(ShortUrlsListFiltering $filtering): array { $qb = $this->createListQueryBuilder($filtering); - $qb->select('DISTINCT s') + $qb->select('DISTINCT s AS shortUrl', 'SUM(v.count) AS visitsCount', 'SUM(v2.count) AS nonBotVisitsCount') + ->addSelect('SUM(v.count)') + ->leftJoin('s.visitsCounts', 'v') + ->leftJoin('s.visitsCounts', 'v2', Join::WITH, $qb->expr()->andX( + $qb->expr()->eq('v.shortUrl', 's'), + $qb->expr()->eq('v.potentialBot', 'false'), + )) + ->groupBy('s') ->setMaxResults($filtering->limit) ->setFirstResult($filtering->offset); $this->processOrderByForList($qb, $filtering); + /** @var array{shortUrl: ShortUrl, visitsCount: string, nonBotVisitsCount: string}[] $result */ $result = $qb->getQuery()->getResult(); - if (OrderableField::isVisitsField($filtering->orderBy->field ?? '')) { - return array_column($result, 0); - } - - return $result; + return map($result, static fn (array $s) => ShortUrlWithVisitsSummary::fromArray($s)); } private function processOrderByForList(QueryBuilder $qb, ShortUrlsListFiltering $filtering): void @@ -51,26 +56,12 @@ class ShortUrlListRepository extends EntitySpecificationRepository implements Sh } $order = $filtering->orderBy->direction; - if (OrderableField::isBasicField($fieldName)) { $qb->orderBy('s.' . $fieldName, $order); - } elseif (OrderableField::isVisitsField($fieldName)) { - $leftJoinConditions = [$qb->expr()->eq('v.shortUrl', 's')]; - if ($fieldName === OrderableField::NON_BOT_VISITS->value) { - $leftJoinConditions[] = $qb->expr()->eq('v.potentialBot', 'false'); - } - - $qb->addSelect('SUM(v.count)') - ->leftJoin('s.visitsCounts', 'v', Join::WITH, $qb->expr()->andX(...$leftJoinConditions)) - ->groupBy('s') - ->orderBy('SUM(v.count)', $order); - - // FIXME This query is inefficient. - // Diagnostic: It might need to use a sub-query, as done with the tags list query. -// $qb->addSelect('COUNT(DISTINCT v)') -// ->leftJoin('s.visits', 'v', Join::WITH, $qb->expr()->andX(...$leftJoinConditions)) -// ->groupBy('s') -// ->orderBy('COUNT(DISTINCT v)', $order); + } elseif (OrderableField::VISITS->value === $fieldName) { + $qb->orderBy('SUM(v.count)', $order); + } elseif (OrderableField::NON_BOT_VISITS->value === $fieldName) { + $qb->orderBy('SUM(v2.count)', $order); } } diff --git a/module/Core/src/ShortUrl/Repository/ShortUrlListRepositoryInterface.php b/module/Core/src/ShortUrl/Repository/ShortUrlListRepositoryInterface.php index 130e0db7..db3f8017 100644 --- a/module/Core/src/ShortUrl/Repository/ShortUrlListRepositoryInterface.php +++ b/module/Core/src/ShortUrl/Repository/ShortUrlListRepositoryInterface.php @@ -4,14 +4,14 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl\Repository; -use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsCountFiltering; use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsListFiltering; interface ShortUrlListRepositoryInterface { /** - * @return ShortUrl[] + * @return ShortUrlWithVisitsSummary[] */ public function findList(ShortUrlsListFiltering $filtering): array; diff --git a/module/Core/src/ShortUrl/ShortUrlListService.php b/module/Core/src/ShortUrl/ShortUrlListService.php index 60f56554..d86c4988 100644 --- a/module/Core/src/ShortUrl/ShortUrlListService.php +++ b/module/Core/src/ShortUrl/ShortUrlListService.php @@ -6,22 +6,22 @@ namespace Shlinkio\Shlink\Core\ShortUrl; use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; -use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlsParams; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; use Shlinkio\Shlink\Core\ShortUrl\Paginator\Adapter\ShortUrlRepositoryAdapter; use Shlinkio\Shlink\Core\ShortUrl\Repository\ShortUrlListRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; -class ShortUrlListService implements ShortUrlListServiceInterface +readonly class ShortUrlListService implements ShortUrlListServiceInterface { public function __construct( - private readonly ShortUrlListRepositoryInterface $repo, - private readonly UrlShortenerOptions $urlShortenerOptions, + private ShortUrlListRepositoryInterface $repo, + private UrlShortenerOptions $urlShortenerOptions, ) { } /** - * @return ShortUrl[]|Paginator + * @return ShortUrlWithVisitsSummary[]|Paginator */ public function listShortUrls(ShortUrlsParams $params, ?ApiKey $apiKey = null): Paginator { diff --git a/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php b/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php index 09b9436b..413f5a69 100644 --- a/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php +++ b/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php @@ -7,7 +7,11 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Transformer; use Shlinkio\Shlink\Common\Rest\DataTransformerInterface; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifierInterface; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; +/** + * @fixme Do not implement DataTransformerInterface, but a separate interface + */ readonly class ShortUrlDataTransformer implements DataTransformerInterface { public function __construct(private ShortUrlStringifierInterface $stringifier) @@ -15,13 +19,14 @@ readonly class ShortUrlDataTransformer implements DataTransformerInterface } /** - * @param ShortUrl $shortUrl + * @param ShortUrlWithVisitsSummary|ShortUrl $data */ - public function transform($shortUrl): array // phpcs:ignore + public function transform($data): array // phpcs:ignore { + $shortUrl = $data instanceof ShortUrlWithVisitsSummary ? $data->shortUrl : $data; return [ 'shortUrl' => $this->stringifier->stringify($shortUrl), - ...$shortUrl->toArray(), + ...$data->toArray(), ]; } }