From feb67e76f0bdfad82bd3c2b953c93a8aacf4aebd Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 14 Apr 2019 09:10:00 +0200 Subject: [PATCH 1/8] Updated commands --- module/CLI/config/cli.config.php | 2 +- module/CLI/config/dependencies.config.php | 4 ++-- .../{ProcessVisitsCommand.php => LocateVisitsCommand.php} | 8 +++++--- module/CLI/src/Command/Visit/UpdateDbCommand.php | 3 ++- ...sVisitsCommandTest.php => LocateVisitsCommandTest.php} | 8 ++++---- 5 files changed, 14 insertions(+), 11 deletions(-) rename module/CLI/src/Command/Visit/{ProcessVisitsCommand.php => LocateVisitsCommand.php} (94%) rename module/CLI/test/Command/Visit/{ProcessVisitsCommandTest.php => LocateVisitsCommandTest.php} (97%) diff --git a/module/CLI/config/cli.config.php b/module/CLI/config/cli.config.php index 84969d1a..2edbae99 100644 --- a/module/CLI/config/cli.config.php +++ b/module/CLI/config/cli.config.php @@ -14,7 +14,7 @@ return [ Command\ShortUrl\GeneratePreviewCommand::NAME => Command\ShortUrl\GeneratePreviewCommand::class, Command\ShortUrl\DeleteShortUrlCommand::NAME => Command\ShortUrl\DeleteShortUrlCommand::class, - Command\Visit\ProcessVisitsCommand::NAME => Command\Visit\ProcessVisitsCommand::class, + Command\Visit\LocateVisitsCommand::NAME => Command\Visit\LocateVisitsCommand::class, Command\Visit\UpdateDbCommand::NAME => Command\Visit\UpdateDbCommand::class, Command\Config\GenerateCharsetCommand::NAME => Command\Config\GenerateCharsetCommand::class, diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 7c34f580..294b7f83 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -26,7 +26,7 @@ return [ Command\ShortUrl\GeneratePreviewCommand::class => ConfigAbstractFactory::class, Command\ShortUrl\DeleteShortUrlCommand::class => ConfigAbstractFactory::class, - Command\Visit\ProcessVisitsCommand::class => ConfigAbstractFactory::class, + Command\Visit\LocateVisitsCommand::class => ConfigAbstractFactory::class, Command\Visit\UpdateDbCommand::class => ConfigAbstractFactory::class, Command\Config\GenerateCharsetCommand::class => InvokableFactory::class, @@ -51,7 +51,7 @@ return [ Command\ShortUrl\GeneratePreviewCommand::class => [Service\ShortUrlService::class, PreviewGenerator::class], Command\ShortUrl\DeleteShortUrlCommand::class => [Service\ShortUrl\DeleteShortUrlService::class], - Command\Visit\ProcessVisitsCommand::class => [ + Command\Visit\LocateVisitsCommand::class => [ Service\VisitService::class, IpLocationResolverInterface::class, Lock\Factory::class, diff --git a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php b/module/CLI/src/Command/Visit/LocateVisitsCommand.php similarity index 94% rename from module/CLI/src/Command/Visit/ProcessVisitsCommand.php rename to module/CLI/src/Command/Visit/LocateVisitsCommand.php index e3db15d0..6f34625b 100644 --- a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php +++ b/module/CLI/src/Command/Visit/LocateVisitsCommand.php @@ -20,9 +20,10 @@ use Symfony\Component\Lock\Factory as Locker; use function sprintf; -class ProcessVisitsCommand extends Command +class LocateVisitsCommand extends Command { - public const NAME = 'visit:process'; + public const NAME = 'visit:locate'; + public const ALIASES = ['visit:process']; /** @var VisitServiceInterface */ private $visitService; @@ -48,7 +49,8 @@ class ProcessVisitsCommand extends Command { $this ->setName(self::NAME) - ->setDescription('Processes visits where location is not set yet'); + ->setAliases(self::ALIASES) + ->setDescription('Resolves visits origin locations.'); } protected function execute(InputInterface $input, OutputInterface $output): ?int diff --git a/module/CLI/src/Command/Visit/UpdateDbCommand.php b/module/CLI/src/Command/Visit/UpdateDbCommand.php index af032e96..3882b2c6 100644 --- a/module/CLI/src/Command/Visit/UpdateDbCommand.php +++ b/module/CLI/src/Command/Visit/UpdateDbCommand.php @@ -15,6 +15,7 @@ use Symfony\Component\Console\Style\SymfonyStyle; use function sprintf; +/** @deprecated */ class UpdateDbCommand extends Command { public const NAME = 'visit:update-db'; @@ -32,7 +33,7 @@ class UpdateDbCommand extends Command { $this ->setName(self::NAME) - ->setDescription('Updates the GeoLite2 database file used to geolocate IP addresses') + ->setDescription('[DEPRECATED] Updates the GeoLite2 database file used to geolocate IP addresses') ->setHelp( 'The GeoLite2 database is updated first Tuesday every month, so this command should be ideally run ' . 'every first Wednesday' diff --git a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php similarity index 97% rename from module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php rename to module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index a28a880a..af536383 100644 --- a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -6,7 +6,7 @@ namespace ShlinkioTest\Shlink\CLI\Command\Visit; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; -use Shlinkio\Shlink\CLI\Command\Visit\ProcessVisitsCommand; +use Shlinkio\Shlink\CLI\Command\Visit\LocateVisitsCommand; use Shlinkio\Shlink\Common\Exception\WrongIpException; use Shlinkio\Shlink\Common\IpGeolocation\IpApiLocationResolver; use Shlinkio\Shlink\Common\IpGeolocation\Model\Location; @@ -24,7 +24,7 @@ use Symfony\Component\Lock; use function array_shift; use function sprintf; -class ProcessVisitsCommandTest extends TestCase +class LocateVisitsCommandTest extends TestCase { /** @var CommandTester */ private $commandTester; @@ -49,7 +49,7 @@ class ProcessVisitsCommandTest extends TestCase }); $this->locker->createLock(Argument::type('string'))->willReturn($this->lock->reveal()); - $command = new ProcessVisitsCommand( + $command = new LocateVisitsCommand( $this->visitService->reveal(), $this->ipResolver->reveal(), $this->locker->reveal() @@ -176,7 +176,7 @@ class ProcessVisitsCommandTest extends TestCase $output = $this->commandTester->getDisplay(); $this->assertStringContainsString( - sprintf('There is already an instance of the "%s" command', ProcessVisitsCommand::NAME), + sprintf('There is already an instance of the "%s" command', LocateVisitsCommand::NAME), $output ); $locateVisits->shouldNotHaveBeenCalled(); From 935562acc9190d0fde5122abbac244016502eb9c Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 14 Apr 2019 10:10:10 +0200 Subject: [PATCH 2/8] Created exception to handle cases in which downloading a new geolite db fails --- .../CLI/src/Exception/ExceptionInterface.php | 10 +++ .../GeolocationDbUpdateFailedException.php | 34 ++++++++ ...GeolocationDbUpdateFailedExceptionTest.php | 79 +++++++++++++++++++ 3 files changed, 123 insertions(+) create mode 100644 module/CLI/src/Exception/ExceptionInterface.php create mode 100644 module/CLI/src/Exception/GeolocationDbUpdateFailedException.php create mode 100644 module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php diff --git a/module/CLI/src/Exception/ExceptionInterface.php b/module/CLI/src/Exception/ExceptionInterface.php new file mode 100644 index 00000000..2ff19593 --- /dev/null +++ b/module/CLI/src/Exception/ExceptionInterface.php @@ -0,0 +1,10 @@ +olderDbExists = $olderDbExists; + parent::__construct($message, $code, $previous); + } + + public static function create(bool $olderDbExists, Throwable $prev = null): self + { + return new self( + $olderDbExists, + 'An error occurred while updating geolocation database, and an older version could not be found', + 0, + $prev + ); + } + + public function olderDbExists(): bool + { + return $this->olderDbExists; + } +} diff --git a/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php b/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php new file mode 100644 index 00000000..b8202967 --- /dev/null +++ b/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php @@ -0,0 +1,79 @@ +assertEquals($olderDbExists, $e->olderDbExists()); + $this->assertEquals('', $e->getMessage()); + $this->assertEquals(0, $e->getCode()); + $this->assertNull($e->getPrevious()); + } + + public function provideOlderDbExists(): iterable + { + yield 'with older DB' => [true]; + yield 'without older DB' => [false]; + } + + /** + * @test + * @dataProvider provideConstructorArgs + */ + public function constructCreatesException(bool $olderDbExists, string $message, int $code, ?Throwable $prev): void + { + $e = new GeolocationDbUpdateFailedException($olderDbExists, $message, $code, $prev); + + $this->assertEquals($olderDbExists, $e->olderDbExists()); + $this->assertEquals($message, $e->getMessage()); + $this->assertEquals($code, $e->getCode()); + $this->assertEquals($prev, $e->getPrevious()); + } + + public function provideConstructorArgs(): iterable + { + yield [true, 'This is a nice error message', 99, new Exception('prev')]; + yield [false, 'Another message', 0, new RuntimeException('prev')]; + yield [true, 'An yet another message', -50, null]; + } + + /** + * @test + * @dataProvider provideCreateArgs + */ + public function createBuildsException(bool $olderDbExists, ?Throwable $prev): void + { + $e = GeolocationDbUpdateFailedException::create($olderDbExists, $prev); + + $this->assertEquals($olderDbExists, $e->olderDbExists()); + $this->assertEquals( + 'An error occurred while updating geolocation database, and an older version could not be found', + $e->getMessage() + ); + $this->assertEquals(0, $e->getCode()); + $this->assertEquals($prev, $e->getPrevious()); + } + + public function provideCreateArgs(): iterable + { + yield 'older DB and no prev' => [true, null]; + yield 'older DB and prev' => [true, new RuntimeException('prev')]; + yield 'no older DB and no prev' => [false, null]; + yield 'no older DB and prev' => [false, new Exception('prev')]; + } +} From df40199134238fa21976d86619e2334bf281e097 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 14 Apr 2019 10:25:28 +0200 Subject: [PATCH 3/8] Renamed common config files so that they have the same preffix --- config/autoload/{zend-expressive.global.php => common.global.php} | 0 config/autoload/{local.php.dist => common.local.php.dist} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename config/autoload/{zend-expressive.global.php => common.global.php} (100%) rename config/autoload/{local.php.dist => common.local.php.dist} (100%) diff --git a/config/autoload/zend-expressive.global.php b/config/autoload/common.global.php similarity index 100% rename from config/autoload/zend-expressive.global.php rename to config/autoload/common.global.php diff --git a/config/autoload/local.php.dist b/config/autoload/common.local.php.dist similarity index 100% rename from config/autoload/local.php.dist rename to config/autoload/common.local.php.dist From b24511b7b5bcef350fab21b5a5c88d10b3b8a522 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 14 Apr 2019 10:49:54 +0200 Subject: [PATCH 4/8] Created service that updated GeoLite database when it is older than 7 days --- module/CLI/src/Util/GeolocationDbUpdater.php | 60 ++++++++ .../Util/GeolocationDbUpdaterInterface.php | 14 ++ .../test/Util/GeolocationDbUpdaterTest.php | 136 ++++++++++++++++++ 3 files changed, 210 insertions(+) create mode 100644 module/CLI/src/Util/GeolocationDbUpdater.php create mode 100644 module/CLI/src/Util/GeolocationDbUpdaterInterface.php create mode 100644 module/CLI/test/Util/GeolocationDbUpdaterTest.php diff --git a/module/CLI/src/Util/GeolocationDbUpdater.php b/module/CLI/src/Util/GeolocationDbUpdater.php new file mode 100644 index 00000000..c4fb5e7f --- /dev/null +++ b/module/CLI/src/Util/GeolocationDbUpdater.php @@ -0,0 +1,60 @@ +dbUpdater = $dbUpdater; + $this->geoLiteDbReader = $geoLiteDbReader; + } + + /** + * @throws GeolocationDbUpdateFailedException + */ + public function checkDbUpdate(callable $handleProgress = null): void + { + try { + $meta = $this->geoLiteDbReader->metadata(); + if ($this->buildIsOlderThanOneWeek($meta->__get('buildEpoch'))) { + $this->downloadNewDb(true, $handleProgress); + } + } catch (InvalidArgumentException $e) { + // This is the exception thrown by the reader when the database file does not exist + $this->downloadNewDb(false, $handleProgress); + } + } + + private function buildIsOlderThanOneWeek(int $buildTimestamp): bool + { + $buildDate = Chronos::createFromTimestamp($buildTimestamp); + $now = Chronos::now(); + return $now->gt($buildDate->addDays(7)); + } + + /** + * @throws GeolocationDbUpdateFailedException + */ + private function downloadNewDb(bool $olderDbExists, callable $handleProgress = null): void + { + try { + $this->dbUpdater->downloadFreshCopy($handleProgress); + } catch (RuntimeException $e) { + throw GeolocationDbUpdateFailedException::create($olderDbExists, $e); + } + } +} diff --git a/module/CLI/src/Util/GeolocationDbUpdaterInterface.php b/module/CLI/src/Util/GeolocationDbUpdaterInterface.php new file mode 100644 index 00000000..9b3d70d0 --- /dev/null +++ b/module/CLI/src/Util/GeolocationDbUpdaterInterface.php @@ -0,0 +1,14 @@ +dbUpdater = $this->prophesize(DbUpdaterInterface::class); + $this->geoLiteDbReader = $this->prophesize(Reader::class); + + $this->geolocationDbUpdater = new GeolocationDbUpdater( + $this->dbUpdater->reveal(), + $this->geoLiteDbReader->reveal() + ); + } + + /** @test */ + public function exceptionIsThrownWhenOlderDbDoesNotExistAndDownloadFails(): void + { + $getMeta = $this->geoLiteDbReader->metadata()->willThrow(InvalidArgumentException::class); + $prev = new RuntimeException(''); + $download = $this->dbUpdater->downloadFreshCopy(null)->willThrow($prev); + + try { + $this->geolocationDbUpdater->checkDbUpdate(); + $this->assertTrue(false); // If this is reached, the test will fail + } catch (Throwable $e) { + /** @var GeolocationDbUpdateFailedException $e */ + $this->assertInstanceOf(GeolocationDbUpdateFailedException::class, $e); + $this->assertSame($prev, $e->getPrevious()); + $this->assertFalse($e->olderDbExists()); + } + + $getMeta->shouldHaveBeenCalledOnce(); + $download->shouldHaveBeenCalledOnce(); + } + + /** + * @test + * @dataProvider provideDaysBiggerThanSeven + */ + public function exceptionIsThrownWhenOlderDbIsTooOldAndDownloadFails(int $days): void + { + $getMeta = $this->geoLiteDbReader->metadata()->willReturn(new Metadata([ + 'binary_format_major_version' => '', + 'binary_format_minor_version' => '', + 'build_epoch' => Chronos::now()->subDays($days)->getTimestamp(), + 'database_type' => '', + 'languages' => '', + 'description' => '', + 'ip_version' => '', + 'node_count' => 1, + 'record_size' => 4, + ])); + $prev = new RuntimeException(''); + $download = $this->dbUpdater->downloadFreshCopy(null)->willThrow($prev); + + try { + $this->geolocationDbUpdater->checkDbUpdate(); + $this->assertTrue(false); // If this is reached, the test will fail + } catch (Throwable $e) { + /** @var GeolocationDbUpdateFailedException $e */ + $this->assertInstanceOf(GeolocationDbUpdateFailedException::class, $e); + $this->assertSame($prev, $e->getPrevious()); + $this->assertTrue($e->olderDbExists()); + } + + $getMeta->shouldHaveBeenCalledOnce(); + $download->shouldHaveBeenCalledOnce(); + } + + public function provideDaysBiggerThanSeven(): iterable + { + yield [8]; + yield [9]; + yield [10]; + yield [100]; + } + + /** + * @test + * @dataProvider provideDaysSmallerThanSeven + */ + public function databaseIsNotUpdatedIfItIsYoungerThanOneWeek(int $days): void + { + $getMeta = $this->geoLiteDbReader->metadata()->willReturn(new Metadata([ + 'binary_format_major_version' => '', + 'binary_format_minor_version' => '', + 'build_epoch' => Chronos::now()->subDays($days)->getTimestamp(), + 'database_type' => '', + 'languages' => '', + 'description' => '', + 'ip_version' => '', + 'node_count' => 1, + 'record_size' => 4, + ])); + $download = $this->dbUpdater->downloadFreshCopy(null)->will(function () { + }); + + $this->geolocationDbUpdater->checkDbUpdate(); + + $getMeta->shouldHaveBeenCalledOnce(); + $download->shouldNotHaveBeenCalled(); + } + + public function provideDaysSmallerThanSeven(): iterable + { + return map(range(0, 6), function (int $days) { + return [$days]; + }); + } +} From 0f48dd567f8cb0a1a480436862b1262d7fbf251b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 14 Apr 2019 11:19:21 +0200 Subject: [PATCH 5/8] Registered GeolocationDbUpdater service and added callable which is invoked when db is going to be updated --- module/CLI/config/dependencies.config.php | 6 ++++++ module/CLI/src/Util/GeolocationDbUpdater.php | 17 ++++++++++++----- .../src/Util/GeolocationDbUpdaterInterface.php | 2 +- .../CLI/test/Util/GeolocationDbUpdaterTest.php | 5 ++++- 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 294b7f83..d296d1aa 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -3,6 +3,8 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI; +use GeoIp2\Database\Reader; +use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdater; use Shlinkio\Shlink\Common\IpGeolocation\GeoLite2\DbUpdater; use Shlinkio\Shlink\Common\IpGeolocation\IpLocationResolverInterface; use Shlinkio\Shlink\Common\Service\PreviewGenerator; @@ -19,6 +21,8 @@ return [ 'factories' => [ Application::class => Factory\ApplicationFactory::class, + GeolocationDbUpdater::class => ConfigAbstractFactory::class, + Command\ShortUrl\GenerateShortUrlCommand::class => ConfigAbstractFactory::class, Command\ShortUrl\ResolveUrlCommand::class => ConfigAbstractFactory::class, Command\ShortUrl\ListShortUrlsCommand::class => ConfigAbstractFactory::class, @@ -44,6 +48,8 @@ return [ ], ConfigAbstractFactory::class => [ + GeolocationDbUpdater::class => [DbUpdater::class, Reader::class], + Command\ShortUrl\GenerateShortUrlCommand::class => [Service\UrlShortener::class, 'config.url_shortener.domain'], Command\ShortUrl\ResolveUrlCommand::class => [Service\UrlShortener::class], Command\ShortUrl\ListShortUrlsCommand::class => [Service\ShortUrlService::class, 'config.url_shortener.domain'], diff --git a/module/CLI/src/Util/GeolocationDbUpdater.php b/module/CLI/src/Util/GeolocationDbUpdater.php index c4fb5e7f..1133d5a2 100644 --- a/module/CLI/src/Util/GeolocationDbUpdater.php +++ b/module/CLI/src/Util/GeolocationDbUpdater.php @@ -26,16 +26,16 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface /** * @throws GeolocationDbUpdateFailedException */ - public function checkDbUpdate(callable $handleProgress = null): void + public function checkDbUpdate(callable $mustBeUpdated = null, callable $handleProgress = null): void { try { $meta = $this->geoLiteDbReader->metadata(); if ($this->buildIsOlderThanOneWeek($meta->__get('buildEpoch'))) { - $this->downloadNewDb(true, $handleProgress); + $this->downloadNewDb(true, $mustBeUpdated, $handleProgress); } } catch (InvalidArgumentException $e) { // This is the exception thrown by the reader when the database file does not exist - $this->downloadNewDb(false, $handleProgress); + $this->downloadNewDb(false, $mustBeUpdated, $handleProgress); } } @@ -49,8 +49,15 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface /** * @throws GeolocationDbUpdateFailedException */ - private function downloadNewDb(bool $olderDbExists, callable $handleProgress = null): void - { + private function downloadNewDb( + bool $olderDbExists, + callable $mustBeUpdated = null, + callable $handleProgress = null + ): void { + if ($mustBeUpdated !== null) { + $mustBeUpdated(); + } + try { $this->dbUpdater->downloadFreshCopy($handleProgress); } catch (RuntimeException $e) { diff --git a/module/CLI/src/Util/GeolocationDbUpdaterInterface.php b/module/CLI/src/Util/GeolocationDbUpdaterInterface.php index 9b3d70d0..1d5bcf48 100644 --- a/module/CLI/src/Util/GeolocationDbUpdaterInterface.php +++ b/module/CLI/src/Util/GeolocationDbUpdaterInterface.php @@ -10,5 +10,5 @@ interface GeolocationDbUpdaterInterface /** * @throws GeolocationDbUpdateFailedException */ - public function checkDbUpdate(callable $handleProgress = null): void; + public function checkDbUpdate(callable $mustBeUpdated = null, callable $handleProgress = null): void; } diff --git a/module/CLI/test/Util/GeolocationDbUpdaterTest.php b/module/CLI/test/Util/GeolocationDbUpdaterTest.php index 2e7a7b10..be6709fa 100644 --- a/module/CLI/test/Util/GeolocationDbUpdaterTest.php +++ b/module/CLI/test/Util/GeolocationDbUpdaterTest.php @@ -41,12 +41,15 @@ class GeolocationDbUpdaterTest extends TestCase /** @test */ public function exceptionIsThrownWhenOlderDbDoesNotExistAndDownloadFails(): void { + $mustBeUpdated = function () { + $this->assertTrue(true); + }; $getMeta = $this->geoLiteDbReader->metadata()->willThrow(InvalidArgumentException::class); $prev = new RuntimeException(''); $download = $this->dbUpdater->downloadFreshCopy(null)->willThrow($prev); try { - $this->geolocationDbUpdater->checkDbUpdate(); + $this->geolocationDbUpdater->checkDbUpdate($mustBeUpdated); $this->assertTrue(false); // If this is reached, the test will fail } catch (Throwable $e) { /** @var GeolocationDbUpdateFailedException $e */ From 6613cb5c60b0429180783fc287eb6a39ad85ebe8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 14 Apr 2019 13:18:03 +0200 Subject: [PATCH 6/8] Updated amount of days to wait for the GeoLite2 database to be updated --- module/CLI/src/Util/GeolocationDbUpdater.php | 8 ++++---- .../CLI/test/Util/GeolocationDbUpdaterTest.php | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/module/CLI/src/Util/GeolocationDbUpdater.php b/module/CLI/src/Util/GeolocationDbUpdater.php index 1133d5a2..067a9561 100644 --- a/module/CLI/src/Util/GeolocationDbUpdater.php +++ b/module/CLI/src/Util/GeolocationDbUpdater.php @@ -30,7 +30,7 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface { try { $meta = $this->geoLiteDbReader->metadata(); - if ($this->buildIsOlderThanOneWeek($meta->__get('buildEpoch'))) { + if ($this->buildIsTooOld($meta->__get('buildEpoch'))) { $this->downloadNewDb(true, $mustBeUpdated, $handleProgress); } } catch (InvalidArgumentException $e) { @@ -39,11 +39,11 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface } } - private function buildIsOlderThanOneWeek(int $buildTimestamp): bool + private function buildIsTooOld(int $buildTimestamp): bool { $buildDate = Chronos::createFromTimestamp($buildTimestamp); $now = Chronos::now(); - return $now->gt($buildDate->addDays(7)); + return $now->gt($buildDate->addDays(35)); } /** @@ -55,7 +55,7 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface callable $handleProgress = null ): void { if ($mustBeUpdated !== null) { - $mustBeUpdated(); + $mustBeUpdated($olderDbExists); } try { diff --git a/module/CLI/test/Util/GeolocationDbUpdaterTest.php b/module/CLI/test/Util/GeolocationDbUpdaterTest.php index be6709fa..3ab69aa7 100644 --- a/module/CLI/test/Util/GeolocationDbUpdaterTest.php +++ b/module/CLI/test/Util/GeolocationDbUpdaterTest.php @@ -64,7 +64,7 @@ class GeolocationDbUpdaterTest extends TestCase /** * @test - * @dataProvider provideDaysBiggerThanSeven + * @dataProvider provideBigDays */ public function exceptionIsThrownWhenOlderDbIsTooOldAndDownloadFails(int $days): void { @@ -96,17 +96,17 @@ class GeolocationDbUpdaterTest extends TestCase $download->shouldHaveBeenCalledOnce(); } - public function provideDaysBiggerThanSeven(): iterable + public function provideBigDays(): iterable { - yield [8]; - yield [9]; - yield [10]; + yield [36]; + yield [50]; + yield [75]; yield [100]; } /** * @test - * @dataProvider provideDaysSmallerThanSeven + * @dataProvider provideSmallDays */ public function databaseIsNotUpdatedIfItIsYoungerThanOneWeek(int $days): void { @@ -130,9 +130,9 @@ class GeolocationDbUpdaterTest extends TestCase $download->shouldNotHaveBeenCalled(); } - public function provideDaysSmallerThanSeven(): iterable + public function provideSmallDays(): iterable { - return map(range(0, 6), function (int $days) { + return map(range(0, 34), function (int $days) { return [$days]; }); } From 4866fe241e1afe5762ea82814f021fc602884243 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 14 Apr 2019 17:59:22 +0200 Subject: [PATCH 7/8] Updated LocateVisitsCommand to update the database if needed --- module/CLI/config/dependencies.config.php | 1 + .../src/Command/Visit/LocateVisitsCommand.php | 67 +++++++++++++++---- .../Command/Visit/LocateVisitsCommandTest.php | 45 ++++++++++++- 3 files changed, 99 insertions(+), 14 deletions(-) diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index d296d1aa..b56ff76e 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -61,6 +61,7 @@ return [ Service\VisitService::class, IpLocationResolverInterface::class, Lock\Factory::class, + GeolocationDbUpdater::class, ], Command\Visit\UpdateDbCommand::class => [DbUpdater::class], diff --git a/module/CLI/src/Command/Visit/LocateVisitsCommand.php b/module/CLI/src/Command/Visit/LocateVisitsCommand.php index 6f34625b..fa458413 100644 --- a/module/CLI/src/Command/Visit/LocateVisitsCommand.php +++ b/module/CLI/src/Command/Visit/LocateVisitsCommand.php @@ -3,7 +3,9 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\Visit; +use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\CLI\Util\ExitCodes; +use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdaterInterface; use Shlinkio\Shlink\Common\Exception\WrongIpException; use Shlinkio\Shlink\Common\IpGeolocation\IpLocationResolverInterface; use Shlinkio\Shlink\Common\IpGeolocation\Model\Location; @@ -13,6 +15,7 @@ use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException; use Shlinkio\Shlink\Core\Service\VisitServiceInterface; use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Helper\ProgressBar; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; @@ -31,18 +34,25 @@ class LocateVisitsCommand extends Command private $ipLocationResolver; /** @var Locker */ private $locker; - /** @var OutputInterface */ - private $output; + /** @var GeolocationDbUpdaterInterface */ + private $dbUpdater; + + /** @var SymfonyStyle */ + private $io; + /** @var ProgressBar */ + private $progressBar; public function __construct( VisitServiceInterface $visitService, IpLocationResolverInterface $ipLocationResolver, - Locker $locker + Locker $locker, + GeolocationDbUpdaterInterface $dbUpdater ) { parent::__construct(); $this->visitService = $visitService; $this->ipLocationResolver = $ipLocationResolver; $this->locker = $locker; + $this->dbUpdater = $dbUpdater; } protected function configure(): void @@ -55,16 +65,17 @@ class LocateVisitsCommand extends Command protected function execute(InputInterface $input, OutputInterface $output): ?int { - $this->output = $output; - $io = new SymfonyStyle($input, $output); + $this->io = new SymfonyStyle($input, $output); $lock = $this->locker->createLock(self::NAME); if (! $lock->acquire()) { - $io->warning(sprintf('There is already an instance of the "%s" command in execution', self::NAME)); + $this->io->warning(sprintf('There is already an instance of the "%s" command in execution', self::NAME)); return ExitCodes::EXIT_WARNING; } try { + $this->checkDbUpdate(); + $this->visitService->locateUnlocatedVisits( [$this, 'getGeolocationDataForVisit'], function (VisitLocation $location) use ($output) { @@ -76,7 +87,7 @@ class LocateVisitsCommand extends Command } ); - $io->success('Finished processing all IPs'); + $this->io->success('Finished processing all IPs'); } finally { $lock->release(); return ExitCodes::EXIT_SUCCESS; @@ -86,7 +97,7 @@ class LocateVisitsCommand extends Command public function getGeolocationDataForVisit(Visit $visit): Location { if (! $visit->hasRemoteAddr()) { - $this->output->writeln( + $this->io->writeln( 'Ignored visit with no IP address', OutputInterface::VERBOSITY_VERBOSE ); @@ -94,21 +105,51 @@ class LocateVisitsCommand extends Command } $ipAddr = $visit->getRemoteAddr(); - $this->output->write(sprintf('Processing IP %s', $ipAddr)); + $this->io->write(sprintf('Processing IP %s', $ipAddr)); if ($ipAddr === IpAddress::LOCALHOST) { - $this->output->writeln(' [Ignored localhost address]'); + $this->io->writeln(' [Ignored localhost address]'); throw IpCannotBeLocatedException::forLocalhost(); } try { return $this->ipLocationResolver->resolveIpLocation($ipAddr); } catch (WrongIpException $e) { - $this->output->writeln(' [An error occurred while locating IP. Skipped]'); - if ($this->output->isVerbose()) { - $this->getApplication()->renderException($e, $this->output); + $this->io->writeln(' [An error occurred while locating IP. Skipped]'); + if ($this->io->isVerbose()) { + $this->getApplication()->renderException($e, $this->io); } throw IpCannotBeLocatedException::forError($e); } } + + private function checkDbUpdate(): void + { + try { + $this->dbUpdater->checkDbUpdate(function (bool $olderDbExists) { + $this->io->writeln( + sprintf('%s GeoLite2 database...', $olderDbExists ? 'Updating' : 'Downloading') + ); + $this->progressBar = new ProgressBar($this->io); + }, function (int $total, int $downloaded) { + $this->progressBar->setMaxSteps($total); + $this->progressBar->setProgress($downloaded); + }); + + if ($this->progressBar !== null) { + $this->progressBar->finish(); + $this->io->newLine(); + } + } catch (GeolocationDbUpdateFailedException $e) { + if (! $e->olderDbExists()) { + $this->io->error('GeoLite2 database download failed. It is not possible to locate visits.'); + throw $e; + } + + $this->io->newLine(); + $this->io->writeln( + '[Warning] GeoLite2 database update failed. Proceeding with old version.' + ); + } + } } diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index af536383..573b0c17 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -7,6 +7,8 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\Visit\LocateVisitsCommand; +use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; +use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdaterInterface; use Shlinkio\Shlink\Common\Exception\WrongIpException; use Shlinkio\Shlink\Common\IpGeolocation\IpApiLocationResolver; use Shlinkio\Shlink\Common\IpGeolocation\Model\Location; @@ -36,11 +38,14 @@ class LocateVisitsCommandTest extends TestCase private $locker; /** @var ObjectProphecy */ private $lock; + /** @var ObjectProphecy */ + private $dbUpdater; public function setUp(): void { $this->visitService = $this->prophesize(VisitService::class); $this->ipResolver = $this->prophesize(IpApiLocationResolver::class); + $this->dbUpdater = $this->prophesize(GeolocationDbUpdaterInterface::class); $this->locker = $this->prophesize(Lock\Factory::class); $this->lock = $this->prophesize(Lock\LockInterface::class); @@ -52,7 +57,8 @@ class LocateVisitsCommandTest extends TestCase $command = new LocateVisitsCommand( $this->visitService->reveal(), $this->ipResolver->reveal(), - $this->locker->reveal() + $this->locker->reveal(), + $this->dbUpdater->reveal() ); $app = new Application(); $app->add($command); @@ -182,4 +188,41 @@ class LocateVisitsCommandTest extends TestCase $locateVisits->shouldNotHaveBeenCalled(); $resolveIpLocation->shouldNotHaveBeenCalled(); } + + /** + * @test + * @dataProvider provideParams + */ + public function showsProperMessageWhenGeoLiteUpdateFails(bool $olderDbExists, string $expectedMessage): void + { + $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will(function () { + }); + $checkDbUpdate = $this->dbUpdater->checkDbUpdate(Argument::cetera())->will( + function (array $args) use ($olderDbExists) { + [$mustBeUpdated, $handleProgress] = $args; + + $mustBeUpdated($olderDbExists); + $handleProgress(100, 50); + + throw GeolocationDbUpdateFailedException::create($olderDbExists); + } + ); + + $this->commandTester->execute([]); + $output = $this->commandTester->getDisplay(); + + $this->assertStringContainsString( + sprintf('%s GeoLite2 database...', $olderDbExists ? 'Updating' : 'Downloading'), + $output + ); + $this->assertStringContainsString($expectedMessage, $output); + $locateVisits->shouldHaveBeenCalledTimes((int) $olderDbExists); + $checkDbUpdate->shouldHaveBeenCalledOnce(); + } + + public function provideParams(): iterable + { + yield [true, '[Warning] GeoLite2 database update failed. Proceeding with old version.']; + yield [false, 'GeoLite2 database download failed. It is not possible to locate visits.']; + } } From 8f1477e893e62917a232ca42ae8e70272143abc8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 14 Apr 2019 18:07:23 +0200 Subject: [PATCH 8/8] Updated changelog --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 295b28ee..91442b4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Added -* *Nothing* +* [#377](https://github.com/shlinkio/shlink/issues/377) Updated `visit:locate` command (formerly `visit:process`) to automatically update the GeoLite2 database if it is too old or it does not exist. + + This simplifies processing visits in a container-based infrastructure, since a fresh container is capable of getting an updated version of the file by itself. + + It also removes the need of asynchronously and programmatically updating the file, which deprecates the `visit:update-db` command. #### Changed