diff --git a/CHANGELOG.md b/CHANGELOG.md index 75a84d8c..d304c548 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Fixed * [#988](https://github.com/shlinkio/shlink/issues/988) Fixed serving zero-byte static files in apache and apache-compatible web servers. * [#990](https://github.com/shlinkio/shlink/issues/990) Fixed short URLs not properly composed in REST API endpoints when both custom domain and custom base path are used. +* [#1002](https://github.com/shlinkio/shlink/issues/1002) Fixed weird behavior in which GeoLite2 metadata's `buildEpoch` is parsed as string instead of int. ## [2.5.2] - 2021-01-24 diff --git a/module/CLI/src/Exception/GeolocationDbUpdateFailedException.php b/module/CLI/src/Exception/GeolocationDbUpdateFailedException.php index 8ddee216..f663fd8f 100644 --- a/module/CLI/src/Exception/GeolocationDbUpdateFailedException.php +++ b/module/CLI/src/Exception/GeolocationDbUpdateFailedException.php @@ -7,18 +7,46 @@ namespace Shlinkio\Shlink\CLI\Exception; use RuntimeException; use Throwable; +use function sprintf; + class GeolocationDbUpdateFailedException extends RuntimeException implements ExceptionInterface { private bool $olderDbExists; - public static function create(bool $olderDbExists, ?Throwable $prev = null): self + public static function withOlderDb(?Throwable $prev = null): self { $e = new self( - 'An error occurred while updating geolocation database, and an older version could not be found', + 'An error occurred while updating geolocation database, but an older DB is already present.', 0, $prev, ); - $e->olderDbExists = $olderDbExists; + $e->olderDbExists = true; + + return $e; + } + + public static function withoutOlderDb(?Throwable $prev = null): self + { + $e = new self( + 'An error occurred while updating geolocation database, and an older version could not be found.', + 0, + $prev, + ); + $e->olderDbExists = false; + + return $e; + } + + /** + * @param mixed $buildEpoch + */ + public static function withInvalidEpochInOldDb($buildEpoch): self + { + $e = new self(sprintf( + 'Build epoch with value "%s" from existing geolocation database, could not be parsed to integer.', + $buildEpoch, + )); + $e->olderDbExists = true; return $e; } diff --git a/module/CLI/src/Util/GeolocationDbUpdater.php b/module/CLI/src/Util/GeolocationDbUpdater.php index fd40fc15..b8f5b756 100644 --- a/module/CLI/src/Util/GeolocationDbUpdater.php +++ b/module/CLI/src/Util/GeolocationDbUpdater.php @@ -6,11 +6,14 @@ namespace Shlinkio\Shlink\CLI\Util; use Cake\Chronos\Chronos; use GeoIp2\Database\Reader; +use MaxMind\Db\Reader\Metadata; use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\IpGeolocation\Exception\RuntimeException; use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdaterInterface; use Symfony\Component\Lock\LockFactory; +use function is_int; + class GeolocationDbUpdater implements GeolocationDbUpdaterInterface { private const LOCK_NAME = 'geolocation-db-update'; @@ -52,7 +55,7 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface } $meta = $this->geoLiteDbReader->metadata(); - if ($this->buildIsTooOld($meta->buildEpoch)) { + if ($this->buildIsTooOld($meta)) { $this->downloadNewDb(true, $mustBeUpdated, $handleProgress); } } @@ -69,14 +72,37 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface try { $this->dbUpdater->downloadFreshCopy($handleProgress); } catch (RuntimeException $e) { - throw GeolocationDbUpdateFailedException::create($olderDbExists, $e); + throw $olderDbExists + ? GeolocationDbUpdateFailedException::withOlderDb($e) + : GeolocationDbUpdateFailedException::withoutOlderDb($e); } } - private function buildIsTooOld(int $buildTimestamp): bool + private function buildIsTooOld(Metadata $meta): bool { + $buildTimestamp = $this->resolveBuildTimestamp($meta); $buildDate = Chronos::createFromTimestamp($buildTimestamp); $now = Chronos::now(); + return $now->gt($buildDate->addDays(35)); } + + private function resolveBuildTimestamp(Metadata $meta): int + { + // In theory the buildEpoch should be an int, but it has been reported to come as a string. + // See https://github.com/shlinkio/shlink/issues/1002 for context + + /** @var int|string $buildEpoch */ + $buildEpoch = $meta->buildEpoch; + if (is_int($buildEpoch)) { + return $buildEpoch; + } + + $intBuildEpoch = (int) $buildEpoch; + if ($buildEpoch === (string) $intBuildEpoch) { + return $intBuildEpoch; + } + + throw GeolocationDbUpdateFailedException::withInvalidEpochInOldDb($buildEpoch); + } } diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index fc64d643..5ba0778a 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -217,7 +217,9 @@ class LocateVisitsCommandTest extends TestCase $mustBeUpdated($olderDbExists); $handleProgress(100, 50); - throw GeolocationDbUpdateFailedException::create($olderDbExists); + throw $olderDbExists + ? GeolocationDbUpdateFailedException::withOlderDb() + : GeolocationDbUpdateFailedException::withoutOlderDb(); }, ); diff --git a/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php b/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php index 33d7d76e..470aed2c 100644 --- a/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php +++ b/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php @@ -14,26 +14,54 @@ class GeolocationDbUpdateFailedExceptionTest extends TestCase { /** * @test - * @dataProvider provideCreateArgs + * @dataProvider providePrev */ - public function createBuildsException(bool $olderDbExists, ?Throwable $prev): void + public function withOlderDbBuildsException(?Throwable $prev): void { - $e = GeolocationDbUpdateFailedException::create($olderDbExists, $prev); + $e = GeolocationDbUpdateFailedException::withOlderDb($prev); - self::assertEquals($olderDbExists, $e->olderDbExists()); + self::assertTrue($e->olderDbExists()); self::assertEquals( - 'An error occurred while updating geolocation database, and an older version could not be found', + 'An error occurred while updating geolocation database, but an older DB is already present.', $e->getMessage(), ); self::assertEquals(0, $e->getCode()); self::assertEquals($prev, $e->getPrevious()); } - public function provideCreateArgs(): iterable + /** + * @test + * @dataProvider providePrev + */ + public function withoutOlderDbBuildsException(?Throwable $prev): void { - 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')]; + $e = GeolocationDbUpdateFailedException::withoutOlderDb($prev); + + self::assertFalse($e->olderDbExists()); + self::assertEquals( + 'An error occurred while updating geolocation database, and an older version could not be found.', + $e->getMessage(), + ); + self::assertEquals(0, $e->getCode()); + self::assertEquals($prev, $e->getPrevious()); + } + + public function providePrev(): iterable + { + yield 'no prev' => [null]; + yield 'RuntimeException' => [new RuntimeException('prev')]; + yield 'Exception' => [new Exception('prev')]; + } + + /** @test */ + public function withInvalidEpochInOldDbBuildsException(): void + { + $e = GeolocationDbUpdateFailedException::withInvalidEpochInOldDb('foobar'); + + self::assertTrue($e->olderDbExists()); + self::assertEquals( + 'Build epoch with value "foobar" from existing geolocation database, could not be parsed to integer.', + $e->getMessage(), + ); } } diff --git a/module/CLI/test/Util/GeolocationDbUpdaterTest.php b/module/CLI/test/Util/GeolocationDbUpdaterTest.php index 71e05d8a..54b07f1f 100644 --- a/module/CLI/test/Util/GeolocationDbUpdaterTest.php +++ b/module/CLI/test/Util/GeolocationDbUpdaterTest.php @@ -80,17 +80,9 @@ 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' => '', - 'build_epoch' => Chronos::now()->subDays($days)->getTimestamp(), - 'database_type' => '', - 'languages' => '', - 'description' => '', - 'ip_version' => '', - 'node_count' => 1, - 'record_size' => 4, - ])); + $getMeta = $this->geoLiteDbReader->metadata()->willReturn($this->buildMetaWithBuildEpoch( + Chronos::now()->subDays($days)->getTimestamp(), + )); $prev = new RuntimeException(''); $download = $this->dbUpdater->downloadFreshCopy(null)->willThrow($prev); @@ -120,21 +112,12 @@ class GeolocationDbUpdaterTest extends TestCase /** * @test * @dataProvider provideSmallDays + * @param string|int $buildEpoch */ - public function databaseIsNotUpdatedIfItIsYoungerThanOneWeek(int $days): void + public function databaseIsNotUpdatedIfItIsYoungerThanOneWeek($buildEpoch): void { $fileExists = $this->dbUpdater->databaseFileExists()->willReturn(true); - $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, - ])); + $getMeta = $this->geoLiteDbReader->metadata()->willReturn($this->buildMetaWithBuildEpoch($buildEpoch)); $download = $this->dbUpdater->downloadFreshCopy(null)->will(function (): void { }); @@ -147,6 +130,48 @@ class GeolocationDbUpdaterTest extends TestCase public function provideSmallDays(): iterable { - return map(range(0, 34), fn (int $days) => [$days]); + $generateParamsWithTimestamp = static function (int $days) { + $timestamp = Chronos::now()->subDays($days)->getTimestamp(); + return [$days % 2 === 0 ? $timestamp : (string) $timestamp]; + }; + + return map(range(0, 34), $generateParamsWithTimestamp); + } + + /** @test */ + public function exceptionIsThrownWhenCheckingExistingDatabaseWithInvalidBuildEpoch(): void + { + $fileExists = $this->dbUpdater->databaseFileExists()->willReturn(true); + $getMeta = $this->geoLiteDbReader->metadata()->willReturn($this->buildMetaWithBuildEpoch('invalid')); + $download = $this->dbUpdater->downloadFreshCopy(null)->will(function (): void { + }); + + $this->expectException(GeolocationDbUpdateFailedException::class); + $this->expectExceptionMessage( + 'Build epoch with value "invalid" from existing geolocation database, could not be parsed to integer.', + ); + $fileExists->shouldBeCalledOnce(); + $getMeta->shouldBeCalledOnce(); + $download->shouldNotBeCalled(); + + $this->geolocationDbUpdater->checkDbUpdate(); + } + + /** + * @param string|int $buildEpoch + */ + private function buildMetaWithBuildEpoch($buildEpoch): Metadata + { + return new Metadata([ + 'binary_format_major_version' => '', + 'binary_format_minor_version' => '', + 'build_epoch' => $buildEpoch, + 'database_type' => '', + 'languages' => '', + 'description' => '', + 'ip_version' => '', + 'node_count' => 1, + 'record_size' => 4, + ]); } } diff --git a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php index fdb5bfec..4d348528 100644 --- a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php +++ b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php @@ -169,7 +169,7 @@ class LocateShortUrlVisitTest extends TestCase /** @test */ public function errorWhenUpdatingGeoLiteWithExistingCopyLogsWarning(): void { - $e = GeolocationDbUpdateFailedException::create(true); + $e = GeolocationDbUpdateFailedException::withOlderDb(); $ipAddr = '1.2.3.0'; $visit = new Visit(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr)); $location = new Location('', '', '', '', 0.0, 0.0, ''); @@ -200,7 +200,7 @@ class LocateShortUrlVisitTest extends TestCase /** @test */ public function errorWhenDownloadingGeoLiteCancelsLocation(): void { - $e = GeolocationDbUpdateFailedException::create(false); + $e = GeolocationDbUpdateFailedException::withoutOlderDb(); $ipAddr = '1.2.3.0'; $visit = new Visit(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr)); $location = new Location('', '', '', '', 0.0, 0.0, '');