From 83b7d5a5f1734afcd917fadbee23d7d8882b28d9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 18 Sep 2022 18:44:01 +0200 Subject: [PATCH] Extracted logic to geolocate a visit, handling possible domain errors --- module/Core/config/dependencies.config.php | 3 + .../Core/config/event_dispatcher.config.php | 3 +- .../EventDispatcher/LocateUnlocatedVisits.php | 23 +------ .../Exception/IpCannotBeLocatedException.php | 23 ++++--- .../src/Visit/Model/UnlocatableIpType.php | 10 +++ .../Core/src/Visit/VisitToLocationHelper.php | 40 +++++++++++ .../Visit/VisitToLocationHelperInterface.php | 17 +++++ .../LocateUnlocatedVisitsTest.php | 54 +++++++++++++++ .../IpCannotBeLocatedExceptionTest.php | 4 ++ module/Core/test/Visit/VisitLocatorTest.php | 2 +- .../test/Visit/VisitToLocationHelperTest.php | 66 +++++++++++++++++++ 11 files changed, 214 insertions(+), 31 deletions(-) create mode 100644 module/Core/src/Visit/Model/UnlocatableIpType.php create mode 100644 module/Core/src/Visit/VisitToLocationHelper.php create mode 100644 module/Core/src/Visit/VisitToLocationHelperInterface.php create mode 100644 module/Core/test/EventDispatcher/LocateUnlocatedVisitsTest.php create mode 100644 module/Core/test/Visit/VisitToLocationHelperTest.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 9855e2aa..49b2857a 100644 --- a/module/Core/config/dependencies.config.php +++ b/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 => [ diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index f6e198b8..3d473010 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/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', diff --git a/module/Core/src/EventDispatcher/LocateUnlocatedVisits.php b/module/Core/src/EventDispatcher/LocateUnlocatedVisits.php index a72e8853..c036c450 100644 --- a/module/Core/src/EventDispatcher/LocateUnlocatedVisits.php +++ b/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 } } diff --git a/module/Core/src/Exception/IpCannotBeLocatedException.php b/module/Core/src/Exception/IpCannotBeLocatedException.php index b1ba731c..2ebc3e62 100644 --- a/module/Core/src/Exception/IpCannotBeLocatedException.php +++ b/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; } } diff --git a/module/Core/src/Visit/Model/UnlocatableIpType.php b/module/Core/src/Visit/Model/UnlocatableIpType.php new file mode 100644 index 00000000..56490209 --- /dev/null +++ b/module/Core/src/Visit/Model/UnlocatableIpType.php @@ -0,0 +1,10 @@ +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); + } + } +} diff --git a/module/Core/src/Visit/VisitToLocationHelperInterface.php b/module/Core/src/Visit/VisitToLocationHelperInterface.php new file mode 100644 index 00000000..7d553527 --- /dev/null +++ b/module/Core/src/Visit/VisitToLocationHelperInterface.php @@ -0,0 +1,17 @@ +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(); + } +} diff --git a/module/Core/test/Exception/IpCannotBeLocatedExceptionTest.php b/module/Core/test/Exception/IpCannotBeLocatedExceptionTest.php index b1487b69..2089daba 100644 --- a/module/Core/test/Exception/IpCannotBeLocatedExceptionTest.php +++ b/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 diff --git a/module/Core/test/Visit/VisitLocatorTest.php b/module/Core/test/Visit/VisitLocatorTest.php index b740d143..21908be8 100644 --- a/module/Core/test/Visit/VisitLocatorTest.php +++ b/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('')); } diff --git a/module/Core/test/Visit/VisitToLocationHelperTest.php b/module/Core/test/Visit/VisitToLocationHelperTest.php new file mode 100644 index 00000000..ee22272f --- /dev/null +++ b/module/Core/test/Visit/VisitToLocationHelperTest.php @@ -0,0 +1,66 @@ +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', ''))); + } +}