Split some logic from VisitRepository into its own injectable repository

CHANGELOG.md

@@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 ### Changed
 * [#1563](https://github.com/shlinkio/shlink/issues/1563) Moved logic to reuse command options to option classes instead of base abstract command classes.
 * [#1569](https://github.com/shlinkio/shlink/issues/1569) Migrated test doubles from phpspec/prophecy to PHPUnit mocks.
+* [#1329](https://github.com/shlinkio/shlink/issues/1329) Split some logic from `VisitRepository` and `ShortUrlRepository` into separated repository classes.
 
 ### Deprecated
 * *Nothing*

composer.json

@@ -45,7 +45,7 @@
         "php-middleware/request-id": "^4.1",
         "pugx/shortid-php": "^1.1",
         "ramsey/uuid": "^4.5",
-        "shlinkio/shlink-common": "dev-main#e2a5bb7 as 5.2",
+        "shlinkio/shlink-common": "dev-main#107b753 as 5.2",
         "shlinkio/shlink-config": "dev-main#96c81fb as 2.3",
         "shlinkio/shlink-event-dispatcher": "^2.6",
         "shlinkio/shlink-importer": "dev-main#c97662b as 5.0",
@@ -135,7 +135,7 @@
         "infect:ci:unit": "@infect:ci:base --coverage=build/coverage-unit --min-msi=80",
         "infect:ci:db": "@infect:ci:base --coverage=build/coverage-db --min-msi=95 --configuration=infection-db.json5",
         "infect:ci:api": "@infect:ci:base --coverage=build/coverage-api --min-msi=80 --configuration=infection-api.json5",
-        "infect:ci:cli": "@infect:ci:base --coverage=build/coverage-cli --min-msi=80 --configuration=infection-cli.json5",
+        "infect:ci:cli": "@infect:ci:base --coverage=build/coverage-cli --min-msi=90 --configuration=infection-cli.json5",
         "infect:ci": "@parallel infect:ci:unit infect:ci:db infect:ci:api infect:ci:cli",
         "infect:test": [
             "@parallel test:unit:ci test:db:sqlite:ci test:api:ci",

module/Core/config/dependencies.config.php

@@ -61,6 +61,10 @@ return [
             Visit\Geolocation\VisitToLocationHelper::class => ConfigAbstractFactory::class,
             Visit\VisitsStatsHelper::class => ConfigAbstractFactory::class,
             Visit\Transformer\OrphanVisitDataTransformer::class => InvokableFactory::class,
+            Visit\Repository\VisitLocationRepository::class => [
+                EntityRepositoryFactory::class,
+                Visit\Entity\Visit::class,
+            ],
 
             Util\UrlValidator::class => ConfigAbstractFactory::class,
             Util\DoctrineBatchHelper::class => ConfigAbstractFactory::class,
@@ -119,7 +123,7 @@ return [
             ShortUrl\Repository\ShortUrlListRepository::class,
             Options\UrlShortenerOptions::class,
         ],
-        Visit\Geolocation\VisitLocator::class => ['em'],
+        Visit\Geolocation\VisitLocator::class => ['em', Visit\Repository\VisitLocationRepository::class],
         Visit\Geolocation\VisitToLocationHelper::class => [IpLocationResolverInterface::class],
         Visit\VisitsStatsHelper::class => ['em'],
         Tag\TagService::class => ['em'],

module/Core/src/Visit/Geolocation/VisitLocator.php

@@ -8,18 +8,15 @@ use Doctrine\ORM\EntityManagerInterface;
 use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
 use Shlinkio\Shlink\Core\Visit\Entity\Visit;
 use Shlinkio\Shlink\Core\Visit\Entity\VisitLocation;
-use Shlinkio\Shlink\Core\Visit\Repository\VisitRepositoryInterface;
+use Shlinkio\Shlink\Core\Visit\Repository\VisitLocationRepositoryInterface;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
 
 class VisitLocator implements VisitLocatorInterface
 {
-    private VisitRepositoryInterface $repo;
-
-    public function __construct(private EntityManagerInterface $em)
-    {
-        /** @var VisitRepositoryInterface $repo */
-        $repo = $em->getRepository(Visit::class);
-        $this->repo = $repo;
+    public function __construct(
+        private readonly EntityManagerInterface $em,
+        private readonly VisitLocationRepositoryInterface $repo,
+    ) {
     }
 
     public function locateUnlocatedVisits(VisitGeolocationHelperInterface $helper): void

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

@@ -0,0 +1,74 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Visit\Repository;
+
+use Doctrine\ORM\QueryBuilder;
+use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository;
+use Shlinkio\Shlink\Core\Visit\Entity\Visit;
+
+class VisitLocationRepository extends EntitySpecificationRepository implements VisitLocationRepositoryInterface
+{
+    /**
+     * @return iterable<Visit>
+     */
+    public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable
+    {
+        $qb = $this->getEntityManager()->createQueryBuilder();
+        $qb->select('v')
+           ->from(Visit::class, 'v')
+           ->where($qb->expr()->isNull('v.visitLocation'));
+
+        return $this->visitsIterableForQuery($qb, $blockSize);
+    }
+
+    /**
+     * @return iterable<Visit>
+     */
+    public function findVisitsWithEmptyLocation(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable
+    {
+        $qb = $this->getEntityManager()->createQueryBuilder();
+        $qb->select('v')
+           ->from(Visit::class, 'v')
+           ->join('v.visitLocation', 'vl')
+           ->where($qb->expr()->isNotNull('v.visitLocation'))
+           ->andWhere($qb->expr()->eq('vl.isEmpty', ':isEmpty'))
+           ->setParameter('isEmpty', true);
+
+        return $this->visitsIterableForQuery($qb, $blockSize);
+    }
+
+    /**
+     * @return iterable<Visit>
+     */
+    public function findAllVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable
+    {
+        $qb = $this->createQueryBuilder('v');
+        return $this->visitsIterableForQuery($qb, $blockSize);
+    }
+
+    private function visitsIterableForQuery(QueryBuilder $qb, int $blockSize): iterable
+    {
+        $originalQueryBuilder = $qb->setMaxResults($blockSize)
+                                   ->orderBy('v.id', 'ASC');
+        $lastId = '0';
+
+        do {
+            $qb = (clone $originalQueryBuilder)->andWhere($qb->expr()->gt('v.id', $lastId));
+            $iterator = $qb->getQuery()->toIterable();
+            $resultsFound = false;
+            /** @var Visit|null $lastProcessedVisit */
+            $lastProcessedVisit = null;
+
+            foreach ($iterator as $key => $visit) {
+                $resultsFound = true;
+                $lastProcessedVisit = $visit;
+                yield $key => $visit;
+            }
+
+            // As the query is ordered by ID, we can take the last one every time in order to exclude the whole list
+            $lastId = $lastProcessedVisit?->getId() ?? $lastId;
+        } while ($resultsFound);
+    }
+}

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

@@ -0,0 +1,27 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Visit\Repository;
+
+use Shlinkio\Shlink\Core\Visit\Entity\Visit;
+
+interface VisitLocationRepositoryInterface
+{
+    public const DEFAULT_BLOCK_SIZE = 10000;
+
+    /**
+     * @return iterable<Visit>
+     */
+    public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable;
+
+    /**
+     * @return iterable<Visit>
+     */
+    public function findVisitsWithEmptyLocation(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable;
+
+    /**
+     * @return iterable<Visit>
+     */
+    public function findAllVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable;
+}

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

@@ -22,65 +22,6 @@ use const PHP_INT_MAX;
 
 class VisitRepository extends EntitySpecificationRepository implements VisitRepositoryInterface
 {
-    /**
-     * @return iterable|Visit[]
-     */
-    public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable
-    {
-        $qb = $this->getEntityManager()->createQueryBuilder();
-        $qb->select('v')
-           ->from(Visit::class, 'v')
-           ->where($qb->expr()->isNull('v.visitLocation'));
-
-        return $this->visitsIterableForQuery($qb, $blockSize);
-    }
-
-    /**
-     * @return iterable|Visit[]
-     */
-    public function findVisitsWithEmptyLocation(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable
-    {
-        $qb = $this->getEntityManager()->createQueryBuilder();
-        $qb->select('v')
-           ->from(Visit::class, 'v')
-           ->join('v.visitLocation', 'vl')
-           ->where($qb->expr()->isNotNull('v.visitLocation'))
-           ->andWhere($qb->expr()->eq('vl.isEmpty', ':isEmpty'))
-           ->setParameter('isEmpty', true);
-
-        return $this->visitsIterableForQuery($qb, $blockSize);
-    }
-
-    public function findAllVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable
-    {
-        $qb = $this->createQueryBuilder('v');
-        return $this->visitsIterableForQuery($qb, $blockSize);
-    }
-
-    private function visitsIterableForQuery(QueryBuilder $qb, int $blockSize): iterable
-    {
-        $originalQueryBuilder = $qb->setMaxResults($blockSize)
-                                   ->orderBy('v.id', 'ASC');
-        $lastId = '0';
-
-        do {
-            $qb = (clone $originalQueryBuilder)->andWhere($qb->expr()->gt('v.id', $lastId));
-            $iterator = $qb->getQuery()->toIterable();
-            $resultsFound = false;
-            /** @var Visit|null $lastProcessedVisit */
-            $lastProcessedVisit = null;
-
-            foreach ($iterator as $key => $visit) {
-                $resultsFound = true;
-                $lastProcessedVisit = $visit;
-                yield $key => $visit;
-            }
-
-            // As the query is ordered by ID, we can take the last one every time in order to exclude the whole list
-            $lastId = $lastProcessedVisit?->getId() ?? $lastId;
-        } while ($resultsFound);
-    }
-
     /**
      * @return Visit[]
      */

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

@@ -11,26 +11,8 @@ use Shlinkio\Shlink\Core\Visit\Entity\Visit;
 use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering;
 use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering;
 
-// TODO Split into VisitsListsRepository and VisitsLocationRepository
 interface VisitRepositoryInterface extends ObjectRepository, EntitySpecificationRepositoryInterface
 {
-    public const DEFAULT_BLOCK_SIZE = 10000;
-
-    /**
-     * @return iterable|Visit[]
-     */
-    public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable;
-
-    /**
-     * @return iterable|Visit[]
-     */
-    public function findVisitsWithEmptyLocation(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable;
-
-    /**
-     * @return iterable|Visit[]
-     */
-    public function findAllVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable;
-
     /**
      * @return Visit[]
      */

module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php

@@ -0,0 +1,63 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioDbTest\Shlink\Core\Visit\Repository;
+
+use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl;
+use Shlinkio\Shlink\Core\Visit\Entity\Visit;
+use Shlinkio\Shlink\Core\Visit\Entity\VisitLocation;
+use Shlinkio\Shlink\Core\Visit\Model\Visitor;
+use Shlinkio\Shlink\Core\Visit\Repository\VisitLocationRepository;
+use Shlinkio\Shlink\IpGeolocation\Model\Location;
+use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase;
+
+use function Functional\map;
+use function range;
+
+class VisitLocationRepositoryTest extends DatabaseTestCase
+{
+    private VisitLocationRepository $repo;
+
+    protected function setUp(): void
+    {
+        $em = $this->getEntityManager();
+        $this->repo = new VisitLocationRepository($em, $em->getClassMetadata(Visit::class));
+    }
+
+    /**
+     * @test
+     * @dataProvider provideBlockSize
+     */
+    public function findVisitsReturnsProperVisits(int $blockSize): void
+    {
+        $shortUrl = ShortUrl::createEmpty();
+        $this->getEntityManager()->persist($shortUrl);
+
+        for ($i = 0; $i < 6; $i++) {
+            $visit = Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance());
+
+            if ($i >= 2) {
+                $location = VisitLocation::fromGeolocation(Location::emptyInstance());
+                $this->getEntityManager()->persist($location);
+                $visit->locate($location);
+            }
+
+            $this->getEntityManager()->persist($visit);
+        }
+        $this->getEntityManager()->flush();
+
+        $withEmptyLocation = $this->repo->findVisitsWithEmptyLocation($blockSize);
+        $unlocated = $this->repo->findUnlocatedVisits($blockSize);
+        $all = $this->repo->findAllVisits($blockSize);
+
+        self::assertCount(2, [...$unlocated]);
+        self::assertCount(4, [...$withEmptyLocation]);
+        self::assertCount(6, [...$all]);
+    }
+
+    public function provideBlockSize(): iterable
+    {
+        return map(range(1, 10), fn (int $value) => [$value]);
+    }
+}

module/Core/test-db/Visit/Repository/VisitRepositoryTest.php

@@ -14,20 +14,16 @@ 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\Entity\VisitLocation;
 use Shlinkio\Shlink\Core\Visit\Model\Visitor;
 use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering;
 use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering;
 use Shlinkio\Shlink\Core\Visit\Repository\VisitRepository;
-use Shlinkio\Shlink\IpGeolocation\Model\Location;
 use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta;
 use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
 use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase;
 
-use function Functional\map;
 use function is_string;
-use function range;
 use function sprintf;
 use function str_pad;
 
@@ -44,52 +40,6 @@ class VisitRepositoryTest extends DatabaseTestCase
         $this->relationResolver = new PersistenceShortUrlRelationResolver($this->getEntityManager());
     }
 
-    /**
-     * @test
-     * @dataProvider provideBlockSize
-     */
-    public function findVisitsReturnsProperVisits(int $blockSize): void
-    {
-        $shortUrl = ShortUrl::createEmpty();
-        $this->getEntityManager()->persist($shortUrl);
-        $countIterable = static function (iterable $results): int {
-            $resultsCount = 0;
-            foreach ($results as $value) {
-                $resultsCount++;
-            }
-
-            return $resultsCount;
-        };
-
-        for ($i = 0; $i < 6; $i++) {
-            $visit = Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance());
-
-            if ($i >= 2) {
-                $location = VisitLocation::fromGeolocation(Location::emptyInstance());
-                $this->getEntityManager()->persist($location);
-                $visit->locate($location);
-            }
-
-            $this->getEntityManager()->persist($visit);
-        }
-        $this->getEntityManager()->flush();
-
-        $withEmptyLocation = $this->repo->findVisitsWithEmptyLocation($blockSize);
-        $unlocated = $this->repo->findUnlocatedVisits($blockSize);
-        $all = $this->repo->findAllVisits($blockSize);
-
-        // Important! assertCount will not work here, as this iterable object loads data dynamically and the count
-        // is 0 if not iterated
-        self::assertEquals(2, $countIterable($unlocated));
-        self::assertEquals(4, $countIterable($withEmptyLocation));
-        self::assertEquals(6, $countIterable($all));
-    }
-
-    public function provideBlockSize(): iterable
-    {
-        return map(range(1, 10), fn (int $value) => [$value]);
-    }
-
     /** @test */
     public function findVisitsByShortCodeReturnsProperData(): void
     {

module/Core/test/Visit/Geolocation/VisitLocatorTest.php

@@ -15,7 +15,7 @@ use Shlinkio\Shlink\Core\Visit\Entity\VisitLocation;
 use Shlinkio\Shlink\Core\Visit\Geolocation\VisitGeolocationHelperInterface;
 use Shlinkio\Shlink\Core\Visit\Geolocation\VisitLocator;
 use Shlinkio\Shlink\Core\Visit\Model\Visitor;
-use Shlinkio\Shlink\Core\Visit\Repository\VisitRepositoryInterface;
+use Shlinkio\Shlink\Core\Visit\Repository\VisitLocationRepositoryInterface;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
 
 use function count;
@@ -28,15 +28,14 @@ class VisitLocatorTest extends TestCase
 {
     private VisitLocator $visitService;
     private MockObject & EntityManager $em;
-    private MockObject & VisitRepositoryInterface $repo;
+    private MockObject & VisitLocationRepositoryInterface $repo;
 
     protected function setUp(): void
     {
         $this->em = $this->createMock(EntityManager::class);
-        $this->repo = $this->createMock(VisitRepositoryInterface::class);
-        $this->em->method('getRepository')->with(Visit::class)->willReturn($this->repo);
+        $this->repo = $this->createMock(VisitLocationRepositoryInterface::class);
 
-        $this->visitService = new VisitLocator($this->em);
+        $this->visitService = new VisitLocator($this->em, $this->repo);
     }
 
     /**
@@ -103,7 +102,7 @@ class VisitLocatorTest extends TestCase
 
         $this->visitService->{$serviceMethodName}(
             new class ($isNonLocatableAddress) implements VisitGeolocationHelperInterface {
-                public function __construct(private bool $isNonLocatableAddress)
+                public function __construct(private readonly bool $isNonLocatableAddress)
                 {
                 }