From 9dcc51abde0f517d5080d3e7db169e325958744e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 29 Oct 2025 10:14:28 +0100 Subject: [PATCH] Allow filtering by domain in VisitRepository::findVisitsByTag --- .../Repository/ShortUrlListRepository.php | 12 +++--- .../src/Visit/Repository/VisitRepository.php | 26 +++++++---- .../Visit/Repository/VisitRepositoryTest.php | 43 +++++++++++++++---- 3 files changed, 57 insertions(+), 24 deletions(-) diff --git a/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php b/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php index 266d19e1..8b720471 100644 --- a/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php +++ b/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php @@ -159,13 +159,11 @@ class ShortUrlListRepository extends EntitySpecificationRepository implements Sh $qb->andWhere($qb->expr()->notIn('s.id', $subQb->getDQL())); } - if ($filtering->domain !== null) { - if ($filtering->domain === Domain::DEFAULT_AUTHORITY) { - $qb->andWhere($qb->expr()->isNull('s.domain')); - } else { - $qb->andWhere($qb->expr()->eq('d.authority', ':domain')) - ->setParameter('domain', $filtering->domain); - } + if ($filtering->domain === Domain::DEFAULT_AUTHORITY) { + $qb->andWhere($qb->expr()->isNull('s.domain')); + } elseif ($filtering->domain !== null) { + $qb->andWhere($qb->expr()->eq('d.authority', ':domain')) + ->setParameter('domain', $filtering->domain); } if ($filtering->excludeMaxVisitsReached) { diff --git a/module/Core/src/Visit/Repository/VisitRepository.php b/module/Core/src/Visit/Repository/VisitRepository.php index 8702a849..ff2fba32 100644 --- a/module/Core/src/Visit/Repository/VisitRepository.php +++ b/module/Core/src/Visit/Repository/VisitRepository.php @@ -18,6 +18,7 @@ use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; +use Shlinkio\Shlink\Core\Visit\Persistence\WithDomainVisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\WithDomainVisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Spec\CountOfNonOrphanVisits; use Shlinkio\Shlink\Core\Visit\Spec\CountOfOrphanVisits; @@ -76,7 +77,7 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo return $this->resolveVisitsWithNativeQuery($qb, $filtering->limit, $filtering->offset); } - public function countVisitsByTag(string $tag, VisitsCountFiltering $filtering): int + public function countVisitsByTag(string $tag, WithDomainVisitsCountFiltering $filtering): int { $qb = $this->createVisitsByTagQueryBuilder($tag, $filtering); $qb->select('COUNT(v.id)'); @@ -84,19 +85,29 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo return (int) $qb->getQuery()->getSingleScalarResult(); } - private function createVisitsByTagQueryBuilder(string $tag, VisitsCountFiltering $filtering): QueryBuilder + private function createVisitsByTagQueryBuilder(string $tag, WithDomainVisitsCountFiltering $filtering): QueryBuilder { + $conn = $this->getEntityManager()->getConnection(); + // Parameters in this query need to be inlined, not bound, as we need to use it as sub-query later. $qb = $this->getEntityManager()->createQueryBuilder(); $qb->from(Visit::class, 'v') ->join('v.shortUrl', 's') ->join('s.tags', 't') - ->where($qb->expr()->eq('t.name', $this->getEntityManager()->getConnection()->quote($tag))); + ->where($qb->expr()->eq('t.name', $conn->quote($tag))); if ($filtering->excludeBots) { $qb->andWhere($qb->expr()->eq('v.potentialBot', 'false')); } + $domain = $filtering->domain; + if ($domain === Domain::DEFAULT_AUTHORITY) { + $qb->andWhere($qb->expr()->isNull('s.domain')); + } elseif ($domain !== null) { + $qb->join('s.domain', 'd') + ->andWhere($qb->expr()->eq('d.authority', $conn->quote($domain))); + } + $this->applyDatesInline($qb, $filtering->dateRange); $this->applySpecification($qb, $filtering->apiKey?->inlinedSpec(), 'v'); @@ -194,11 +205,10 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo return (int) $this->matchSingleScalarResult(new CountOfNonOrphanVisits($filtering)); } - private function createAllVisitsQueryBuilder( - VisitsListFiltering|OrphanVisitsListFiltering|WithDomainVisitsListFiltering $filtering, - ): QueryBuilder { - // Parameters in this query need to be inlined, not bound, as we need to use it as sub-query later - // Since they are not provided by the caller, it's reasonably safe + private function createAllVisitsQueryBuilder(VisitsCountFiltering $filtering): QueryBuilder + { + // Parameters in this query need to be inlined, not bound, as we need to use it as sub-query later. + // Since they are not provided by the caller, it's reasonably safe. $qb = $this->getEntityManager()->createQueryBuilder(); $qb->from(Visit::class, 'v'); diff --git a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php index 755b9bf8..523db21d 100644 --- a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php @@ -22,6 +22,7 @@ use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; +use Shlinkio\Shlink\Core\Visit\Persistence\WithDomainVisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\WithDomainVisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Repository\OrphanVisitsCountRepository; use Shlinkio\Shlink\Core\Visit\Repository\ShortUrlVisitsCountRepository; @@ -204,21 +205,40 @@ class VisitRepositoryTest extends DatabaseTestCase { $foo = 'foo'; - $this->createShortUrlsAndVisits(false, [$foo]); + $shortUrl1 = ShortUrl::create(ShortUrlCreation::fromRawData([ + ShortUrlInputFilter::LONG_URL => 'https://longUrl', + ShortUrlInputFilter::TAGS => [$foo], + ShortUrlInputFilter::DOMAIN => 'foo.com', + ]), $this->relationResolver); + $this->getEntityManager()->persist($shortUrl1); + $this->createVisitsForShortUrl($shortUrl1, 6); + + $shortUrl2 = ShortUrl::create(ShortUrlCreation::fromRawData([ + ShortUrlInputFilter::LONG_URL => 'https://longUrl', + ShortUrlInputFilter::TAGS => [$foo], + ]), $this->relationResolver); + $this->getEntityManager()->persist($shortUrl2); + $this->createVisitsForShortUrl($shortUrl2, 6); + $this->getEntityManager()->flush(); - $this->createShortUrlsAndVisits(false, [$foo]); - $this->getEntityManager()->flush(); - - self::assertEquals(0, $this->repo->countVisitsByTag('invalid', new VisitsCountFiltering())); - self::assertEquals(12, $this->repo->countVisitsByTag($foo, new VisitsCountFiltering())); - self::assertEquals(8, $this->repo->countVisitsByTag($foo, new VisitsCountFiltering(null, true))); - self::assertEquals(4, $this->repo->countVisitsByTag($foo, new VisitsCountFiltering( + self::assertEquals(0, $this->repo->countVisitsByTag('invalid', new WithDomainVisitsCountFiltering())); + self::assertEquals(12, $this->repo->countVisitsByTag($foo, new WithDomainVisitsCountFiltering())); + self::assertEquals(8, $this->repo->countVisitsByTag($foo, new WithDomainVisitsCountFiltering( + excludeBots: true, + ))); + self::assertEquals(4, $this->repo->countVisitsByTag($foo, new WithDomainVisitsCountFiltering( DateRange::between(Chronos::parse('2016-01-02'), Chronos::parse('2016-01-03')), ))); - self::assertEquals(8, $this->repo->countVisitsByTag($foo, new VisitsCountFiltering( + self::assertEquals(8, $this->repo->countVisitsByTag($foo, new WithDomainVisitsCountFiltering( DateRange::since(Chronos::parse('2016-01-03')), ))); + self::assertEquals(6, $this->repo->countVisitsByTag($foo, new WithDomainVisitsCountFiltering( + domain: 'foo.com', + ))); + self::assertEquals(6, $this->repo->countVisitsByTag($foo, new WithDomainVisitsCountFiltering( + domain: Domain::DEFAULT_AUTHORITY, + ))); } #[Test] @@ -534,6 +554,7 @@ class VisitRepositoryTest extends DatabaseTestCase /** * @return array{string, string, ShortUrl} + * @fixme This method does too many things and is not intuitive. It should be removed or simplified */ private function createShortUrlsAndVisits( bool|string $withDomain = true, @@ -566,6 +587,10 @@ class VisitRepositoryTest extends DatabaseTestCase return [$shortCode, $domain, $shortUrl]; } + /** + * @param int $amount - How many visits in total. Defaults to 6 + * @param int $botsAmount - How many of the visits should be bots. Defaults to 2 + */ private function createVisitsForShortUrl(ShortUrl $shortUrl, int $amount = 6, int $botsAmount = 2): void { for ($i = 0; $i < $amount; $i++) {