Merge pull request #2081 from acelaya-forks/feature/performant-count-visits

Load non-orphan visits overview via short url visits counts

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*
@@ -23,7 +27,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

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')

module/Core/src/Visit/Repository/ShortUrlVisitsCountRepository.php

@@ -0,0 +1,31 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Visit\Repository;
+
+use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository;
+use Shlinkio\Shlink\Core\Visit\Entity\ShortUrlVisitsCount;
+use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering;
+
+class ShortUrlVisitsCountRepository extends EntitySpecificationRepository implements
+    ShortUrlVisitsCountRepositoryInterface
+{
+    public function countNonOrphanVisits(VisitsCountFiltering $filtering): int
+    {
+        $qb = $this->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();
+    }
+}

module/Core/src/Visit/Repository/ShortUrlVisitsCountRepositoryInterface.php

@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Visit\Repository;
+
+use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering;
+
+interface ShortUrlVisitsCountRepositoryInterface
+{
+    public function countNonOrphanVisits(VisitsCountFiltering $filtering): int;
+}

module/Core/src/Visit/Spec/CountOfNonOrphanVisits.php

@@ -13,7 +13,7 @@ use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin;
 
 class CountOfNonOrphanVisits extends BaseSpecification
 {
-    public function __construct(private VisitsCountFiltering $filtering)
+    public function __construct(private readonly VisitsCountFiltering $filtering)
     {
         parent::__construct();
     }

module/Core/src/Visit/VisitsStatsHelper.php

@@ -17,6 +17,7 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier;
 use Shlinkio\Shlink\Core\ShortUrl\Repository\ShortUrlRepositoryInterface;
 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\VisitsParams;
@@ -28,13 +29,14 @@ 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\ShortUrlVisitsCountRepository;
 use Shlinkio\Shlink\Core\Visit\Repository\VisitRepository;
 use Shlinkio\Shlink\Core\Visit\Repository\VisitRepositoryInterface;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
 
-class VisitsStatsHelper implements VisitsStatsHelperInterface
+readonly class VisitsStatsHelper implements VisitsStatsHelperInterface
 {
-    public function __construct(private readonly EntityManagerInterface $em)
+    public function __construct(private EntityManagerInterface $em)
     {
     }
 
@@ -42,11 +44,13 @@ class VisitsStatsHelper implements VisitsStatsHelperInterface
     {
         /** @var VisitRepository $visitsRepo */
         $visitsRepo = $this->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(

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)));
     }

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);