From d4d97c3182e8fd0910836114edf33247a3bfca29 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 13 Dec 2024 10:33:53 +0100 Subject: [PATCH 1/7] Create new table to track geolocation updates --- ...Geolocation.Entity.GeolocationDbUpdate.php | 64 +++++++++++++++++ .../Core/migrations/Version20241212131058.php | 68 +++++++++++++++++++ .../Entity/GeolocationDbUpdate.php | 42 ++++++++++++ .../Entity/GeolocationDbUpdateStatus.php | 10 +++ 4 files changed, 184 insertions(+) create mode 100644 module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Geolocation.Entity.GeolocationDbUpdate.php create mode 100644 module/Core/migrations/Version20241212131058.php create mode 100644 module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php create mode 100644 module/Core/src/Geolocation/Entity/GeolocationDbUpdateStatus.php 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 new file mode 100644 index 00000000..ecf21fb2 --- /dev/null +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Geolocation.Entity.GeolocationDbUpdate.php @@ -0,0 +1,64 @@ +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('filename', Types::STRING), $emConfig) + ->columnName('filename') + ->length(512) + ->nullable() + ->build(); + + fieldWithUtf8Charset($builder->createField('error', Types::STRING), $emConfig) + ->columnName('error') + ->length(1024) + ->nullable() + ->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 status and filesystem_id, as we'll usually filter the query by those fields + $builder->addIndex(['status', 'filesystem_id'], 'IDX_geolocation_status_filesystem'); +}; diff --git a/module/Core/migrations/Version20241212131058.php b/module/Core/migrations/Version20241212131058.php new file mode 100644 index 00000000..1ba4bc02 --- /dev/null +++ b/module/Core/migrations/Version20241212131058.php @@ -0,0 +1,68 @@ +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('filename', Types::STRING, [ + 'length' => 512, + 'default' => null, + 'notnull' => false, + ]); + $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 status and filesystem_id, as we'll usually filter the query by those fields + $table->addIndex(['status', '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); + } +} diff --git a/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php b/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php new file mode 100644 index 00000000..0f376cb9 --- /dev/null +++ b/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php @@ -0,0 +1,42 @@ +dateUpdated = Chronos::now(); + $this->filename = $filename; + $this->status = GeolocationDbUpdateStatus::SUCCESS; + } + + public function finishWithError(string $error): void + { + $this->dateUpdated = Chronos::now(); + $this->error = $error; + $this->status = GeolocationDbUpdateStatus::ERROR; + } +} diff --git a/module/Core/src/Geolocation/Entity/GeolocationDbUpdateStatus.php b/module/Core/src/Geolocation/Entity/GeolocationDbUpdateStatus.php new file mode 100644 index 00000000..8216f2bd --- /dev/null +++ b/module/Core/src/Geolocation/Entity/GeolocationDbUpdateStatus.php @@ -0,0 +1,10 @@ + Date: Sun, 15 Dec 2024 10:05:32 +0100 Subject: [PATCH 2/7] 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); } } From f10a9d3972a295c02e0246eb600a1e4e1518c84b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 15 Dec 2024 10:08:22 +0100 Subject: [PATCH 3/7] Simplify geolocation_db_updates indexes --- ...kio.Shlink.Core.Geolocation.Entity.GeolocationDbUpdate.php | 4 ++-- module/Core/migrations/Version20241212131058.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) 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 20a67c98..e7f10ca1 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 @@ -53,6 +53,6 @@ return static function (ClassMetadata $metadata, array $emConfig): void { // Index on date_updated, as we'll usually sort the query by this field $builder->addIndex(['date_updated'], 'IDX_geolocation_date_updated'); - // Index on status and filesystem_id, as we'll usually filter the query by those fields - $builder->addIndex(['status', 'filesystem_id'], 'IDX_geolocation_status_filesystem'); + // Index on filesystem_id, as we'll usually filter the query by this field + $builder->addIndex(['filesystem_id'], 'IDX_geolocation_status_filesystem'); }; diff --git a/module/Core/migrations/Version20241212131058.php b/module/Core/migrations/Version20241212131058.php index 59286931..e27958f6 100644 --- a/module/Core/migrations/Version20241212131058.php +++ b/module/Core/migrations/Version20241212131058.php @@ -46,8 +46,8 @@ final class Version20241212131058 extends AbstractMigration // Index on date_updated, as we'll usually sort the query by this field $table->addIndex(['date_updated'], 'IDX_geolocation_date_updated'); - // Index on status and filesystem_id, as we'll usually filter the query by those fields - $table->addIndex(['status', 'filesystem_id'], 'IDX_geolocation_status_filesystem'); + // 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 From 853c50a81974851b68c46f95e457ee0d782a8725 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 15 Dec 2024 11:34:38 +0100 Subject: [PATCH 4/7] Fix some cases of database download in GeolocationDbUpdater --- .../Visit/DownloadGeoLiteDbCommand.php | 5 +++ .../Visit/DownloadGeoLiteDbCommandTest.php | 11 ++--- .../Entity/GeolocationDbUpdate.php | 17 ++++--- .../src/Geolocation/GeolocationDbUpdater.php | 45 +++++++++++-------- .../src/Geolocation/GeolocationResult.php | 1 + 5 files changed, 46 insertions(+), 33 deletions(-) diff --git a/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php b/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php index 8e873b1d..e1ea5fce 100644 --- a/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php +++ b/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php @@ -51,6 +51,11 @@ class DownloadGeoLiteDbCommand extends Command implements GeolocationDownloadPro 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 ($this->progressBar === null) { $this->io->info('GeoLite2 db file is up to date.'); } else { diff --git a/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php b/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php index 7fa46a05..f232bdca 100644 --- a/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php +++ b/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php @@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\CLI\Command\Visit; 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,17 @@ class DownloadGeoLiteDbCommandTest extends TestCase } #[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'])] + 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); } diff --git a/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php b/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php index 1e1d544f..35a9697d 100644 --- a/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php +++ b/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php @@ -49,17 +49,11 @@ class GeolocationDbUpdate extends AbstractEntity } /** - * This update would require a new download if: - * - It is successful and older than 30 days - * - It is error and older than 2 days + * @param positive-int $days */ - public function needsUpdate(): bool + public function isOlderThan(int $days): 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, - }; + return Chronos::now()->greaterThan($this->dateUpdated->addDays($days)); } public function isInProgress(): bool @@ -71,4 +65,9 @@ class GeolocationDbUpdate extends AbstractEntity { return $this->status === GeolocationDbUpdateStatus::ERROR; } + + public function isSuccess(): bool + { + return $this->status === GeolocationDbUpdateStatus::SUCCESS; + } } diff --git a/module/Core/src/Geolocation/GeolocationDbUpdater.php b/module/Core/src/Geolocation/GeolocationDbUpdater.php index e5bd0ea1..f6ca7b49 100644 --- a/module/Core/src/Geolocation/GeolocationDbUpdater.php +++ b/module/Core/src/Geolocation/GeolocationDbUpdater.php @@ -13,8 +13,6 @@ use Shlinkio\Shlink\IpGeolocation\Exception\MissingLicenseException; use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdaterInterface; use Symfony\Component\Lock\LockFactory; -use function count; -use function Shlinkio\Shlink\Core\ArrayUtils\every; use function sprintf; readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface @@ -26,6 +24,7 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface private LockFactory $locker, private TrackingOptions $trackingOptions, private EntityManagerInterface $em, + private int $maxRecentAttemptsToCheck = 15, // TODO Make this configurable ) { } @@ -56,16 +55,12 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface private function downloadIfNeeded( GeolocationDownloadProgressHandlerInterface|null $downloadProgressHandler, ): GeolocationResult { - $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, + limit: $this->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. @@ -73,19 +68,31 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface return GeolocationResult::CHECK_SKIPPED; } - // 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; + $amountOfErrorsSinceLastSuccess = 0; + foreach ($recentDownloads as $recentDownload) { + // Count attempts until a success is found + if ($recentDownload->isSuccess()) { + break; + } + $amountOfErrorsSinceLastSuccess++; } - // 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()) { + // If max amount of consecutive errors has been reached and the most recent one is not old enough, skip download + // for 2 days to avoid hitting potential API limits in geolocation services + $lastAttemptIsError = $mostRecentDownload !== null && $mostRecentDownload->isError(); + // FIXME Once max errors are reached there will be one attempt every 2 days, but it should be 15 attempts every + // 2 days. Leaving like this for simplicity for now. + $maxConsecutiveErrorsReached = $amountOfErrorsSinceLastSuccess === $this->maxRecentAttemptsToCheck; + if ($lastAttemptIsError && $maxConsecutiveErrorsReached && ! $mostRecentDownload->isOlderThan(days: 2)) { + return GeolocationResult::MAX_ERRORS_REACHED; + } + + // Try to download if: + // - There are no attempts or the database file does not exist + // - Last update errored (and implicitly, the max amount of consecutive errors has not been reached) + // - Most recent attempt is older than 30 days (and implicitly, successful) + $olderDbExists = $mostRecentDownload !== null && $this->dbUpdater->databaseFileExists(); + if (! $olderDbExists || $lastAttemptIsError || $mostRecentDownload->isOlderThan(days: 30)) { return $this->downloadAndTrackUpdate($downloadProgressHandler, $olderDbExists); } @@ -112,7 +119,7 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface return GeolocationResult::LICENSE_MISSING; } catch (GeolocationDbUpdateFailedException $e) { $dbUpdate->finishWithError( - sprintf('%s. Prev: %s', $e->getMessage(), $e->getPrevious()?->getMessage() ?? '-'), + sprintf('%s Prev: %s', $e->getMessage(), $e->getPrevious()?->getMessage() ?? '-'), ); throw $e; } finally { diff --git a/module/Core/src/Geolocation/GeolocationResult.php b/module/Core/src/Geolocation/GeolocationResult.php index 3b472d09..1eecedda 100644 --- a/module/Core/src/Geolocation/GeolocationResult.php +++ b/module/Core/src/Geolocation/GeolocationResult.php @@ -5,6 +5,7 @@ namespace Shlinkio\Shlink\Core\Geolocation; enum GeolocationResult { case CHECK_SKIPPED; + case MAX_ERRORS_REACHED; case LICENSE_MISSING; case DB_CREATED; case DB_UPDATED; From 72a962ec6d00dc348b588c6b9fdac33e7a0c6aa6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 15 Dec 2024 12:03:01 +0100 Subject: [PATCH 5/7] Handle differently when trying to update geolocation and already in progress --- module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php | 5 +++++ .../test/Command/Visit/DownloadGeoLiteDbCommandTest.php | 1 + module/Core/src/Geolocation/GeolocationDbUpdater.php | 6 +++++- module/Core/src/Geolocation/GeolocationResult.php | 8 ++++++++ 4 files changed, 19 insertions(+), 1 deletion(-) diff --git a/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php b/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php index e1ea5fce..cd7c4ffb 100644 --- a/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php +++ b/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php @@ -56,6 +56,11 @@ class DownloadGeoLiteDbCommand extends Command implements GeolocationDownloadPro 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 { diff --git a/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php b/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php index f232bdca..01322f0b 100644 --- a/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php +++ b/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php @@ -77,6 +77,7 @@ class DownloadGeoLiteDbCommandTest extends TestCase #[Test] #[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($result); diff --git a/module/Core/src/Geolocation/GeolocationDbUpdater.php b/module/Core/src/Geolocation/GeolocationDbUpdater.php index f6ca7b49..662e6b7a 100644 --- a/module/Core/src/Geolocation/GeolocationDbUpdater.php +++ b/module/Core/src/Geolocation/GeolocationDbUpdater.php @@ -12,6 +12,7 @@ use Shlinkio\Shlink\IpGeolocation\Exception\DbUpdateException; use Shlinkio\Shlink\IpGeolocation\Exception\MissingLicenseException; use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdaterInterface; use Symfony\Component\Lock\LockFactory; +use Throwable; use function sprintf; @@ -65,7 +66,7 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface // 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; + return GeolocationResult::UPDATE_IN_PROGRESS; } $amountOfErrorsSinceLastSuccess = 0; @@ -122,6 +123,9 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface sprintf('%s Prev: %s', $e->getMessage(), $e->getPrevious()?->getMessage() ?? '-'), ); throw $e; + } catch (Throwable $e) { + $dbUpdate->finishWithError(sprintf('Unknown error: %s', $e->getMessage())); + throw $e; } finally { $this->em->flush(); } diff --git a/module/Core/src/Geolocation/GeolocationResult.php b/module/Core/src/Geolocation/GeolocationResult.php index 1eecedda..eb5b4a5c 100644 --- a/module/Core/src/Geolocation/GeolocationResult.php +++ b/module/Core/src/Geolocation/GeolocationResult.php @@ -4,10 +4,18 @@ namespace Shlinkio\Shlink\Core\Geolocation; enum GeolocationResult { + /** Geolocation is not relevant, so updates are skipped */ case CHECK_SKIPPED; + /** Update is skipped because max amount of consecutive errors was reached */ case MAX_ERRORS_REACHED; + /** Update was skipped because a geolocation license key was not provided */ case LICENSE_MISSING; + /** A geolocation database didn't exist and has been created */ case DB_CREATED; + /** An outdated geolocation database existed and has been updated */ case DB_UPDATED; + /** Geolocation database does not need to be updated yet */ case DB_IS_UP_TO_DATE; + /** Geolocation db update is currently in progress */ + case UPDATE_IN_PROGRESS; } From e715a0fb6f15a9132d58559ba95ebab1664b9cc6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 16 Dec 2024 09:23:30 +0100 Subject: [PATCH 6/7] Track reason for which a geolocation db download was attempted --- ...Geolocation.Entity.GeolocationDbUpdate.php | 5 +++++ .../Core/migrations/Version20241212131058.php | 1 + .../Entity/GeolocationDbUpdate.php | 5 +++-- .../src/Geolocation/GeolocationDbUpdater.php | 19 ++++++++++++++----- 4 files changed, 23 insertions(+), 7 deletions(-) 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 e7f10ca1..7b60abdc 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 @@ -46,6 +46,11 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->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) diff --git a/module/Core/migrations/Version20241212131058.php b/module/Core/migrations/Version20241212131058.php index e27958f6..23e61803 100644 --- a/module/Core/migrations/Version20241212131058.php +++ b/module/Core/migrations/Version20241212131058.php @@ -38,6 +38,7 @@ final class Version20241212131058 extends AbstractMigration ]); $table->addColumn('filesystem_id', Types::STRING, ['length' => 512]); + $table->addColumn('reason', Types::STRING, ['length' => 1024]); $table->addColumn('error', Types::STRING, [ 'length' => 1024, 'default' => null, diff --git a/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php b/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php index 35a9697d..42cdfa4b 100644 --- a/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php +++ b/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php @@ -13,6 +13,7 @@ class GeolocationDbUpdate extends AbstractEntity { private function __construct( private readonly string $filesystemId, + private readonly string $reason, private GeolocationDbUpdateStatus $status = GeolocationDbUpdateStatus::IN_PROGRESS, private readonly Chronos $dateCreated = new Chronos(), private Chronos $dateUpdated = new Chronos(), @@ -20,9 +21,9 @@ class GeolocationDbUpdate extends AbstractEntity ) { } - public static function forFilesystemId(string|null $filesystemId = null): self + public static function withReason(string $reason, string|null $filesystemId = null): self { - return new self($filesystemId ?? self::currentFilesystemId()); + return new self($reason, $filesystemId ?? self::currentFilesystemId()); } public static function currentFilesystemId(): string diff --git a/module/Core/src/Geolocation/GeolocationDbUpdater.php b/module/Core/src/Geolocation/GeolocationDbUpdater.php index 662e6b7a..7be1cd56 100644 --- a/module/Core/src/Geolocation/GeolocationDbUpdater.php +++ b/module/Core/src/Geolocation/GeolocationDbUpdater.php @@ -89,12 +89,20 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface } // Try to download if: - // - There are no attempts or the database file does not exist + // - There are no attempts tracked + // - The database file does not exist // - Last update errored (and implicitly, the max amount of consecutive errors has not been reached) // - Most recent attempt is older than 30 days (and implicitly, successful) - $olderDbExists = $mostRecentDownload !== null && $this->dbUpdater->databaseFileExists(); - if (! $olderDbExists || $lastAttemptIsError || $mostRecentDownload->isOlderThan(days: 30)) { - return $this->downloadAndTrackUpdate($downloadProgressHandler, $olderDbExists); + $reasonMatch = match (true) { + $mostRecentDownload === null => [false, 'No download attempts tracked for this instance'], + $this->dbUpdater->databaseFileExists() => [false, 'Geolocation db file does not exist'], + $lastAttemptIsError => [true, 'Max consecutive errors not reached'], + $mostRecentDownload->isOlderThan(days: 30) => [true, 'Last successful attempt'], + default => null, + }; + if ($reasonMatch !== null) { + [$olderDbExists, $reason] = $reasonMatch; + return $this->downloadAndTrackUpdate($downloadProgressHandler, $olderDbExists, $reason); } return GeolocationResult::DB_IS_UP_TO_DATE; @@ -106,8 +114,9 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface private function downloadAndTrackUpdate( GeolocationDownloadProgressHandlerInterface|null $downloadProgressHandler, bool $olderDbExists, + string $reason, ): GeolocationResult { - $dbUpdate = GeolocationDbUpdate::forFilesystemId(); + $dbUpdate = GeolocationDbUpdate::withReason($reason); $this->em->persist($dbUpdate); $this->em->flush(); From 509ef668e6f9664f0edd32d60f2cad9af2b96745 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 16 Dec 2024 19:50:06 +0100 Subject: [PATCH 7/7] Fix GeolocationDbUpdater test --- CHANGELOG.md | 9 +- .../Entity/GeolocationDbUpdate.php | 15 +- .../src/Geolocation/GeolocationDbUpdater.php | 4 +- .../Geolocation/GeolocationDbUpdaterTest.php | 202 +++++++++++++----- 4 files changed, 161 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 12b5aae4..fb43ec7e 100644 --- a/CHANGELOG.md +++ b/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* diff --git a/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php b/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php index 42cdfa4b..f3735a64 100644 --- a/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php +++ b/module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php @@ -6,14 +6,15 @@ namespace Shlinkio\Shlink\Core\Geolocation\Entity; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Common\Entity\AbstractEntity; +use Shlinkio\Shlink\Core\Exception\RuntimeException; use function stat; class GeolocationDbUpdate extends AbstractEntity { private function __construct( + public readonly string $reason, private readonly string $filesystemId, - private readonly string $reason, private GeolocationDbUpdateStatus $status = GeolocationDbUpdateStatus::IN_PROGRESS, private readonly Chronos $dateCreated = new Chronos(), private Chronos $dateUpdated = new Chronos(), @@ -21,32 +22,34 @@ class GeolocationDbUpdate extends AbstractEntity ) { } - public static function withReason(string $reason, string|null $filesystemId = null): self + public static function withReason(string $reason): self { - return new self($reason, $filesystemId ?? self::currentFilesystemId()); + return new self($reason, self::currentFilesystemId()); } public static function currentFilesystemId(): string { $system = stat(__FILE__); if (! $system) { - // TODO Throw error + throw new RuntimeException('It was not possible to resolve filesystem ID via stat function'); } return (string) $system['dev']; } - public function finishSuccessfully(): void + public function finishSuccessfully(): self { $this->dateUpdated = Chronos::now(); $this->status = GeolocationDbUpdateStatus::SUCCESS; + return $this; } - public function finishWithError(string $error): void + public function finishWithError(string $error): self { $this->error = $error; $this->dateUpdated = Chronos::now(); $this->status = GeolocationDbUpdateStatus::ERROR; + return $this; } /** diff --git a/module/Core/src/Geolocation/GeolocationDbUpdater.php b/module/Core/src/Geolocation/GeolocationDbUpdater.php index 7be1cd56..9e63cf5c 100644 --- a/module/Core/src/Geolocation/GeolocationDbUpdater.php +++ b/module/Core/src/Geolocation/GeolocationDbUpdater.php @@ -95,9 +95,9 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface // - Most recent attempt is older than 30 days (and implicitly, successful) $reasonMatch = match (true) { $mostRecentDownload === null => [false, 'No download attempts tracked for this instance'], - $this->dbUpdater->databaseFileExists() => [false, 'Geolocation db file does not exist'], + ! $this->dbUpdater->databaseFileExists() => [false, 'Geolocation db file does not exist'], $lastAttemptIsError => [true, 'Max consecutive errors not reached'], - $mostRecentDownload->isOlderThan(days: 30) => [true, 'Last successful attempt'], + $mostRecentDownload->isOlderThan(days: 30) => [true, 'Last successful attempt is old enough'], default => null, }; if ($reasonMatch !== null) { diff --git a/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php b/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php index d2ec1bfa..c2983030 100644 --- a/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php +++ b/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php @@ -6,14 +6,16 @@ namespace ShlinkioTest\Shlink\Core\Geolocation; use Cake\Chronos\Chronos; use Closure; -use GeoIp2\Database\Reader; -use MaxMind\Db\Reader\Metadata; +use Doctrine\ORM\EntityManagerInterface; +use Doctrine\ORM\EntityRepository; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +use RuntimeException; use Shlinkio\Shlink\Core\Config\Options\TrackingOptions; use Shlinkio\Shlink\Core\Exception\GeolocationDbUpdateFailedException; +use Shlinkio\Shlink\Core\Geolocation\Entity\GeolocationDbUpdate; use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdater; use Shlinkio\Shlink\Core\Geolocation\GeolocationDownloadProgressHandlerInterface; use Shlinkio\Shlink\Core\Geolocation\GeolocationResult; @@ -29,17 +31,24 @@ use function range; class GeolocationDbUpdaterTest extends TestCase { private MockObject & DbUpdaterInterface $dbUpdater; - private MockObject & Reader $geoLiteDbReader; private MockObject & Lock\LockInterface $lock; + private MockObject & EntityManagerInterface $em; + /** @var MockObject&EntityRepository */ + private MockObject & EntityRepository $repo; /** @var GeolocationDownloadProgressHandlerInterface&object{beforeDownloadCalled: bool, handleProgressCalled: bool} */ private GeolocationDownloadProgressHandlerInterface $progressHandler; protected function setUp(): void { $this->dbUpdater = $this->createMock(DbUpdaterInterface::class); - $this->geoLiteDbReader = $this->createMock(Reader::class); + $this->lock = $this->createMock(Lock\SharedLockInterface::class); $this->lock->method('acquire')->with($this->isTrue())->willReturn(true); + + $this->em = $this->createMock(EntityManagerInterface::class); + $this->repo = $this->createMock(EntityRepository::class); + $this->em->method('getRepository')->willReturn($this->repo); + $this->progressHandler = new class implements GeolocationDownloadProgressHandlerInterface { public function __construct( public bool $beforeDownloadCalled = false, @@ -59,6 +68,32 @@ class GeolocationDbUpdaterTest extends TestCase }; } + #[Test] + public function properResultIsReturnedIfMostRecentUpdateIsInProgress(): void + { + $this->repo->expects($this->once())->method('findBy')->willReturn([GeolocationDbUpdate::withReason('')]); + $this->dbUpdater->expects($this->never())->method('databaseFileExists'); + + $result = $this->geolocationDbUpdater()->checkDbUpdate(); + + self::assertEquals(GeolocationResult::UPDATE_IN_PROGRESS, $result); + } + + #[Test] + public function properResultIsReturnedIfMaxConsecutiveErrorsAreReached(): void + { + $this->repo->expects($this->once())->method('findBy')->willReturn([ + GeolocationDbUpdate::withReason('')->finishWithError(''), + GeolocationDbUpdate::withReason('')->finishWithError(''), + GeolocationDbUpdate::withReason('')->finishWithError(''), + ]); + $this->dbUpdater->expects($this->never())->method('databaseFileExists'); + + $result = $this->geolocationDbUpdater()->checkDbUpdate(); + + self::assertEquals(GeolocationResult::MAX_ERRORS_REACHED, $result); + } + #[Test] public function properResultIsReturnedWhenLicenseIsMissing(): void { @@ -66,7 +101,9 @@ class GeolocationDbUpdaterTest extends TestCase $this->dbUpdater->expects($this->once())->method('downloadFreshCopy')->willThrowException( new MissingLicenseException(''), ); - $this->geoLiteDbReader->expects($this->never())->method('metadata'); + $this->repo->expects($this->once())->method('findBy')->willReturn([ + GeolocationDbUpdate::withReason('')->finishSuccessfully(), + ]); $result = $this->geolocationDbUpdater()->checkDbUpdate($this->progressHandler); @@ -74,16 +111,19 @@ class GeolocationDbUpdaterTest extends TestCase self::assertEquals(GeolocationResult::LICENSE_MISSING, $result); } - #[Test] - public function exceptionIsThrownWhenOlderDbDoesNotExistAndDownloadFails(): void + #[Test, DataProvider('provideDbDoesNotExist')] + public function exceptionIsThrownWhenOlderDbDoesNotExistAndDownloadFails(Closure $setUp): void { $prev = new DbUpdateException(''); - $this->dbUpdater->expects($this->once())->method('databaseFileExists')->willReturn(false); + $expectedReason = $setUp($this); + $this->dbUpdater->expects($this->once())->method('downloadFreshCopy')->with( $this->isInstanceOf(Closure::class), )->willThrowException($prev); - $this->geoLiteDbReader->expects($this->never())->method('metadata'); + $this->em->expects($this->once())->method('persist')->with($this->callback( + fn (GeolocationDbUpdate $newUpdate): bool => $newUpdate->reason === $expectedReason, + )); try { $this->geolocationDbUpdater()->checkDbUpdate($this->progressHandler); @@ -96,17 +136,31 @@ class GeolocationDbUpdaterTest extends TestCase } } + public static function provideDbDoesNotExist(): iterable + { + yield 'file does not exist' => [function (self $test): string { + $test->repo->expects($test->once())->method('findBy')->willReturn([ + GeolocationDbUpdate::withReason('')->finishSuccessfully(), + ]); + $test->dbUpdater->expects($test->once())->method('databaseFileExists')->willReturn(false); + return 'Geolocation db file does not exist'; + }]; + yield 'no attempts' => [function (self $test): string { + $test->repo->expects($test->once())->method('findBy')->willReturn([]); + $test->dbUpdater->expects($test->never())->method('databaseFileExists'); + return 'No download attempts tracked for this instance'; + }]; + } + #[Test, DataProvider('provideBigDays')] - public function exceptionIsThrownWhenOlderDbIsTooOldAndDownloadFails(int $days): void + public function exceptionIsThrownWhenOlderDbIsOldEnoughAndDownloadFails(int $days): void { $prev = new DbUpdateException(''); $this->dbUpdater->expects($this->once())->method('databaseFileExists')->willReturn(true); $this->dbUpdater->expects($this->once())->method('downloadFreshCopy')->with( $this->isInstanceOf(Closure::class), )->willThrowException($prev); - $this->geoLiteDbReader->expects($this->once())->method('metadata')->with()->willReturn( - $this->buildMetaWithBuildEpoch(Chronos::now()->subDays($days)->getTimestamp()), - ); + $this->repo->expects($this->once())->method('findBy')->willReturn([self::createFinishedOldUpdate($days)]); try { $this->geolocationDbUpdater()->checkDbUpdate(); @@ -120,74 +174,109 @@ class GeolocationDbUpdaterTest extends TestCase public static function provideBigDays(): iterable { - yield [36]; + yield [31]; yield [50]; yield [75]; yield [100]; } - #[Test, DataProvider('provideSmallDays')] - public function databaseIsNotUpdatedIfItIsNewEnough(string|int $buildEpoch): void + #[Test] + public function exceptionIsThrownWhenUnknownErrorHappens(): void + { + $this->dbUpdater->expects($this->once())->method('downloadFreshCopy')->with( + $this->isInstanceOf(Closure::class), + )->willThrowException(new RuntimeException('An error occurred')); + + $newUpdate = null; + $this->em->expects($this->once())->method('persist')->with($this->callback( + function (GeolocationDbUpdate $u) use (&$newUpdate): bool { + $newUpdate = $u; + return true; + }, + )); + + try { + $this->geolocationDbUpdater()->checkDbUpdate($this->progressHandler); + self::fail(); + } catch (Throwable) { + } + + self::assertTrue($this->progressHandler->beforeDownloadCalled); + self::assertNotNull($newUpdate); + self::assertTrue($newUpdate->isError()); + } + + #[Test, DataProvider('provideNotAldEnoughDays')] + public function databaseIsNotUpdatedIfItIsNewEnough(int $days): void { $this->dbUpdater->expects($this->once())->method('databaseFileExists')->willReturn(true); $this->dbUpdater->expects($this->never())->method('downloadFreshCopy'); - $this->geoLiteDbReader->expects($this->once())->method('metadata')->with()->willReturn( - $this->buildMetaWithBuildEpoch($buildEpoch), - ); + $this->repo->expects($this->once())->method('findBy')->willReturn([self::createFinishedOldUpdate($days)]); $result = $this->geolocationDbUpdater()->checkDbUpdate(); self::assertEquals(GeolocationResult::DB_IS_UP_TO_DATE, $result); } - public static function provideSmallDays(): iterable + public static function provideNotAldEnoughDays(): iterable { - $generateParamsWithTimestamp = static function (int $days) { - $timestamp = Chronos::now()->subDays($days)->getTimestamp(); - return [$days % 2 === 0 ? $timestamp : (string) $timestamp]; - }; - - return array_map($generateParamsWithTimestamp, range(0, 34)); + return array_map(static fn (int $value) => [$value], range(0, 29)); } - #[Test] - public function exceptionIsThrownWhenCheckingExistingDatabaseWithInvalidBuildEpoch(): void - { - $this->dbUpdater->expects($this->once())->method('databaseFileExists')->willReturn(true); - $this->dbUpdater->expects($this->never())->method('downloadFreshCopy'); - $this->geoLiteDbReader->expects($this->once())->method('metadata')->with()->willReturn( - $this->buildMetaWithBuildEpoch('invalid'), - ); + #[Test, DataProvider('provideUpdatesThatWillDownload')] + public function properResultIsReturnedWhenDownloadSucceeds( + array $updates, + GeolocationResult $expectedResult, + string $expectedReason, + ): void { + $this->repo->expects($this->once())->method('findBy')->willReturn($updates); + $this->dbUpdater->method('databaseFileExists')->willReturn(true); + $this->dbUpdater->expects($this->once())->method('downloadFreshCopy'); + $this->em->expects($this->once())->method('persist')->with($this->callback( + fn (GeolocationDbUpdate $newUpdate): bool => $newUpdate->reason === $expectedReason, + )); - $this->expectException(GeolocationDbUpdateFailedException::class); - $this->expectExceptionMessage( - 'Build epoch with value "invalid" from existing geolocation database, could not be parsed to integer.', - ); + $result = $this->geolocationDbUpdater()->checkDbUpdate(); - $this->geolocationDbUpdater()->checkDbUpdate(); + self::assertEquals($expectedResult, $result); } - private function buildMetaWithBuildEpoch(string|int $buildEpoch): Metadata + public static function provideUpdatesThatWillDownload(): iterable { - 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, - ]); + yield 'no updates' => [[], GeolocationResult::DB_CREATED, 'No download attempts tracked for this instance']; + yield 'old successful update' => [ + [self::createFinishedOldUpdate(days: 31)], + GeolocationResult::DB_UPDATED, + 'Last successful attempt is old enough', + ]; + yield 'not enough errors' => [ + [self::createFinishedOldUpdate(days: 3, successful: false)], + GeolocationResult::DB_UPDATED, + 'Max consecutive errors not reached', + ]; + } + + public static function createFinishedOldUpdate(int $days, bool $successful = true): GeolocationDbUpdate + { + Chronos::setTestNow(Chronos::now()->subDays($days)); + $update = GeolocationDbUpdate::withReason(''); + if ($successful) { + $update->finishSuccessfully(); + } else { + $update->finishWithError(''); + } + Chronos::setTestNow(); + + return $update; } #[Test, DataProvider('provideTrackingOptions')] public function downloadDbIsSkippedIfTrackingIsDisabled(TrackingOptions $options): void { - $result = $this->geolocationDbUpdater($options)->checkDbUpdate(); $this->dbUpdater->expects($this->never())->method('databaseFileExists'); - $this->geoLiteDbReader->expects($this->never())->method('metadata'); + $this->em->expects($this->never())->method('getRepository'); + + $result = $this->geolocationDbUpdater($options)->checkDbUpdate(); self::assertEquals(GeolocationResult::CHECK_SKIPPED, $result); } @@ -204,11 +293,6 @@ class GeolocationDbUpdaterTest extends TestCase $locker = $this->createMock(Lock\LockFactory::class); $locker->method('createLock')->with($this->isType('string'))->willReturn($this->lock); - return new GeolocationDbUpdater( - $this->dbUpdater, - fn () => $this->geoLiteDbReader, - $locker, - $options ?? new TrackingOptions(), - ); + return new GeolocationDbUpdater($this->dbUpdater, $locker, $options ?? new TrackingOptions(), $this->em, 3); } }