From 36680e82aa7433ff4772f1af21e36ac75902adb1 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 18 Sep 2022 19:21:59 +0200 Subject: [PATCH] Reduced duplication in LocateVisitsCommand by reusing VisitToLocationHelper --- module/CLI/config/dependencies.config.php | 3 +- .../src/Command/Visit/LocateVisitsCommand.php | 44 ++++++++----------- .../Command/Visit/LocateVisitsCommandTest.php | 39 +++++++--------- 3 files changed, 37 insertions(+), 49 deletions(-) diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index a9de1f0c..47a47735 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -20,7 +20,6 @@ use Shlinkio\Shlink\Core\Visit; use Shlinkio\Shlink\Installer\Factory\ProcessHelperFactory; use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdater; use Shlinkio\Shlink\IpGeolocation\GeoLite2\GeoLite2Options; -use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; use Shlinkio\Shlink\Rest\Service\ApiKeyService; use Symfony\Component\Console as SymfonyCli; use Symfony\Component\Lock\LockFactory; @@ -97,7 +96,7 @@ return [ Command\Visit\DownloadGeoLiteDbCommand::class => [GeoLite\GeolocationDbUpdater::class], Command\Visit\LocateVisitsCommand::class => [ Visit\VisitLocator::class, - IpLocationResolverInterface::class, + Visit\VisitToLocationHelper::class, LockFactory::class, ], Command\Visit\GetOrphanVisitsCommand::class => [Visit\VisitsStatsHelper::class], diff --git a/module/CLI/src/Command/Visit/LocateVisitsCommand.php b/module/CLI/src/Command/Visit/LocateVisitsCommand.php index 47aa50d1..59db9367 100644 --- a/module/CLI/src/Command/Visit/LocateVisitsCommand.php +++ b/module/CLI/src/Command/Visit/LocateVisitsCommand.php @@ -11,11 +11,11 @@ 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\Visit\Model\UnlocatableIpType; 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; use Symfony\Component\Console\Exception\RuntimeException; use Symfony\Component\Console\Input\ArrayInput; use Symfony\Component\Console\Input\InputInterface; @@ -35,7 +35,7 @@ class LocateVisitsCommand extends AbstractLockedCommand implements VisitGeolocat public function __construct( private readonly VisitLocatorInterface $visitLocator, - private readonly IpLocationResolverInterface $ipLocationResolver, + private readonly VisitToLocationHelperInterface $visitToLocation, LockFactory $locker, ) { parent::__construct($locker); @@ -132,39 +132,33 @@ class LocateVisitsCommand extends AbstractLockedCommand implements VisitGeolocat */ public function geolocateVisit(Visit $visit): Location { - if (! $visit->hasRemoteAddr()) { - $this->io->writeln( - 'Ignored visit with no IP address', - OutputInterface::VERBOSITY_VERBOSE, - ); - throw IpCannotBeLocatedException::forEmptyAddress(); - } - - $ipAddr = $visit->getRemoteAddr() ?? ''; + $ipAddr = $visit->getRemoteAddr() ?? '?'; $this->io->write(sprintf('Processing IP %s', $ipAddr)); - if ($ipAddr === IpAddress::LOCALHOST) { - $this->io->writeln(' [Ignored localhost address]'); - throw IpCannotBeLocatedException::forLocalhost(); - } try { - return $this->ipLocationResolver->resolveIpLocation($ipAddr); - } catch (WrongIpException $e) { - $this->io->writeln(' [An error occurred while locating IP. Skipped]'); - if ($this->io->isVerbose()) { + return $this->visitToLocation->resolveVisitLocation($visit); + } catch (IpCannotBeLocatedException $e) { + $this->io->writeln(match ($e->type) { + UnlocatableIpType::EMPTY_ADDRESS => ' [Ignored visit with no IP address]', + UnlocatableIpType::LOCALHOST => ' [Ignored localhost address]', + UnlocatableIpType::ERROR => ' [An error occurred while locating IP. Skipped]', + }); + + if ($e->type === UnlocatableIpType::ERROR && $this->io->isVerbose()) { $this->getApplication()?->renderThrowable($e, $this->io); } - throw IpCannotBeLocatedException::forError($e); + throw $e; } } public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void { - $message = ! $visitLocation->isEmpty() - ? sprintf(' [Address located in "%s"]', $visitLocation->getCountryName()) - : ' [Address not found]'; - $this->io->writeln($message); + if (! $visitLocation->isEmpty()) { + $this->io->writeln(sprintf(' [Address located in "%s"]', $visitLocation->getCountryName())); + } elseif ($visit->hasRemoteAddr() && $visit->getRemoteAddr() !== IpAddress::LOCALHOST) { + $this->io->writeln(' [Could not locate address]'); + } } private function checkDbUpdate(): void diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index 4111c1dc..63ad3e52 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -10,16 +10,16 @@ use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\Visit\DownloadGeoLiteDbCommand; use Shlinkio\Shlink\CLI\Command\Visit\LocateVisitsCommand; use Shlinkio\Shlink\CLI\Util\ExitCodes; -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\Visit\VisitGeolocationHelperInterface; use Shlinkio\Shlink\Core\Visit\VisitLocator; +use Shlinkio\Shlink\Core\Visit\VisitToLocationHelperInterface; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; -use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; use ShlinkioTest\Shlink\CLI\CliTestUtilsTrait; use Symfony\Component\Console\Exception\RuntimeException; use Symfony\Component\Console\Output\OutputInterface; @@ -36,14 +36,14 @@ class LocateVisitsCommandTest extends TestCase private CommandTester $commandTester; private ObjectProphecy $visitService; - private ObjectProphecy $ipResolver; + private ObjectProphecy $visitToLocation; private ObjectProphecy $lock; private ObjectProphecy $downloadDbCommand; protected function setUp(): void { $this->visitService = $this->prophesize(VisitLocator::class); - $this->ipResolver = $this->prophesize(IpLocationResolverInterface::class); + $this->visitToLocation = $this->prophesize(VisitToLocationHelperInterface::class); $locker = $this->prophesize(Lock\LockFactory::class); $this->lock = $this->prophesize(Lock\LockInterface::class); @@ -54,7 +54,7 @@ class LocateVisitsCommandTest extends TestCase $command = new LocateVisitsCommand( $this->visitService->reveal(), - $this->ipResolver->reveal(), + $this->visitToLocation->reveal(), $locker->reveal(), ); @@ -84,7 +84,7 @@ class LocateVisitsCommandTest extends TestCase $mockMethodBehavior, ); $locateAllVisits = $this->visitService->locateAllVisits(Argument::cetera())->will($mockMethodBehavior); - $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn( + $resolveIpLocation = $this->visitToLocation->resolveVisitLocation(Argument::any())->willReturn( Location::emptyInstance(), ); @@ -117,36 +117,29 @@ class LocateVisitsCommandTest extends TestCase * @test * @dataProvider provideIgnoredAddresses */ - public function localhostAndEmptyAddressesAreIgnored(?string $address, string $message): void + public function localhostAndEmptyAddressesAreIgnored(IpCannotBeLocatedException $e, string $message): void { - $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', $address, '')); + $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()); $location = VisitLocation::fromGeolocation(Location::emptyInstance()); $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will( $this->invokeHelperMethods($visit, $location), ); - $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn( - Location::emptyInstance(), - ); + $resolveIpLocation = $this->visitToLocation->resolveVisitLocation(Argument::any())->willThrow($e); $this->commandTester->execute([], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); $output = $this->commandTester->getDisplay(); + self::assertStringContainsString('Processing IP', $output); self::assertStringContainsString($message, $output); - if (empty($address)) { - self::assertStringNotContainsString('Processing IP', $output); - } else { - self::assertStringContainsString('Processing IP', $output); - } $locateVisits->shouldHaveBeenCalledOnce(); - $resolveIpLocation->shouldNotHaveBeenCalled(); + $resolveIpLocation->shouldHaveBeenCalledOnce(); } public function provideIgnoredAddresses(): iterable { - yield 'with empty address' => ['', 'Ignored visit with no IP address']; - yield 'with null address' => [null, 'Ignored visit with no IP address']; - yield 'with localhost address' => [IpAddress::LOCALHOST, 'Ignored localhost address']; + yield 'empty address' => [IpCannotBeLocatedException::forEmptyAddress(), 'Ignored visit with no IP address']; + yield 'localhost address' => [IpCannotBeLocatedException::forLocalhost(), 'Ignored localhost address']; } /** @test */ @@ -158,7 +151,9 @@ class LocateVisitsCommandTest extends TestCase $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will( $this->invokeHelperMethods($visit, $location), ); - $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willThrow(WrongIpException::class); + $resolveIpLocation = $this->visitToLocation->resolveVisitLocation(Argument::any())->willThrow( + IpCannotBeLocatedException::forError(WrongIpException::fromIpAddress('1.2.3.4')), + ); $this->commandTester->execute([], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); @@ -187,7 +182,7 @@ class LocateVisitsCommandTest extends TestCase $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will(function (): void { }); - $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]); + $resolveIpLocation = $this->visitToLocation->resolveVisitLocation(Argument::any()); $this->commandTester->execute([], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); $output = $this->commandTester->getDisplay();