diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c252ab0..5db6cad2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,29 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). +## [Unreleased] + +#### Added + +* *Nothing* + +#### Changed + +* *Nothing* + +#### Deprecated + +* *Nothing* + +#### Removed + +* *Nothing* + +#### Fixed + +* [#362](https://github.com/shlinkio/shlink/issues/362) Fixed all visits without an IP address being processed every time the `visit:process` command is executed. + + ## 1.16.0 - 2019-02-23 #### Added diff --git a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php index 25006cc9..8de39651 100644 --- a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php +++ b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php @@ -65,7 +65,11 @@ class ProcessVisitsCommand extends Command $this->visitService->locateUnlocatedVisits( [$this, 'getGeolocationDataForVisit'], function (VisitLocation $location) use ($output) { - $output->writeln(sprintf(' [Address located at "%s"]', $location->getCountryName())); + if (! $location->isEmpty()) { + $output->writeln( + sprintf(' [Address located at "%s"]', $location->getCountryName()) + ); + } } ); @@ -83,14 +87,14 @@ class ProcessVisitsCommand extends Command 'Ignored visit with no IP address', OutputInterface::VERBOSITY_VERBOSE ); - throw new IpCannotBeLocatedException('Ignored visit with no IP address'); + throw IpCannotBeLocatedException::forEmptyAddress(); } $ipAddr = $visit->getRemoteAddr(); $this->output->write(sprintf('Processing IP %s', $ipAddr)); if ($ipAddr === IpAddress::LOCALHOST) { $this->output->writeln(' [Ignored localhost address]'); - throw new IpCannotBeLocatedException('Ignored localhost address'); + throw IpCannotBeLocatedException::forLocalhost(); } try { @@ -101,7 +105,7 @@ class ProcessVisitsCommand extends Command $this->getApplication()->renderException($e, $this->output); } - throw new IpCannotBeLocatedException('An error occurred while locating IP', $e->getCode(), $e); + throw IpCannotBeLocatedException::forError($e); } } } diff --git a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php index dbb311f2..9c008ed3 100644 --- a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php @@ -116,6 +116,11 @@ class ProcessVisitsCommandTest extends TestCase $output = $this->commandTester->getDisplay(); $this->assertStringContainsString($message, $output); + if (empty($address)) { + $this->assertStringNotContainsString('Processing IP', $output); + } else { + $this->assertStringContainsString('Processing IP', $output); + } $locateVisits->shouldHaveBeenCalledOnce(); $resolveIpLocation->shouldNotHaveBeenCalled(); } diff --git a/module/Core/src/Entity/VisitLocation.php b/module/Core/src/Entity/VisitLocation.php index b8376c01..fad9c986 100644 --- a/module/Core/src/Entity/VisitLocation.php +++ b/module/Core/src/Entity/VisitLocation.php @@ -72,4 +72,16 @@ class VisitLocation extends AbstractEntity implements VisitLocationInterface 'timezone' => $this->timezone, ]; } + + public function isEmpty(): bool + { + return + $this->countryCode === '' && + $this->countryName === '' && + $this->regionName === '' && + $this->cityName === '' && + ((float) $this->latitude) === 0.0 && + ((float) $this->longitude) === 0.0 && + $this->timezone === ''; + } } diff --git a/module/Core/src/Exception/IpCannotBeLocatedException.php b/module/Core/src/Exception/IpCannotBeLocatedException.php index e172e9c9..3908385e 100644 --- a/module/Core/src/Exception/IpCannotBeLocatedException.php +++ b/module/Core/src/Exception/IpCannotBeLocatedException.php @@ -3,6 +3,43 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Exception; +use Throwable; + class IpCannotBeLocatedException extends RuntimeException { + /** @var bool */ + private $isNonLocatableAddress; + + public function __construct( + bool $isNonLocatableAddress, + string $message, + int $code = 0, + ?Throwable $previous = null + ) { + $this->isNonLocatableAddress = $isNonLocatableAddress; + parent::__construct($message, $code, $previous); + } + + public static function forEmptyAddress(): self + { + return new self(true, 'Ignored visit with no IP address'); + } + + public static function forLocalhost(): self + { + return new self(true, 'Ignored localhost address'); + } + + public static function forError(Throwable $e): self + { + return new self(false, 'An error occurred while locating IP', $e->getCode(), $e); + } + + /** + * Tells if this error belongs to an address that will never be possible locate + */ + public function isNonLocatableAddress(): bool + { + return $this->isNonLocatableAddress; + } } diff --git a/module/Core/src/Service/VisitService.php b/module/Core/src/Service/VisitService.php index 4abbf4a2..dc3a11c9 100644 --- a/module/Core/src/Service/VisitService.php +++ b/module/Core/src/Service/VisitService.php @@ -30,12 +30,18 @@ class VisitService implements VisitServiceInterface foreach ($results as $visit) { $count++; + try { /** @var Location $location */ $location = $geolocateVisit($visit); } catch (IpCannotBeLocatedException $e) { - // Skip if the visit's IP could not be located - continue; + if (! $e->isNonLocatableAddress()) { + // Skip if the visit's IP could not be located because of an error + continue; + } + + // If the IP address is non-locatable, locate it as empty to prevent next processes to pick it again + $location = Location::emptyInstance(); } $location = new VisitLocation($location); diff --git a/module/Core/test/Entity/VisitLocationTest.php b/module/Core/test/Entity/VisitLocationTest.php index 6e008e15..d331d5c6 100644 --- a/module/Core/test/Entity/VisitLocationTest.php +++ b/module/Core/test/Entity/VisitLocationTest.php @@ -18,4 +18,28 @@ class VisitLocationTest extends TestCase $this->assertSame('1000.7', $location->getLatitude()); $this->assertSame('-2000.4', $location->getLongitude()); } + + /** + * @test + * @dataProvider provideArgs + */ + public function isEmptyReturnsTrueWhenAllValuesAreEmpty(array $args, bool $isEmpty): void + { + $payload = new Location(...$args); + $location = new VisitLocation($payload); + + $this->assertEquals($isEmpty, $location->isEmpty()); + } + + public function provideArgs(): iterable + { + yield [['', '', '', '', 0.0, 0.0, ''], true]; + yield [['', '', '', '', 0.0, 0.0, 'dd'], false]; + yield [['', '', '', 'dd', 0.0, 0.0, ''], false]; + yield [['', '', 'dd', '', 0.0, 0.0, ''], false]; + yield [['', 'dd', '', '', 0.0, 0.0, ''], false]; + yield [['dd', '', '', '', 0.0, 0.0, ''], false]; + yield [['', '', '', '', 1.0, 0.0, ''], false]; + yield [['', '', '', '', 0.0, 1.0, ''], false]; + } } diff --git a/module/Core/test/Exception/IpCannotBeLocatedExceptionTest.php b/module/Core/test/Exception/IpCannotBeLocatedExceptionTest.php new file mode 100644 index 00000000..4a00d5b9 --- /dev/null +++ b/module/Core/test/Exception/IpCannotBeLocatedExceptionTest.php @@ -0,0 +1,95 @@ +assertTrue($e->isNonLocatableAddress()); + $this->assertEquals('Ignored visit with no IP address', $e->getMessage()); + $this->assertEquals(0, $e->getCode()); + $this->assertNull($e->getPrevious()); + } + + /** @test */ + public function forLocalhostInitializesException(): void + { + $e = IpCannotBeLocatedException::forLocalhost(); + + $this->assertTrue($e->isNonLocatableAddress()); + $this->assertEquals('Ignored localhost address', $e->getMessage()); + $this->assertEquals(0, $e->getCode()); + $this->assertNull($e->getPrevious()); + } + + /** + * @test + * @dataProvider provideErrors + */ + public function forErrorInitializesException(Throwable $prev): void + { + $e = IpCannotBeLocatedException::forError($prev); + + $this->assertFalse($e->isNonLocatableAddress()); + $this->assertEquals('An error occurred while locating IP', $e->getMessage()); + $this->assertEquals($prev->getCode(), $e->getCode()); + $this->assertSame($prev, $e->getPrevious()); + } + + public function provideErrors(): iterable + { + yield 'Simple exception with positive code' => [new Exception('Some message', 100)]; + yield 'Runtime exception with negative code' => [new RuntimeException('Something went wrong', -50)]; + yield 'Logic exception with default code' => [new LogicException('Conditions unmet')]; + } + + /** + * @test + * @dataProvider provideConstructorArgs + */ + public function constructorInitializesException(): void + { + $args = func_get_args(); + [$isNonLocatableAddress, $message] = $args; + $code = $args[2] ?? 0; + $prev = $args[3] ?? null; + + switch (count($args)) { + case 2: + $e = new IpCannotBeLocatedException($isNonLocatableAddress, $message); + break; + case 3: + $e = new IpCannotBeLocatedException($isNonLocatableAddress, $message, $code); + break; + default: + $e = new IpCannotBeLocatedException($isNonLocatableAddress, $message, $code, $prev); + } + + $this->assertEquals($isNonLocatableAddress, $e->isNonLocatableAddress()); + $this->assertEquals($message, $e->getMessage()); + $this->assertEquals($code, $e->getCode()); + $this->assertEquals($prev, $e->getPrevious()); + } + + public function provideConstructorArgs(): iterable + { + yield 'without default args' => [true, 'Message']; + yield 'without prev' => [true, 'Message', random_int(1, 100)]; + yield 'without all args' => [false, 'Foo', random_int(1, 100), new RuntimeException('Foo')]; + } +} diff --git a/module/Core/test/Service/VisitServiceTest.php b/module/Core/test/Service/VisitServiceTest.php index be2bf8ba..a2821fa6 100644 --- a/module/Core/test/Service/VisitServiceTest.php +++ b/module/Core/test/Service/VisitServiceTest.php @@ -70,8 +70,11 @@ class VisitServiceTest extends TestCase $clear->shouldHaveBeenCalledTimes(floor(count($unlocatedVisits) / 200) + 1); } - /** @test */ - public function visitsWhichCannotBeLocatedAreIgnored(): void + /** + * @test + * @dataProvider provideIsNonLocatableAddress + */ + public function visitsWhichCannotBeLocatedAreIgnoredOrLocatedAsEmpty(bool $isNonLocatableAddress): void { $unlocatedVisits = [ new Visit(new ShortUrl('foo'), Visitor::emptyInstance()), @@ -88,14 +91,20 @@ class VisitServiceTest extends TestCase $clear = $this->em->clear()->will(function () { }); - $this->visitService->locateUnlocatedVisits(function () { - throw new IpCannotBeLocatedException('Cannot be located'); + $this->visitService->locateUnlocatedVisits(function () use ($isNonLocatableAddress) { + throw new IpCannotBeLocatedException($isNonLocatableAddress, 'Cannot be located'); }); $findUnlocatedVisits->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); - $persist->shouldNotHaveBeenCalled(); + $persist->shouldHaveBeenCalledTimes($isNonLocatableAddress ? 1 : 0); $flush->shouldHaveBeenCalledOnce(); $clear->shouldHaveBeenCalledOnce(); } + + public function provideIsNonLocatableAddress(): iterable + { + yield 'The address is locatable' => [false]; + yield 'The address is non-locatable' => [true]; + } }