Created new method to locate empty visits

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",

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],
     ],
 

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,

module/CLI/src/Command/Visit/LocateVisitsCommand.php

@@ -4,7 +4,6 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\CLI\Command\Visit;
 
-use Exception;
 use Shlinkio\Shlink\CLI\Command\Util\AbstractLockedCommand;
 use Shlinkio\Shlink\CLI\Command\Util\LockedCommandConfig;
 use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException;
@@ -14,12 +13,13 @@ use Shlinkio\Shlink\Common\Util\IpAddress;
 use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Entity\VisitLocation;
 use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
-use Shlinkio\Shlink\Core\Service\VisitServiceInterface;
+use Shlinkio\Shlink\Core\Visit\VisitLocatorInterface;
 use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
 use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface;
 use Symfony\Component\Console\Helper\ProgressBar;
 use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Input\InputOption;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Style\SymfonyStyle;
 use Symfony\Component\Lock\LockFactory;
@@ -31,7 +31,7 @@ class LocateVisitsCommand extends AbstractLockedCommand
 {
     public const NAME = 'visit:locate';
 
-    private VisitServiceInterface $visitService;
+    private VisitLocatorInterface $visitLocator;
     private IpLocationResolverInterface $ipLocationResolver;
     private GeolocationDbUpdaterInterface $dbUpdater;
 
@@ -39,13 +39,13 @@ class LocateVisitsCommand extends AbstractLockedCommand
     private ?ProgressBar $progressBar = null;
 
     public function __construct(
-        VisitServiceInterface $visitService,
+        VisitLocatorInterface $visitLocator,
         IpLocationResolverInterface $ipLocationResolver,
         LockFactory $locker,
         GeolocationDbUpdaterInterface $dbUpdater
     ) {
         parent::__construct($locker);
-        $this->visitService = $visitService;
+        $this->visitLocator = $visitLocator;
         $this->ipLocationResolver = $ipLocationResolver;
         $this->dbUpdater = $dbUpdater;
     }
@@ -54,32 +54,46 @@ class LocateVisitsCommand extends AbstractLockedCommand
     {
         $this
             ->setName(self::NAME)
-            ->setDescription('Resolves visits origin locations.');
+            ->setDescription('Resolves visits origin locations.')
+            ->addOption(
+                'retry',
+                'r',
+                InputOption::VALUE_NONE,
+                'Will retry visits that were located with an empty location, in case it was a temporal issue.',
+            )
+            ->addOption(
+                'all',
+                'a',
+                InputOption::VALUE_NONE,
+                'Will locate all visits, ignoring if they have already been located.',
+            );
     }
 
     protected function lockedExecute(InputInterface $input, OutputInterface $output): int
     {
         $this->io = new SymfonyStyle($input, $output);
+        $retry = $input->getOption('retry');
+        $geolocateVisit = [$this, 'getGeolocationDataForVisit'];
+        $notifyVisitWithLocation = static function (VisitLocation $location) use ($output): void {
+            $message = ! $location->isEmpty()
+                ? sprintf(' [<info>Address located in "%s"</info>]', $location->getCountryName())
+                : ' [<comment>Address not found</comment>]';
+            $output->writeln($message);
+        };
 
         try {
             $this->checkDbUpdate();
 
-            $this->visitService->locateUnlocatedVisits(
+            $this->visitLocator->locateUnlocatedVisits($geolocateVisit, $notifyVisitWithLocation);
-                [$this, 'getGeolocationDataForVisit'],
+            if ($retry) {
-                static function (VisitLocation $location) use ($output): void {
+                $this->visitLocator->locateVisitsWithEmptyLocation($geolocateVisit, $notifyVisitWithLocation);
-                    if (!$location->isEmpty()) {
+            }
-                        $output->writeln(
-                            sprintf(' [<info>Address located at "%s"</info>]', $location->getCountryName()),
-                        );
-                    }
-                },
-            );
 
             $this->io->success('Finished processing all IPs');
             return ExitCodes::EXIT_SUCCESS;
         } catch (Throwable $e) {
             $this->io->error($e->getMessage());
-            if ($e instanceof Exception && $this->io->isVerbose()) {
+            if ($e instanceof Throwable && $this->io->isVerbose()) {
                 $this->getApplication()->renderThrowable($e, $this->io);
             }
 

module/CLI/test/Command/Visit/LocateVisitsCommandTest.php

@@ -15,7 +15,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Entity\VisitLocation;
 use Shlinkio\Shlink\Core\Model\Visitor;
-use Shlinkio\Shlink\Core\Service\VisitService;
+use Shlinkio\Shlink\Core\Visit\VisitLocator;
 use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
 use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface;
@@ -38,7 +38,7 @@ class LocateVisitsCommandTest extends TestCase
 
     public function setUp(): void
     {
-        $this->visitService = $this->prophesize(VisitService::class);
+        $this->visitService = $this->prophesize(VisitLocator::class);
         $this->ipResolver = $this->prophesize(IpLocationResolverInterface::class);
         $this->dbUpdater = $this->prophesize(GeolocationDbUpdaterInterface::class);
 

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',

module/Core/src/Repository/VisitRepository.php

@@ -14,7 +14,8 @@ class VisitRepository extends EntityRepository implements VisitRepositoryInterfa
     /**
      * This method will allow you to iterate the whole list of unlocated visits, but loading them into memory in
      * smaller blocks of a specific size.
-     * This will have side effects if you update those rows while you iterate them.
+     * This will have side effects if you update those rows while you iterate them, in a way that they are no longer
+     * unlocated.
      * If you plan to do so, pass the first argument as false in order to disable applying offsets while slicing the
      * dataset
      *
@@ -23,8 +24,8 @@ class VisitRepository extends EntityRepository implements VisitRepositoryInterfa
     public function findUnlocatedVisits(bool $applyOffset = true, int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable
     {
         $dql = <<<DQL
-SELECT v FROM Shlinkio\Shlink\Core\Entity\Visit AS v WHERE v.visitLocation IS NULL
+            SELECT v FROM Shlinkio\Shlink\Core\Entity\Visit AS v WHERE v.visitLocation IS NULL
-DQL;
+        DQL;
         $query = $this->getEntityManager()->createQuery($dql)
                                           ->setMaxResults($blockSize);
         $remainingVisitsToProcess = $this->count(['visitLocation' => null]);

module/Core/src/Service/VisitServiceInterface.php

@@ -1,10 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Shlinkio\Shlink\Core\Service;
-
-interface VisitServiceInterface
-{
-    public function locateUnlocatedVisits(callable $geolocateVisit, ?callable $notifyVisitWithLocation = null): void;
-}

module/Core/src/Visit/VisitLocator.php

@@ -2,7 +2,7 @@
 
 declare(strict_types=1);
 
-namespace Shlinkio\Shlink\Core\Service;
+namespace Shlinkio\Shlink\Core\Visit;
 
 use Doctrine\ORM\EntityManagerInterface;
 use Shlinkio\Shlink\Core\Entity\Visit;
@@ -11,7 +11,7 @@ use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
 use Shlinkio\Shlink\Core\Repository\VisitRepository;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
 
-class VisitService implements VisitServiceInterface
+class VisitLocator implements VisitLocatorInterface
 {
     private EntityManagerInterface $em;
 
@@ -20,11 +20,23 @@ class VisitService implements VisitServiceInterface
         $this->em = $em;
     }
 
-    public function locateUnlocatedVisits(callable $geolocateVisit, ?callable $notifyVisitWithLocation = null): void
+    public function locateUnlocatedVisits(callable $geolocateVisit, callable $notifyVisitWithLocation): void
     {
         /** @var VisitRepository $repo */
         $repo = $this->em->getRepository(Visit::class);
-        $results = $repo->findUnlocatedVisits(false);
+        $this->locateVisits($repo->findUnlocatedVisits(false), $geolocateVisit, $notifyVisitWithLocation);
+    }
+
+    public function locateVisitsWithEmptyLocation(callable $geolocateVisit, callable $notifyVisitWithLocation): void
+    {
+        $this->locateVisits([], $geolocateVisit, $notifyVisitWithLocation);
+    }
+
+    /**
+     * @param iterable|Visit[] $results
+     */
+    private function locateVisits(iterable $results, callable $geolocateVisit, callable $notifyVisitWithLocation): void
+    {
         $count = 0;
         $persistBlock = 200;
 
@@ -58,13 +70,11 @@ class VisitService implements VisitServiceInterface
         $this->em->clear();
     }
 
-    private function locateVisit(Visit $visit, VisitLocation $location, ?callable $notifyVisitWithLocation): void
+    private function locateVisit(Visit $visit, VisitLocation $location, callable $notifyVisitWithLocation): void
     {
         $visit->locate($location);
         $this->em->persist($visit);
 
-        if ($notifyVisitWithLocation !== null) {
+        $notifyVisitWithLocation($location, $visit);
-            $notifyVisitWithLocation($location, $visit);
-        }
     }
 }

module/Core/src/Visit/VisitLocatorInterface.php

@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Visit;
+
+interface VisitLocatorInterface
+{
+    public function locateUnlocatedVisits(callable $geolocateVisit, callable $notifyVisitWithLocation): void;
+
+    public function locateVisitsWithEmptyLocation(callable $geolocateVisit, callable $notifyVisitWithLocation): void;
+}

module/Core/test/Visit/VisitLocatorTest.php

@@ -2,7 +2,7 @@
 
 declare(strict_types=1);
 
-namespace ShlinkioTest\Shlink\Core\Service;
+namespace ShlinkioTest\Shlink\Core\Visit;
 
 use Doctrine\ORM\EntityManager;
 use Exception;
@@ -15,7 +15,7 @@ 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 Shlinkio\Shlink\Core\Visit\VisitLocator;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
 
 use function array_shift;
@@ -26,15 +26,15 @@ use function Functional\map;
 use function range;
 use function sprintf;
 
-class VisitServiceTest extends TestCase
+class VisitLocatorTest extends TestCase
 {
-    private VisitService $visitService;
+    private VisitLocator $visitService;
     private ObjectProphecy $em;
 
     public function setUp(): void
     {
         $this->em = $this->prophesize(EntityManager::class);
-        $this->visitService = new VisitService($this->em->reveal());
+        $this->visitService = new VisitLocator($this->em->reveal());
     }
 
     /** @test */
@@ -95,6 +95,7 @@ class VisitServiceTest extends TestCase
             throw $isNonLocatableAddress
                 ? new IpCannotBeLocatedException('Cannot be located')
                 : IpCannotBeLocatedException::forError(new Exception(''));
+        }, static function (): void {
         });
 
         $findUnlocatedVisits->shouldHaveBeenCalledOnce();