From 55e2780f50619255114e5859a50ff823aa1d9eaa Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 31 Mar 2024 10:33:31 +0200 Subject: [PATCH 1/3] Load non-orphan visits overview via short url visits counts --- CHANGELOG.md | 2 +- ....Core.Visit.Entity.ShortUrlVisitsCount.php | 3 +- .../ShortUrlVisitsCountRepository.php | 31 +++++++++++++++++++ ...ShortUrlVisitsCountRepositoryInterface.php | 12 +++++++ .../src/Visit/Spec/CountOfNonOrphanVisits.php | 2 +- module/Core/src/Visit/VisitsStatsHelper.php | 12 ++++--- .../Core/test/Visit/VisitsStatsHelperTest.php | 16 +++++++--- 7 files changed, 67 insertions(+), 11 deletions(-) create mode 100644 module/Core/src/Visit/Repository/ShortUrlVisitsCountRepository.php create mode 100644 module/Core/src/Visit/Repository/ShortUrlVisitsCountRepositoryInterface.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b5f5cff..0dfd04c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * *Nothing* ### Fixed -* *Nothing* +* Fix error when importing short URLs and visits from a Shlink 4.x instance ## [4.0.3] - 2024-03-15 diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.ShortUrlVisitsCount.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.ShortUrlVisitsCount.php index d4a8546b..07977a50 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.ShortUrlVisitsCount.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.ShortUrlVisitsCount.php @@ -11,7 +11,8 @@ use Doctrine\ORM\Mapping\ClassMetadata; return static function (ClassMetadata $metadata, array $emConfig): void { $builder = new ClassMetadataBuilder($metadata); - $builder->setTable(determineTableName('short_url_visits_counts', $emConfig)); + $builder->setTable(determineTableName('short_url_visits_counts', $emConfig)) + ->setCustomRepositoryClass(Visit\Repository\ShortUrlVisitsCountRepository::class); $builder->createField('id', Types::BIGINT) ->columnName('id') diff --git a/module/Core/src/Visit/Repository/ShortUrlVisitsCountRepository.php b/module/Core/src/Visit/Repository/ShortUrlVisitsCountRepository.php new file mode 100644 index 00000000..f07aab33 --- /dev/null +++ b/module/Core/src/Visit/Repository/ShortUrlVisitsCountRepository.php @@ -0,0 +1,31 @@ +getEntityManager()->createQueryBuilder(); + $qb->select('COALESCE(SUM(vc.count), 0)') + ->from(ShortUrlVisitsCount::class, 'vc') + ->join('vc.shortUrl', 's'); + + + if ($filtering->excludeBots) { + $qb->andWhere($qb->expr()->eq('vc.potentialBot', ':potentialBot')) + ->setParameter('potentialBot', false); + } + + $this->applySpecification($qb, $filtering->apiKey?->spec(), 's'); + + return (int) $qb->getQuery()->getSingleScalarResult(); + } +} diff --git a/module/Core/src/Visit/Repository/ShortUrlVisitsCountRepositoryInterface.php b/module/Core/src/Visit/Repository/ShortUrlVisitsCountRepositoryInterface.php new file mode 100644 index 00000000..fd966039 --- /dev/null +++ b/module/Core/src/Visit/Repository/ShortUrlVisitsCountRepositoryInterface.php @@ -0,0 +1,12 @@ +em->getRepository(Visit::class); + /** @var ShortUrlVisitsCountRepository $visitsCountRepo */ + $visitsCountRepo = $this->em->getRepository(ShortUrlVisitsCount::class); return new VisitsStats( - nonOrphanVisitsTotal: $visitsRepo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $apiKey)), + nonOrphanVisitsTotal: $visitsCountRepo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $apiKey)), orphanVisitsTotal: $visitsRepo->countOrphanVisits(new OrphanVisitsCountFiltering(apiKey: $apiKey)), - nonOrphanVisitsNonBots: $visitsRepo->countNonOrphanVisits( + nonOrphanVisitsNonBots: $visitsCountRepo->countNonOrphanVisits( new VisitsCountFiltering(excludeBots: true, apiKey: $apiKey), ), orphanVisitsNonBots: $visitsRepo->countOrphanVisits( diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index f6bb5464..e109852a 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -22,6 +22,7 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Tag\Entity\Tag; use Shlinkio\Shlink\Core\Tag\Repository\TagRepository; +use Shlinkio\Shlink\Core\Visit\Entity\ShortUrlVisitsCount; use Shlinkio\Shlink\Core\Visit\Entity\Visit; use Shlinkio\Shlink\Core\Visit\Model\OrphanVisitsParams; use Shlinkio\Shlink\Core\Visit\Model\Visitor; @@ -31,6 +32,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\Repository\ShortUrlVisitsCountRepository; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepository; use Shlinkio\Shlink\Core\Visit\VisitsStatsHelper; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -54,9 +56,9 @@ class VisitsStatsHelperTest extends TestCase #[Test, DataProvider('provideCounts')] public function returnsExpectedVisitsStats(int $expectedCount, ?ApiKey $apiKey): void { - $repo = $this->createMock(VisitRepository::class); $callCount = 0; - $repo->expects($this->exactly(2))->method('countNonOrphanVisits')->willReturnCallback( + $visitsCountRepo = $this->createMock(ShortUrlVisitsCountRepository::class); + $visitsCountRepo->expects($this->exactly(2))->method('countNonOrphanVisits')->willReturnCallback( function (VisitsCountFiltering $options) use ($expectedCount, $apiKey, &$callCount) { Assert::assertEquals($callCount !== 0, $options->excludeBots); Assert::assertEquals($apiKey, $options->apiKey); @@ -65,10 +67,16 @@ class VisitsStatsHelperTest extends TestCase return $expectedCount * 3; }, ); - $repo->expects($this->exactly(2))->method('countOrphanVisits')->with( + + $visitsRepo = $this->createMock(VisitRepository::class); + $visitsRepo->expects($this->exactly(2))->method('countOrphanVisits')->with( $this->isInstanceOf(VisitsCountFiltering::class), )->willReturn($expectedCount); - $this->em->expects($this->once())->method('getRepository')->with(Visit::class)->willReturn($repo); + + $this->em->expects($this->exactly(2))->method('getRepository')->willReturnMap([ + [Visit::class, $visitsRepo], + [ShortUrlVisitsCount::class, $visitsCountRepo], + ]); $stats = $this->helper->getVisitsStats($apiKey); From ab6fa490e5ac36c923411e2d26a8b837ed9e27ed Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 31 Mar 2024 12:37:22 +0200 Subject: [PATCH 2/3] Test ShortUrlVisitsCountRepository via VisitRepositoryTest --- .../Visit/Repository/VisitRepositoryTest.php | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php index 90496e1e..d14e7078 100644 --- a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php @@ -14,6 +14,7 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; 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\ShortUrlVisitsCount; use Shlinkio\Shlink\Core\Visit\Entity\Visit; use Shlinkio\Shlink\Core\Visit\Model\OrphanVisitType; use Shlinkio\Shlink\Core\Visit\Model\Visitor; @@ -21,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\Repository\ShortUrlVisitsCountRepository; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepository; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; @@ -36,11 +38,15 @@ use const STR_PAD_LEFT; class VisitRepositoryTest extends DatabaseTestCase { private VisitRepository $repo; + private ShortUrlVisitsCountRepository $countRepo; private PersistenceShortUrlRelationResolver $relationResolver; protected function setUp(): void { $this->repo = $this->getEntityManager()->getRepository(Visit::class); + // Testing the ShortUrlVisitsCountRepository in this very same test, helps checking the fact that results should + // match what VisitRepository returns + $this->countRepo = $this->getEntityManager()->getRepository(ShortUrlVisitsCount::class); $this->relationResolver = new PersistenceShortUrlRelationResolver($this->getEntityManager()); } @@ -308,9 +314,15 @@ class VisitRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); self::assertEquals(4 + 5 + 7, $this->repo->countNonOrphanVisits(new VisitsCountFiltering())); + self::assertEquals(4 + 5 + 7, $this->countRepo->countNonOrphanVisits(new VisitsCountFiltering())); self::assertEquals(4, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $apiKey1))); + self::assertEquals(4, $this->countRepo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $apiKey1))); self::assertEquals(5 + 7, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $apiKey2))); + self::assertEquals(5 + 7, $this->countRepo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $apiKey2))); self::assertEquals(4 + 7, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $domainApiKey))); + self::assertEquals(4 + 7, $this->countRepo->countNonOrphanVisits(new VisitsCountFiltering( + apiKey: $domainApiKey, + ))); self::assertEquals(0, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering( apiKey: $noOrphanVisitsApiKey, ))); @@ -323,7 +335,12 @@ class VisitRepositoryTest extends DatabaseTestCase self::assertEquals(1, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(DateRange::since( Chronos::parse('2016-01-07')->startOfDay(), ), false, $apiKey2))); - self::assertEquals(3 + 5, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(null, true, $apiKey2))); + self::assertEquals(3 + 5, $this->repo->countNonOrphanVisits( + new VisitsCountFiltering(excludeBots: true, apiKey: $apiKey2), + )); + self::assertEquals(3 + 5, $this->countRepo->countNonOrphanVisits( + new VisitsCountFiltering(excludeBots: true, apiKey: $apiKey2), + )); self::assertEquals(4, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering())); self::assertEquals(3, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering(excludeBots: true))); } From 6e82509964d4fc69cb2e3afb48183e56a6fad388 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 31 Mar 2024 12:39:37 +0200 Subject: [PATCH 3/3] Update changelog --- CHANGELOG.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dfd04c8..f6ec4f68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Changed * [#2034](https://github.com/shlinkio/shlink/issues/2034) Modernize entities, using constructor property promotion and readonly wherever possible. -* [#2036](https://github.com/shlinkio/shlink/issues/2036) Deep performance improvement when listing short URLs ordered by visits counts. +* [#2036](https://github.com/shlinkio/shlink/issues/2036) Deep performance improvement in some endpoints which involve counting visits: - This has been achieved by introducing a new table which tracks slotted visits counts. We can then `SUM` all counts for certain visit, avoiding `COUNT(visits)` aggregates which are less performant when there are a lot of visits. + * listing short URLs ordered by visits counts. + * loading tags with stats. + * visits overview. + + This has been achieved by introducing a new table which tracks slotted visits counts. We can then `SUM` all counts for certain short URL, avoiding `COUNT(visits)` aggregates which are much less performant when there are a lot of visits. ### Deprecated * *Nothing*