From c88401ef29a22ab29fcafbecd18d635f838f1109 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 23 Mar 2020 20:37:45 +0100 Subject: [PATCH 01/11] Added isEmpty column to VisitLocation --- data/migrations/Version20200323190014.php | 44 +++++++++++++++++++ ...inkio.Shlink.Core.Entity.VisitLocation.php | 6 +++ module/Core/src/Entity/VisitLocation.php | 28 +++++++----- 3 files changed, 66 insertions(+), 12 deletions(-) create mode 100644 data/migrations/Version20200323190014.php diff --git a/data/migrations/Version20200323190014.php b/data/migrations/Version20200323190014.php new file mode 100644 index 00000000..fe3c340d --- /dev/null +++ b/data/migrations/Version20200323190014.php @@ -0,0 +1,44 @@ +getTable('visit_locations'); + $this->skipIf($visitLocations->hasColumn('is_empty')); + + $visitLocations->addColumn('is_empty', Types::BOOLEAN, ['default' => false]); + } + + public function postUp(Schema $schema): void + { + $qb = $this->connection->createQueryBuilder(); + $qb->update('visit_locations') + ->set('is_empty', true) + ->where($qb->expr()->eq('country_code', ':empty')) + ->andWhere($qb->expr()->eq('country_name', ':empty')) + ->andWhere($qb->expr()->eq('region_name', ':empty')) + ->andWhere($qb->expr()->eq('city_name', ':empty')) + ->andWhere($qb->expr()->eq('timezone', ':empty')) + ->andWhere($qb->expr()->eq('lat', 0)) + ->andWhere($qb->expr()->eq('lon', 0)) + ->setParameter('empty', '') + ->execute(); + } + + public function down(Schema $schema): void + { + $visitLocations = $schema->getTable('visit_locations'); + $this->skipIf(!$visitLocations->hasColumn('is_empty')); + + $visitLocations->dropColumn('is_empty'); + } +} diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php index fde00abc..955fa1fa 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php @@ -44,4 +44,10 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->columnName('lon') ->nullable(false) ->build(); + + $builder->createField('isEmpty', Types::BOOLEAN) + ->columnName('is_empty') + ->option('default', false) + ->nullable(false) + ->build(); }; diff --git a/module/Core/src/Entity/VisitLocation.php b/module/Core/src/Entity/VisitLocation.php index 641b078e..ef545bba 100644 --- a/module/Core/src/Entity/VisitLocation.php +++ b/module/Core/src/Entity/VisitLocation.php @@ -17,6 +17,7 @@ class VisitLocation extends AbstractEntity implements VisitLocationInterface private float $latitude; private float $longitude; private string $timezone; + private bool $isEmpty; public function __construct(Location $location) { @@ -43,6 +44,11 @@ class VisitLocation extends AbstractEntity implements VisitLocationInterface return $this->cityName; } + public function isEmpty(): bool + { + return $this->isEmpty; + } + private function exchangeLocationInfo(Location $info): void { $this->countryCode = $info->countryCode(); @@ -52,6 +58,15 @@ class VisitLocation extends AbstractEntity implements VisitLocationInterface $this->latitude = $info->latitude(); $this->longitude = $info->longitude(); $this->timezone = $info->timeZone(); + $this->isEmpty = ( + $this->countryCode === '' && + $this->countryName === '' && + $this->regionName === '' && + $this->cityName === '' && + $this->latitude === 0.0 && + $this->longitude === 0.0 && + $this->timezone === '' + ); } public function jsonSerialize(): array @@ -64,18 +79,7 @@ class VisitLocation extends AbstractEntity implements VisitLocationInterface 'latitude' => $this->latitude, 'longitude' => $this->longitude, 'timezone' => $this->timezone, + 'isEmpty' => $this->isEmpty, ]; } - - public function isEmpty(): bool - { - return - $this->countryCode === '' && - $this->countryName === '' && - $this->regionName === '' && - $this->cityName === '' && - $this->latitude === 0.0 && - $this->longitude === 0.0 && - $this->timezone === ''; - } } From b8522b8c176ec35ed08bac9d73610bf57586af1e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 26 Mar 2020 22:17:13 +0100 Subject: [PATCH 02/11] Created new method to locate empty visits --- composer.json | 2 +- config/autoload/request_id.global.php | 5 +- module/CLI/config/dependencies.config.php | 3 +- .../src/Command/Visit/LocateVisitsCommand.php | 48 ++++++++++++------- .../Command/Visit/LocateVisitsCommandTest.php | 4 +- module/Core/config/dependencies.config.php | 4 +- .../Core/src/Repository/VisitRepository.php | 7 +-- .../src/Service/VisitServiceInterface.php | 10 ---- .../VisitLocator.php} | 26 ++++++---- .../Core/src/Visit/VisitLocatorInterface.php | 12 +++++ .../VisitLocatorTest.php} | 11 +++-- 11 files changed, 82 insertions(+), 50 deletions(-) delete mode 100644 module/Core/src/Service/VisitServiceInterface.php rename module/Core/src/{Service/VisitService.php => Visit/VisitLocator.php} (71%) create mode 100644 module/Core/src/Visit/VisitLocatorInterface.php rename module/Core/test/{Service/VisitServiceTest.php => Visit/VisitLocatorTest.php} (93%) diff --git a/composer.json b/composer.json index 5ee67a4d..9cadd738 100644 --- a/composer.json +++ b/composer.json @@ -52,7 +52,7 @@ "shlinkio/shlink-common": "^3.0", "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^1.4", - "shlinkio/shlink-installer": "^4.3", + "shlinkio/shlink-installer": "^4.3.1", "shlinkio/shlink-ip-geolocation": "^1.4", "symfony/console": "^5.0", "symfony/filesystem": "^5.0", diff --git a/config/autoload/request_id.global.php b/config/autoload/request_id.global.php index 0a9ed6ce..f057bb09 100644 --- a/config/autoload/request_id.global.php +++ b/config/autoload/request_id.global.php @@ -28,7 +28,10 @@ return [ 'config.request_id.allow_override', 'config.request_id.header_name', ], - RequestId\RequestIdMiddleware::class => [RequestId\RequestIdProviderFactory::class], + RequestId\RequestIdMiddleware::class => [ + RequestId\RequestIdProviderFactory::class, + 'config.request_id.header_name', + ], RequestId\MonologProcessor::class => [RequestId\RequestIdMiddleware::class], ], diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 5edb6499..0f2e70a5 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -11,6 +11,7 @@ use Laminas\ServiceManager\Factory\InvokableFactory; use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdater; use Shlinkio\Shlink\Common\Doctrine\NoDbNameConnectionFactory; use Shlinkio\Shlink\Core\Service; +use Shlinkio\Shlink\Core\Visit; use Shlinkio\Shlink\Installer\Factory\ProcessHelperFactory; use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdater; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; @@ -67,7 +68,7 @@ return [ Command\ShortUrl\DeleteShortUrlCommand::class => [Service\ShortUrl\DeleteShortUrlService::class], Command\Visit\LocateVisitsCommand::class => [ - Service\VisitService::class, + Visit\VisitLocator::class, IpLocationResolverInterface::class, LockFactory::class, GeolocationDbUpdater::class, diff --git a/module/CLI/src/Command/Visit/LocateVisitsCommand.php b/module/CLI/src/Command/Visit/LocateVisitsCommand.php index b19e8b19..935b9eee 100644 --- a/module/CLI/src/Command/Visit/LocateVisitsCommand.php +++ b/module/CLI/src/Command/Visit/LocateVisitsCommand.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\Visit; -use Exception; use Shlinkio\Shlink\CLI\Command\Util\AbstractLockedCommand; use Shlinkio\Shlink\CLI\Command\Util\LockedCommandConfig; use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; @@ -14,12 +13,13 @@ use Shlinkio\Shlink\Common\Util\IpAddress; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException; -use Shlinkio\Shlink\Core\Service\VisitServiceInterface; +use Shlinkio\Shlink\Core\Visit\VisitLocatorInterface; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; use Symfony\Component\Console\Helper\ProgressBar; use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; use Symfony\Component\Lock\LockFactory; @@ -31,7 +31,7 @@ class LocateVisitsCommand extends AbstractLockedCommand { public const NAME = 'visit:locate'; - private VisitServiceInterface $visitService; + private VisitLocatorInterface $visitLocator; private IpLocationResolverInterface $ipLocationResolver; private GeolocationDbUpdaterInterface $dbUpdater; @@ -39,13 +39,13 @@ class LocateVisitsCommand extends AbstractLockedCommand private ?ProgressBar $progressBar = null; public function __construct( - VisitServiceInterface $visitService, + VisitLocatorInterface $visitLocator, IpLocationResolverInterface $ipLocationResolver, LockFactory $locker, GeolocationDbUpdaterInterface $dbUpdater ) { parent::__construct($locker); - $this->visitService = $visitService; + $this->visitLocator = $visitLocator; $this->ipLocationResolver = $ipLocationResolver; $this->dbUpdater = $dbUpdater; } @@ -54,32 +54,46 @@ class LocateVisitsCommand extends AbstractLockedCommand { $this ->setName(self::NAME) - ->setDescription('Resolves visits origin locations.'); + ->setDescription('Resolves visits origin locations.') + ->addOption( + 'retry', + 'r', + InputOption::VALUE_NONE, + 'Will retry visits that were located with an empty location, in case it was a temporal issue.', + ) + ->addOption( + 'all', + 'a', + InputOption::VALUE_NONE, + 'Will locate all visits, ignoring if they have already been located.', + ); } protected function lockedExecute(InputInterface $input, OutputInterface $output): int { $this->io = new SymfonyStyle($input, $output); + $retry = $input->getOption('retry'); + $geolocateVisit = [$this, 'getGeolocationDataForVisit']; + $notifyVisitWithLocation = static function (VisitLocation $location) use ($output): void { + $message = ! $location->isEmpty() + ? sprintf(' [Address located in "%s"]', $location->getCountryName()) + : ' [Address not found]'; + $output->writeln($message); + }; try { $this->checkDbUpdate(); - $this->visitService->locateUnlocatedVisits( - [$this, 'getGeolocationDataForVisit'], - static function (VisitLocation $location) use ($output): void { - if (!$location->isEmpty()) { - $output->writeln( - sprintf(' [Address located at "%s"]', $location->getCountryName()), - ); - } - }, - ); + $this->visitLocator->locateUnlocatedVisits($geolocateVisit, $notifyVisitWithLocation); + if ($retry) { + $this->visitLocator->locateVisitsWithEmptyLocation($geolocateVisit, $notifyVisitWithLocation); + } $this->io->success('Finished processing all IPs'); return ExitCodes::EXIT_SUCCESS; } catch (Throwable $e) { $this->io->error($e->getMessage()); - if ($e instanceof Exception && $this->io->isVerbose()) { + if ($e instanceof Throwable && $this->io->isVerbose()) { $this->getApplication()->renderThrowable($e, $this->io); } diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index 90073f10..94cd0bd1 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -15,7 +15,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Service\VisitService; +use Shlinkio\Shlink\Core\Visit\VisitLocator; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; @@ -38,7 +38,7 @@ class LocateVisitsCommandTest extends TestCase public function setUp(): void { - $this->visitService = $this->prophesize(VisitService::class); + $this->visitService = $this->prophesize(VisitLocator::class); $this->ipResolver = $this->prophesize(IpLocationResolverInterface::class); $this->dbUpdater = $this->prophesize(GeolocationDbUpdaterInterface::class); diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 2800cdd6..13a74c36 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -28,7 +28,7 @@ return [ Service\UrlShortener::class => ConfigAbstractFactory::class, Service\VisitsTracker::class => ConfigAbstractFactory::class, Service\ShortUrlService::class => ConfigAbstractFactory::class, - Service\VisitService::class => ConfigAbstractFactory::class, + Visit\VisitLocator::class => ConfigAbstractFactory::class, Service\Tag\TagService::class => ConfigAbstractFactory::class, Service\ShortUrl\DeleteShortUrlService::class => ConfigAbstractFactory::class, Service\ShortUrl\ShortUrlResolver::class => ConfigAbstractFactory::class, @@ -57,7 +57,7 @@ return [ Service\UrlShortener::class => [Util\UrlValidator::class, 'em', Resolver\PersistenceDomainResolver::class], Service\VisitsTracker::class => ['em', EventDispatcherInterface::class], Service\ShortUrlService::class => ['em', Service\ShortUrl\ShortUrlResolver::class, Util\UrlValidator::class], - Service\VisitService::class => ['em'], + Visit\VisitLocator::class => ['em'], Service\Tag\TagService::class => ['em'], Service\ShortUrl\DeleteShortUrlService::class => [ 'em', diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 454323ef..28eb2466 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -14,7 +14,8 @@ class VisitRepository extends EntityRepository implements VisitRepositoryInterfa /** * This method will allow you to iterate the whole list of unlocated visits, but loading them into memory in * smaller blocks of a specific size. - * This will have side effects if you update those rows while you iterate them. + * This will have side effects if you update those rows while you iterate them, in a way that they are no longer + * unlocated. * If you plan to do so, pass the first argument as false in order to disable applying offsets while slicing the * dataset * @@ -23,8 +24,8 @@ class VisitRepository extends EntityRepository implements VisitRepositoryInterfa public function findUnlocatedVisits(bool $applyOffset = true, int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable { $dql = <<getEntityManager()->createQuery($dql) ->setMaxResults($blockSize); $remainingVisitsToProcess = $this->count(['visitLocation' => null]); diff --git a/module/Core/src/Service/VisitServiceInterface.php b/module/Core/src/Service/VisitServiceInterface.php deleted file mode 100644 index 78543549..00000000 --- a/module/Core/src/Service/VisitServiceInterface.php +++ /dev/null @@ -1,10 +0,0 @@ -em = $em; } - public function locateUnlocatedVisits(callable $geolocateVisit, ?callable $notifyVisitWithLocation = null): void + public function locateUnlocatedVisits(callable $geolocateVisit, callable $notifyVisitWithLocation): void { /** @var VisitRepository $repo */ $repo = $this->em->getRepository(Visit::class); - $results = $repo->findUnlocatedVisits(false); + $this->locateVisits($repo->findUnlocatedVisits(false), $geolocateVisit, $notifyVisitWithLocation); + } + + public function locateVisitsWithEmptyLocation(callable $geolocateVisit, callable $notifyVisitWithLocation): void + { + $this->locateVisits([], $geolocateVisit, $notifyVisitWithLocation); + } + + /** + * @param iterable|Visit[] $results + */ + private function locateVisits(iterable $results, callable $geolocateVisit, callable $notifyVisitWithLocation): void + { $count = 0; $persistBlock = 200; @@ -58,13 +70,11 @@ class VisitService implements VisitServiceInterface $this->em->clear(); } - private function locateVisit(Visit $visit, VisitLocation $location, ?callable $notifyVisitWithLocation): void + private function locateVisit(Visit $visit, VisitLocation $location, callable $notifyVisitWithLocation): void { $visit->locate($location); $this->em->persist($visit); - if ($notifyVisitWithLocation !== null) { - $notifyVisitWithLocation($location, $visit); - } + $notifyVisitWithLocation($location, $visit); } } diff --git a/module/Core/src/Visit/VisitLocatorInterface.php b/module/Core/src/Visit/VisitLocatorInterface.php new file mode 100644 index 00000000..ea06c1b1 --- /dev/null +++ b/module/Core/src/Visit/VisitLocatorInterface.php @@ -0,0 +1,12 @@ +em = $this->prophesize(EntityManager::class); - $this->visitService = new VisitService($this->em->reveal()); + $this->visitService = new VisitLocator($this->em->reveal()); } /** @test */ @@ -95,6 +95,7 @@ class VisitServiceTest extends TestCase throw $isNonLocatableAddress ? new IpCannotBeLocatedException('Cannot be located') : IpCannotBeLocatedException::forError(new Exception('')); + }, static function (): void { }); $findUnlocatedVisits->shouldHaveBeenCalledOnce(); From f730c24ecb9f6a365209eab429b1e409df4195d6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 26 Mar 2020 22:56:53 +0100 Subject: [PATCH 03/11] Created method to return visits with empty location --- .../Core/src/Repository/VisitRepository.php | 46 +++++++++++++++++-- .../Repository/VisitRepositoryInterface.php | 19 ++++++-- module/Core/src/Visit/VisitLocator.php | 4 +- .../Repository/VisitRepositoryTest.php | 34 +++++++------- 4 files changed, 77 insertions(+), 26 deletions(-) diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 28eb2466..aa3aa57d 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -5,32 +5,70 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Repository; use Doctrine\ORM\EntityRepository; +use Doctrine\ORM\Query; use Doctrine\ORM\QueryBuilder; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\Visit; class VisitRepository extends EntityRepository implements VisitRepositoryInterface { + private const DEFAULT_BLOCK_SIZE = 10000; + /** * This method will allow you to iterate the whole list of unlocated visits, but loading them into memory in * smaller blocks of a specific size. * This will have side effects if you update those rows while you iterate them, in a way that they are no longer * unlocated. * If you plan to do so, pass the first argument as false in order to disable applying offsets while slicing the - * dataset + * dataset. * * @return iterable|Visit[] */ - public function findUnlocatedVisits(bool $applyOffset = true, int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable + public function findUnlocatedVisits(bool $applyOffset = true): iterable { $dql = <<getEntityManager()->createQuery($dql) - ->setMaxResults($blockSize); + $query = $this->getEntityManager()->createQuery($dql); $remainingVisitsToProcess = $this->count(['visitLocation' => null]); + + return $this->findVisitsForQuery($query, $remainingVisitsToProcess, $applyOffset); + } + + /** + * This method will allow you to iterate the whole list of unlocated visits, but loading them into memory in + * smaller blocks of a specific size. + * This will have side effects if you update those rows while you iterate them, in a way that they are no longer + * unlocated. + * If you plan to do so, pass the first argument as false in order to disable applying offsets while slicing the + * dataset. + * + * @return iterable|Visit[] + */ + public function findVisitsWithEmptyLocation(bool $applyOffset = true): iterable + { + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->from(Visit::class, 'v') + ->join('v.visitLocation', 'vl') + ->where($qb->expr()->isNotNull('v.visitLocation')) + ->andWhere($qb->expr()->eq('vl.isEmpty', ':isEmpty')) + ->setParameter('isEmpty', true); + $countQb = clone $qb; + + $query = $qb->select('v')->getQuery(); + $remainingVisitsToProcess = (int) $countQb->select('COUNT(DISTINCT v.id)')->getQuery()->getSingleScalarResult(); + + return $this->findVisitsForQuery($query, $remainingVisitsToProcess, $applyOffset); + } + + private function findVisitsForQuery(Query $query, int $remainingVisitsToProcess, bool $applyOffset = true): iterable + { + $blockSize = self::DEFAULT_BLOCK_SIZE; + $query = $query->setMaxResults($blockSize); $offset = 0; + // FIXME Do not use the $applyOffset workaround. Instead, always start with first result, but skip already + // processed results. That should work both if any entry is edited or not while ($remainingVisitsToProcess > 0) { $iterator = $query->setFirstResult($applyOffset ? $offset : null)->iterate(); foreach ($iterator as $key => [$value]) { diff --git a/module/Core/src/Repository/VisitRepositoryInterface.php b/module/Core/src/Repository/VisitRepositoryInterface.php index 0d0b66d0..ceb07564 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -10,18 +10,29 @@ use Shlinkio\Shlink\Core\Entity\Visit; interface VisitRepositoryInterface extends ObjectRepository { - public const DEFAULT_BLOCK_SIZE = 10000; + /** + * This method will allow you to iterate the whole list of unlocated visits, but loading them into memory in + * smaller blocks of a specific size. + * This will have side effects if you update those rows while you iterate them, in a way that they are no longer + * unlocated. + * If you plan to do so, pass the first argument as false in order to disable applying offsets while slicing the + * dataset. + * + * @return iterable|Visit[] + */ + public function findUnlocatedVisits(bool $applyOffset = true): iterable; /** * This method will allow you to iterate the whole list of unlocated visits, but loading them into memory in * smaller blocks of a specific size. - * This will have side effects if you update those rows while you iterate them. + * This will have side effects if you update those rows while you iterate them, in a way that they are no longer + * unlocated. * If you plan to do so, pass the first argument as false in order to disable applying offsets while slicing the - * dataset + * dataset. * * @return iterable|Visit[] */ - public function findUnlocatedVisits(bool $applyOffset = true, int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; + public function findVisitsWithEmptyLocation(bool $applyOffset = true): iterable; /** * @return Visit[] diff --git a/module/Core/src/Visit/VisitLocator.php b/module/Core/src/Visit/VisitLocator.php index bcfc9e33..9c6ed098 100644 --- a/module/Core/src/Visit/VisitLocator.php +++ b/module/Core/src/Visit/VisitLocator.php @@ -29,7 +29,9 @@ class VisitLocator implements VisitLocatorInterface public function locateVisitsWithEmptyLocation(callable $geolocateVisit, callable $notifyVisitWithLocation): void { - $this->locateVisits([], $geolocateVisit, $notifyVisitWithLocation); + /** @var VisitRepository $repo */ + $repo = $this->em->getRepository(Visit::class); + $this->locateVisits($repo->findVisitsWithEmptyLocation(false), $geolocateVisit, $notifyVisitWithLocation); } /** diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php index bd1a0f2d..60420e70 100644 --- a/module/Core/test-db/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Repository/VisitRepositoryTest.php @@ -36,19 +36,24 @@ class VisitRepositoryTest extends DatabaseTestCase $this->repo = $this->getEntityManager()->getRepository(Visit::class); } - /** - * @test - * @dataProvider provideBlockSize - */ - public function findUnlocatedVisitsReturnsProperVisits(int $blockSize): void + /** @test */ + public function findVisitsReturnsProperVisits(): void { $shortUrl = new ShortUrl(''); $this->getEntityManager()->persist($shortUrl); + $countIterable = function (iterable $results): int { + $resultsCount = 0; + foreach ($results as $value) { + $resultsCount++; + } + + return $resultsCount; + }; for ($i = 0; $i < 6; $i++) { $visit = new Visit($shortUrl, Visitor::emptyInstance()); - if ($i % 2 === 0) { + if ($i >= 2) { $location = new VisitLocation(Location::emptyInstance()); $this->getEntityManager()->persist($location); $visit->locate($location); @@ -58,18 +63,13 @@ class VisitRepositoryTest extends DatabaseTestCase } $this->getEntityManager()->flush(); - $resultsCount = 0; - $results = $this->repo->findUnlocatedVisits(true, $blockSize); - foreach ($results as $value) { - $resultsCount++; - } + $withEmptyLocation = $this->repo->findVisitsWithEmptyLocation(); + $unlocated = $this->repo->findUnlocatedVisits(); - $this->assertEquals(3, $resultsCount); - } - - public function provideBlockSize(): iterable - { - return map(range(1, 5), fn (int $value) => [$value]); + // Important! assertCount will not work here, as this iterable object loads data dynamically and counts to + // 0 if not iterated + $this->assertEquals(2, $countIterable($unlocated)); + $this->assertEquals(4, $countIterable($withEmptyLocation)); } /** @test */ From 43a3d469e7f74b244cb75778d38838cb7f24589f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 27 Mar 2020 22:01:26 +0100 Subject: [PATCH 04/11] Improved how visits with some conditions are fetched from the database, so all internal logic is 100% transparent regardless the purpose --- .../Core/src/Repository/VisitRepository.php | 70 +++++++------------ .../Repository/VisitRepositoryInterface.php | 26 ++----- module/Core/src/Visit/VisitLocator.php | 4 +- .../Repository/VisitRepositoryTest.php | 16 +++-- module/Core/test/Visit/VisitLocatorTest.php | 4 +- 5 files changed, 49 insertions(+), 71 deletions(-) diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index aa3aa57d..ed18df10 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -5,79 +5,61 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Repository; use Doctrine\ORM\EntityRepository; -use Doctrine\ORM\Query; use Doctrine\ORM\QueryBuilder; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\Visit; class VisitRepository extends EntityRepository implements VisitRepositoryInterface { - private const DEFAULT_BLOCK_SIZE = 10000; - /** - * This method will allow you to iterate the whole list of unlocated visits, but loading them into memory in - * smaller blocks of a specific size. - * This will have side effects if you update those rows while you iterate them, in a way that they are no longer - * unlocated. - * If you plan to do so, pass the first argument as false in order to disable applying offsets while slicing the - * dataset. - * * @return iterable|Visit[] */ - public function findUnlocatedVisits(bool $applyOffset = true): iterable + public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable { - $dql = <<getEntityManager()->createQuery($dql); - $remainingVisitsToProcess = $this->count(['visitLocation' => null]); + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->select('v') + ->from(Visit::class, 'v') + ->where($qb->expr()->isNull('v.visitLocation')); - return $this->findVisitsForQuery($query, $remainingVisitsToProcess, $applyOffset); + return $this->findVisitsForQuery($qb, $blockSize); } /** - * This method will allow you to iterate the whole list of unlocated visits, but loading them into memory in - * smaller blocks of a specific size. - * This will have side effects if you update those rows while you iterate them, in a way that they are no longer - * unlocated. - * If you plan to do so, pass the first argument as false in order to disable applying offsets while slicing the - * dataset. - * * @return iterable|Visit[] */ - public function findVisitsWithEmptyLocation(bool $applyOffset = true): iterable + public function findVisitsWithEmptyLocation(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable { $qb = $this->getEntityManager()->createQueryBuilder(); - $qb->from(Visit::class, 'v') + $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); - $countQb = clone $qb; - $query = $qb->select('v')->getQuery(); - $remainingVisitsToProcess = (int) $countQb->select('COUNT(DISTINCT v.id)')->getQuery()->getSingleScalarResult(); - - return $this->findVisitsForQuery($query, $remainingVisitsToProcess, $applyOffset); + return $this->findVisitsForQuery($qb, $blockSize); } - private function findVisitsForQuery(Query $query, int $remainingVisitsToProcess, bool $applyOffset = true): iterable + private function findVisitsForQuery(QueryBuilder $qb, int $blockSize): iterable { - $blockSize = self::DEFAULT_BLOCK_SIZE; - $query = $query->setMaxResults($blockSize); - $offset = 0; + $originalQueryBuilder = $qb->setMaxResults($blockSize) + ->orderBy('v.id', 'ASC'); + $lastId = '0'; - // FIXME Do not use the $applyOffset workaround. Instead, always start with first result, but skip already - // processed results. That should work both if any entry is edited or not - while ($remainingVisitsToProcess > 0) { - $iterator = $query->setFirstResult($applyOffset ? $offset : null)->iterate(); - foreach ($iterator as $key => [$value]) { - yield $key => $value; + do { + $qb = (clone $originalQueryBuilder)->andWhere($qb->expr()->gt('v.id', $lastId)); + $iterator = $qb->getQuery()->iterate(); + $resultsFound = false; + + /** @var Visit $visit */ + foreach ($iterator as $key => [$visit]) { + $resultsFound = true; + yield $key => $visit; } - $remainingVisitsToProcess -= $blockSize; - $offset += $blockSize; - } + // As the query is ordered by ID, we can take the last one every time in order to exclude the whole list + $lastId = isset($visit) ? $visit->getId() : $lastId; + } while ($resultsFound); } /** diff --git a/module/Core/src/Repository/VisitRepositoryInterface.php b/module/Core/src/Repository/VisitRepositoryInterface.php index ceb07564..b076cff3 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -10,29 +10,17 @@ use Shlinkio\Shlink\Core\Entity\Visit; interface VisitRepositoryInterface extends ObjectRepository { - /** - * This method will allow you to iterate the whole list of unlocated visits, but loading them into memory in - * smaller blocks of a specific size. - * This will have side effects if you update those rows while you iterate them, in a way that they are no longer - * unlocated. - * If you plan to do so, pass the first argument as false in order to disable applying offsets while slicing the - * dataset. - * - * @return iterable|Visit[] - */ - public function findUnlocatedVisits(bool $applyOffset = true): iterable; + public const DEFAULT_BLOCK_SIZE = 10000; /** - * This method will allow you to iterate the whole list of unlocated visits, but loading them into memory in - * smaller blocks of a specific size. - * This will have side effects if you update those rows while you iterate them, in a way that they are no longer - * unlocated. - * If you plan to do so, pass the first argument as false in order to disable applying offsets while slicing the - * dataset. - * * @return iterable|Visit[] */ - public function findVisitsWithEmptyLocation(bool $applyOffset = true): iterable; + public function findUnlocatedVisits(int $defaultBlockSize = self::DEFAULT_BLOCK_SIZE): iterable; + + /** + * @return iterable|Visit[] + */ + public function findVisitsWithEmptyLocation(int $defaultBlockSize = self::DEFAULT_BLOCK_SIZE): iterable; /** * @return Visit[] diff --git a/module/Core/src/Visit/VisitLocator.php b/module/Core/src/Visit/VisitLocator.php index 9c6ed098..ee5acdf3 100644 --- a/module/Core/src/Visit/VisitLocator.php +++ b/module/Core/src/Visit/VisitLocator.php @@ -24,14 +24,14 @@ class VisitLocator implements VisitLocatorInterface { /** @var VisitRepository $repo */ $repo = $this->em->getRepository(Visit::class); - $this->locateVisits($repo->findUnlocatedVisits(false), $geolocateVisit, $notifyVisitWithLocation); + $this->locateVisits($repo->findUnlocatedVisits(), $geolocateVisit, $notifyVisitWithLocation); } public function locateVisitsWithEmptyLocation(callable $geolocateVisit, callable $notifyVisitWithLocation): void { /** @var VisitRepository $repo */ $repo = $this->em->getRepository(Visit::class); - $this->locateVisits($repo->findVisitsWithEmptyLocation(false), $geolocateVisit, $notifyVisitWithLocation); + $this->locateVisits($repo->findVisitsWithEmptyLocation(), $geolocateVisit, $notifyVisitWithLocation); } /** diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php index 60420e70..b72453d6 100644 --- a/module/Core/test-db/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Repository/VisitRepositoryTest.php @@ -36,8 +36,11 @@ class VisitRepositoryTest extends DatabaseTestCase $this->repo = $this->getEntityManager()->getRepository(Visit::class); } - /** @test */ - public function findVisitsReturnsProperVisits(): void + /** + * @test + * @dataProvider provideBlockSize + */ + public function findVisitsReturnsProperVisits(int $blockSize): void { $shortUrl = new ShortUrl(''); $this->getEntityManager()->persist($shortUrl); @@ -63,8 +66,8 @@ class VisitRepositoryTest extends DatabaseTestCase } $this->getEntityManager()->flush(); - $withEmptyLocation = $this->repo->findVisitsWithEmptyLocation(); - $unlocated = $this->repo->findUnlocatedVisits(); + $withEmptyLocation = $this->repo->findVisitsWithEmptyLocation($blockSize); + $unlocated = $this->repo->findUnlocatedVisits($blockSize); // Important! assertCount will not work here, as this iterable object loads data dynamically and counts to // 0 if not iterated @@ -72,6 +75,11 @@ class VisitRepositoryTest extends DatabaseTestCase $this->assertEquals(4, $countIterable($withEmptyLocation)); } + 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/VisitLocatorTest.php b/module/Core/test/Visit/VisitLocatorTest.php index 400a27c7..9e223e15 100644 --- a/module/Core/test/Visit/VisitLocatorTest.php +++ b/module/Core/test/Visit/VisitLocatorTest.php @@ -46,7 +46,7 @@ class VisitLocatorTest extends TestCase ); $repo = $this->prophesize(VisitRepository::class); - $findUnlocatedVisits = $repo->findUnlocatedVisits(false)->willReturn($unlocatedVisits); + $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits); $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { @@ -81,7 +81,7 @@ class VisitLocatorTest extends TestCase ]; $repo = $this->prophesize(VisitRepository::class); - $findUnlocatedVisits = $repo->findUnlocatedVisits(false)->willReturn($unlocatedVisits); + $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits); $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { From fcce18b0599b4eaa7279408b5ecc0ad4773b2561 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 28 Mar 2020 08:05:15 +0100 Subject: [PATCH 05/11] Changed VisitLocator signature so that it expects an object implementing an interface instead of two arbitrary callbacks --- .../src/Command/Visit/LocateVisitsCommand.php | 27 ++++---- .../Command/Visit/LocateVisitsCommandTest.php | 37 +++++------ .../Visit/VisitGeolocationHelperInterface.php | 20 ++++++ module/Core/src/Visit/VisitLocator.php | 30 ++++----- .../Core/src/Visit/VisitLocatorInterface.php | 4 +- module/Core/test/Visit/VisitLocatorTest.php | 61 +++++++++++++------ 6 files changed, 110 insertions(+), 69 deletions(-) create mode 100644 module/Core/src/Visit/VisitGeolocationHelperInterface.php diff --git a/module/CLI/src/Command/Visit/LocateVisitsCommand.php b/module/CLI/src/Command/Visit/LocateVisitsCommand.php index 935b9eee..89a8aa71 100644 --- a/module/CLI/src/Command/Visit/LocateVisitsCommand.php +++ b/module/CLI/src/Command/Visit/LocateVisitsCommand.php @@ -13,6 +13,7 @@ use Shlinkio\Shlink\Common\Util\IpAddress; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException; +use Shlinkio\Shlink\Core\Visit\VisitGeolocationHelperInterface; use Shlinkio\Shlink\Core\Visit\VisitLocatorInterface; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; @@ -27,7 +28,7 @@ use Throwable; use function sprintf; -class LocateVisitsCommand extends AbstractLockedCommand +class LocateVisitsCommand extends AbstractLockedCommand implements VisitGeolocationHelperInterface { public const NAME = 'visit:locate'; @@ -73,20 +74,13 @@ class LocateVisitsCommand extends AbstractLockedCommand { $this->io = new SymfonyStyle($input, $output); $retry = $input->getOption('retry'); - $geolocateVisit = [$this, 'getGeolocationDataForVisit']; - $notifyVisitWithLocation = static function (VisitLocation $location) use ($output): void { - $message = ! $location->isEmpty() - ? sprintf(' [Address located in "%s"]', $location->getCountryName()) - : ' [Address not found]'; - $output->writeln($message); - }; try { $this->checkDbUpdate(); - $this->visitLocator->locateUnlocatedVisits($geolocateVisit, $notifyVisitWithLocation); + $this->visitLocator->locateUnlocatedVisits($this); if ($retry) { - $this->visitLocator->locateVisitsWithEmptyLocation($geolocateVisit, $notifyVisitWithLocation); + $this->visitLocator->locateVisitsWithEmptyLocation($this); } $this->io->success('Finished processing all IPs'); @@ -101,7 +95,10 @@ class LocateVisitsCommand extends AbstractLockedCommand } } - public function getGeolocationDataForVisit(Visit $visit): Location + /** + * @throws IpCannotBeLocatedException + */ + public function geolocateVisit(Visit $visit): Location { if (! $visit->hasRemoteAddr()) { $this->io->writeln( @@ -130,6 +127,14 @@ class LocateVisitsCommand extends AbstractLockedCommand } } + public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void + { + $message = ! $visitLocation->isEmpty() + ? sprintf(' [Address located in "%s"]', $visitLocation->getCountryName()) + : ' [Address not found]'; + $this->io->writeln($message); + } + private function checkDbUpdate(): void { try { diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index 94cd0bd1..30be9846 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -15,6 +15,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Visit\VisitGeolocationHelperInterface; use Shlinkio\Shlink\Core\Visit\VisitLocator; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; @@ -24,7 +25,6 @@ use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Tester\CommandTester; use Symfony\Component\Lock; -use function array_shift; use function sprintf; class LocateVisitsCommandTest extends TestCase @@ -68,13 +68,7 @@ class LocateVisitsCommandTest extends TestCase $location = new VisitLocation(Location::emptyInstance()); $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will( - function (array $args) use ($visit, $location): void { - $firstCallback = array_shift($args); - $firstCallback($visit); - - $secondCallback = array_shift($args); - $secondCallback($location, $visit); - }, + $this->invokeHelperMethods($visit, $location), ); $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn( Location::emptyInstance(), @@ -98,13 +92,7 @@ class LocateVisitsCommandTest extends TestCase $location = new VisitLocation(Location::emptyInstance()); $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will( - function (array $args) use ($visit, $location): void { - $firstCallback = array_shift($args); - $firstCallback($visit); - - $secondCallback = array_shift($args); - $secondCallback($location, $visit); - }, + $this->invokeHelperMethods($visit, $location), ); $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn( Location::emptyInstance(), @@ -137,13 +125,7 @@ class LocateVisitsCommandTest extends TestCase $location = new VisitLocation(Location::emptyInstance()); $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will( - function (array $args) use ($visit, $location): void { - $firstCallback = array_shift($args); - $firstCallback($visit); - - $secondCallback = array_shift($args); - $secondCallback($location, $visit); - }, + $this->invokeHelperMethods($visit, $location), ); $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willThrow(WrongIpException::class); @@ -156,6 +138,17 @@ class LocateVisitsCommandTest extends TestCase $resolveIpLocation->shouldHaveBeenCalledOnce(); } + private function invokeHelperMethods(Visit $visit, VisitLocation $location): callable + { + return function (array $args) use ($visit, $location): void { + /** @var VisitGeolocationHelperInterface $helper */ + [$helper] = $args; + + $helper->geolocateVisit($visit); + $helper->onVisitLocated($location, $visit); + }; + } + /** @test */ public function noActionIsPerformedIfLockIsAcquired(): void { diff --git a/module/Core/src/Visit/VisitGeolocationHelperInterface.php b/module/Core/src/Visit/VisitGeolocationHelperInterface.php new file mode 100644 index 00000000..95cca4a7 --- /dev/null +++ b/module/Core/src/Visit/VisitGeolocationHelperInterface.php @@ -0,0 +1,20 @@ +em = $em; + + /** @var VisitRepositoryInterface $repo */ + $repo = $em->getRepository(Visit::class); + $this->repo = $repo; } - public function locateUnlocatedVisits(callable $geolocateVisit, callable $notifyVisitWithLocation): void + public function locateUnlocatedVisits(VisitGeolocationHelperInterface $helper): void { - /** @var VisitRepository $repo */ - $repo = $this->em->getRepository(Visit::class); - $this->locateVisits($repo->findUnlocatedVisits(), $geolocateVisit, $notifyVisitWithLocation); + $this->locateVisits($this->repo->findUnlocatedVisits(), $helper); } - public function locateVisitsWithEmptyLocation(callable $geolocateVisit, callable $notifyVisitWithLocation): void + public function locateVisitsWithEmptyLocation(VisitGeolocationHelperInterface $helper): void { - /** @var VisitRepository $repo */ - $repo = $this->em->getRepository(Visit::class); - $this->locateVisits($repo->findVisitsWithEmptyLocation(), $geolocateVisit, $notifyVisitWithLocation); + $this->locateVisits($this->repo->findVisitsWithEmptyLocation(), $helper); } /** * @param iterable|Visit[] $results */ - private function locateVisits(iterable $results, callable $geolocateVisit, callable $notifyVisitWithLocation): void + private function locateVisits(iterable $results, VisitGeolocationHelperInterface $helper): void { $count = 0; $persistBlock = 200; @@ -46,8 +47,7 @@ class VisitLocator implements VisitLocatorInterface $count++; try { - /** @var Location $location */ - $location = $geolocateVisit($visit); + $location = $helper->geolocateVisit($visit); } catch (IpCannotBeLocatedException $e) { if (! $e->isNonLocatableAddress()) { // Skip if the visit's IP could not be located because of an error @@ -59,7 +59,7 @@ class VisitLocator implements VisitLocatorInterface } $location = new VisitLocation($location); - $this->locateVisit($visit, $location, $notifyVisitWithLocation); + $this->locateVisit($visit, $location, $helper); // Flush and clear after X iterations if ($count % $persistBlock === 0) { @@ -72,11 +72,11 @@ class VisitLocator implements VisitLocatorInterface $this->em->clear(); } - private function locateVisit(Visit $visit, VisitLocation $location, callable $notifyVisitWithLocation): void + private function locateVisit(Visit $visit, VisitLocation $location, VisitGeolocationHelperInterface $helper): void { $visit->locate($location); $this->em->persist($visit); - $notifyVisitWithLocation($location, $visit); + $helper->onVisitLocated($location, $visit); } } diff --git a/module/Core/src/Visit/VisitLocatorInterface.php b/module/Core/src/Visit/VisitLocatorInterface.php index ea06c1b1..6e06d296 100644 --- a/module/Core/src/Visit/VisitLocatorInterface.php +++ b/module/Core/src/Visit/VisitLocatorInterface.php @@ -6,7 +6,7 @@ namespace Shlinkio\Shlink\Core\Visit; interface VisitLocatorInterface { - public function locateUnlocatedVisits(callable $geolocateVisit, callable $notifyVisitWithLocation): void; + public function locateUnlocatedVisits(VisitGeolocationHelperInterface $helper): void; - public function locateVisitsWithEmptyLocation(callable $geolocateVisit, callable $notifyVisitWithLocation): void; + public function locateVisitsWithEmptyLocation(VisitGeolocationHelperInterface $helper): void; } diff --git a/module/Core/test/Visit/VisitLocatorTest.php b/module/Core/test/Visit/VisitLocatorTest.php index 9e223e15..1972860b 100644 --- a/module/Core/test/Visit/VisitLocatorTest.php +++ b/module/Core/test/Visit/VisitLocatorTest.php @@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\Core\Visit; use Doctrine\ORM\EntityManager; use Exception; +use PHPUnit\Framework\Assert; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; @@ -14,7 +15,8 @@ use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException; use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Repository\VisitRepository; +use Shlinkio\Shlink\Core\Repository\VisitRepositoryInterface; +use Shlinkio\Shlink\Core\Visit\VisitGeolocationHelperInterface; use Shlinkio\Shlink\Core\Visit\VisitLocator; use Shlinkio\Shlink\IpGeolocation\Model\Location; @@ -30,10 +32,14 @@ class VisitLocatorTest extends TestCase { private VisitLocator $visitService; private ObjectProphecy $em; + private ObjectProphecy $repo; public function setUp(): void { $this->em = $this->prophesize(EntityManager::class); + $this->repo = $this->prophesize(VisitRepositoryInterface::class); + $this->em->getRepository(Visit::class)->willReturn($this->repo->reveal()); + $this->visitService = new VisitLocator($this->em->reveal()); } @@ -45,9 +51,7 @@ class VisitLocatorTest extends TestCase fn (int $i) => new Visit(new ShortUrl(sprintf('short_code_%s', $i)), Visitor::emptyInstance()), ); - $repo = $this->prophesize(VisitRepository::class); - $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits); - $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); + $findUnlocatedVisits = $this->repo->findUnlocatedVisits()->willReturn($unlocatedVisits); $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { }); @@ -56,15 +60,22 @@ class VisitLocatorTest extends TestCase $clear = $this->em->clear()->will(function (): void { }); - $this->visitService->locateUnlocatedVisits(fn () => Location::emptyInstance(), function (): void { - $args = func_get_args(); + $this->visitService->locateUnlocatedVisits(new class implements VisitGeolocationHelperInterface { + public function geolocateVisit(Visit $visit): Location + { + return Location::emptyInstance(); + } - $this->assertInstanceOf(VisitLocation::class, array_shift($args)); - $this->assertInstanceOf(Visit::class, array_shift($args)); + public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void + { + $args = func_get_args(); + + Assert::assertInstanceOf(VisitLocation::class, array_shift($args)); + Assert::assertInstanceOf(Visit::class, array_shift($args)); + } }); $findUnlocatedVisits->shouldHaveBeenCalledOnce(); - $getRepo->shouldHaveBeenCalledOnce(); $persist->shouldHaveBeenCalledTimes(count($unlocatedVisits)); $flush->shouldHaveBeenCalledTimes(floor(count($unlocatedVisits) / 200) + 1); $clear->shouldHaveBeenCalledTimes(floor(count($unlocatedVisits) / 200) + 1); @@ -80,9 +91,7 @@ class VisitLocatorTest extends TestCase new Visit(new ShortUrl('foo'), Visitor::emptyInstance()), ]; - $repo = $this->prophesize(VisitRepository::class); - $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits); - $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); + $findUnlocatedVisits = $this->repo->findUnlocatedVisits()->willReturn($unlocatedVisits); $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { }); @@ -91,15 +100,29 @@ class VisitLocatorTest extends TestCase $clear = $this->em->clear()->will(function (): void { }); - $this->visitService->locateUnlocatedVisits(function () use ($isNonLocatableAddress): void { - throw $isNonLocatableAddress - ? new IpCannotBeLocatedException('Cannot be located') - : IpCannotBeLocatedException::forError(new Exception('')); - }, static function (): void { - }); + $this->visitService->locateUnlocatedVisits( + new class ($isNonLocatableAddress) implements VisitGeolocationHelperInterface { + private bool $isNonLocatableAddress; + + public function __construct(bool $isNonLocatableAddress) + { + $this->isNonLocatableAddress = $isNonLocatableAddress; + } + + public function geolocateVisit(Visit $visit): Location + { + throw $this->isNonLocatableAddress + ? new IpCannotBeLocatedException('Cannot be located') + : IpCannotBeLocatedException::forError(new Exception('')); + } + + public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void + { + } + }, + ); $findUnlocatedVisits->shouldHaveBeenCalledOnce(); - $getRepo->shouldHaveBeenCalledOnce(); $persist->shouldHaveBeenCalledTimes($isNonLocatableAddress ? 1 : 0); $flush->shouldHaveBeenCalledOnce(); $clear->shouldHaveBeenCalledOnce(); From fb8ab0b5fe1b7f730b46ff576abcd7665d43bde7 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 28 Mar 2020 09:12:15 +0100 Subject: [PATCH 06/11] Implemented how to reprocess the locations of all existing visits --- .../src/Command/Visit/LocateVisitsCommand.php | 51 ++++++++++++++++--- .../Core/src/Repository/VisitRepository.php | 9 ++++ .../Repository/VisitRepositoryInterface.php | 9 +++- module/Core/src/Visit/VisitLocator.php | 5 ++ .../Core/src/Visit/VisitLocatorInterface.php | 2 + .../Repository/VisitRepositoryTest.php | 6 ++- 6 files changed, 71 insertions(+), 11 deletions(-) diff --git a/module/CLI/src/Command/Visit/LocateVisitsCommand.php b/module/CLI/src/Command/Visit/LocateVisitsCommand.php index 89a8aa71..1a7d0bd0 100644 --- a/module/CLI/src/Command/Visit/LocateVisitsCommand.php +++ b/module/CLI/src/Command/Visit/LocateVisitsCommand.php @@ -18,6 +18,7 @@ use Shlinkio\Shlink\Core\Visit\VisitLocatorInterface; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; +use Symfony\Component\Console\Exception\RuntimeException; use Symfony\Component\Console\Helper\ProgressBar; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; @@ -60,30 +61,66 @@ class LocateVisitsCommand extends AbstractLockedCommand implements VisitGeolocat 'retry', 'r', InputOption::VALUE_NONE, - 'Will retry visits that were located with an empty location, in case it was a temporal issue.', + 'Will retry the location of visits that were located with a not-found location, in case it was due to ' + . 'a temporal issue.', ) ->addOption( 'all', 'a', InputOption::VALUE_NONE, - 'Will locate all visits, ignoring if they have already been located.', + 'When provided together with --retry, will locate all existing visits, regardless the fact that they ' + . 'have already been located.', ); } + protected function interact(InputInterface $input, OutputInterface $output): void + { + $this->io = new SymfonyStyle($input, $output); + $retry = $input->getOption('retry'); + $all = $input->getOption('all'); + + if ($all && !$retry) { + $this->io->writeln( + 'The --all flag has no effect on its own. You have to provide it ' + . 'together with --retry.', + ); + } + + if ($all && $retry && ! $this->warnAndVerifyContinue()) { + throw new RuntimeException('Execution aborted'); + } + } + + private function warnAndVerifyContinue(): bool + { + $this->io->warning([ + 'You are about to process the location of all existing visits your short URLs received.', + 'Since shlink saves visitors IP addresses anonymized, you could end up losing precision on some of ' + . 'your visits.', + 'Also, if you have a large amount of visits, this can be a very time consuming process. ' + . 'Continue at your own risk.', + ]); + return $this->io->confirm('Do you want to proceed?', false); + } + protected function lockedExecute(InputInterface $input, OutputInterface $output): int { - $this->io = new SymfonyStyle($input, $output); $retry = $input->getOption('retry'); + $all = $retry && $input->getOption('all'); try { $this->checkDbUpdate(); - $this->visitLocator->locateUnlocatedVisits($this); - if ($retry) { - $this->visitLocator->locateVisitsWithEmptyLocation($this); + if ($all) { + $this->visitLocator->locateAllVisits($this); + } else { + $this->visitLocator->locateUnlocatedVisits($this); + if ($retry) { + $this->visitLocator->locateVisitsWithEmptyLocation($this); + } } - $this->io->success('Finished processing all IPs'); + $this->io->success('Finished locating visits'); return ExitCodes::EXIT_SUCCESS; } catch (Throwable $e) { $this->io->error($e->getMessage()); diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index ed18df10..61b2afb8 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -40,6 +40,15 @@ class VisitRepository extends EntityRepository implements VisitRepositoryInterfa return $this->findVisitsForQuery($qb, $blockSize); } + public function findAllVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable + { + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->select('v') + ->from(Visit::class, 'v'); + + return $this->findVisitsForQuery($qb, $blockSize); + } + private function findVisitsForQuery(QueryBuilder $qb, int $blockSize): iterable { $originalQueryBuilder = $qb->setMaxResults($blockSize) diff --git a/module/Core/src/Repository/VisitRepositoryInterface.php b/module/Core/src/Repository/VisitRepositoryInterface.php index b076cff3..f9cbc8d9 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -15,12 +15,17 @@ interface VisitRepositoryInterface extends ObjectRepository /** * @return iterable|Visit[] */ - public function findUnlocatedVisits(int $defaultBlockSize = self::DEFAULT_BLOCK_SIZE): iterable; + public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; /** * @return iterable|Visit[] */ - public function findVisitsWithEmptyLocation(int $defaultBlockSize = self::DEFAULT_BLOCK_SIZE): iterable; + 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/src/Visit/VisitLocator.php b/module/Core/src/Visit/VisitLocator.php index 5a66208b..f955ef29 100644 --- a/module/Core/src/Visit/VisitLocator.php +++ b/module/Core/src/Visit/VisitLocator.php @@ -35,6 +35,11 @@ class VisitLocator implements VisitLocatorInterface $this->locateVisits($this->repo->findVisitsWithEmptyLocation(), $helper); } + public function locateAllVisits(VisitGeolocationHelperInterface $helper): void + { + $this->locateVisits($this->repo->findAllVisits(), $helper); + } + /** * @param iterable|Visit[] $results */ diff --git a/module/Core/src/Visit/VisitLocatorInterface.php b/module/Core/src/Visit/VisitLocatorInterface.php index 6e06d296..1c99de36 100644 --- a/module/Core/src/Visit/VisitLocatorInterface.php +++ b/module/Core/src/Visit/VisitLocatorInterface.php @@ -9,4 +9,6 @@ interface VisitLocatorInterface public function locateUnlocatedVisits(VisitGeolocationHelperInterface $helper): void; public function locateVisitsWithEmptyLocation(VisitGeolocationHelperInterface $helper): void; + + public function locateAllVisits(VisitGeolocationHelperInterface $helper): void; } diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php index b72453d6..034b15f9 100644 --- a/module/Core/test-db/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Repository/VisitRepositoryTest.php @@ -68,11 +68,13 @@ class VisitRepositoryTest extends DatabaseTestCase $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 counts to - // 0 if not iterated + // Important! assertCount will not work here, as this iterable object loads data dynamically and the count + // is 0 if not iterated $this->assertEquals(2, $countIterable($unlocated)); $this->assertEquals(4, $countIterable($withEmptyLocation)); + $this->assertEquals(6, $countIterable($all)); } public function provideBlockSize(): iterable From 55778eb810f8185bf4ada57b7dcd92e4f2d6bd80 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 28 Mar 2020 09:27:45 +0100 Subject: [PATCH 07/11] Ensured old visit locations are deleted when relocating a visit that has already been located --- module/CLI/src/Command/ShortUrl/GetVisitsCommand.php | 3 ++- module/Core/src/Entity/Visit.php | 5 ++--- module/Core/src/Visit/VisitLocator.php | 7 +++++++ .../Core/test/EventDispatcher/LocateShortUrlVisitTest.php | 3 +-- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php index 43949993..a0c2c91a 100644 --- a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php +++ b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php @@ -12,6 +12,7 @@ use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; +use Shlinkio\Shlink\Core\Visit\Model\UnknownVisitLocation; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; @@ -76,7 +77,7 @@ class GetVisitsCommand extends AbstractWithDateRangeCommand $rows = map($paginator->getCurrentItems(), function (Visit $visit) { $rowData = $visit->jsonSerialize(); - $rowData['country'] = $visit->getVisitLocation()->getCountryName(); + $rowData['country'] = ($visit->getVisitLocation() ?? new UnknownVisitLocation())->getCountryName(); return select_keys($rowData, ['referer', 'date', 'userAgent', 'country']); }); ShlinkTable::fromOutput($output)->render(['Referer', 'Date', 'User agent', 'Country'], $rows); diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index d278ed6a..e8cbb119 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -10,7 +10,6 @@ use Shlinkio\Shlink\Common\Entity\AbstractEntity; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; use Shlinkio\Shlink\Common\Util\IpAddress; use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Visit\Model\UnknownVisitLocation; use Shlinkio\Shlink\Core\Visit\Model\VisitLocationInterface; class Visit extends AbstractEntity implements JsonSerializable @@ -60,9 +59,9 @@ class Visit extends AbstractEntity implements JsonSerializable return $this->shortUrl; } - public function getVisitLocation(): VisitLocationInterface + public function getVisitLocation(): ?VisitLocationInterface { - return $this->visitLocation ?? new UnknownVisitLocation(); + return $this->visitLocation; } public function isLocatable(): bool diff --git a/module/Core/src/Visit/VisitLocator.php b/module/Core/src/Visit/VisitLocator.php index f955ef29..46a30559 100644 --- a/module/Core/src/Visit/VisitLocator.php +++ b/module/Core/src/Visit/VisitLocator.php @@ -79,9 +79,16 @@ class VisitLocator implements VisitLocatorInterface private function locateVisit(Visit $visit, VisitLocation $location, VisitGeolocationHelperInterface $helper): void { + $prevLocation = $visit->getVisitLocation(); + $visit->locate($location); $this->em->persist($visit); + // In order to avoid leaving orphan locations, remove the previous one + if ($prevLocation !== null) { + $this->em->remove($prevLocation); + } + $helper->onVisitLocated($location, $visit); } } diff --git a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php index c8f6f388..087c0e0b 100644 --- a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php +++ b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php @@ -20,7 +20,6 @@ use Shlinkio\Shlink\Core\EventDispatcher\LocateShortUrlVisit; use Shlinkio\Shlink\Core\EventDispatcher\ShortUrlVisited; use Shlinkio\Shlink\Core\EventDispatcher\VisitLocated; use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Visit\Model\UnknownVisitLocation; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; @@ -218,7 +217,7 @@ class LocateShortUrlVisitTest extends TestCase ($this->locateVisit)($event); - $this->assertEquals($visit->getVisitLocation(), new UnknownVisitLocation()); + $this->assertNull($visit->getVisitLocation()); $findVisit->shouldHaveBeenCalledOnce(); $flush->shouldNotHaveBeenCalled(); $resolveIp->shouldNotHaveBeenCalled(); From c012b4740db3a7feb7f39954a3c2942d6802e725 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 28 Mar 2020 10:00:46 +0100 Subject: [PATCH 08/11] Updated VisitLocator test so that it covers all public methods --- module/Core/test/Visit/VisitLocatorTest.php | 59 ++++++++++++++++----- 1 file changed, 46 insertions(+), 13 deletions(-) diff --git a/module/Core/test/Visit/VisitLocatorTest.php b/module/Core/test/Visit/VisitLocatorTest.php index 1972860b..d856262c 100644 --- a/module/Core/test/Visit/VisitLocatorTest.php +++ b/module/Core/test/Visit/VisitLocatorTest.php @@ -9,6 +9,7 @@ use Exception; use PHPUnit\Framework\Assert; use PHPUnit\Framework\TestCase; use Prophecy\Argument; +use Prophecy\Prophecy\MethodProphecy; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; @@ -43,15 +44,20 @@ class VisitLocatorTest extends TestCase $this->visitService = new VisitLocator($this->em->reveal()); } - /** @test */ - public function locateVisitsIteratesAndLocatesUnlocatedVisits(): void - { + /** + * @test + * @dataProvider provideMethodNames + */ + public function locateVisitsIteratesAndLocatesExpectedVisits( + string $serviceMethodName, + string $expectedRepoMethodName + ): void { $unlocatedVisits = map( range(1, 200), fn (int $i) => new Visit(new ShortUrl(sprintf('short_code_%s', $i)), Visitor::emptyInstance()), ); - $findUnlocatedVisits = $this->repo->findUnlocatedVisits()->willReturn($unlocatedVisits); + $findVisits = $this->mockRepoMethod($expectedRepoMethodName)->willReturn($unlocatedVisits); $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { }); @@ -60,7 +66,7 @@ class VisitLocatorTest extends TestCase $clear = $this->em->clear()->will(function (): void { }); - $this->visitService->locateUnlocatedVisits(new class implements VisitGeolocationHelperInterface { + $this->visitService->{$serviceMethodName}(new class implements VisitGeolocationHelperInterface { public function geolocateVisit(Visit $visit): Location { return Location::emptyInstance(); @@ -75,23 +81,33 @@ class VisitLocatorTest extends TestCase } }); - $findUnlocatedVisits->shouldHaveBeenCalledOnce(); + $findVisits->shouldHaveBeenCalledOnce(); $persist->shouldHaveBeenCalledTimes(count($unlocatedVisits)); $flush->shouldHaveBeenCalledTimes(floor(count($unlocatedVisits) / 200) + 1); $clear->shouldHaveBeenCalledTimes(floor(count($unlocatedVisits) / 200) + 1); } + public function provideMethodNames(): iterable + { + yield 'locateUnlocatedVisits' => ['locateUnlocatedVisits', 'findUnlocatedVisits']; + yield 'locateVisitsWithEmptyLocation' => ['locateVisitsWithEmptyLocation', 'findVisitsWithEmptyLocation']; + yield 'locateAllVisits' => ['locateAllVisits', 'findAllVisits']; + } + /** * @test * @dataProvider provideIsNonLocatableAddress */ - public function visitsWhichCannotBeLocatedAreIgnoredOrLocatedAsEmpty(bool $isNonLocatableAddress): void - { + public function visitsWhichCannotBeLocatedAreIgnoredOrLocatedAsEmpty( + string $serviceMethodName, + string $expectedRepoMethodName, + bool $isNonLocatableAddress + ): void { $unlocatedVisits = [ new Visit(new ShortUrl('foo'), Visitor::emptyInstance()), ]; - $findUnlocatedVisits = $this->repo->findUnlocatedVisits()->willReturn($unlocatedVisits); + $findVisits = $this->mockRepoMethod($expectedRepoMethodName)->willReturn($unlocatedVisits); $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { }); @@ -100,7 +116,7 @@ class VisitLocatorTest extends TestCase $clear = $this->em->clear()->will(function (): void { }); - $this->visitService->locateUnlocatedVisits( + $this->visitService->{$serviceMethodName}( new class ($isNonLocatableAddress) implements VisitGeolocationHelperInterface { private bool $isNonLocatableAddress; @@ -122,7 +138,7 @@ class VisitLocatorTest extends TestCase }, ); - $findUnlocatedVisits->shouldHaveBeenCalledOnce(); + $findVisits->shouldHaveBeenCalledOnce(); $persist->shouldHaveBeenCalledTimes($isNonLocatableAddress ? 1 : 0); $flush->shouldHaveBeenCalledOnce(); $clear->shouldHaveBeenCalledOnce(); @@ -130,7 +146,24 @@ class VisitLocatorTest extends TestCase public function provideIsNonLocatableAddress(): iterable { - yield 'The address is locatable' => [false]; - yield 'The address is non-locatable' => [true]; + yield 'locateUnlocatedVisits - locatable address' => ['locateUnlocatedVisits', 'findUnlocatedVisits', false]; + yield 'locateUnlocatedVisits - non-locatable address' => ['locateUnlocatedVisits', 'findUnlocatedVisits', true]; + yield 'locateVisitsWithEmptyLocation - locatable address' => [ + 'locateVisitsWithEmptyLocation', + 'findVisitsWithEmptyLocation', + false, + ]; + yield 'locateVisitsWithEmptyLocation - non-locatable address' => [ + 'locateVisitsWithEmptyLocation', + 'findVisitsWithEmptyLocation', + true, + ]; + yield 'locateAllVisits - locatable address' => ['locateAllVisits', 'findAllVisits', false]; + yield 'locateAllVisits - non-locatable address' => ['locateAllVisits', 'findAllVisits', true]; + } + + private function mockRepoMethod(string $methodName): MethodProphecy + { + return (new MethodProphecy($this->repo, $methodName, new Argument\ArgumentsWildcard([]))); } } From 4d39c7041b1167d190d187bb3e620968f433568d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 28 Mar 2020 10:23:34 +0100 Subject: [PATCH 09/11] Improved LocateVisitsCommandtest so that it covers all possible workflows --- .../Command/Visit/LocateVisitsCommandTest.php | 76 +++++++++++++++++-- 1 file changed, 68 insertions(+), 8 deletions(-) diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index 30be9846..803ae472 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -21,12 +21,15 @@ use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; use Symfony\Component\Console\Application; +use Symfony\Component\Console\Exception\RuntimeException; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Tester\CommandTester; use Symfony\Component\Lock; use function sprintf; +use const PHP_EOL; + class LocateVisitsCommandTest extends TestCase { private CommandTester $commandTester; @@ -61,25 +64,53 @@ class LocateVisitsCommandTest extends TestCase $this->commandTester = new CommandTester($command); } - /** @test */ - public function allPendingVisitsAreProcessed(): void - { + /** + * @test + * @dataProvider provideArgs + */ + public function expectedSetOfVisitsIsProcessedBasedOnArgs( + int $expectedUnlocatedCalls, + int $expectedEmptyCalls, + int $expectedAllCalls, + bool $expectWarningPrint, + array $args + ): void { $visit = new Visit(new ShortUrl(''), new Visitor('', '', '1.2.3.4')); $location = new VisitLocation(Location::emptyInstance()); + $mockMethodBehavior = $this->invokeHelperMethods($visit, $location); - $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will( - $this->invokeHelperMethods($visit, $location), + $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will($mockMethodBehavior); + $locateEmptyVisits = $this->visitService->locateVisitsWithEmptyLocation(Argument::cetera())->will( + $mockMethodBehavior, ); + $locateAllVisits = $this->visitService->locateAllVisits(Argument::cetera())->will($mockMethodBehavior); $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn( Location::emptyInstance(), ); - $this->commandTester->execute([]); + $this->commandTester->setInputs(['y']); + $this->commandTester->execute($args); $output = $this->commandTester->getDisplay(); $this->assertStringContainsString('Processing IP 1.2.3.0', $output); - $locateVisits->shouldHaveBeenCalledOnce(); - $resolveIpLocation->shouldHaveBeenCalledOnce(); + if ($expectWarningPrint) { + $this->assertStringContainsString('Continue at your own risk', $output); + } else { + $this->assertStringNotContainsString('Continue at your own risk', $output); + } + $locateVisits->shouldHaveBeenCalledTimes($expectedUnlocatedCalls); + $locateEmptyVisits->shouldHaveBeenCalledTimes($expectedEmptyCalls); + $locateAllVisits->shouldHaveBeenCalledTimes($expectedAllCalls); + $resolveIpLocation->shouldHaveBeenCalledTimes( + $expectedUnlocatedCalls + $expectedEmptyCalls + $expectedAllCalls, + ); + } + + public function provideArgs(): iterable + { + yield 'no args' => [1, 0, 0, false, []]; + yield 'retry' => [1, 1, 0, false, ['--retry' => true]]; + yield 'all' => [0, 0, 1, true, ['--retry' => true, '--all' => true]]; } /** @@ -205,4 +236,33 @@ class LocateVisitsCommandTest extends TestCase yield [true, '[Warning] GeoLite2 database update failed. Proceeding with old version.']; yield [false, 'GeoLite2 database download failed. It is not possible to locate visits.']; } + + /** @test */ + public function providingAllFlagOnItsOwnDisplaysNotice(): void + { + $this->commandTester->execute(['--all' => true]); + $output = $this->commandTester->getDisplay(); + + $this->assertStringContainsString('The --all flag has no effect on its own', $output); + } + + /** + * @test + * @dataProvider provideAbortInputs + */ + public function processingAllCancelsCommandIfUserDoesNotActivelyAgreeToConfirmation(array $inputs): void + { + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Execution aborted'); + + $this->commandTester->setInputs($inputs); + $this->commandTester->execute(['--all' => true, '--retry' => true]); + } + + public function provideAbortInputs(): iterable + { + yield 'n' => [['n']]; + yield 'no' => [['no']]; + yield 'default' => [[PHP_EOL]]; + } } From 2a30afbe7da43e7a086dced0e1954194f7823e56 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 28 Mar 2020 10:29:12 +0100 Subject: [PATCH 10/11] Updated changelog --- CHANGELOG.md | 6 +++++- docker/README.md | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be5c993d..718a094e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). -## [Unreleased] +## 2.1.0 - 2020-03-28 #### Added @@ -17,6 +17,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Changed * [#656](https://github.com/shlinkio/shlink/issues/656) Updated to PHPUnit 9. +* [#641](https://github.com/shlinkio/shlink/issues/641) Added two new flags to the `visit:locate` command, `--retry` and `--all`. + + * When `--retry` is provided, it will try to re-locate visits which IP address was originally considered not found, in case it was a temporal issue. + * When `--all` is provided together with `--retry`, it will try to re-locate all existing visits. A warning and confirmation are displayed, as this can have side effects. #### Deprecated diff --git a/docker/README.md b/docker/README.md index f8e596ce..3977fa37 100644 --- a/docker/README.md +++ b/docker/README.md @@ -37,10 +37,10 @@ Or you can list all tags with: docker exec -it shlink_container shlink tag:list ``` -Or process remaining visits with: +Or locate remaining visits with: ```bash -docker exec -it shlink_container shlink visit:process +docker exec -it shlink_container shlink visit:locate ``` All shlink commands will work the same way. From 53ba58d7e9478366f2c21e1e2efa17f9084d6f8d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 28 Mar 2020 10:37:41 +0100 Subject: [PATCH 11/11] Moved initialization of the io object in LocateVisitsCommand to the initialize method --- module/CLI/src/Command/Visit/LocateVisitsCommand.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/module/CLI/src/Command/Visit/LocateVisitsCommand.php b/module/CLI/src/Command/Visit/LocateVisitsCommand.php index 1a7d0bd0..bf1ac14b 100644 --- a/module/CLI/src/Command/Visit/LocateVisitsCommand.php +++ b/module/CLI/src/Command/Visit/LocateVisitsCommand.php @@ -73,9 +73,13 @@ class LocateVisitsCommand extends AbstractLockedCommand implements VisitGeolocat ); } - protected function interact(InputInterface $input, OutputInterface $output): void + protected function initialize(InputInterface $input, OutputInterface $output): void { $this->io = new SymfonyStyle($input, $output); + } + + protected function interact(InputInterface $input, OutputInterface $output): void + { $retry = $input->getOption('retry'); $all = $input->getOption('all');