Extracted logic to geolocate a visit, handling possible domain errors

module/Core/config/dependencies.config.php

@@ -11,6 +11,7 @@ use Shlinkio\Shlink\Config\Factory\ValinorConfigFactory;
 use Shlinkio\Shlink\Core\ErrorHandler;
 use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions;
 use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface;
+use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface;
 
 return [
 
@@ -44,6 +45,7 @@ return [
             Visit\VisitsTracker::class => ConfigAbstractFactory::class,
             Visit\RequestTracker::class => ConfigAbstractFactory::class,
             Visit\VisitLocator::class => ConfigAbstractFactory::class,
+            Visit\VisitToLocationHelper::class => ConfigAbstractFactory::class,
             Visit\VisitsStatsHelper::class => ConfigAbstractFactory::class,
             Visit\Transformer\OrphanVisitDataTransformer::class => InvokableFactory::class,
 
@@ -108,6 +110,7 @@ return [
             ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class,
         ],
         Visit\VisitLocator::class => ['em'],
+        Visit\VisitToLocationHelper::class => [IpLocationResolverInterface::class],
         Visit\VisitsStatsHelper::class => ['em'],
         Tag\TagService::class => ['em'],
         Service\ShortUrl\DeleteShortUrlService::class => [

module/Core/config/event_dispatcher.config.php

@@ -11,6 +11,7 @@ use Shlinkio\Shlink\Common\Cache\RedisPublishingHelper;
 use Shlinkio\Shlink\Common\Mercure\MercureHubPublishingHelper;
 use Shlinkio\Shlink\Common\RabbitMq\RabbitMqPublishingHelper;
 use Shlinkio\Shlink\Core\Visit\VisitLocator;
+use Shlinkio\Shlink\Core\Visit\VisitToLocationHelper;
 use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdater;
 use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface;
 
@@ -91,7 +92,7 @@ return [
             DbUpdater::class,
             EventDispatcherInterface::class,
         ],
-        EventDispatcher\LocateUnlocatedVisits::class => [VisitLocator::class, IpLocationResolverInterface::class],
+        EventDispatcher\LocateUnlocatedVisits::class => [VisitLocator::class, VisitToLocationHelper::class],
         EventDispatcher\NotifyVisitToWebHooks::class => [
             'httpClient',
             'em',

module/Core/src/EventDispatcher/LocateUnlocatedVisits.php

@@ -4,22 +4,20 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Core\EventDispatcher;
 
-use Shlinkio\Shlink\Common\Util\IpAddress;
 use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Entity\VisitLocation;
 use Shlinkio\Shlink\Core\EventDispatcher\Event\GeoLiteDbCreated;
 use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
 use Shlinkio\Shlink\Core\Visit\VisitGeolocationHelperInterface;
 use Shlinkio\Shlink\Core\Visit\VisitLocatorInterface;
-use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException;
+use Shlinkio\Shlink\Core\Visit\VisitToLocationHelperInterface;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
-use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface;
 
 class LocateUnlocatedVisits implements VisitGeolocationHelperInterface
 {
     public function __construct(
         private readonly VisitLocatorInterface $locator,
-        private readonly IpLocationResolverInterface $ipLocationResolver,
+        private readonly VisitToLocationHelperInterface $visitToLocation,
     ) {
     }
 
@@ -33,25 +31,10 @@ class LocateUnlocatedVisits implements VisitGeolocationHelperInterface
      */
     public function geolocateVisit(Visit $visit): Location
     {
-        // TODO This method duplicates code from LocateVisitsCommand. Move to a common place.
-        if (! $visit->hasRemoteAddr()) {
-            throw IpCannotBeLocatedException::forEmptyAddress();
-        }
-
-        $ipAddr = $visit->getRemoteAddr() ?? '';
-        if ($ipAddr === IpAddress::LOCALHOST) {
-            throw IpCannotBeLocatedException::forLocalhost();
-        }
-
-        try {
-            return $this->ipLocationResolver->resolveIpLocation($ipAddr);
-        } catch (WrongIpException $e) {
-            throw IpCannotBeLocatedException::forError($e);
-        }
+        return $this->visitToLocation->resolveVisitLocation($visit);
     }
 
     public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void
     {
-        // Do nothing
     }
 }

module/Core/src/Exception/IpCannotBeLocatedException.php

@@ -4,35 +4,40 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Core\Exception;
 
+use Shlinkio\Shlink\Core\Visit\Model\UnlocatableIpType;
 use Throwable;
 
 class IpCannotBeLocatedException extends RuntimeException
 {
-    private bool $isNonLocatableAddress = true;
+    private function __construct(
+        string $message,
+        public readonly UnlocatableIpType $type,
+        int $code = 0,
+        ?Throwable $previous = null,
+    ) {
+        parent::__construct($message, $code, $previous);
+    }
 
     public static function forEmptyAddress(): self
     {
-        return new self('Ignored visit with no IP address');
+        return new self('Ignored visit with no IP address', UnlocatableIpType::EMPTY_ADDRESS);
     }
 
     public static function forLocalhost(): self
     {
-        return new self('Ignored localhost address');
+        return new self('Ignored localhost address', UnlocatableIpType::LOCALHOST);
     }
 
     public static function forError(Throwable $e): self
     {
-        $e = new self('An error occurred while locating IP', $e->getCode(), $e);
-        $e->isNonLocatableAddress = false;
-
-        return $e;
+        return new self('An error occurred while locating IP', UnlocatableIpType::ERROR, $e->getCode(), $e);
     }
 
     /**
-     * Tells if this error belongs to an address that will never be possible locate
+     * Tells if this belongs to an address that will never be possible to locate
      */
     public function isNonLocatableAddress(): bool
     {
-        return $this->isNonLocatableAddress;
+        return $this->type !== UnlocatableIpType::ERROR;
     }
 }

module/Core/src/Visit/Model/UnlocatableIpType.php

@@ -0,0 +1,10 @@
+<?php
+
+namespace Shlinkio\Shlink\Core\Visit\Model;
+
+enum UnlocatableIpType
+{
+    case EMPTY_ADDRESS;
+    case LOCALHOST;
+    case ERROR;
+}

module/Core/src/Visit/VisitToLocationHelper.php

@@ -0,0 +1,40 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Visit;
+
+use Shlinkio\Shlink\Common\Util\IpAddress;
+use Shlinkio\Shlink\Core\Entity\Visit;
+use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
+use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException;
+use Shlinkio\Shlink\IpGeolocation\Model\Location;
+use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface;
+
+class VisitToLocationHelper implements VisitToLocationHelperInterface
+{
+    public function __construct(private readonly IpLocationResolverInterface $ipLocationResolver)
+    {
+    }
+
+    /**
+     * @throws IpCannotBeLocatedException
+     */
+    public function resolveVisitLocation(Visit $visit): Location
+    {
+        if (! $visit->hasRemoteAddr()) {
+            throw IpCannotBeLocatedException::forEmptyAddress();
+        }
+
+        $ipAddr = $visit->getRemoteAddr() ?? '';
+        if ($ipAddr === IpAddress::LOCALHOST) {
+            throw IpCannotBeLocatedException::forLocalhost();
+        }
+
+        try {
+            return $this->ipLocationResolver->resolveIpLocation($ipAddr);
+        } catch (WrongIpException $e) {
+            throw IpCannotBeLocatedException::forError($e);
+        }
+    }
+}

module/Core/src/Visit/VisitToLocationHelperInterface.php

@@ -0,0 +1,17 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Visit;
+
+use Shlinkio\Shlink\Core\Entity\Visit;
+use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
+use Shlinkio\Shlink\IpGeolocation\Model\Location;
+
+interface VisitToLocationHelperInterface
+{
+    /**
+     * @throws IpCannotBeLocatedException
+     */
+    public function resolveVisitLocation(Visit $visit): Location;
+}

module/Core/test/EventDispatcher/LocateUnlocatedVisitsTest.php

@@ -0,0 +1,54 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\EventDispatcher;
+
+use PHPUnit\Framework\TestCase;
+use Prophecy\PhpUnit\ProphecyTrait;
+use Prophecy\Prophecy\ObjectProphecy;
+use Shlinkio\Shlink\Core\Entity\Visit;
+use Shlinkio\Shlink\Core\EventDispatcher\Event\GeoLiteDbCreated;
+use Shlinkio\Shlink\Core\EventDispatcher\LocateUnlocatedVisits;
+use Shlinkio\Shlink\Core\Model\Visitor;
+use Shlinkio\Shlink\Core\Visit\VisitLocatorInterface;
+use Shlinkio\Shlink\Core\Visit\VisitToLocationHelperInterface;
+use Shlinkio\Shlink\IpGeolocation\Model\Location;
+
+class LocateUnlocatedVisitsTest extends TestCase
+{
+    use ProphecyTrait;
+
+    private LocateUnlocatedVisits $listener;
+    private ObjectProphecy $locator;
+    private ObjectProphecy $visitToLocation;
+
+    protected function setUp(): void
+    {
+        $this->locator = $this->prophesize(VisitLocatorInterface::class);
+        $this->visitToLocation = $this->prophesize(VisitToLocationHelperInterface::class);
+
+        $this->listener = new LocateUnlocatedVisits($this->locator->reveal(), $this->visitToLocation->reveal());
+    }
+
+    /** @test */
+    public function locatorIsCalledWhenInvoked(): void
+    {
+        ($this->listener)(new GeoLiteDbCreated());
+        $this->locator->locateUnlocatedVisits($this->listener)->shouldHaveBeenCalledOnce();
+    }
+
+    /** @test */
+    public function visitToLocationHelperIsCalledToGeolocateVisits(): void
+    {
+        $visit = Visit::forBasePath(Visitor::emptyInstance());
+        $location = Location::emptyInstance();
+
+        $resolve = $this->visitToLocation->resolveVisitLocation($visit)->willReturn($location);
+
+        $result = $this->listener->geolocateVisit($visit);
+
+        self::assertSame($location, $result);
+        $resolve->shouldHaveBeenCalledOnce();
+    }
+}

module/Core/test/Exception/IpCannotBeLocatedExceptionTest.php

@@ -9,6 +9,7 @@ use LogicException;
 use PHPUnit\Framework\TestCase;
 use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
 use Shlinkio\Shlink\Core\Exception\RuntimeException;
+use Shlinkio\Shlink\Core\Visit\Model\UnlocatableIpType;
 use Throwable;
 
 class IpCannotBeLocatedExceptionTest extends TestCase
@@ -22,6 +23,7 @@ class IpCannotBeLocatedExceptionTest extends TestCase
         self::assertEquals('Ignored visit with no IP address', $e->getMessage());
         self::assertEquals(0, $e->getCode());
         self::assertNull($e->getPrevious());
+        self::assertEquals(UnlocatableIpType::EMPTY_ADDRESS, $e->type);
     }
 
     /** @test */
@@ -33,6 +35,7 @@ class IpCannotBeLocatedExceptionTest extends TestCase
         self::assertEquals('Ignored localhost address', $e->getMessage());
         self::assertEquals(0, $e->getCode());
         self::assertNull($e->getPrevious());
+        self::assertEquals(UnlocatableIpType::LOCALHOST, $e->type);
     }
 
     /**
@@ -47,6 +50,7 @@ class IpCannotBeLocatedExceptionTest extends TestCase
         self::assertEquals('An error occurred while locating IP', $e->getMessage());
         self::assertEquals($prev->getCode(), $e->getCode());
         self::assertSame($prev, $e->getPrevious());
+        self::assertEquals(UnlocatableIpType::ERROR, $e->type);
     }
 
     public function provideErrors(): iterable

module/Core/test/Visit/VisitLocatorTest.php

@@ -129,7 +129,7 @@ class VisitLocatorTest extends TestCase
                 public function geolocateVisit(Visit $visit): Location
                 {
                     throw $this->isNonLocatableAddress
-                        ? new IpCannotBeLocatedException('Cannot be located')
+                        ? IpCannotBeLocatedException::forEmptyAddress()
                         : IpCannotBeLocatedException::forError(new Exception(''));
                 }
 

module/Core/test/Visit/VisitToLocationHelperTest.php

@@ -0,0 +1,66 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\Visit;
+
+use PHPUnit\Framework\TestCase;
+use Prophecy\Argument;
+use Prophecy\PhpUnit\ProphecyTrait;
+use Prophecy\Prophecy\ObjectProphecy;
+use Shlinkio\Shlink\Common\Util\IpAddress;
+use Shlinkio\Shlink\Core\Entity\Visit;
+use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
+use Shlinkio\Shlink\Core\Model\Visitor;
+use Shlinkio\Shlink\Core\Visit\VisitToLocationHelper;
+use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException;
+use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface;
+
+class VisitToLocationHelperTest extends TestCase
+{
+    use ProphecyTrait;
+
+    private VisitToLocationHelper $helper;
+    private ObjectProphecy $ipLocationResolver;
+
+    protected function setUp(): void
+    {
+        $this->ipLocationResolver = $this->prophesize(IpLocationResolverInterface::class);
+        $this->helper = new VisitToLocationHelper($this->ipLocationResolver->reveal());
+    }
+
+    /**
+     * @test
+     * @dataProvider provideNonLocatableVisits
+     */
+    public function throwsExpectedErrorForNonLocatableVisit(
+        Visit $visit,
+        IpCannotBeLocatedException $expectedException,
+    ): void {
+        $this->expectExceptionObject($expectedException);
+        $this->ipLocationResolver->resolveIpLocation(Argument::cetera())->shouldNotBeCalled();
+
+        $this->helper->resolveVisitLocation($visit);
+    }
+
+    public function provideNonLocatableVisits(): iterable
+    {
+        yield [Visit::forBasePath(Visitor::emptyInstance()), IpCannotBeLocatedException::forEmptyAddress()];
+        yield [
+            Visit::forBasePath(new Visitor('foo', 'bar', IpAddress::LOCALHOST, '')),
+            IpCannotBeLocatedException::forLocalhost(),
+        ];
+    }
+
+    /** @test */
+    public function throwsGenericErrorWhenResolvingIpFails(): void
+    {
+        $e = new WrongIpException('');
+
+        $this->expectExceptionObject(IpCannotBeLocatedException::forError($e));
+        $this->ipLocationResolver->resolveIpLocation(Argument::cetera())->willThrow($e)
+                                                                        ->shouldBeCalledOnce();
+
+        $this->helper->resolveVisitLocation(Visit::forBasePath(new Visitor('foo', 'bar', '1.2.3.4', '')));
+    }
+}