Improve how geolocation DB files are downloaded/updated

CHANGELOG.md

@@ -15,13 +15,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
     This option effectively replaces the old `REDIRECT_APPEND_EXTRA_PATH` option, which is now deprecated and will be removed in Shlink 5.0.0
 
 ### Changed
-* * [#2281](https://github.com/shlinkio/shlink/issues/2281) Update docker image to PHP 8.4
+* [#2281](https://github.com/shlinkio/shlink/issues/2281) Update docker image to PHP 8.4
+* [#2124](https://github.com/shlinkio/shlink/issues/2124) Improve how Shlink decides if a GeoLite db file needs to be downloaded, and reduces the chances for API limits to be reached.
+
+    Now Shlink tracks all download attempts, and knows which of them failed and succeeded. This lets it know when was the last error or success, how many consecutive errors have happened, etc.
+
+    It also tracks now the reason for a download to be attempted, and the error that happened when one fails.
 
 ### Deprecated
 * *Nothing*
 
 ### Removed
-* * [#2247](https://github.com/shlinkio/shlink/issues/2247) Drop support for PHP 8.2
+* [#2247](https://github.com/shlinkio/shlink/issues/2247) Drop support for PHP 8.2
 
 ### Fixed
 * *Nothing*

module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php

@@ -51,6 +51,16 @@ protected function execute(InputInterface $input, OutputInterface $output): int
                 return ExitCode::EXIT_WARNING;
             }
 
+            if ($result === GeolocationResult::MAX_ERRORS_REACHED) {
+                $this->io->warning('Max consecutive errors reached. Cannot retry for a couple of days.');
+                return ExitCode::EXIT_WARNING;
+            }
+
+            if ($result === GeolocationResult::UPDATE_IN_PROGRESS) {
+                $this->io->warning('A geolocation db is already being downloaded by another process.');
+                return ExitCode::EXIT_WARNING;
+            }
+
             if ($this->progressBar === null) {
                 $this->io->info('GeoLite2 db file is up to date.');
             } else {
@@ -66,7 +76,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 
     private function processGeoLiteUpdateError(GeolocationDbUpdateFailedException $e, SymfonyStyle $io): int
     {
-        $olderDbExists = $e->olderDbExists();
+        $olderDbExists = $e->olderDbExists;
 
         if ($olderDbExists) {
             $io->warning(

module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php

@@ -6,6 +6,7 @@
 
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\Attributes\TestWith;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Shlinkio\Shlink\CLI\Command\Visit\DownloadGeoLiteDbCommand;
@@ -74,17 +75,18 @@ public static function provideFailureParams(): iterable
     }
 
     #[Test]
-    public function warningIsPrintedWhenLicenseIsMissing(): void
+    #[TestWith([GeolocationResult::LICENSE_MISSING, 'It was not possible to download GeoLite2 db'])]
+    #[TestWith([GeolocationResult::MAX_ERRORS_REACHED, 'Max consecutive errors reached'])]
+    #[TestWith([GeolocationResult::UPDATE_IN_PROGRESS, 'A geolocation db is already being downloaded'])]
+    public function warningIsPrintedForSomeResults(GeolocationResult $result, string $expectedWarningMessage): void
     {
-        $this->dbUpdater->expects($this->once())->method('checkDbUpdate')->withAnyParameters()->willReturn(
-            GeolocationResult::LICENSE_MISSING,
-        );
+        $this->dbUpdater->expects($this->once())->method('checkDbUpdate')->withAnyParameters()->willReturn($result);
 
         $this->commandTester->execute([]);
         $output = $this->commandTester->getDisplay();
         $exitCode = $this->commandTester->getStatusCode();
 
-        self::assertStringContainsString('[WARNING] It was not possible to download GeoLite2 db', $output);
+        self::assertStringContainsString('[WARNING] ' . $expectedWarningMessage, $output);
         self::assertSame(ExitCode::EXIT_WARNING, $exitCode);
     }
 

module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php

@@ -19,7 +19,7 @@ public function withOlderDbBuildsException(Throwable|null $prev): void
     {
         $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 @@ public function withoutOlderDbBuildsException(Throwable|null $prev): void
     {
         $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 @@ public static function providePrev(): iterable
         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(),
-        );
-    }
 }

module/Core/config/dependencies.config.php

@@ -13,7 +13,6 @@
 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 @@
 
         GeolocationDbUpdater::class => [
             DbUpdater::class,
-            GeoLite2ReaderFactory::class,
             LOCAL_LOCK_FACTORY,
             Config\Options\TrackingOptions::class,
+            'em',
         ],
         Geolocation\Middleware\IpGeolocationMiddleware::class => [
             IpLocationResolverInterface::class,

module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Geolocation.Entity.GeolocationDbUpdate.php

@@ -0,0 +1,63 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core;
+
+use Doctrine\DBAL\Types\Types;
+use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder;
+use Doctrine\ORM\Mapping\Builder\FieldBuilder;
+use Doctrine\ORM\Mapping\ClassMetadata;
+use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType;
+use Shlinkio\Shlink\Core\Geolocation\Entity\GeolocationDbUpdateStatus;
+
+return static function (ClassMetadata $metadata, array $emConfig): void {
+    $builder = new ClassMetadataBuilder($metadata);
+
+    $builder->setTable(determineTableName('geolocation_db_updates', $emConfig));
+
+    $builder->createField('id', Types::BIGINT)
+            ->columnName('id')
+            ->makePrimaryKey()
+            ->generatedValue('IDENTITY')
+            ->option('unsigned', true)
+            ->build();
+
+    $builder->createField('dateCreated', ChronosDateTimeType::CHRONOS_DATETIME)
+            ->columnName('date_created')
+            ->build();
+
+    $builder->createField('dateUpdated', ChronosDateTimeType::CHRONOS_DATETIME)
+            ->columnName('date_updated')
+            ->nullable()
+            ->build();
+
+    (new FieldBuilder($builder, [
+        'fieldName' => 'status',
+        'type' => Types::STRING,
+        'enumType' => GeolocationDbUpdateStatus::class,
+    ]))->columnName('status')
+       ->length(128)
+       ->build();
+
+    fieldWithUtf8Charset($builder->createField('error', Types::STRING), $emConfig)
+        ->columnName('error')
+        ->length(1024)
+        ->nullable()
+        ->build();
+
+    fieldWithUtf8Charset($builder->createField('reason', Types::STRING), $emConfig)
+        ->columnName('reason')
+        ->length(1024)
+        ->build();
+
+    fieldWithUtf8Charset($builder->createField('filesystemId', Types::STRING), $emConfig)
+        ->columnName('filesystem_id')
+        ->length(512)
+        ->build();
+
+    // Index on date_updated, as we'll usually sort the query by this field
+    $builder->addIndex(['date_updated'], 'IDX_geolocation_date_updated');
+    // Index on filesystem_id, as we'll usually filter the query by this field
+    $builder->addIndex(['filesystem_id'], 'IDX_geolocation_status_filesystem');
+};

module/Core/migrations/Version20241212131058.php

@@ -0,0 +1,64 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkMigrations;
+
+use Doctrine\DBAL\Platforms\MySQLPlatform;
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\DBAL\Types\Types;
+use Doctrine\Migrations\AbstractMigration;
+use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType;
+
+/**
+ * Create a new table to track geolocation db updates
+ */
+final class Version20241212131058 extends AbstractMigration
+{
+    private const string TABLE_NAME = 'geolocation_db_updates';
+
+    public function up(Schema $schema): void
+    {
+        $this->skipIf($schema->hasTable(self::TABLE_NAME));
+
+        $table = $schema->createTable(self::TABLE_NAME);
+        $table->addColumn('id', Types::BIGINT, [
+            'unsigned' => true,
+            'autoincrement' => true,
+            'notnull' => true,
+        ]);
+        $table->setPrimaryKey(['id']);
+
+        $table->addColumn('date_created', ChronosDateTimeType::CHRONOS_DATETIME, ['default' => 'CURRENT_TIMESTAMP']);
+        $table->addColumn('date_updated', ChronosDateTimeType::CHRONOS_DATETIME, ['default' => 'CURRENT_TIMESTAMP']);
+
+        $table->addColumn('status', Types::STRING, [
+            'length' => 128,
+            'default' => 'in-progress', // in-progress, success, error
+        ]);
+        $table->addColumn('filesystem_id', Types::STRING, ['length' => 512]);
+
+        $table->addColumn('reason', Types::STRING, ['length' => 1024]);
+        $table->addColumn('error', Types::STRING, [
+            'length' => 1024,
+            'default' => null,
+            'notnull' => false,
+        ]);
+
+        // Index on date_updated, as we'll usually sort the query by this field
+        $table->addIndex(['date_updated'], 'IDX_geolocation_date_updated');
+        // Index on filesystem_id, as we'll usually filter the query by this field
+        $table->addIndex(['filesystem_id'], 'IDX_geolocation_status_filesystem');
+    }
+
+    public function down(Schema $schema): void
+    {
+        $this->skipIf(! $schema->hasTable(self::TABLE_NAME));
+        $schema->dropTable(self::TABLE_NAME);
+    }
+
+    public function isTransactional(): bool
+    {
+        return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform);
+    }
+}

module/Core/src/EventDispatcher/UpdateGeoLiteDb.php

@@ -14,6 +14,7 @@
 
 use function sprintf;
 
+/** @todo Rename to UpdateGeolocationDb */
 readonly class UpdateGeoLiteDb
 {
     public function __construct(

module/Core/src/Exception/GeolocationDbUpdateFailedException.php

@@ -7,52 +7,28 @@
 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;
     }
 }