From c6fdd8a59f109c69ddd9218bc19b1a0ced5e003d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 23 Jul 2019 16:36:56 +0200 Subject: [PATCH 1/6] Improvements and ensured LocateVisitsCommand does not swallow exceptions --- config/autoload/logger.local.php.dist | 36 +++++++++++++++---- data/infra/swoole.Dockerfile | 4 ++- .../src/Command/Visit/LocateVisitsCommand.php | 14 ++++++-- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/config/autoload/logger.local.php.dist b/config/autoload/logger.local.php.dist index 47203ed0..59feeb7b 100644 --- a/config/autoload/logger.local.php.dist +++ b/config/autoload/logger.local.php.dist @@ -1,16 +1,40 @@ [ - 'handlers' => [ - 'shlink_rotating_handler' => [ - 'level' => Logger::DEBUG, - ], +// For swoole, send logs to standard output +$logger = $isSwoole ? [ + 'handlers' => [ + 'shlink_rotating_handler' => [ + 'level' => Logger::EMERGENCY, // This basically disables regular file logs + ], + 'shlink_stdout_handler' => [ + 'class' => StreamHandler::class, + 'level' => Logger::INFO, + 'stream' => 'php://stdout', + 'formatter' => 'dashed', ], ], + 'loggers' => [ + 'Shlink' => [ + 'handlers' => ['shlink_stdout_handler'], + ], + ], +] : [ + 'handlers' => [ + 'shlink_rotating_handler' => [ + 'level' => Logger::DEBUG, + ], + ], +]; + +return [ + + 'logger' => $logger, + ]; diff --git a/data/infra/swoole.Dockerfile b/data/infra/swoole.Dockerfile index d3d2eae8..87c16064 100644 --- a/data/infra/swoole.Dockerfile +++ b/data/infra/swoole.Dockerfile @@ -84,7 +84,9 @@ WORKDIR /home/shlink # Expose swoole port EXPOSE 8080 -CMD /usr/local/bin/composer update && \ +CMD \ + # Install dependencies if the vendor dir does not exist + if [[ ! -d "./vendor" ]]; then /usr/local/bin/composer install ; fi && \ # When restarting the container, swoole might think it is already in execution # This forces the app to be started every second until the exit code is 0 until php ./vendor/bin/zend-expressive-swoole start; do sleep 1 ; done diff --git a/module/CLI/src/Command/Visit/LocateVisitsCommand.php b/module/CLI/src/Command/Visit/LocateVisitsCommand.php index fa458413..ef1f0298 100644 --- a/module/CLI/src/Command/Visit/LocateVisitsCommand.php +++ b/module/CLI/src/Command/Visit/LocateVisitsCommand.php @@ -3,6 +3,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\Visit; +use Exception; use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\CLI\Util\ExitCodes; use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdaterInterface; @@ -78,8 +79,8 @@ class LocateVisitsCommand extends Command $this->visitService->locateUnlocatedVisits( [$this, 'getGeolocationDataForVisit'], - function (VisitLocation $location) use ($output) { - if (! $location->isEmpty()) { + static function (VisitLocation $location) use ($output) { + if (!$location->isEmpty()) { $output->writeln( sprintf(' [Address located at "%s"]', $location->getCountryName()) ); @@ -88,9 +89,16 @@ class LocateVisitsCommand extends Command ); $this->io->success('Finished processing all IPs'); + return ExitCodes::EXIT_SUCCESS; + } catch (Exception $e) { + $this->io->error($e->getMessage()); + if ($this->io->isVerbose()) { + $this->getApplication()->renderException($e, $this->io); + } + + return ExitCodes::EXIT_FAILURE; } finally { $lock->release(); - return ExitCodes::EXIT_SUCCESS; } } From 999beef349a5758db872ec789d053cf357549597 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 23 Jul 2019 17:07:40 +0200 Subject: [PATCH 2/6] Fixed GeolocationDbUpdater so that it does not try to interact with the reader if the file does not exist, preventing later errors --- config/autoload/logger.local.php.dist | 2 +- module/CLI/src/Util/GeolocationDbUpdater.php | 27 ++++++++++++------- .../Util/GeolocationDbUpdaterInterface.php | 2 +- .../src/IpGeolocation/GeoLite2/DbUpdater.php | 5 ++++ .../GeoLite2/DbUpdaterInterface.php | 2 ++ 5 files changed, 27 insertions(+), 11 deletions(-) diff --git a/config/autoload/logger.local.php.dist b/config/autoload/logger.local.php.dist index 59feeb7b..cf7e4801 100644 --- a/config/autoload/logger.local.php.dist +++ b/config/autoload/logger.local.php.dist @@ -14,7 +14,7 @@ $logger = $isSwoole ? [ ], 'shlink_stdout_handler' => [ 'class' => StreamHandler::class, - 'level' => Logger::INFO, + 'level' => Logger::DEBUG, 'stream' => 'php://stdout', 'formatter' => 'dashed', ], diff --git a/module/CLI/src/Util/GeolocationDbUpdater.php b/module/CLI/src/Util/GeolocationDbUpdater.php index 34354a0e..4f7881a1 100644 --- a/module/CLI/src/Util/GeolocationDbUpdater.php +++ b/module/CLI/src/Util/GeolocationDbUpdater.php @@ -5,11 +5,11 @@ namespace Shlinkio\Shlink\CLI\Util; use Cake\Chronos\Chronos; use GeoIp2\Database\Reader; -use InvalidArgumentException; use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\Common\Exception\RuntimeException; use Shlinkio\Shlink\Common\IpGeolocation\GeoLite2\DbUpdaterInterface; use Symfony\Component\Lock\Factory as Locker; +use Throwable; class GeolocationDbUpdater implements GeolocationDbUpdaterInterface { @@ -32,24 +32,33 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface /** * @throws GeolocationDbUpdateFailedException */ - public function checkDbUpdate(callable $mustBeUpdated = null, callable $handleProgress = null): void + public function checkDbUpdate(?callable $mustBeUpdated = null, ?callable $handleProgress = null): void { $lock = $this->locker->createLock(self::LOCK_NAME); $lock->acquire(true); // Block until lock is released try { - $meta = $this->geoLiteDbReader->metadata(); - if ($this->buildIsTooOld($meta->__get('buildEpoch'))) { - $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, $mustBeUpdated, $handleProgress); + $this->downloadIfNeeded($mustBeUpdated, $handleProgress); + } catch (Throwable $e) { + throw $e; } finally { $lock->release(); } } + private function downloadIfNeeded(?callable $mustBeUpdated, ?callable $handleProgress): void + { + if (! $this->dbUpdater->databaseFileExists()) { + $this->downloadNewDb(false, $mustBeUpdated, $handleProgress); + return; + } + + $meta = $this->geoLiteDbReader->metadata(); + if ($this->buildIsTooOld($meta->__get('buildEpoch'))) { + $this->downloadNewDb(true, $mustBeUpdated, $handleProgress); + } + } + private function buildIsTooOld(int $buildTimestamp): bool { $buildDate = Chronos::createFromTimestamp($buildTimestamp); diff --git a/module/CLI/src/Util/GeolocationDbUpdaterInterface.php b/module/CLI/src/Util/GeolocationDbUpdaterInterface.php index 1d5bcf48..b27ae01d 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 $mustBeUpdated = null, callable $handleProgress = null): void; + public function checkDbUpdate(?callable $mustBeUpdated = null, ?callable $handleProgress = null): void; } diff --git a/module/Common/src/IpGeolocation/GeoLite2/DbUpdater.php b/module/Common/src/IpGeolocation/GeoLite2/DbUpdater.php index 039ce477..4efebdbe 100644 --- a/module/Common/src/IpGeolocation/GeoLite2/DbUpdater.php +++ b/module/Common/src/IpGeolocation/GeoLite2/DbUpdater.php @@ -98,4 +98,9 @@ class DbUpdater implements DbUpdaterInterface // Ignore any error produced when trying to delete temp files } } + + public function databaseFileExists(): bool + { + return $this->filesystem->exists($this->options->getDbLocation()); + } } diff --git a/module/Common/src/IpGeolocation/GeoLite2/DbUpdaterInterface.php b/module/Common/src/IpGeolocation/GeoLite2/DbUpdaterInterface.php index bf304c6f..e5694aeb 100644 --- a/module/Common/src/IpGeolocation/GeoLite2/DbUpdaterInterface.php +++ b/module/Common/src/IpGeolocation/GeoLite2/DbUpdaterInterface.php @@ -7,6 +7,8 @@ use Shlinkio\Shlink\Common\Exception\RuntimeException; interface DbUpdaterInterface { + public function databaseFileExists(): bool; + /** * @throws RuntimeException */ From 173bfbd300e9b621aff157a58b2e67fc989f6925 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 23 Jul 2019 22:04:01 +0200 Subject: [PATCH 3/6] Updated tests to fit current implementations --- module/CLI/src/Util/GeolocationDbUpdater.php | 24 +++++++++---------- .../test/Util/GeolocationDbUpdaterTest.php | 11 +++++++-- .../IpGeolocation/GeoLite2/DbUpdaterTest.php | 21 +++++++++++++++- 3 files changed, 41 insertions(+), 15 deletions(-) diff --git a/module/CLI/src/Util/GeolocationDbUpdater.php b/module/CLI/src/Util/GeolocationDbUpdater.php index 4f7881a1..e7ea60b9 100644 --- a/module/CLI/src/Util/GeolocationDbUpdater.php +++ b/module/CLI/src/Util/GeolocationDbUpdater.php @@ -46,6 +46,9 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface } } + /** + * @throws GeolocationDbUpdateFailedException + */ private function downloadIfNeeded(?callable $mustBeUpdated, ?callable $handleProgress): void { if (! $this->dbUpdater->databaseFileExists()) { @@ -59,21 +62,11 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface } } - private function buildIsTooOld(int $buildTimestamp): bool - { - $buildDate = Chronos::createFromTimestamp($buildTimestamp); - $now = Chronos::now(); - return $now->gt($buildDate->addDays(35)); - } - /** * @throws GeolocationDbUpdateFailedException */ - private function downloadNewDb( - bool $olderDbExists, - callable $mustBeUpdated = null, - callable $handleProgress = null - ): void { + private function downloadNewDb(bool $olderDbExists, ?callable $mustBeUpdated, ?callable $handleProgress): void + { if ($mustBeUpdated !== null) { $mustBeUpdated($olderDbExists); } @@ -84,4 +77,11 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface throw GeolocationDbUpdateFailedException::create($olderDbExists, $e); } } + + private function buildIsTooOld(int $buildTimestamp): bool + { + $buildDate = Chronos::createFromTimestamp($buildTimestamp); + $now = Chronos::now(); + return $now->gt($buildDate->addDays(35)); + } } diff --git a/module/CLI/test/Util/GeolocationDbUpdaterTest.php b/module/CLI/test/Util/GeolocationDbUpdaterTest.php index e12dcfd9..9e06c907 100644 --- a/module/CLI/test/Util/GeolocationDbUpdaterTest.php +++ b/module/CLI/test/Util/GeolocationDbUpdaterTest.php @@ -58,8 +58,10 @@ class GeolocationDbUpdaterTest extends TestCase $mustBeUpdated = function () { $this->assertTrue(true); }; - $getMeta = $this->geoLiteDbReader->metadata()->willThrow(InvalidArgumentException::class); $prev = new RuntimeException(''); + + $fileExists = $this->dbUpdater->databaseFileExists()->willReturn(false); + $getMeta = $this->geoLiteDbReader->metadata(); $download = $this->dbUpdater->downloadFreshCopy(null)->willThrow($prev); try { @@ -72,7 +74,8 @@ class GeolocationDbUpdaterTest extends TestCase $this->assertFalse($e->olderDbExists()); } - $getMeta->shouldHaveBeenCalledOnce(); + $fileExists->shouldHaveBeenCalledOnce(); + $getMeta->shouldNotHaveBeenCalled(); $download->shouldHaveBeenCalledOnce(); } @@ -82,6 +85,7 @@ class GeolocationDbUpdaterTest extends TestCase */ public function exceptionIsThrownWhenOlderDbIsTooOldAndDownloadFails(int $days): void { + $fileExists = $this->dbUpdater->databaseFileExists()->willReturn(true); $getMeta = $this->geoLiteDbReader->metadata()->willReturn(new Metadata([ 'binary_format_major_version' => '', 'binary_format_minor_version' => '', @@ -106,6 +110,7 @@ class GeolocationDbUpdaterTest extends TestCase $this->assertTrue($e->olderDbExists()); } + $fileExists->shouldHaveBeenCalledOnce(); $getMeta->shouldHaveBeenCalledOnce(); $download->shouldHaveBeenCalledOnce(); } @@ -124,6 +129,7 @@ class GeolocationDbUpdaterTest extends TestCase */ public function databaseIsNotUpdatedIfItIsYoungerThanOneWeek(int $days): void { + $fileExists = $this->dbUpdater->databaseFileExists()->willReturn(true); $getMeta = $this->geoLiteDbReader->metadata()->willReturn(new Metadata([ 'binary_format_major_version' => '', 'binary_format_minor_version' => '', @@ -140,6 +146,7 @@ class GeolocationDbUpdaterTest extends TestCase $this->geolocationDbUpdater->checkDbUpdate(); + $fileExists->shouldHaveBeenCalledOnce(); $getMeta->shouldHaveBeenCalledOnce(); $download->shouldNotHaveBeenCalled(); } diff --git a/module/Common/test/IpGeolocation/GeoLite2/DbUpdaterTest.php b/module/Common/test/IpGeolocation/GeoLite2/DbUpdaterTest.php index d2c9cc5e..f897d9e1 100644 --- a/module/Common/test/IpGeolocation/GeoLite2/DbUpdaterTest.php +++ b/module/Common/test/IpGeolocation/GeoLite2/DbUpdaterTest.php @@ -32,7 +32,7 @@ class DbUpdaterTest extends TestCase $this->filesystem = $this->prophesize(Filesystem::class); $this->options = new GeoLite2Options([ 'temp_dir' => __DIR__ . '/../../../test-resources', - 'db_location' => '', + 'db_location' => 'db_location', 'download_from' => '', ]); @@ -110,4 +110,23 @@ class DbUpdaterTest extends TestCase $copy->shouldHaveBeenCalledOnce(); $remove->shouldHaveBeenCalledOnce(); } + + /** + * @test + * @dataProvider provideExists + */ + public function databaseFileExistsChecksIfTheFilesExistsInTheFilesystem(bool $expected): void + { + $exists = $this->filesystem->exists('db_location')->willReturn($expected); + + $result = $this->dbUpdater->databaseFileExists(); + + $this->assertEquals($expected, $result); + $exists->shouldHaveBeenCalledOnce(); + } + + public function provideExists(): iterable + { + return [[true], [false]]; + } } From 9fe2111d62f42af9daaf07a17eb52822b12595e9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 23 Jul 2019 22:06:09 +0200 Subject: [PATCH 4/6] Updated changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be28d018..f22a4ff9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,7 +47,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Fixed -* *Nothing* +* [#416](https://github.com/shlinkio/shlink/issues/416) Fixed error thrown when trying to locate visits after the GeoLite2 DB is downloaded for the first time. ## 1.17.0 - 2019-05-13 From a6727c5382675747859d203a079a5d4c69451150 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 23 Jul 2019 22:09:38 +0200 Subject: [PATCH 5/6] Fixed coding styles --- module/CLI/test/Util/GeolocationDbUpdaterTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/module/CLI/test/Util/GeolocationDbUpdaterTest.php b/module/CLI/test/Util/GeolocationDbUpdaterTest.php index 9e06c907..6dd11807 100644 --- a/module/CLI/test/Util/GeolocationDbUpdaterTest.php +++ b/module/CLI/test/Util/GeolocationDbUpdaterTest.php @@ -5,7 +5,6 @@ namespace ShlinkioTest\Shlink\CLI\Util; use Cake\Chronos\Chronos; use GeoIp2\Database\Reader; -use InvalidArgumentException; use MaxMind\Db\Reader\Metadata; use PHPUnit\Framework\TestCase; use Prophecy\Argument; From c9ec3b3b4228073ed78a7ef580ae32fbf90f05fd Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 23 Jul 2019 22:17:49 +0200 Subject: [PATCH 6/6] Fixed composer commands to be more aqurate based on their name --- composer.json | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/composer.json b/composer.json index e6e20989..87432cea 100644 --- a/composer.json +++ b/composer.json @@ -115,15 +115,18 @@ "test:ci": [ "@test:unit:ci", "@test:db", - "@test:db:mysql", - "@test:db:postgres", "@test:api" ], "test:unit": "phpdbg -qrr vendor/bin/phpunit --order-by=random --coverage-php build/coverage-unit.cov --testdox", "test:unit:ci": "phpdbg -qrr vendor/bin/phpunit --order-by=random --coverage-php build/coverage-unit.cov --coverage-clover=build/clover.xml --coverage-xml=build/coverage-xml --log-junit=build/phpunit.junit.xml --testdox", - "test:db": "APP_ENV=test phpdbg -qrr vendor/bin/phpunit --order-by=random -c phpunit-db.xml --coverage-php build/coverage-db.cov --testdox", - "test:db:mysql": "DB_DRIVER=mysql composer test:db", - "test:db:postgres": "DB_DRIVER=postgres composer test:db", + "test:db": [ + "@test:db:sqlite", + "@test:db:mysql", + "@test:db:postgres" + ], + "test:db:sqlite": "APP_ENV=test phpdbg -qrr vendor/bin/phpunit --order-by=random -c phpunit-db.xml --coverage-php build/coverage-db.cov --testdox", + "test:db:mysql": "DB_DRIVER=mysql composer test:db:sqlite", + "test:db:postgres": "DB_DRIVER=postgres composer test:db:sqlite", "test:api": "bin/test/run-api-tests.sh", "test:pretty": [ @@ -150,7 +153,8 @@ "test:ci": "Runs all test suites, generating all needed reports and logs for CI envs", "test:unit": "Runs unit test suites", "test:unit:ci": "Runs unit test suites, generating all needed reports and logs for CI envs", - "test:db": "Runs database test suites on a SQLite database", + "test:db": "Runs database test suites on a SQLite, MySQL and PostgreSQL", + "test:db:sqlite": "Runs database test suites on a SQLite database", "test:db:mysql": "Runs database test suites on a MySQL database", "test:db:postgres": "Runs database test suites on a PostgreSQL database", "test:api": "Runs API test suites",