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/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/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/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. 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/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/CLI/src/Command/Visit/LocateVisitsCommand.php b/module/CLI/src/Command/Visit/LocateVisitsCommand.php index b19e8b19..bf1ac14b 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,15 @@ 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\VisitGeolocationHelperInterface; +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; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; use Symfony\Component\Lock\LockFactory; @@ -27,11 +29,11 @@ use Throwable; use function sprintf; -class LocateVisitsCommand extends AbstractLockedCommand +class LocateVisitsCommand extends AbstractLockedCommand implements VisitGeolocationHelperInterface { public const NAME = 'visit:locate'; - private VisitServiceInterface $visitService; + private VisitLocatorInterface $visitLocator; private IpLocationResolverInterface $ipLocationResolver; private GeolocationDbUpdaterInterface $dbUpdater; @@ -39,13 +41,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 +56,79 @@ 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 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, + 'When provided together with --retry, will locate all existing visits, regardless the fact that they ' + . 'have already been located.', + ); + } + + 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'); + + 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->visitService->locateUnlocatedVisits( - [$this, 'getGeolocationDataForVisit'], - static function (VisitLocation $location) use ($output): void { - if (!$location->isEmpty()) { - $output->writeln( - sprintf(' [Address located at "%s"]', $location->getCountryName()), - ); - } - }, - ); + 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()); - if ($e instanceof Exception && $this->io->isVerbose()) { + if ($e instanceof Throwable && $this->io->isVerbose()) { $this->getApplication()->renderThrowable($e, $this->io); } @@ -87,7 +136,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( @@ -116,6 +168,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 90073f10..803ae472 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -15,18 +15,21 @@ 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\VisitGeolocationHelperInterface; +use Shlinkio\Shlink\Core\Visit\VisitLocator; 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 array_shift; use function sprintf; +use const PHP_EOL; + class LocateVisitsCommandTest extends TestCase { private CommandTester $commandTester; @@ -38,7 +41,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); @@ -61,31 +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( - function (array $args) use ($visit, $location): void { - $firstCallback = array_shift($args); - $firstCallback($visit); - - $secondCallback = array_shift($args); - $secondCallback($location, $visit); - }, + $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]]; } /** @@ -98,13 +123,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 +156,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 +169,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 { @@ -212,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]]; + } } 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/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/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/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 === ''; - } } diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 454323ef..61b2afb8 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -12,33 +12,63 @@ use Shlinkio\Shlink\Core\Entity\Visit; class VisitRepository extends EntityRepository implements VisitRepositoryInterface { /** - * 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. - * 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, int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable + public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable { - $dql = <<getEntityManager()->createQuery($dql) - ->setMaxResults($blockSize); - $remainingVisitsToProcess = $this->count(['visitLocation' => null]); - $offset = 0; + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->select('v') + ->from(Visit::class, 'v') + ->where($qb->expr()->isNull('v.visitLocation')); - while ($remainingVisitsToProcess > 0) { - $iterator = $query->setFirstResult($applyOffset ? $offset : null)->iterate(); - foreach ($iterator as $key => [$value]) { - yield $key => $value; + return $this->findVisitsForQuery($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->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) + ->orderBy('v.id', 'ASC'); + $lastId = '0'; + + 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 0d0b66d0..f9cbc8d9 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -13,15 +13,19 @@ 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. - * 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, int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; + 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/src/Service/VisitService.php b/module/Core/src/Service/VisitService.php deleted file mode 100644 index 20a30f4c..00000000 --- a/module/Core/src/Service/VisitService.php +++ /dev/null @@ -1,70 +0,0 @@ -em = $em; - } - - public function locateUnlocatedVisits(callable $geolocateVisit, ?callable $notifyVisitWithLocation = null): void - { - /** @var VisitRepository $repo */ - $repo = $this->em->getRepository(Visit::class); - $results = $repo->findUnlocatedVisits(false); - $count = 0; - $persistBlock = 200; - - foreach ($results as $visit) { - $count++; - - try { - /** @var Location $location */ - $location = $geolocateVisit($visit); - } catch (IpCannotBeLocatedException $e) { - if (! $e->isNonLocatableAddress()) { - // Skip if the visit's IP could not be located because of an error - continue; - } - - // If the IP address is non-locatable, locate it as empty to prevent next processes to pick it again - $location = Location::emptyInstance(); - } - - $location = new VisitLocation($location); - $this->locateVisit($visit, $location, $notifyVisitWithLocation); - - // Flush and clear after X iterations - if ($count % $persistBlock === 0) { - $this->em->flush(); - $this->em->clear(); - } - } - - $this->em->flush(); - $this->em->clear(); - } - - private function locateVisit(Visit $visit, VisitLocation $location, ?callable $notifyVisitWithLocation): void - { - $visit->locate($location); - $this->em->persist($visit); - - if ($notifyVisitWithLocation !== null) { - $notifyVisitWithLocation($location, $visit); - } - } -} 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; + + /** @var VisitRepositoryInterface $repo */ + $repo = $em->getRepository(Visit::class); + $this->repo = $repo; + } + + public function locateUnlocatedVisits(VisitGeolocationHelperInterface $helper): void + { + $this->locateVisits($this->repo->findUnlocatedVisits(), $helper); + } + + public function locateVisitsWithEmptyLocation(VisitGeolocationHelperInterface $helper): void + { + $this->locateVisits($this->repo->findVisitsWithEmptyLocation(), $helper); + } + + public function locateAllVisits(VisitGeolocationHelperInterface $helper): void + { + $this->locateVisits($this->repo->findAllVisits(), $helper); + } + + /** + * @param iterable|Visit[] $results + */ + private function locateVisits(iterable $results, VisitGeolocationHelperInterface $helper): void + { + $count = 0; + $persistBlock = 200; + + foreach ($results as $visit) { + $count++; + + try { + $location = $helper->geolocateVisit($visit); + } catch (IpCannotBeLocatedException $e) { + if (! $e->isNonLocatableAddress()) { + // Skip if the visit's IP could not be located because of an error + continue; + } + + // If the IP address is non-locatable, locate it as empty to prevent next processes to pick it again + $location = Location::emptyInstance(); + } + + $location = new VisitLocation($location); + $this->locateVisit($visit, $location, $helper); + + // Flush and clear after X iterations + if ($count % $persistBlock === 0) { + $this->em->flush(); + $this->em->clear(); + } + } + + $this->em->flush(); + $this->em->clear(); + } + + 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/src/Visit/VisitLocatorInterface.php b/module/Core/src/Visit/VisitLocatorInterface.php new file mode 100644 index 00000000..1c99de36 --- /dev/null +++ b/module/Core/src/Visit/VisitLocatorInterface.php @@ -0,0 +1,14 @@ +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 +66,20 @@ class VisitRepositoryTest extends DatabaseTestCase } $this->getEntityManager()->flush(); - $resultsCount = 0; - $results = $this->repo->findUnlocatedVisits(true, $blockSize); - foreach ($results as $value) { - $resultsCount++; - } + $withEmptyLocation = $this->repo->findVisitsWithEmptyLocation($blockSize); + $unlocated = $this->repo->findUnlocatedVisits($blockSize); + $all = $this->repo->findAllVisits($blockSize); - $this->assertEquals(3, $resultsCount); + // 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 { - return map(range(1, 5), fn (int $value) => [$value]); + return map(range(1, 10), fn (int $value) => [$value]); } /** @test */ 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(); diff --git a/module/Core/test/Service/VisitServiceTest.php b/module/Core/test/Service/VisitServiceTest.php deleted file mode 100644 index 26e95557..00000000 --- a/module/Core/test/Service/VisitServiceTest.php +++ /dev/null @@ -1,112 +0,0 @@ -em = $this->prophesize(EntityManager::class); - $this->visitService = new VisitService($this->em->reveal()); - } - - /** @test */ - public function locateVisitsIteratesAndLocatesUnlocatedVisits(): void - { - $unlocatedVisits = map( - range(1, 200), - fn (int $i) => new Visit(new ShortUrl(sprintf('short_code_%s', $i)), Visitor::emptyInstance()), - ); - - $repo = $this->prophesize(VisitRepository::class); - $findUnlocatedVisits = $repo->findUnlocatedVisits(false)->willReturn($unlocatedVisits); - $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); - - $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { - }); - $flush = $this->em->flush()->will(function (): void { - }); - $clear = $this->em->clear()->will(function (): void { - }); - - $this->visitService->locateUnlocatedVisits(fn () => Location::emptyInstance(), function (): void { - $args = func_get_args(); - - $this->assertInstanceOf(VisitLocation::class, array_shift($args)); - $this->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); - } - - /** - * @test - * @dataProvider provideIsNonLocatableAddress - */ - public function visitsWhichCannotBeLocatedAreIgnoredOrLocatedAsEmpty(bool $isNonLocatableAddress): void - { - $unlocatedVisits = [ - new Visit(new ShortUrl('foo'), Visitor::emptyInstance()), - ]; - - $repo = $this->prophesize(VisitRepository::class); - $findUnlocatedVisits = $repo->findUnlocatedVisits(false)->willReturn($unlocatedVisits); - $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); - - $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { - }); - $flush = $this->em->flush()->will(function (): void { - }); - $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('')); - }); - - $findUnlocatedVisits->shouldHaveBeenCalledOnce(); - $getRepo->shouldHaveBeenCalledOnce(); - $persist->shouldHaveBeenCalledTimes($isNonLocatableAddress ? 1 : 0); - $flush->shouldHaveBeenCalledOnce(); - $clear->shouldHaveBeenCalledOnce(); - } - - public function provideIsNonLocatableAddress(): iterable - { - yield 'The address is locatable' => [false]; - yield 'The address is non-locatable' => [true]; - } -} diff --git a/module/Core/test/Visit/VisitLocatorTest.php b/module/Core/test/Visit/VisitLocatorTest.php new file mode 100644 index 00000000..d856262c --- /dev/null +++ b/module/Core/test/Visit/VisitLocatorTest.php @@ -0,0 +1,169 @@ +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()); + } + + /** + * @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()), + ); + + $findVisits = $this->mockRepoMethod($expectedRepoMethodName)->willReturn($unlocatedVisits); + + $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { + }); + $flush = $this->em->flush()->will(function (): void { + }); + $clear = $this->em->clear()->will(function (): void { + }); + + $this->visitService->{$serviceMethodName}(new class implements VisitGeolocationHelperInterface { + public function geolocateVisit(Visit $visit): Location + { + return Location::emptyInstance(); + } + + 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)); + } + }); + + $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( + string $serviceMethodName, + string $expectedRepoMethodName, + bool $isNonLocatableAddress + ): void { + $unlocatedVisits = [ + new Visit(new ShortUrl('foo'), Visitor::emptyInstance()), + ]; + + $findVisits = $this->mockRepoMethod($expectedRepoMethodName)->willReturn($unlocatedVisits); + + $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { + }); + $flush = $this->em->flush()->will(function (): void { + }); + $clear = $this->em->clear()->will(function (): void { + }); + + $this->visitService->{$serviceMethodName}( + 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 + { + } + }, + ); + + $findVisits->shouldHaveBeenCalledOnce(); + $persist->shouldHaveBeenCalledTimes($isNonLocatableAddress ? 1 : 0); + $flush->shouldHaveBeenCalledOnce(); + $clear->shouldHaveBeenCalledOnce(); + } + + public function provideIsNonLocatableAddress(): iterable + { + 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([]))); + } +}