From 449a58879660319dfb3ee6c5b5af72eaeb0fc436 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 19 Feb 2025 19:33:44 +0100 Subject: [PATCH] Fix 500 error when listing non-orphan visits with short-url-depending API key --- CHANGELOG.md | 4 ++- .../src/Visit/Repository/VisitRepository.php | 8 ++++- .../Visit/Repository/VisitRepositoryTest.php | 29 +++++++++---------- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e674634..81cb8ba0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). -## [Unreleased] +## [4.4.4] - 2025-02-19 ### Added * *Nothing* @@ -19,6 +19,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Fixed * [#2366](https://github.com/shlinkio/shlink/issues/2366) Fix error "Cannot use 'SCRIPT' with redis-cluster" thrown when creating a lock while using a redis cluster. +* [#2368](https://github.com/shlinkio/shlink/issues/2368) Fix error when listing non-orphan visits using API key with `AUTHORED_SHORT_URLS` role. + ## [4.4.3] - 2025-02-15 ### Added diff --git a/module/Core/src/Visit/Repository/VisitRepository.php b/module/Core/src/Visit/Repository/VisitRepository.php index 9c4668c1..bfba5178 100644 --- a/module/Core/src/Visit/Repository/VisitRepository.php +++ b/module/Core/src/Visit/Repository/VisitRepository.php @@ -21,6 +21,7 @@ use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Spec\CountOfNonOrphanVisits; use Shlinkio\Shlink\Core\Visit\Spec\CountOfOrphanVisits; use Shlinkio\Shlink\Rest\ApiKey\Role; +use Shlinkio\Shlink\Rest\Entity\ApiKey; use const PHP_INT_MAX; @@ -177,7 +178,12 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo $qb = $this->createAllVisitsQueryBuilder($filtering); $qb->andWhere($qb->expr()->isNotNull('v.shortUrl')); - $this->applySpecification($qb, $filtering->apiKey?->inlinedSpec()); + $apiKey = $filtering->apiKey; + if (ApiKey::isShortUrlRestricted($apiKey)) { + $qb->join('v.shortUrl', 's'); + } + + $this->applySpecification($qb, $apiKey?->inlinedSpec(), 'v'); return $this->resolveVisitsWithNativeQuery($qb, $filtering->limit, $filtering->offset); } diff --git a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php index 29227b8b..fe53629f 100644 --- a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php @@ -470,22 +470,18 @@ class VisitRepositoryTest extends DatabaseTestCase #[Test] public function findNonOrphanVisitsReturnsExpectedResult(): void { - $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData(['longUrl' => 'https://1'])); - $this->getEntityManager()->persist($shortUrl); - $this->createVisitsForShortUrl($shortUrl, 7); + $authoredApiKey = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls())); + $this->getEntityManager()->persist($authoredApiKey); - $shortUrl2 = ShortUrl::create(ShortUrlCreation::fromRawData(['longUrl' => 'https://2'])); - $this->getEntityManager()->persist($shortUrl2); - $this->createVisitsForShortUrl($shortUrl2, 4); - - $shortUrl3 = ShortUrl::create(ShortUrlCreation::fromRawData(['longUrl' => 'https://3'])); - $this->getEntityManager()->persist($shortUrl3); - $this->createVisitsForShortUrl($shortUrl3, 10); + $this->createShortUrlsAndVisits(withDomain: false, visitsAmount: 7); + $this->createShortUrlsAndVisits(withDomain: false, apiKey: $authoredApiKey, visitsAmount: 4); + $this->createShortUrlsAndVisits(withDomain: false, visitsAmount: 10); $this->getEntityManager()->flush(); self::assertCount(21, $this->repo->findNonOrphanVisits(new VisitsListFiltering())); self::assertCount(21, $this->repo->findNonOrphanVisits(new VisitsListFiltering(DateRange::allTime()))); + self::assertCount(4, $this->repo->findNonOrphanVisits(new VisitsListFiltering(apiKey: $authoredApiKey))); self::assertCount(7, $this->repo->findNonOrphanVisits(new VisitsListFiltering(DateRange::since( Chronos::parse('2016-01-05')->endOfDay(), )))); @@ -503,11 +499,11 @@ class VisitRepositoryTest extends DatabaseTestCase self::assertCount(3, $this->repo->findNonOrphanVisits(new VisitsListFiltering(DateRange::between( Chronos::parse('2016-01-03')->startOfDay(), Chronos::parse('2016-01-08')->endOfDay(), - ), false, null, 10, 10))); - self::assertCount(15, $this->repo->findNonOrphanVisits(new VisitsListFiltering(null, true))); - self::assertCount(10, $this->repo->findNonOrphanVisits(new VisitsListFiltering(null, false, null, 10))); - self::assertCount(1, $this->repo->findNonOrphanVisits(new VisitsListFiltering(null, false, null, 10, 20))); - self::assertCount(5, $this->repo->findNonOrphanVisits(new VisitsListFiltering(null, false, null, 5, 5))); + ), limit: 10, offset: 10))); + self::assertCount(15, $this->repo->findNonOrphanVisits(new VisitsListFiltering(excludeBots: true))); + self::assertCount(10, $this->repo->findNonOrphanVisits(new VisitsListFiltering(limit: 10))); + self::assertCount(1, $this->repo->findNonOrphanVisits(new VisitsListFiltering(limit: 10, offset: 20))); + self::assertCount(5, $this->repo->findNonOrphanVisits(new VisitsListFiltering(limit: 5, offset: 5))); } #[Test] @@ -535,6 +531,7 @@ class VisitRepositoryTest extends DatabaseTestCase bool|string $withDomain = true, array $tags = [], ApiKey|null $apiKey = null, + int $visitsAmount = 6, ): array { $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ ShortUrlInputFilter::LONG_URL => 'https://longUrl', @@ -545,7 +542,7 @@ class VisitRepositoryTest extends DatabaseTestCase $shortCode = $shortUrl->getShortCode(); $this->getEntityManager()->persist($shortUrl); - $this->createVisitsForShortUrl($shortUrl); + $this->createVisitsForShortUrl($shortUrl, $visitsAmount); if ($withDomain !== false) { $shortUrlWithDomain = ShortUrl::create(ShortUrlCreation::fromRawData([