From c1906606c63516cacab5d09e502096312c849000 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Nov 2018 07:47:42 +0100 Subject: [PATCH 1/5] Updated VisitService to have a method which locates visits and allows entity manager to be cleared --- .../Command/Visit/ProcessVisitsCommand.php | 84 ++++++++++--------- .../Command/ShortUrl/GetVisitsCommandTest.php | 2 +- .../Visit/ProcessVisitsCommandTest.php | 4 +- module/Core/src/Entity/Visit.php | 2 +- module/Core/src/Service/VisitService.php | 15 ++-- .../src/Service/VisitServiceInterface.php | 8 +- .../Repository/VisitRepositoryTest.php | 2 +- module/Core/test/Service/VisitServiceTest.php | 5 +- 8 files changed, 66 insertions(+), 56 deletions(-) diff --git a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php index a5a33fcb..117ad80f 100644 --- a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php +++ b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\CLI\Command\Visit; use Shlinkio\Shlink\Common\Exception\WrongIpException; 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\Service\VisitServiceInterface; use Symfony\Component\Console\Command\Command; @@ -57,47 +58,52 @@ class ProcessVisitsCommand extends Command $visits = $this->visitService->getUnlocatedVisits(); foreach ($visits as $visit) { - if (! $visit->hasRemoteAddr()) { - $io->writeln( - sprintf('%s', $this->translator->translate('Ignored visit with no IP address')), - OutputInterface::VERBOSITY_VERBOSE - ); - continue; - } - - $ipAddr = $visit->getRemoteAddr(); - $io->write(sprintf('%s %s', $this->translator->translate('Processing IP'), $ipAddr)); - if ($ipAddr === IpAddress::LOCALHOST) { - $io->writeln( - sprintf(' [%s]', $this->translator->translate('Ignored localhost address')) - ); - continue; - } - - try { - $result = $this->ipLocationResolver->resolveIpLocation($ipAddr); - - $location = new VisitLocation($result); - $visit->setVisitLocation($location); - $this->visitService->saveVisit($visit); - - $io->writeln(sprintf( - ' [' . $this->translator->translate('Address located at "%s"') . ']', - $location->getCountryName() - )); - } catch (WrongIpException $e) { - $io->writeln( - sprintf( - ' [%s]', - $this->translator->translate('An error occurred while locating IP. Skipped') - ) - ); - if ($io->isVerbose()) { - $this->getApplication()->renderException($e, $output); - } - } + $this->processVisit($io, $visit, false); } $io->success($this->translator->translate('Finished processing all IPs')); } + + private function processVisit(SymfonyStyle $io, Visit $visit, bool $clear): void + { + if (! $visit->hasRemoteAddr()) { + $io->writeln( + sprintf('%s', $this->translator->translate('Ignored visit with no IP address')), + OutputInterface::VERBOSITY_VERBOSE + ); + return; + } + + $ipAddr = $visit->getRemoteAddr(); + $io->write(sprintf('%s %s', $this->translator->translate('Processing IP'), $ipAddr)); + if ($ipAddr === IpAddress::LOCALHOST) { + $io->writeln( + sprintf(' [%s]', $this->translator->translate('Ignored localhost address')) + ); + return; + } + + try { + $result = $this->ipLocationResolver->resolveIpLocation($ipAddr); + } catch (WrongIpException $e) { + $io->writeln( + sprintf( + ' [%s]', + $this->translator->translate('An error occurred while locating IP. Skipped') + ) + ); + if ($io->isVerbose()) { + $this->getApplication()->renderException($e, $output); + } + + return; + } + + $location = new VisitLocation($result); + $this->visitService->locateVisit($visit, $location, $clear); + $io->writeln(sprintf( + ' [' . $this->translator->translate('Address located at "%s"') . ']', + $location->getCountryName() + )); + } } diff --git a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php index f7523d78..d36c798a 100644 --- a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php @@ -81,7 +81,7 @@ class GetVisitsCommandTest extends TestCase { $shortCode = 'abc123'; $this->visitsTracker->info($shortCode, Argument::any())->willReturn([ - (new Visit(new ShortUrl(''), new Visitor('bar', 'foo', '')))->setVisitLocation( + (new Visit(new ShortUrl(''), new Visitor('bar', 'foo', '')))->locate( new VisitLocation(['country_name' => 'Spain']) ), ])->shouldBeCalledOnce(); diff --git a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php index 5a28e5ab..a0266d4e 100644 --- a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php @@ -64,7 +64,7 @@ class ProcessVisitsCommandTest extends TestCase $this->visitService->getUnlocatedVisits()->willReturn($visits) ->shouldBeCalledOnce(); - $this->visitService->saveVisit(Argument::any())->shouldBeCalledTimes(count($visits)); + $this->visitService->locateVisit(Argument::cetera())->shouldBeCalledTimes(count($visits)); $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]) ->shouldBeCalledTimes(count($visits)); @@ -96,7 +96,7 @@ class ProcessVisitsCommandTest extends TestCase $this->visitService->getUnlocatedVisits()->willReturn($visits) ->shouldBeCalledOnce(); - $this->visitService->saveVisit(Argument::any())->shouldBeCalledTimes(count($visits) - 4); + $this->visitService->locateVisit(Argument::cetera())->shouldBeCalledTimes(count($visits) - 4); $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]) ->shouldBeCalledTimes(count($visits) - 4); diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index 54926e9e..3fd98db0 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -93,7 +93,7 @@ class Visit extends AbstractEntity implements JsonSerializable return $this->visitLocation; } - public function setVisitLocation(VisitLocation $visitLocation): self + public function locate(VisitLocation $visitLocation): self { $this->visitLocation = $visitLocation; return $this; diff --git a/module/Core/src/Service/VisitService.php b/module/Core/src/Service/VisitService.php index ab18ce1c..9aea80cb 100644 --- a/module/Core/src/Service/VisitService.php +++ b/module/Core/src/Service/VisitService.php @@ -5,6 +5,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\Repository\VisitRepository; class VisitService implements VisitServiceInterface @@ -22,19 +23,23 @@ class VisitService implements VisitServiceInterface /** * @return Visit[] */ - public function getUnlocatedVisits() + public function getUnlocatedVisits(): array { /** @var VisitRepository $repo */ $repo = $this->em->getRepository(Visit::class); return $repo->findUnlocatedVisits(); } - /** - * @param Visit $visit - */ - public function saveVisit(Visit $visit) + public function locateVisit(Visit $visit, VisitLocation $location, bool $clear = false): void { + $visit->locate($location); + $this->em->persist($visit); $this->em->flush(); + + if ($clear) { + $this->em->clear(VisitLocation::class); + $this->em->clear(Visit::class); + } } } diff --git a/module/Core/src/Service/VisitServiceInterface.php b/module/Core/src/Service/VisitServiceInterface.php index 46f6ceb7..2b5d5811 100644 --- a/module/Core/src/Service/VisitServiceInterface.php +++ b/module/Core/src/Service/VisitServiceInterface.php @@ -4,16 +4,14 @@ 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(); + public function getUnlocatedVisits(): array; - /** - * @param Visit $visit - */ - public function saveVisit(Visit $visit); + public function locateVisit(Visit $visit, VisitLocation $location, bool $clear = false): void; } diff --git a/module/Core/test-func/Repository/VisitRepositoryTest.php b/module/Core/test-func/Repository/VisitRepositoryTest.php index 886cce70..683009cd 100644 --- a/module/Core/test-func/Repository/VisitRepositoryTest.php +++ b/module/Core/test-func/Repository/VisitRepositoryTest.php @@ -45,7 +45,7 @@ class VisitRepositoryTest extends DatabaseTestCase if ($i % 2 === 0) { $location = new VisitLocation([]); $this->getEntityManager()->persist($location); - $visit->setVisitLocation($location); + $visit->locate($location); } $this->getEntityManager()->persist($visit); diff --git a/module/Core/test/Service/VisitServiceTest.php b/module/Core/test/Service/VisitServiceTest.php index 728d18ee..cc8193be 100644 --- a/module/Core/test/Service/VisitServiceTest.php +++ b/module/Core/test/Service/VisitServiceTest.php @@ -8,6 +8,7 @@ use PHPUnit\Framework\TestCase; 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\Model\Visitor; use Shlinkio\Shlink\Core\Repository\VisitRepository; use Shlinkio\Shlink\Core\Service\VisitService; @@ -32,12 +33,12 @@ class VisitServiceTest extends TestCase /** * @test */ - public function saveVisitsPersistsProvidedVisit() + public function locateVisitPersistsProvidedVisit() { $visit = new Visit(new ShortUrl(''), Visitor::emptyInstance()); $this->em->persist($visit)->shouldBeCalledOnce(); $this->em->flush()->shouldBeCalledOnce(); - $this->visitService->saveVisit($visit); + $this->visitService->locateVisit($visit, new VisitLocation([])); } /** From 1bc01057f314d064874fc29cc0d2127151dac1d3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Nov 2018 08:02:42 +0100 Subject: [PATCH 2/5] Reduced the number of arguments in private method --- .../Command/Visit/ProcessVisitsCommand.php | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php index 117ad80f..d7489329 100644 --- a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php +++ b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php @@ -19,6 +19,7 @@ use function sprintf; class ProcessVisitsCommand extends Command { public const NAME = 'visit:process'; + private const CLEAR_INTERVAL = 100; /** * @var VisitServiceInterface @@ -57,17 +58,18 @@ class ProcessVisitsCommand extends Command $io = new SymfonyStyle($input, $output); $visits = $this->visitService->getUnlocatedVisits(); - foreach ($visits as $visit) { - $this->processVisit($io, $visit, false); + 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(SymfonyStyle $io, Visit $visit, bool $clear): void + private function processVisit(OutputInterface $output, Visit $visit, bool $clear): void { if (! $visit->hasRemoteAddr()) { - $io->writeln( + $output->writeln( sprintf('%s', $this->translator->translate('Ignored visit with no IP address')), OutputInterface::VERBOSITY_VERBOSE ); @@ -75,9 +77,9 @@ class ProcessVisitsCommand extends Command } $ipAddr = $visit->getRemoteAddr(); - $io->write(sprintf('%s %s', $this->translator->translate('Processing IP'), $ipAddr)); + $output->write(sprintf('%s %s', $this->translator->translate('Processing IP'), $ipAddr)); if ($ipAddr === IpAddress::LOCALHOST) { - $io->writeln( + $output->writeln( sprintf(' [%s]', $this->translator->translate('Ignored localhost address')) ); return; @@ -86,13 +88,13 @@ class ProcessVisitsCommand extends Command try { $result = $this->ipLocationResolver->resolveIpLocation($ipAddr); } catch (WrongIpException $e) { - $io->writeln( + $output->writeln( sprintf( ' [%s]', $this->translator->translate('An error occurred while locating IP. Skipped') ) ); - if ($io->isVerbose()) { + if ($output->isVerbose()) { $this->getApplication()->renderException($e, $output); } @@ -101,7 +103,7 @@ class ProcessVisitsCommand extends Command $location = new VisitLocation($result); $this->visitService->locateVisit($visit, $location, $clear); - $io->writeln(sprintf( + $output->writeln(sprintf( ' [' . $this->translator->translate('Address located at "%s"') . ']', $location->getCountryName() )); From 0aae0d888cb211b28be4e778cf0d01c944622754 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Nov 2018 09:42:15 +0100 Subject: [PATCH 3/5] Moved visits iteration logic from command to service to allow lazy loading of entries in resultset --- .../Command/Visit/ProcessVisitsCommand.php | 90 ++++++------- .../Visit/ProcessVisitsCommandTest.php | 127 +++++++++++++----- .../Exception/IpCannotBeLocatedException.php | 8 ++ .../Core/src/Repository/VisitRepository.php | 7 +- .../Repository/VisitRepositoryInterface.php | 5 +- module/Core/src/Service/VisitService.php | 29 ++-- .../src/Service/VisitServiceInterface.php | 10 +- .../Repository/VisitRepositoryTest.php | 8 +- module/Core/test/Service/VisitServiceTest.php | 69 ++++++++-- 9 files changed, 229 insertions(+), 124 deletions(-) create mode 100644 module/Core/src/Exception/IpCannotBeLocatedException.php 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(); } } From 4760406221c993403a762449f28fbcc6d55dfa8b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Nov 2018 09:47:14 +0100 Subject: [PATCH 4/5] Updated changelog --- CHANGELOG.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37a749f4..355d89b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,29 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org). +## [Unreleased] + +#### Added + +* *Nothing* + +#### Changed + +* *Nothing* + +#### Deprecated + +* *Nothing* + +#### Removed + +* *Nothing* + +#### Fixed + +* [#271](https://github.com/shlinkio/shlink/issues/271) Fixed memory leak produced when processing high amounts of visits at the same time. + + ## 1.14.0 - 2018-11-16 #### Added From d44bc4b182e1c80442c5de83470d4278dc65339d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Nov 2018 09:49:44 +0100 Subject: [PATCH 5/5] Added small hint in README --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 46bbb6ca..b659c987 100644 --- a/README.md +++ b/README.md @@ -114,6 +114,8 @@ Those tasks can be performed using shlink's CLI, so it should be easy to schedul Running this will improve the performance of the `doma.in/abc123/preview` URLs, which return a preview of the site. +*Any of those commands accept the `-q` flag, which makes it not display any output. This is recommended when configuring the commands as cron jobs.* + ## Update to new version When a new Shlink version is available, you don't need to repeat the entire process yourself. Instead, follow these steps: