Moved visits iteration logic from command to service to allow lazy loading of entries in resultset

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);
+        $this->visitService->locateVisits(function (Visit $visit) use ($output) {
-        $visits = $this->visitService->getUnlocatedVisits();
+            if (! $visit->hasRemoteAddr()) {
-
+                $output->writeln(
-        foreach ($visits as $i => $visit) {
+                    sprintf('<comment>%s</comment>', $this->translator->translate('Ignored visit with no IP address')),
-            $clear = ($i % self::CLEAR_INTERVAL) === 0;
+                    OutputInterface::VERBOSITY_VERBOSE
-            $this->processVisit($output, $visit, $clear);
+                );
-        }
+                throw new IpCannotBeLocatedException('Ignored visit with no IP address');
-
-        $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('<comment>%s</comment>', $this->translator->translate('Ignored visit with no IP address')),
-                OutputInterface::VERBOSITY_VERBOSE
-            );
-            return;
-        }
-
-        $ipAddr = $visit->getRemoteAddr();
-        $output->write(sprintf('%s <fg=blue>%s</>', $this->translator->translate('Processing IP'), $ipAddr));
-        if ($ipAddr === IpAddress::LOCALHOST) {
-            $output->writeln(
-                sprintf(' [<comment>%s</comment>]', $this->translator->translate('Ignored localhost address'))
-            );
-            return;
-        }
-
-        try {
-            $result = $this->ipLocationResolver->resolveIpLocation($ipAddr);
-        } catch (WrongIpException $e) {
-            $output->writeln(
-                sprintf(
-                    ' [<fg=red>%s</>]',
-                    $this->translator->translate('An error occurred while locating IP. Skipped')
-                )
-            );
-            if ($output->isVerbose()) {
-                $this->getApplication()->renderException($e, $output);
             }
 
-            return;
+            $ipAddr = $visit->getRemoteAddr();
-        }
+            $output->write(sprintf('%s <fg=blue>%s</>', $this->translator->translate('Processing IP'), $ipAddr));
+            if ($ipAddr === IpAddress::LOCALHOST) {
+                $output->writeln(
+                    sprintf(' [<comment>%s</comment>]', $this->translator->translate('Ignored localhost address'))
+                );
+                throw new IpCannotBeLocatedException('Ignored localhost address');
+            }
 
-        $location = new VisitLocation($result);
+            try {
-        $this->visitService->locateVisit($visit, $location, $clear);
+                return $this->ipLocationResolver->resolveIpLocation($ipAddr);
-        $output->writeln(sprintf(
+            } catch (WrongIpException $e) {
-            ' [<info>' . $this->translator->translate('Address located at "%s"') . '</info>]',
+                $output->writeln(
-            $location->getCountryName()
+                    sprintf(
-        ));
+                        ' [<fg=red>%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(
+                ' [<info>' . $this->translator->translate('Address located at "%s"') . '</info>]',
+                $location->getCountryName()
+            ));
+        });
+
+        $io = new SymfonyStyle($input, $output);
+        $io->success($this->translator->translate('Finished processing all IPs'));
     }
 }

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 = [
+        $locateVisits = $this->visitService->locateVisits(Argument::cetera())->will(
-            new Visit($shortUrl, new Visitor('', '', '1.2.3.4')),
+            function (array $args) use ($visit, $location) {
-            new Visit($shortUrl, new Visitor('', '', '4.3.2.1')),
+                $firstCallback = array_shift($args);
-            new Visit($shortUrl, new Visitor('', '', '12.34.56.78')),
+                $firstCallback($visit);
-        ];
-        $this->visitService->getUnlocatedVisits()->willReturn($visits)
-                                                 ->shouldBeCalledOnce();
 
-        $this->visitService->locateVisit(Argument::cetera())->shouldBeCalledTimes(count($visits));
+                $secondCallback = array_shift($args);
-        $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([])
+                $secondCallback($location, $visit);
-                                                             ->shouldBeCalledTimes(count($visits));
+            }
+        );
+        $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);
+        $locateVisits->shouldHaveBeenCalledOnce();
-        $this->assertContains('Processing IP 12.34.56.0', $output);
+        $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 = [
+        $locateVisits = $this->visitService->locateVisits(Argument::cetera())->will(
-            new Visit($shortUrl, new Visitor('', '', '1.2.3.4')),
+            function (array $args) use ($visit, $location) {
-            new Visit($shortUrl, new Visitor('', '', '4.3.2.1')),
+                $firstCallback = array_shift($args);
-            new Visit($shortUrl, new Visitor('', '', '12.34.56.78')),
+                $firstCallback($visit);
-            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();
 
-        $this->visitService->locateVisit(Argument::cetera())->shouldBeCalledTimes(count($visits) - 4);
+                $secondCallback = array_shift($args);
-        $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([])
+                $secondCallback($location, $visit);
-                                                             ->shouldBeCalledTimes(count($visits) - 4);
+            }
+        );
+        $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willThrow(WrongIpException::class);
 
-        $this->commandTester->execute([
+        try {
-            'command' => 'visit:process',
+            $this->commandTester->execute([
-        ], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]);
+                'command' => 'visit:process',
-        $output = $this->commandTester->getDisplay();
+            ], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]);
-        $this->assertContains('Ignored localhost address', $output);
+        } catch (Throwable $e) {
-        $this->assertContains('Ignored visit with no IP address', $output);
+            $output = $this->commandTester->getDisplay();
+
+            $this->assertInstanceOf(IpCannotBeLocatedException::class, $e);
+
+            $this->assertContains('An error occurred while locating IP. Skipped', $output);
+            $locateVisits->shouldHaveBeenCalledOnce();
+            $resolveIpLocation->shouldHaveBeenCalledOnce();
+        }
     }
 }

module/Core/src/Exception/IpCannotBeLocatedException.php

@@ -0,0 +1,8 @@
+<?php
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Exception;
+
+class IpCannotBeLocatedException extends RuntimeException
+{
+}

module/Core/src/Repository/VisitRepository.php

@@ -10,15 +10,12 @@ use Shlinkio\Shlink\Core\Entity\Visit;
 
 class VisitRepository extends EntityRepository implements VisitRepositoryInterface
 {
-    /**
+    public function findUnlocatedVisits(): iterable
-     * @return Visit[]
-     */
-    public function findUnlocatedVisits(): array
     {
         $qb = $this->createQueryBuilder('v');
         $qb->where($qb->expr()->isNull('v.visitLocation'));
 
-        return $qb->getQuery()->getResult();
+        return $qb->getQuery()->iterate();
     }
 
     /**

module/Core/src/Repository/VisitRepositoryInterface.php

@@ -10,10 +10,7 @@ use Shlinkio\Shlink\Core\Entity\Visit;
 
 interface VisitRepositoryInterface extends ObjectRepository
 {
-    /**
+    public function findUnlocatedVisits(): iterable;
-     * @return Visit[]
-     */
-    public function findUnlocatedVisits(): array;
 
     /**
      * @param ShortUrl|int $shortUrl

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;
     }
 
-    /**
+    public function locateVisits(callable $getGeolocationData, ?callable $locatedVisit = null): void
-     * @return Visit[]
-     */
-    public function getUnlocatedVisits(): array
     {
         /** @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) {
+        if ($locatedVisit !== null) {
-            $this->em->clear(VisitLocation::class);
+            $locatedVisit($location, $visit);
-            $this->em->clear(Visit::class);
         }
+
+        $this->em->clear();
     }
 }

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
 {
-    /**
+    public function locateVisits(callable $getGeolocationData, ?callable $locatedVisit = null): void;
-     * @return Visit[]
-     */
-    public function getUnlocatedVisits(): array;
-
-    public function locateVisit(Visit $visit, VisitLocation $location, bool $clear = false): void;
 }

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);
     }
 
     /**

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());
+        $unlocatedVisits = [
-        $this->em->persist($visit)->shouldBeCalledOnce();
+            [new Visit(new ShortUrl('foo'), Visitor::emptyInstance())],
-        $this->em->flush()->shouldBeCalledOnce();
+            [new Visit(new ShortUrl('bar'), Visitor::emptyInstance())],
-        $this->visitService->locateVisit($visit, new VisitLocation([]));
+        ];
+
+        $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();
+        $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits);
-        $this->em->getRepository(Visit::class)->willReturn($repo->reveal())->shouldBeCalledOnce();
+        $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal());
-        $this->visitService->getUnlocatedVisits();
+
+        $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();
     }
 }