From 46acf4de1c5183d8467bac08dea12a8923af8ef8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 10 Feb 2024 13:57:16 +0100 Subject: [PATCH] Support filtering orphan visits by type in VisitRepository --- .../Core/src/Visit/Model/OrphanVisitType.php | 12 +++ module/Core/src/Visit/Model/VisitType.php | 6 +- .../Adapter/OrphanVisitsPaginatorAdapter.php | 8 +- .../OrphanVisitsCountFiltering.php | 21 +++++ .../Persistence/OrphanVisitsListFiltering.php | 23 ++++++ .../Persistence/VisitsCountFiltering.php | 5 -- .../src/Visit/Repository/VisitRepository.php | 15 +++- .../Repository/VisitRepositoryInterface.php | 6 +- .../src/Visit/Spec/CountOfOrphanVisits.php | 8 +- module/Core/src/Visit/VisitsStatsHelper.php | 7 +- .../Visit/Repository/VisitRepositoryTest.php | 80 ++++++++++++------- .../OrphanVisitsPaginatorAdapterTest.php | 18 ++--- .../Core/test/Visit/VisitsStatsHelperTest.php | 6 +- 13 files changed, 154 insertions(+), 61 deletions(-) create mode 100644 module/Core/src/Visit/Model/OrphanVisitType.php create mode 100644 module/Core/src/Visit/Persistence/OrphanVisitsCountFiltering.php create mode 100644 module/Core/src/Visit/Persistence/OrphanVisitsListFiltering.php diff --git a/module/Core/src/Visit/Model/OrphanVisitType.php b/module/Core/src/Visit/Model/OrphanVisitType.php new file mode 100644 index 00000000..4150a959 --- /dev/null +++ b/module/Core/src/Visit/Model/OrphanVisitType.php @@ -0,0 +1,12 @@ +value; + case BASE_URL = OrphanVisitType::BASE_URL->value; + case REGULAR_404 = OrphanVisitType::REGULAR_404->value; } diff --git a/module/Core/src/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php b/module/Core/src/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php index e871d125..1f38ab6a 100644 --- a/module/Core/src/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php +++ b/module/Core/src/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php @@ -6,8 +6,8 @@ namespace Shlinkio\Shlink\Core\Visit\Paginator\Adapter; use Shlinkio\Shlink\Core\Paginator\Adapter\AbstractCacheableCountPaginatorAdapter; use Shlinkio\Shlink\Core\Visit\Model\VisitsParams; -use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; -use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; +use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsCountFiltering; +use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -22,7 +22,7 @@ class OrphanVisitsPaginatorAdapter extends AbstractCacheableCountPaginatorAdapte protected function doCount(): int { - return $this->repo->countOrphanVisits(new VisitsCountFiltering( + return $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering( dateRange: $this->params->dateRange, excludeBots: $this->params->excludeBots, apiKey: $this->apiKey, @@ -31,7 +31,7 @@ class OrphanVisitsPaginatorAdapter extends AbstractCacheableCountPaginatorAdapte public function getSlice(int $offset, int $length): iterable { - return $this->repo->findOrphanVisits(new VisitsListFiltering( + return $this->repo->findOrphanVisits(new OrphanVisitsListFiltering( dateRange: $this->params->dateRange, excludeBots: $this->params->excludeBots, apiKey: $this->apiKey, diff --git a/module/Core/src/Visit/Persistence/OrphanVisitsCountFiltering.php b/module/Core/src/Visit/Persistence/OrphanVisitsCountFiltering.php new file mode 100644 index 00000000..88676df8 --- /dev/null +++ b/module/Core/src/Visit/Persistence/OrphanVisitsCountFiltering.php @@ -0,0 +1,21 @@ +apiKey?->hasRole(Role::NO_ORPHAN_VISITS)) { return []; @@ -146,10 +148,17 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo $qb = $this->createAllVisitsQueryBuilder($filtering); $qb->andWhere($qb->expr()->isNull('v.shortUrl')); + + // Parameters in this query need to be inlined, not bound, as we need to use it as sub-query later + if ($filtering->type) { + $conn = $this->getEntityManager()->getConnection(); + $qb->andWhere($qb->expr()->eq('v.type', $conn->quote($filtering->type->value))); + } + return $this->resolveVisitsWithNativeQuery($qb, $filtering->limit, $filtering->offset); } - public function countOrphanVisits(VisitsCountFiltering $filtering): int + public function countOrphanVisits(OrphanVisitsCountFiltering $filtering): int { if ($filtering->apiKey?->hasRole(Role::NO_ORPHAN_VISITS)) { return 0; @@ -176,7 +185,7 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo return (int) $this->matchSingleScalarResult(new CountOfNonOrphanVisits($filtering)); } - private function createAllVisitsQueryBuilder(VisitsListFiltering $filtering): QueryBuilder + private function createAllVisitsQueryBuilder(VisitsListFiltering|OrphanVisitsListFiltering $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 diff --git a/module/Core/src/Visit/Repository/VisitRepositoryInterface.php b/module/Core/src/Visit/Repository/VisitRepositoryInterface.php index 4e53db2b..9904181b 100644 --- a/module/Core/src/Visit/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Visit/Repository/VisitRepositoryInterface.php @@ -8,6 +8,8 @@ use Doctrine\Persistence\ObjectRepository; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepositoryInterface; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Visit\Entity\Visit; +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; @@ -37,9 +39,9 @@ interface VisitRepositoryInterface extends ObjectRepository, EntitySpecification /** * @return Visit[] */ - public function findOrphanVisits(VisitsListFiltering $filtering): array; + public function findOrphanVisits(OrphanVisitsListFiltering $filtering): array; - public function countOrphanVisits(VisitsCountFiltering $filtering): int; + public function countOrphanVisits(OrphanVisitsCountFiltering $filtering): int; /** * @return Visit[] diff --git a/module/Core/src/Visit/Spec/CountOfOrphanVisits.php b/module/Core/src/Visit/Spec/CountOfOrphanVisits.php index 106350c6..9d9cab56 100644 --- a/module/Core/src/Visit/Spec/CountOfOrphanVisits.php +++ b/module/Core/src/Visit/Spec/CountOfOrphanVisits.php @@ -8,11 +8,11 @@ use Happyr\DoctrineSpecification\Spec; use Happyr\DoctrineSpecification\Specification\BaseSpecification; use Happyr\DoctrineSpecification\Specification\Specification; use Shlinkio\Shlink\Core\Spec\InDateRange; -use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; +use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsCountFiltering; class CountOfOrphanVisits extends BaseSpecification { - public function __construct(private VisitsCountFiltering $filtering) + public function __construct(private readonly OrphanVisitsCountFiltering $filtering) { parent::__construct(); } @@ -28,6 +28,10 @@ class CountOfOrphanVisits extends BaseSpecification $conditions[] = Spec::eq('potentialBot', false); } + if ($this->filtering->type) { + $conditions[] = Spec::eq('type', $this->filtering->type->value); + } + return Spec::countOf(Spec::andX(...$conditions)); } } diff --git a/module/Core/src/Visit/VisitsStatsHelper.php b/module/Core/src/Visit/VisitsStatsHelper.php index bdd2fd3b..23ea8fc2 100644 --- a/module/Core/src/Visit/VisitsStatsHelper.php +++ b/module/Core/src/Visit/VisitsStatsHelper.php @@ -25,6 +25,7 @@ use Shlinkio\Shlink\Core\Visit\Paginator\Adapter\NonOrphanVisitsPaginatorAdapter use Shlinkio\Shlink\Core\Visit\Paginator\Adapter\OrphanVisitsPaginatorAdapter; use Shlinkio\Shlink\Core\Visit\Paginator\Adapter\ShortUrlVisitsPaginatorAdapter; use Shlinkio\Shlink\Core\Visit\Paginator\Adapter\TagVisitsPaginatorAdapter; +use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepository; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepositoryInterface; @@ -42,13 +43,13 @@ class VisitsStatsHelper implements VisitsStatsHelperInterface $visitsRepo = $this->em->getRepository(Visit::class); return new VisitsStats( - nonOrphanVisitsTotal: $visitsRepo->countNonOrphanVisits(VisitsCountFiltering::withApiKey($apiKey)), - orphanVisitsTotal: $visitsRepo->countOrphanVisits(VisitsCountFiltering::withApiKey($apiKey)), + nonOrphanVisitsTotal: $visitsRepo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $apiKey)), + orphanVisitsTotal: $visitsRepo->countOrphanVisits(new OrphanVisitsCountFiltering(apiKey: $apiKey)), nonOrphanVisitsNonBots: $visitsRepo->countNonOrphanVisits( new VisitsCountFiltering(excludeBots: true, apiKey: $apiKey), ), orphanVisitsNonBots: $visitsRepo->countOrphanVisits( - new VisitsCountFiltering(excludeBots: true, apiKey: $apiKey), + new OrphanVisitsCountFiltering(excludeBots: true, apiKey: $apiKey), ), ); } diff --git a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php index cca71a14..90496e1e 100644 --- a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php @@ -15,7 +15,10 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver; use Shlinkio\Shlink\Core\Visit\Entity\Visit; +use Shlinkio\Shlink\Core\Visit\Model\OrphanVisitType; use Shlinkio\Shlink\Core\Visit\Model\Visitor; +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\Repository\VisitRepository; @@ -305,10 +308,12 @@ class VisitRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); self::assertEquals(4 + 5 + 7, $this->repo->countNonOrphanVisits(new VisitsCountFiltering())); - self::assertEquals(4, $this->repo->countNonOrphanVisits(VisitsCountFiltering::withApiKey($apiKey1))); - self::assertEquals(5 + 7, $this->repo->countNonOrphanVisits(VisitsCountFiltering::withApiKey($apiKey2))); - self::assertEquals(4 + 7, $this->repo->countNonOrphanVisits(VisitsCountFiltering::withApiKey($domainApiKey))); - self::assertEquals(0, $this->repo->countOrphanVisits(VisitsCountFiltering::withApiKey($noOrphanVisitsApiKey))); + self::assertEquals(4, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $apiKey1))); + self::assertEquals(5 + 7, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $apiKey2))); + self::assertEquals(4 + 7, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $domainApiKey))); + self::assertEquals(0, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering( + apiKey: $noOrphanVisitsApiKey, + ))); self::assertEquals(4, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(DateRange::since( Chronos::parse('2016-01-05')->startOfDay(), )))); @@ -319,8 +324,8 @@ class VisitRepositoryTest extends DatabaseTestCase Chronos::parse('2016-01-07')->startOfDay(), ), false, $apiKey2))); self::assertEquals(3 + 5, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(null, true, $apiKey2))); - self::assertEquals(4, $this->repo->countOrphanVisits(new VisitsCountFiltering())); - self::assertEquals(3, $this->repo->countOrphanVisits(new VisitsCountFiltering(null, true))); + self::assertEquals(4, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering())); + self::assertEquals(3, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering(excludeBots: true))); } #[Test] @@ -353,27 +358,36 @@ class VisitRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - self::assertCount(0, $this->repo->findOrphanVisits(new VisitsListFiltering(apiKey: $noOrphanVisitsApiKey))); - self::assertCount(18, $this->repo->findOrphanVisits(new VisitsListFiltering())); - self::assertCount(15, $this->repo->findOrphanVisits(new VisitsListFiltering(null, true))); - self::assertCount(5, $this->repo->findOrphanVisits(new VisitsListFiltering(null, false, null, 5))); - self::assertCount(10, $this->repo->findOrphanVisits(new VisitsListFiltering(null, false, null, 15, 8))); - self::assertCount(9, $this->repo->findOrphanVisits(new VisitsListFiltering( - DateRange::since(Chronos::parse('2020-01-04')), - false, - null, - 15, + self::assertCount(0, $this->repo->findOrphanVisits(new OrphanVisitsListFiltering( + apiKey: $noOrphanVisitsApiKey, ))); - self::assertCount(2, $this->repo->findOrphanVisits(new VisitsListFiltering( - DateRange::between(Chronos::parse('2020-01-02'), Chronos::parse('2020-01-03')), - false, - null, - 6, - 4, + self::assertCount(18, $this->repo->findOrphanVisits(new OrphanVisitsListFiltering())); + self::assertCount(15, $this->repo->findOrphanVisits(new OrphanVisitsListFiltering(excludeBots: true))); + self::assertCount(5, $this->repo->findOrphanVisits(new OrphanVisitsListFiltering(limit: 5))); + self::assertCount(10, $this->repo->findOrphanVisits(new OrphanVisitsListFiltering(limit: 15, offset: 8))); + self::assertCount(9, $this->repo->findOrphanVisits(new OrphanVisitsListFiltering( + dateRange: DateRange::since(Chronos::parse('2020-01-04')), + limit: 15, ))); - self::assertCount(3, $this->repo->findOrphanVisits(new VisitsListFiltering( + self::assertCount(2, $this->repo->findOrphanVisits(new OrphanVisitsListFiltering( + dateRange: DateRange::between(Chronos::parse('2020-01-02'), Chronos::parse('2020-01-03')), + limit: 6, + offset: 4, + ))); + self::assertCount(2, $this->repo->findOrphanVisits(new OrphanVisitsListFiltering( + dateRange: DateRange::between(Chronos::parse('2020-01-02'), Chronos::parse('2020-01-03')), + type: OrphanVisitType::INVALID_SHORT_URL, + ))); + self::assertCount(3, $this->repo->findOrphanVisits(new OrphanVisitsListFiltering( DateRange::until(Chronos::parse('2020-01-01')), ))); + self::assertCount(6, $this->repo->findOrphanVisits(new OrphanVisitsListFiltering( + type: OrphanVisitType::REGULAR_404, + ))); + self::assertCount(4, $this->repo->findOrphanVisits(new OrphanVisitsListFiltering( + type: OrphanVisitType::BASE_URL, + limit: 4, + ))); } #[Test] @@ -400,17 +414,27 @@ class VisitRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - self::assertEquals(18, $this->repo->countOrphanVisits(new VisitsCountFiltering())); - self::assertEquals(18, $this->repo->countOrphanVisits(new VisitsCountFiltering(DateRange::allTime()))); + self::assertEquals(18, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering())); + self::assertEquals(18, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering(DateRange::allTime()))); self::assertEquals(9, $this->repo->countOrphanVisits( - new VisitsCountFiltering(DateRange::since(Chronos::parse('2020-01-04'))), + new OrphanVisitsCountFiltering(DateRange::since(Chronos::parse('2020-01-04'))), )); - self::assertEquals(6, $this->repo->countOrphanVisits(new VisitsCountFiltering( + self::assertEquals(6, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering( DateRange::between(Chronos::parse('2020-01-02'), Chronos::parse('2020-01-03')), ))); self::assertEquals(3, $this->repo->countOrphanVisits( - new VisitsCountFiltering(DateRange::until(Chronos::parse('2020-01-01'))), + new OrphanVisitsCountFiltering(DateRange::until(Chronos::parse('2020-01-01'))), )); + self::assertEquals(2, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering( + dateRange: DateRange::between(Chronos::parse('2020-01-02'), Chronos::parse('2020-01-03')), + type: OrphanVisitType::BASE_URL, + ))); + self::assertEquals(6, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering( + type: OrphanVisitType::INVALID_SHORT_URL, + ))); + self::assertEquals(6, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering( + type: OrphanVisitType::REGULAR_404, + ))); } #[Test] diff --git a/module/Core/test/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php b/module/Core/test/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php index 04e3f84c..623482a6 100644 --- a/module/Core/test/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php +++ b/module/Core/test/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php @@ -12,8 +12,8 @@ use Shlinkio\Shlink\Core\Visit\Entity\Visit; use Shlinkio\Shlink\Core\Visit\Model\Visitor; use Shlinkio\Shlink\Core\Visit\Model\VisitsParams; use Shlinkio\Shlink\Core\Visit\Paginator\Adapter\OrphanVisitsPaginatorAdapter; -use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; -use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; +use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsCountFiltering; +use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -38,7 +38,7 @@ class OrphanVisitsPaginatorAdapterTest extends TestCase { $expectedCount = 5; $this->repo->expects($this->once())->method('countOrphanVisits')->with( - new VisitsCountFiltering($this->params->dateRange, apiKey: $this->apiKey), + new OrphanVisitsCountFiltering($this->params->dateRange, apiKey: $this->apiKey), )->willReturn($expectedCount); $result = $this->adapter->getNbResults(); @@ -55,12 +55,12 @@ class OrphanVisitsPaginatorAdapterTest extends TestCase { $visitor = Visitor::emptyInstance(); $list = [Visit::forRegularNotFound($visitor), Visit::forInvalidShortUrl($visitor)]; - $this->repo->expects($this->once())->method('findOrphanVisits')->with(new VisitsListFiltering( - $this->params->dateRange, - $this->params->excludeBots, - $this->apiKey, - $limit, - $offset, + $this->repo->expects($this->once())->method('findOrphanVisits')->with(new OrphanVisitsListFiltering( + dateRange: $this->params->dateRange, + excludeBots: $this->params->excludeBots, + apiKey: $this->apiKey, + limit: $limit, + offset: $offset, ))->willReturn($list); $result = $this->adapter->getSlice($offset, $limit); diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index dd11fdef..3af7b739 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -26,6 +26,8 @@ use Shlinkio\Shlink\Core\Visit\Entity\Visit; use Shlinkio\Shlink\Core\Visit\Model\Visitor; use Shlinkio\Shlink\Core\Visit\Model\VisitsParams; use Shlinkio\Shlink\Core\Visit\Model\VisitsStats; +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\Repository\VisitRepository; @@ -251,10 +253,10 @@ class VisitsStatsHelperTest extends TestCase $list = array_map(static fn () => Visit::forBasePath(Visitor::emptyInstance()), range(0, 3)); $repo = $this->createMock(VisitRepository::class); $repo->expects($this->once())->method('countOrphanVisits')->with( - $this->isInstanceOf(VisitsCountFiltering::class), + $this->isInstanceOf(OrphanVisitsCountFiltering::class), )->willReturn(count($list)); $repo->expects($this->once())->method('findOrphanVisits')->with( - $this->isInstanceOf(VisitsListFiltering::class), + $this->isInstanceOf(OrphanVisitsListFiltering::class), )->willReturn($list); $this->em->expects($this->once())->method('getRepository')->with(Visit::class)->willReturn($repo);