diff --git a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php index d7489329..38dff527 100644 --- a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php +++ b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php @@ -8,6 +8,7 @@ use Shlinkio\Shlink\Common\IpGeolocation\IpLocationResolverInterface; 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 Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; @@ -19,7 +20,6 @@ use function sprintf; class ProcessVisitsCommand extends Command { public const NAME = 'visit:process'; - private const CLEAR_INTERVAL = 100; /** * @var VisitServiceInterface @@ -55,57 +55,47 @@ class ProcessVisitsCommand extends Command protected function execute(InputInterface $input, OutputInterface $output): void { - $io = new SymfonyStyle($input, $output); - $visits = $this->visitService->getUnlocatedVisits(); - - foreach ($visits as $i => $visit) { - $clear = ($i % self::CLEAR_INTERVAL) === 0; - $this->processVisit($output, $visit, $clear); - } - - $io->success($this->translator->translate('Finished processing all IPs')); - } - - private function processVisit(OutputInterface $output, Visit $visit, bool $clear): void - { - if (! $visit->hasRemoteAddr()) { - $output->writeln( - sprintf('%s', $this->translator->translate('Ignored visit with no IP address')), - OutputInterface::VERBOSITY_VERBOSE - ); - return; - } - - $ipAddr = $visit->getRemoteAddr(); - $output->write(sprintf('%s %s', $this->translator->translate('Processing IP'), $ipAddr)); - if ($ipAddr === IpAddress::LOCALHOST) { - $output->writeln( - sprintf(' [%s]', $this->translator->translate('Ignored localhost address')) - ); - return; - } - - try { - $result = $this->ipLocationResolver->resolveIpLocation($ipAddr); - } catch (WrongIpException $e) { - $output->writeln( - sprintf( - ' [%s]', - $this->translator->translate('An error occurred while locating IP. Skipped') - ) - ); - if ($output->isVerbose()) { - $this->getApplication()->renderException($e, $output); + $this->visitService->locateVisits(function (Visit $visit) use ($output) { + if (! $visit->hasRemoteAddr()) { + $output->writeln( + sprintf('%s', $this->translator->translate('Ignored visit with no IP address')), + OutputInterface::VERBOSITY_VERBOSE + ); + throw new IpCannotBeLocatedException('Ignored visit with no IP address'); } - return; - } + $ipAddr = $visit->getRemoteAddr(); + $output->write(sprintf('%s %s', $this->translator->translate('Processing IP'), $ipAddr)); + if ($ipAddr === IpAddress::LOCALHOST) { + $output->writeln( + sprintf(' [%s]', $this->translator->translate('Ignored localhost address')) + ); + throw new IpCannotBeLocatedException('Ignored localhost address'); + } - $location = new VisitLocation($result); - $this->visitService->locateVisit($visit, $location, $clear); - $output->writeln(sprintf( - ' [' . $this->translator->translate('Address located at "%s"') . ']', - $location->getCountryName() - )); + try { + return $this->ipLocationResolver->resolveIpLocation($ipAddr); + } catch (WrongIpException $e) { + $output->writeln( + sprintf( + ' [%s]', + $this->translator->translate('An error occurred while locating IP. Skipped') + ) + ); + if ($output->isVerbose()) { + $this->getApplication()->renderException($e, $output); + } + + throw new IpCannotBeLocatedException('An error occurred while locating IP', 0, $e); + } + }, function (VisitLocation $location) use ($output) { + $output->writeln(sprintf( + ' [' . $this->translator->translate('Address located at "%s"') . ']', + $location->getCountryName() + )); + }); + + $io = new SymfonyStyle($input, $output); + $io->success($this->translator->translate('Finished processing all IPs')); } } diff --git a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php index a0266d4e..efa77752 100644 --- a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php @@ -7,16 +7,21 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\Visit\ProcessVisitsCommand; +use Shlinkio\Shlink\Common\Exception\WrongIpException; use Shlinkio\Shlink\Common\IpGeolocation\IpApiLocationResolver; +use Shlinkio\Shlink\Common\Util\IpAddress; use Shlinkio\Shlink\Core\Entity\ShortUrl; 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\Service\VisitService; use Symfony\Component\Console\Application; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Tester\CommandTester; +use Throwable; use Zend\I18n\Translator\Translator; -use function count; +use function array_shift; class ProcessVisitsCommandTest extends TestCase { @@ -52,59 +57,107 @@ class ProcessVisitsCommandTest extends TestCase /** * @test */ - public function allReturnedVisitsIpsAreProcessed() + public function allPendingVisitsAreProcessed() { - $shortUrl = new ShortUrl(''); + $visit = new Visit(new ShortUrl(''), new Visitor('', '', '1.2.3.4')); + $location = new VisitLocation([]); - $visits = [ - new Visit($shortUrl, new Visitor('', '', '1.2.3.4')), - new Visit($shortUrl, new Visitor('', '', '4.3.2.1')), - new Visit($shortUrl, new Visitor('', '', '12.34.56.78')), - ]; - $this->visitService->getUnlocatedVisits()->willReturn($visits) - ->shouldBeCalledOnce(); + $locateVisits = $this->visitService->locateVisits(Argument::cetera())->will( + function (array $args) use ($visit, $location) { + $firstCallback = array_shift($args); + $firstCallback($visit); - $this->visitService->locateVisit(Argument::cetera())->shouldBeCalledTimes(count($visits)); - $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]) - ->shouldBeCalledTimes(count($visits)); + $secondCallback = array_shift($args); + $secondCallback($location, $visit); + } + ); + $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]); $this->commandTester->execute([ 'command' => 'visit:process', ]); $output = $this->commandTester->getDisplay(); + $this->assertContains('Processing IP 1.2.3.0', $output); - $this->assertContains('Processing IP 4.3.2.0', $output); - $this->assertContains('Processing IP 12.34.56.0', $output); + $locateVisits->shouldHaveBeenCalledOnce(); + $resolveIpLocation->shouldHaveBeenCalledOnce(); + } + + /** + * @test + * @dataProvider provideIgnoredAddresses + */ + public function localhostAndEmptyAddressesAreIgnored(?string $address, string $message) + { + $visit = new Visit(new ShortUrl(''), new Visitor('', '', $address)); + $location = new VisitLocation([]); + + $locateVisits = $this->visitService->locateVisits(Argument::cetera())->will( + function (array $args) use ($visit, $location) { + $firstCallback = array_shift($args); + $firstCallback($visit); + + $secondCallback = array_shift($args); + $secondCallback($location, $visit); + } + ); + $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]); + + try { + $this->commandTester->execute([ + 'command' => 'visit:process', + ], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); + } catch (Throwable $e) { + $output = $this->commandTester->getDisplay(); + + $this->assertInstanceOf(IpCannotBeLocatedException::class, $e); + + $this->assertContains($message, $output); + $locateVisits->shouldHaveBeenCalledOnce(); + $resolveIpLocation->shouldNotHaveBeenCalled(); + } + } + + public function provideIgnoredAddresses(): array + { + return [ + ['', 'Ignored visit with no IP address'], + [null, 'Ignored visit with no IP address'], + [IpAddress::LOCALHOST, 'Ignored localhost address'], + ]; } /** * @test */ - public function localhostAndEmptyAddressIsIgnored() + public function errorWhileLocatingIpIsDisplayed() { - $shortUrl = new ShortUrl(''); + $visit = new Visit(new ShortUrl(''), new Visitor('', '', '1.2.3.4')); + $location = new VisitLocation([]); - $visits = [ - new Visit($shortUrl, new Visitor('', '', '1.2.3.4')), - new Visit($shortUrl, new Visitor('', '', '4.3.2.1')), - new Visit($shortUrl, new Visitor('', '', '12.34.56.78')), - new Visit($shortUrl, new Visitor('', '', '127.0.0.1')), - new Visit($shortUrl, new Visitor('', '', '127.0.0.1')), - new Visit($shortUrl, new Visitor('', '', '')), - new Visit($shortUrl, new Visitor('', '', null)), - ]; - $this->visitService->getUnlocatedVisits()->willReturn($visits) - ->shouldBeCalledOnce(); + $locateVisits = $this->visitService->locateVisits(Argument::cetera())->will( + function (array $args) use ($visit, $location) { + $firstCallback = array_shift($args); + $firstCallback($visit); - $this->visitService->locateVisit(Argument::cetera())->shouldBeCalledTimes(count($visits) - 4); - $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]) - ->shouldBeCalledTimes(count($visits) - 4); + $secondCallback = array_shift($args); + $secondCallback($location, $visit); + } + ); + $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willThrow(WrongIpException::class); - $this->commandTester->execute([ - 'command' => 'visit:process', - ], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); - $output = $this->commandTester->getDisplay(); - $this->assertContains('Ignored localhost address', $output); - $this->assertContains('Ignored visit with no IP address', $output); + try { + $this->commandTester->execute([ + 'command' => 'visit:process', + ], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); + } catch (Throwable $e) { + $output = $this->commandTester->getDisplay(); + + $this->assertInstanceOf(IpCannotBeLocatedException::class, $e); + + $this->assertContains('An error occurred while locating IP. Skipped', $output); + $locateVisits->shouldHaveBeenCalledOnce(); + $resolveIpLocation->shouldHaveBeenCalledOnce(); + } } } diff --git a/module/Core/src/Exception/IpCannotBeLocatedException.php b/module/Core/src/Exception/IpCannotBeLocatedException.php new file mode 100644 index 00000000..e172e9c9 --- /dev/null +++ b/module/Core/src/Exception/IpCannotBeLocatedException.php @@ -0,0 +1,8 @@ +createQueryBuilder('v'); $qb->where($qb->expr()->isNull('v.visitLocation')); - return $qb->getQuery()->getResult(); + return $qb->getQuery()->iterate(); } /** diff --git a/module/Core/src/Repository/VisitRepositoryInterface.php b/module/Core/src/Repository/VisitRepositoryInterface.php index 4ee3ebde..72df2ed1 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -10,10 +10,7 @@ use Shlinkio\Shlink\Core\Entity\Visit; interface VisitRepositoryInterface extends ObjectRepository { - /** - * @return Visit[] - */ - public function findUnlocatedVisits(): array; + public function findUnlocatedVisits(): iterable; /** * @param ShortUrl|int $shortUrl diff --git a/module/Core/src/Service/VisitService.php b/module/Core/src/Service/VisitService.php index 9aea80cb..dea78f0f 100644 --- a/module/Core/src/Service/VisitService.php +++ b/module/Core/src/Service/VisitService.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Service; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; +use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException; use Shlinkio\Shlink\Core\Repository\VisitRepository; class VisitService implements VisitServiceInterface @@ -20,26 +21,36 @@ class VisitService implements VisitServiceInterface $this->em = $em; } - /** - * @return Visit[] - */ - public function getUnlocatedVisits(): array + public function locateVisits(callable $getGeolocationData, ?callable $locatedVisit = null): void { /** @var VisitRepository $repo */ $repo = $this->em->getRepository(Visit::class); - return $repo->findUnlocatedVisits(); + $results = $repo->findUnlocatedVisits(); + + foreach ($results as [$visit]) { + try { + $locationData = $getGeolocationData($visit); + } catch (IpCannotBeLocatedException $e) { + // Skip if the visit's IP could not be located + continue; + } + + $location = new VisitLocation($locationData); + $this->locateVisit($visit, $location, $locatedVisit); + } } - public function locateVisit(Visit $visit, VisitLocation $location, bool $clear = false): void + private function locateVisit(Visit $visit, VisitLocation $location, ?callable $locatedVisit): void { $visit->locate($location); $this->em->persist($visit); $this->em->flush(); - if ($clear) { - $this->em->clear(VisitLocation::class); - $this->em->clear(Visit::class); + if ($locatedVisit !== null) { + $locatedVisit($location, $visit); } + + $this->em->clear(); } } diff --git a/module/Core/src/Service/VisitServiceInterface.php b/module/Core/src/Service/VisitServiceInterface.php index 2b5d5811..907729fa 100644 --- a/module/Core/src/Service/VisitServiceInterface.php +++ b/module/Core/src/Service/VisitServiceInterface.php @@ -3,15 +3,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; -use Shlinkio\Shlink\Core\Entity\Visit; -use Shlinkio\Shlink\Core\Entity\VisitLocation; - interface VisitServiceInterface { - /** - * @return Visit[] - */ - public function getUnlocatedVisits(): array; - - public function locateVisit(Visit $visit, VisitLocation $location, bool $clear = false): void; + public function locateVisits(callable $getGeolocationData, ?callable $locatedVisit = null): void; } diff --git a/module/Core/test-func/Repository/VisitRepositoryTest.php b/module/Core/test-func/Repository/VisitRepositoryTest.php index 683009cd..1ef3e7d3 100644 --- a/module/Core/test-func/Repository/VisitRepositoryTest.php +++ b/module/Core/test-func/Repository/VisitRepositoryTest.php @@ -52,7 +52,13 @@ class VisitRepositoryTest extends DatabaseTestCase } $this->getEntityManager()->flush(); - $this->assertCount(3, $this->repo->findUnlocatedVisits()); + $resultsCount = 0; + $results = $this->repo->findUnlocatedVisits(); + foreach ($results as $value) { + $resultsCount++; + } + + $this->assertEquals(3, $resultsCount); } /** diff --git a/module/Core/test/Service/VisitServiceTest.php b/module/Core/test/Service/VisitServiceTest.php index cc8193be..04a90f84 100644 --- a/module/Core/test/Service/VisitServiceTest.php +++ b/module/Core/test/Service/VisitServiceTest.php @@ -5,13 +5,18 @@ namespace ShlinkioTest\Shlink\Core\Service; use Doctrine\ORM\EntityManager; use PHPUnit\Framework\TestCase; +use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; 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\Service\VisitService; +use function array_shift; +use function count; +use function func_get_args; class VisitServiceTest extends TestCase { @@ -33,22 +38,68 @@ class VisitServiceTest extends TestCase /** * @test */ - public function locateVisitPersistsProvidedVisit() + public function locateVisitsIteratesAndLocatesUnlocatedVisits() { - $visit = new Visit(new ShortUrl(''), Visitor::emptyInstance()); - $this->em->persist($visit)->shouldBeCalledOnce(); - $this->em->flush()->shouldBeCalledOnce(); - $this->visitService->locateVisit($visit, new VisitLocation([])); + $unlocatedVisits = [ + [new Visit(new ShortUrl('foo'), Visitor::emptyInstance())], + [new Visit(new ShortUrl('bar'), Visitor::emptyInstance())], + ]; + + $repo = $this->prophesize(VisitRepository::class); + $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits); + $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); + + $persist = $this->em->persist(Argument::type(Visit::class))->will(function () { + }); + $flush = $this->em->flush()->will(function () { + }); + $clear = $this->em->clear()->will(function () { + }); + + $this->visitService->locateVisits(function () { + return []; + }, function () { + $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(count($unlocatedVisits)); + $clear->shouldHaveBeenCalledTimes(count($unlocatedVisits)); } /** * @test */ - public function getUnlocatedVisitsFallbacksToRepository() + public function visitsWhichCannotBeLocatedAreIgnored() { + $unlocatedVisits = [ + [new Visit(new ShortUrl('foo'), Visitor::emptyInstance())], + ]; + $repo = $this->prophesize(VisitRepository::class); - $repo->findUnlocatedVisits()->shouldBeCalledOnce(); - $this->em->getRepository(Visit::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); - $this->visitService->getUnlocatedVisits(); + $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits); + $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); + + $persist = $this->em->persist(Argument::type(Visit::class))->will(function () { + }); + $flush = $this->em->flush()->will(function () { + }); + $clear = $this->em->clear()->will(function () { + }); + + $this->visitService->locateVisits(function () { + throw new IpCannotBeLocatedException('Cannot be located'); + }); + + $findUnlocatedVisits->shouldHaveBeenCalledOnce(); + $getRepo->shouldHaveBeenCalledOnce(); + $persist->shouldNotHaveBeenCalled(); + $flush->shouldNotHaveBeenCalled(); + $clear->shouldNotHaveBeenCalled(); } }