From a77e07f906f88c342dca1f845f2a6944e9dfc299 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 15 Dec 2024 10:05:32 +0100 Subject: [PATCH] Refactor geolocation download logic based on database table --- .../Visit/DownloadGeoLiteDbCommand.php | 2 +- ...GeolocationDbUpdateFailedExceptionTest.php | 16 +-- module/Core/config/dependencies.config.php | 3 +- ...Geolocation.Entity.GeolocationDbUpdate.php | 6 - .../Core/migrations/Version20241212131058.php | 5 - .../src/EventDispatcher/UpdateGeoLiteDb.php | 1 + .../GeolocationDbUpdateFailedException.php | 40 ++----- .../Entity/GeolocationDbUpdate.php | 44 ++++++- .../src/Geolocation/GeolocationDbUpdater.php | 111 ++++++++++-------- .../Geolocation/GeolocationDbUpdaterTest.php | 4 +- 10 files changed, 117 insertions(+), 115 deletions(-) diff --git a/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php b/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php index b0a22c97..8e873b1d 100644 --- a/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php +++ b/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php @@ -66,7 +66,7 @@ class DownloadGeoLiteDbCommand extends Command implements GeolocationDownloadPro private function processGeoLiteUpdateError(GeolocationDbUpdateFailedException $e, SymfonyStyle $io): int { - $olderDbExists = $e->olderDbExists(); + $olderDbExists = $e->olderDbExists; if ($olderDbExists) { $io->warning( diff --git a/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php b/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php index a1d6db65..86ed9548 100644 --- a/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php +++ b/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php @@ -19,7 +19,7 @@ class GeolocationDbUpdateFailedExceptionTest extends TestCase { $e = GeolocationDbUpdateFailedException::withOlderDb($prev); - self::assertTrue($e->olderDbExists()); + self::assertTrue($e->olderDbExists); self::assertEquals( 'An error occurred while updating geolocation database, but an older DB is already present.', $e->getMessage(), @@ -33,7 +33,7 @@ class GeolocationDbUpdateFailedExceptionTest extends TestCase { $e = GeolocationDbUpdateFailedException::withoutOlderDb($prev); - self::assertFalse($e->olderDbExists()); + self::assertFalse($e->olderDbExists); self::assertEquals( 'An error occurred while updating geolocation database, and an older version could not be found.', $e->getMessage(), @@ -48,16 +48,4 @@ class GeolocationDbUpdateFailedExceptionTest extends TestCase 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/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index b16a4c5c..adc9ae2a 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -13,7 +13,6 @@ use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdater; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifier; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdater; -use Shlinkio\Shlink\IpGeolocation\GeoLite2\GeoLite2ReaderFactory; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; use Symfony\Component\Lock; @@ -247,9 +246,9 @@ return [ GeolocationDbUpdater::class => [ DbUpdater::class, - GeoLite2ReaderFactory::class, LOCAL_LOCK_FACTORY, Config\Options\TrackingOptions::class, + 'em', ], Geolocation\Middleware\IpGeolocationMiddleware::class => [ IpLocationResolverInterface::class, diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Geolocation.Entity.GeolocationDbUpdate.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Geolocation.Entity.GeolocationDbUpdate.php index ecf21fb2..20a67c98 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Geolocation.Entity.GeolocationDbUpdate.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Geolocation.Entity.GeolocationDbUpdate.php @@ -40,12 +40,6 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->length(128) ->build(); - fieldWithUtf8Charset($builder->createField('filename', Types::STRING), $emConfig) - ->columnName('filename') - ->length(512) - ->nullable() - ->build(); - fieldWithUtf8Charset($builder->createField('error', Types::STRING), $emConfig) ->columnName('error') ->length(1024) diff --git a/module/Core/migrations/Version20241212131058.php b/module/Core/migrations/Version20241212131058.php index 1ba4bc02..59286931 100644 --- a/module/Core/migrations/Version20241212131058.php +++ b/module/Core/migrations/Version20241212131058.php @@ -38,11 +38,6 @@ final class Version20241212131058 extends AbstractMigration ]); $table->addColumn('filesystem_id', Types::STRING, ['length' => 512]); - $table->addColumn('filename', Types::STRING, [ - 'length' => 512, - 'default' => null, - 'notnull' => false, - ]); $table->addColumn('error', Types::STRING, [ 'length' => 1024, 'default' => null, diff --git a/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php b/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php index 87108279..8d288959 100644 --- a/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php +++ b/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php @@ -14,6 +14,7 @@ use Throwable; use function sprintf; +/** @todo Rename to UpdateGeolocationDb */ readonly class UpdateGeoLiteDb { public function __construct( diff --git a/module/Core/src/Exception/GeolocationDbUpdateFailedException.php b/module/Core/src/Exception/GeolocationDbUpdateFailedException.php index f3c3f65f..a818b263 100644 --- a/module/Core/src/Exception/GeolocationDbUpdateFailedException.php +++ b/module/Core/src/Exception/GeolocationDbUpdateFailedException.php @@ -7,52 +7,28 @@ namespace Shlinkio\Shlink\Core\Exception; use RuntimeException; use Throwable; -use function sprintf; - class GeolocationDbUpdateFailedException extends RuntimeException implements ExceptionInterface { - private bool $olderDbExists; - - private function __construct(string $message, Throwable|null $previous = null) + private function __construct(string $message, public readonly bool $olderDbExists, Throwable|null $prev = null) { - parent::__construct($message, previous: $previous); + parent::__construct($message, previous: $prev); } public static function withOlderDb(Throwable|null $prev = null): self { - $e = new self( + return new self( 'An error occurred while updating geolocation database, but an older DB is already present.', - $prev, + olderDbExists: true, + prev: $prev, ); - $e->olderDbExists = true; - - return $e; } public static function withoutOlderDb(Throwable|null $prev = null): self { - $e = new self( + return new self( 'An error occurred while updating geolocation database, and an older version could not be found.', - $prev, + olderDbExists: false, + prev: $prev, ); - $e->olderDbExists = false; - - return $e; - } - - public static function withInvalidEpochInOldDb(mixed $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; - } - - public function olderDbExists(): bool - { - return $this->olderDbExists; } } diff --git a/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php b/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php index 0f376cb9..1e1d544f 100644 --- a/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php +++ b/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php @@ -16,27 +16,59 @@ class GeolocationDbUpdate extends AbstractEntity private GeolocationDbUpdateStatus $status = GeolocationDbUpdateStatus::IN_PROGRESS, private readonly Chronos $dateCreated = new Chronos(), private Chronos $dateUpdated = new Chronos(), - private string|null $filename = null, private string|null $error = null, ) { } - public static function createForCurrentFilesystem(): self + public static function forFilesystemId(string|null $filesystemId = null): self { - return new self(stat(__FILE__)['dev']); + return new self($filesystemId ?? self::currentFilesystemId()); } - public function finishSuccessfully(string $filename): void + public static function currentFilesystemId(): string + { + $system = stat(__FILE__); + if (! $system) { + // TODO Throw error + } + + return (string) $system['dev']; + } + + public function finishSuccessfully(): void { $this->dateUpdated = Chronos::now(); - $this->filename = $filename; $this->status = GeolocationDbUpdateStatus::SUCCESS; } public function finishWithError(string $error): void { - $this->dateUpdated = Chronos::now(); $this->error = $error; + $this->dateUpdated = Chronos::now(); $this->status = GeolocationDbUpdateStatus::ERROR; } + + /** + * This update would require a new download if: + * - It is successful and older than 30 days + * - It is error and older than 2 days + */ + public function needsUpdate(): bool + { + return match ($this->status) { + GeolocationDbUpdateStatus::SUCCESS => Chronos::now()->greaterThan($this->dateUpdated->addDays(30)), + GeolocationDbUpdateStatus::ERROR => Chronos::now()->greaterThan($this->dateUpdated->addDays(2)), + default => false, + }; + } + + public function isInProgress(): bool + { + return $this->status === GeolocationDbUpdateStatus::IN_PROGRESS; + } + + public function isError(): bool + { + return $this->status === GeolocationDbUpdateStatus::ERROR; + } } diff --git a/module/Core/src/Geolocation/GeolocationDbUpdater.php b/module/Core/src/Geolocation/GeolocationDbUpdater.php index 9c7d61d3..e5bd0ea1 100644 --- a/module/Core/src/Geolocation/GeolocationDbUpdater.php +++ b/module/Core/src/Geolocation/GeolocationDbUpdater.php @@ -4,36 +4,29 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Geolocation; -use Cake\Chronos\Chronos; -use Closure; -use GeoIp2\Database\Reader; -use MaxMind\Db\Reader\Metadata; +use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Config\Options\TrackingOptions; use Shlinkio\Shlink\Core\Exception\GeolocationDbUpdateFailedException; +use Shlinkio\Shlink\Core\Geolocation\Entity\GeolocationDbUpdate; use Shlinkio\Shlink\IpGeolocation\Exception\DbUpdateException; use Shlinkio\Shlink\IpGeolocation\Exception\MissingLicenseException; use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdaterInterface; use Symfony\Component\Lock\LockFactory; -use function is_int; +use function count; +use function Shlinkio\Shlink\Core\ArrayUtils\every; +use function sprintf; -class GeolocationDbUpdater implements GeolocationDbUpdaterInterface +readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface { private const string LOCK_NAME = 'geolocation-db-update'; - /** @var Closure(): Reader */ - private readonly Closure $geoLiteDbReaderFactory; - - /** - * @param callable(): Reader $geoLiteDbReaderFactory - */ public function __construct( - private readonly DbUpdaterInterface $dbUpdater, - callable $geoLiteDbReaderFactory, - private readonly LockFactory $locker, - private readonly TrackingOptions $trackingOptions, + private DbUpdaterInterface $dbUpdater, + private LockFactory $locker, + private TrackingOptions $trackingOptions, + private EntityManagerInterface $em, ) { - $this->geoLiteDbReaderFactory = $geoLiteDbReaderFactory(...); } /** @@ -46,6 +39,7 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface return GeolocationResult::CHECK_SKIPPED; } + $lock = $this->locker->createLock(self::LOCK_NAME); $lock->acquire(blocking: true); @@ -62,43 +56,68 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface private function downloadIfNeeded( GeolocationDownloadProgressHandlerInterface|null $downloadProgressHandler, ): GeolocationResult { - if (! $this->dbUpdater->databaseFileExists()) { - return $this->downloadNewDb($downloadProgressHandler, olderDbExists: false); + $maxRecentAttemptsToCheck = 15; // TODO Make this configurable + + // Get last 15 download attempts + $recentDownloads = $this->em->getRepository(GeolocationDbUpdate::class)->findBy( + criteria: ['filesystemId' => GeolocationDbUpdate::currentFilesystemId()], + orderBy: ['dateUpdated' => 'DESC'], + limit: $maxRecentAttemptsToCheck, + ); + $mostRecentDownload = $recentDownloads[0] ?? null; + $amountOfRecentAttempts = count($recentDownloads); + + // If most recent attempt is in progress, skip check. + // This is a safety check in case the lock is released before the previous download has finished. + if ($mostRecentDownload?->isInProgress()) { + return GeolocationResult::CHECK_SKIPPED; } - $meta = ($this->geoLiteDbReaderFactory)()->metadata(); - if ($this->buildIsTooOld($meta)) { - return $this->downloadNewDb($downloadProgressHandler, olderDbExists: true); + // If all recent attempts are errors, and the most recent one is not old enough, skip download + if ( + $amountOfRecentAttempts === $maxRecentAttemptsToCheck + && every($recentDownloads, static fn (GeolocationDbUpdate $update) => $update->isError()) + && ! $mostRecentDownload->needsUpdate() + ) { + return GeolocationResult::CHECK_SKIPPED; + } + + // Try to download if there are no attempts, the database file does not exist or most recent attempt was + // successful and is old enough + $olderDbExists = $amountOfRecentAttempts > 0 && $this->dbUpdater->databaseFileExists(); + if (! $olderDbExists || $mostRecentDownload->needsUpdate()) { + return $this->downloadAndTrackUpdate($downloadProgressHandler, $olderDbExists); } return GeolocationResult::DB_IS_UP_TO_DATE; } - private function buildIsTooOld(Metadata $meta): bool - { - $buildTimestamp = $this->resolveBuildTimestamp($meta); - $buildDate = Chronos::createFromTimestamp($buildTimestamp); + /** + * @throws GeolocationDbUpdateFailedException + */ + private function downloadAndTrackUpdate( + GeolocationDownloadProgressHandlerInterface|null $downloadProgressHandler, + bool $olderDbExists, + ): GeolocationResult { + $dbUpdate = GeolocationDbUpdate::forFilesystemId(); + $this->em->persist($dbUpdate); + $this->em->flush(); - return Chronos::now()->greaterThan($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; + try { + $result = $this->downloadNewDb($downloadProgressHandler, $olderDbExists); + $dbUpdate->finishSuccessfully(); + return $result; + } catch (MissingLicenseException) { + $dbUpdate->finishWithError('Geolocation license key is missing'); + return GeolocationResult::LICENSE_MISSING; + } catch (GeolocationDbUpdateFailedException $e) { + $dbUpdate->finishWithError( + sprintf('%s. Prev: %s', $e->getMessage(), $e->getPrevious()?->getMessage() ?? '-'), + ); + throw $e; + } finally { + $this->em->flush(); } - - $intBuildEpoch = (int) $buildEpoch; - if ($buildEpoch === (string) $intBuildEpoch) { - return $intBuildEpoch; - } - - throw GeolocationDbUpdateFailedException::withInvalidEpochInOldDb($buildEpoch); } /** @@ -116,8 +135,6 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface => $downloadProgressHandler?->handleProgress($total, $downloaded, $olderDbExists), ); return $olderDbExists ? GeolocationResult::DB_UPDATED : GeolocationResult::DB_CREATED; - } catch (MissingLicenseException) { - return GeolocationResult::LICENSE_MISSING; } catch (DbUpdateException $e) { throw $olderDbExists ? GeolocationDbUpdateFailedException::withOlderDb($e) diff --git a/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php b/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php index 5c76747b..d2ec1bfa 100644 --- a/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php +++ b/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php @@ -91,7 +91,7 @@ class GeolocationDbUpdaterTest extends TestCase } catch (Throwable $e) { self::assertInstanceOf(GeolocationDbUpdateFailedException::class, $e); self::assertSame($prev, $e->getPrevious()); - self::assertFalse($e->olderDbExists()); + self::assertFalse($e->olderDbExists); self::assertTrue($this->progressHandler->beforeDownloadCalled); } } @@ -114,7 +114,7 @@ class GeolocationDbUpdaterTest extends TestCase } catch (Throwable $e) { self::assertInstanceOf(GeolocationDbUpdateFailedException::class, $e); self::assertSame($prev, $e->getPrevious()); - self::assertTrue($e->olderDbExists()); + self::assertTrue($e->olderDbExists); } }