Refactor geolocation download logic based on database table

This commit is contained in:
Alejandro Celaya 2024-12-15 10:05:32 +01:00
parent d4d97c3182
commit a77e07f906
10 changed files with 117 additions and 115 deletions

View File

@ -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(

View File

@ -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(),
);
}
}

View File

@ -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,

View File

@ -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)

View File

@ -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,

View File

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

View File

@ -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;
}
}

View File

@ -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;
}
}

View File

@ -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));
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();
}
private function resolveBuildTimestamp(Metadata $meta): int
{
// In theory the buildEpoch should be an int, but it has been reported to come as a string.
// See https://github.com/shlinkio/shlink/issues/1002 for context
/** @var int|string $buildEpoch */
$buildEpoch = $meta->buildEpoch;
if (is_int($buildEpoch)) {
return $buildEpoch;
}
$intBuildEpoch = (int) $buildEpoch;
if ($buildEpoch === (string) $intBuildEpoch) {
return $intBuildEpoch;
}
throw GeolocationDbUpdateFailedException::withInvalidEpochInOldDb($buildEpoch);
}
/**
@ -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)

View File

@ -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);
}
}