From 73c8b538825f077d6c2ca2b4173c8aca1c11178f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 14 Dec 2022 12:28:23 +0100 Subject: [PATCH] Split some logic from VisitRepository into its own injectable repository --- CHANGELOG.md | 1 + composer.json | 4 +- module/Core/config/dependencies.config.php | 6 +- .../src/Visit/Geolocation/VisitLocator.php | 13 ++-- .../Repository/VisitLocationRepository.php | 74 +++++++++++++++++++ .../VisitLocationRepositoryInterface.php | 27 +++++++ .../src/Visit/Repository/VisitRepository.php | 59 --------------- .../Repository/VisitRepositoryInterface.php | 18 ----- .../VisitLocationRepositoryTest.php | 63 ++++++++++++++++ .../Visit/Repository/VisitRepositoryTest.php | 50 ------------- .../Visit/Geolocation/VisitLocatorTest.php | 11 ++- 11 files changed, 182 insertions(+), 144 deletions(-) create mode 100644 module/Core/src/Visit/Repository/VisitLocationRepository.php create mode 100644 module/Core/src/Visit/Repository/VisitLocationRepositoryInterface.php create mode 100644 module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index ce5b8204..94fae122 100644 --- a/CHANGELOG.md +++ b/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* diff --git a/composer.json b/composer.json index 033920ca..6d169d8a 100644 --- a/composer.json +++ b/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", diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 708bb8a3..0a501566 100644 --- a/module/Core/config/dependencies.config.php +++ b/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'], diff --git a/module/Core/src/Visit/Geolocation/VisitLocator.php b/module/Core/src/Visit/Geolocation/VisitLocator.php index 12900260..4b3b8e22 100644 --- a/module/Core/src/Visit/Geolocation/VisitLocator.php +++ b/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 diff --git a/module/Core/src/Visit/Repository/VisitLocationRepository.php b/module/Core/src/Visit/Repository/VisitLocationRepository.php new file mode 100644 index 00000000..6db1a4f8 --- /dev/null +++ b/module/Core/src/Visit/Repository/VisitLocationRepository.php @@ -0,0 +1,74 @@ + + */ + 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 + */ + 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 + */ + 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); + } +} diff --git a/module/Core/src/Visit/Repository/VisitLocationRepositoryInterface.php b/module/Core/src/Visit/Repository/VisitLocationRepositoryInterface.php new file mode 100644 index 00000000..083d61f2 --- /dev/null +++ b/module/Core/src/Visit/Repository/VisitLocationRepositoryInterface.php @@ -0,0 +1,27 @@ + + */ + public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; + + /** + * @return iterable + */ + public function findVisitsWithEmptyLocation(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; + + /** + * @return iterable + */ + public function findAllVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; +} diff --git a/module/Core/src/Visit/Repository/VisitRepository.php b/module/Core/src/Visit/Repository/VisitRepository.php index af1647c7..7021e70b 100644 --- a/module/Core/src/Visit/Repository/VisitRepository.php +++ b/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[] */ diff --git a/module/Core/src/Visit/Repository/VisitRepositoryInterface.php b/module/Core/src/Visit/Repository/VisitRepositoryInterface.php index ebc4f4fe..4e53db2b 100644 --- a/module/Core/src/Visit/Repository/VisitRepositoryInterface.php +++ b/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[] */ diff --git a/module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php new file mode 100644 index 00000000..77f4c1e6 --- /dev/null +++ b/module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php @@ -0,0 +1,63 @@ +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]); + } +} diff --git a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php index da475832..eb806208 100644 --- a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php +++ b/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 { diff --git a/module/Core/test/Visit/Geolocation/VisitLocatorTest.php b/module/Core/test/Visit/Geolocation/VisitLocatorTest.php index a39940ae..ba0d70c4 100644 --- a/module/Core/test/Visit/Geolocation/VisitLocatorTest.php +++ b/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) { }