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;