From b8ac9f3673c8e6252cb022a2bbbb3195728514ee Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 11 Dec 2024 08:27:50 +0100 Subject: [PATCH] Add more strict parameter for GeolocationDbUpdater --- .../Visit/DownloadGeoLiteDbCommand.php | 36 +++++++++------- .../test/GeoLite/GeolocationDbUpdaterTest.php | 34 ++++++++++----- .../src/EventDispatcher/UpdateGeoLiteDb.php | 41 +++++++++++++------ .../src/Geolocation/GeolocationDbUpdater.php | 38 +++++++---------- .../GeolocationDbUpdaterInterface.php | 3 +- ...cationDownloadProgressHandlerInterface.php | 21 ++++++++++ 6 files changed, 110 insertions(+), 63 deletions(-) create mode 100644 module/Core/src/Geolocation/GeolocationDownloadProgressHandlerInterface.php diff --git a/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php b/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php index f6110d07a..b0a22c97b 100644 --- a/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php +++ b/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php @@ -7,6 +7,7 @@ use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\Core\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdaterInterface; +use Shlinkio\Shlink\Core\Geolocation\GeolocationDownloadProgressHandlerInterface; use Shlinkio\Shlink\Core\Geolocation\GeolocationResult; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Helper\ProgressBar; @@ -16,13 +17,14 @@ use function sprintf; -class DownloadGeoLiteDbCommand extends Command +class DownloadGeoLiteDbCommand extends Command implements GeolocationDownloadProgressHandlerInterface { public const string NAME = 'visit:download-db'; private ProgressBar|null $progressBar = null; + private SymfonyStyle $io; - public function __construct(private GeolocationDbUpdaterInterface $dbUpdater) + public function __construct(private readonly GeolocationDbUpdaterInterface $dbUpdater) { parent::__construct(); } @@ -39,32 +41,26 @@ protected function configure(): void protected function execute(InputInterface $input, OutputInterface $output): int { - $io = new SymfonyStyle($input, $output); + $this->io = new SymfonyStyle($input, $output); try { - $result = $this->dbUpdater->checkDbUpdate(function (bool $olderDbExists) use ($io): void { - $io->text(sprintf('%s GeoLite2 db file...', $olderDbExists ? 'Updating' : 'Downloading')); - $this->progressBar = new ProgressBar($io); - }, function (int $total, int $downloaded): void { - $this->progressBar?->setMaxSteps($total); - $this->progressBar?->setProgress($downloaded); - }); + $result = $this->dbUpdater->checkDbUpdate($this); if ($result === GeolocationResult::LICENSE_MISSING) { - $io->warning('It was not possible to download GeoLite2 db, because a license was not provided.'); + $this->io->warning('It was not possible to download GeoLite2 db, because a license was not provided.'); return ExitCode::EXIT_WARNING; } if ($this->progressBar === null) { - $io->info('GeoLite2 db file is up to date.'); + $this->io->info('GeoLite2 db file is up to date.'); } else { $this->progressBar->finish(); - $io->success('GeoLite2 db file properly downloaded.'); + $this->io->success('GeoLite2 db file properly downloaded.'); } return ExitCode::EXIT_SUCCESS; } catch (GeolocationDbUpdateFailedException $e) { - return $this->processGeoLiteUpdateError($e, $io); + return $this->processGeoLiteUpdateError($e, $this->io); } } @@ -86,4 +82,16 @@ private function processGeoLiteUpdateError(GeolocationDbUpdateFailedException $e return $olderDbExists ? ExitCode::EXIT_WARNING : ExitCode::EXIT_FAILURE; } + + public function beforeDownload(bool $olderDbExists): void + { + $this->io->text(sprintf('%s GeoLite2 db file...', $olderDbExists ? 'Updating' : 'Downloading')); + $this->progressBar = new ProgressBar($this->io); + } + + public function handleProgress(int $total, int $downloaded, bool $olderDbExists): void + { + $this->progressBar?->setMaxSteps($total); + $this->progressBar?->setProgress($downloaded); + } } diff --git a/module/CLI/test/GeoLite/GeolocationDbUpdaterTest.php b/module/CLI/test/GeoLite/GeolocationDbUpdaterTest.php index dc1d614c5..5a4518010 100644 --- a/module/CLI/test/GeoLite/GeolocationDbUpdaterTest.php +++ b/module/CLI/test/GeoLite/GeolocationDbUpdaterTest.php @@ -14,6 +14,7 @@ use Shlinkio\Shlink\Core\Config\Options\TrackingOptions; use Shlinkio\Shlink\Core\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdater; +use Shlinkio\Shlink\Core\Geolocation\GeolocationDownloadProgressHandlerInterface; use Shlinkio\Shlink\Core\Geolocation\GeolocationResult; use Shlinkio\Shlink\IpGeolocation\Exception\DbUpdateException; use Shlinkio\Shlink\IpGeolocation\Exception\MissingLicenseException; @@ -29,6 +30,8 @@ class GeolocationDbUpdaterTest extends TestCase private MockObject & DbUpdaterInterface $dbUpdater; private MockObject & Reader $geoLiteDbReader; private MockObject & Lock\LockInterface $lock; + /** @var GeolocationDownloadProgressHandlerInterface&object{beforeDownloadCalled: bool, handleProgressCalled: bool} */ + private GeolocationDownloadProgressHandlerInterface $progressHandler; protected function setUp(): void { @@ -36,6 +39,23 @@ protected function setUp(): void $this->geoLiteDbReader = $this->createMock(Reader::class); $this->lock = $this->createMock(Lock\SharedLockInterface::class); $this->lock->method('acquire')->with($this->isTrue())->willReturn(true); + $this->progressHandler = new class implements GeolocationDownloadProgressHandlerInterface { + public function __construct( + public bool $beforeDownloadCalled = false, + public bool $handleProgressCalled = false, + ) { + } + + public function beforeDownload(bool $olderDbExists): void + { + $this->beforeDownloadCalled = true; + } + + public function handleProgress(int $total, int $downloaded, bool $olderDbExists): void + { + $this->handleProgressCalled = true; + } + }; } #[Test] @@ -47,12 +67,9 @@ public function properResultIsReturnedWhenLicenseIsMissing(): void ); $this->geoLiteDbReader->expects($this->never())->method('metadata'); - $isCalled = false; - $result = $this->geolocationDbUpdater()->checkDbUpdate(function () use (&$isCalled): void { - $isCalled = true; - }); + $result = $this->geolocationDbUpdater()->checkDbUpdate($this->progressHandler); - self::assertTrue($isCalled); + self::assertTrue($this->progressHandler->beforeDownloadCalled); self::assertEquals(GeolocationResult::LICENSE_MISSING, $result); } @@ -67,17 +84,14 @@ public function exceptionIsThrownWhenOlderDbDoesNotExistAndDownloadFails(): void )->willThrowException($prev); $this->geoLiteDbReader->expects($this->never())->method('metadata'); - $isCalled = false; try { - $this->geolocationDbUpdater()->checkDbUpdate(function () use (&$isCalled): void { - $isCalled = true; - }); + $this->geolocationDbUpdater()->checkDbUpdate($this->progressHandler); self::fail(); } catch (Throwable $e) { self::assertInstanceOf(GeolocationDbUpdateFailedException::class, $e); self::assertSame($prev, $e->getPrevious()); self::assertFalse($e->olderDbExists()); - self::assertTrue($isCalled); + self::assertTrue($this->progressHandler->beforeDownloadCalled); } } diff --git a/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php b/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php index 7f14cc24e..871082798 100644 --- a/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php +++ b/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php @@ -8,6 +8,7 @@ use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Core\EventDispatcher\Event\GeoLiteDbCreated; use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdaterInterface; +use Shlinkio\Shlink\Core\Geolocation\GeolocationDownloadProgressHandlerInterface; use Shlinkio\Shlink\Core\Geolocation\GeolocationResult; use Throwable; @@ -24,21 +25,35 @@ public function __construct( public function __invoke(): void { - $beforeDownload = fn (bool $olderDbExists) => $this->logger->notice( - sprintf('%s GeoLite2 db file...', $olderDbExists ? 'Updating' : 'Downloading'), - ); - $messageLogged = false; - $handleProgress = function (int $total, int $downloaded, bool $olderDbExists) use (&$messageLogged): void { - if ($messageLogged || $total > $downloaded) { - return; - } + try { + $result = $this->dbUpdater->checkDbUpdate( + new class ($this->logger) implements GeolocationDownloadProgressHandlerInterface { + public function __construct( + private readonly LoggerInterface $logger, + private bool $messageLogged = false, + ) { + } - $messageLogged = true; - $this->logger->notice(sprintf('Finished %s GeoLite2 db file', $olderDbExists ? 'updating' : 'downloading')); - }; + public function beforeDownload(bool $olderDbExists): void + { + $this->logger->notice( + sprintf('%s GeoLite2 db file...', $olderDbExists ? 'Updating' : 'Downloading'), + ); + } - try { - $result = $this->dbUpdater->checkDbUpdate($beforeDownload, $handleProgress); + public function handleProgress(int $total, int $downloaded, bool $olderDbExists): void + { + if ($this->messageLogged || $total > $downloaded) { + return; + } + + $this->messageLogged = true; + $this->logger->notice( + sprintf('Finished %s GeoLite2 db file', $olderDbExists ? 'updating' : 'downloading'), + ); + } + }, + ); if ($result === GeolocationResult::DB_CREATED) { $this->eventDispatcher->dispatch(new GeoLiteDbCreated()); } diff --git a/module/Core/src/Geolocation/GeolocationDbUpdater.php b/module/Core/src/Geolocation/GeolocationDbUpdater.php index 73515a45a..b8d77a4b3 100644 --- a/module/Core/src/Geolocation/GeolocationDbUpdater.php +++ b/module/Core/src/Geolocation/GeolocationDbUpdater.php @@ -12,7 +12,6 @@ use Shlinkio\Shlink\Core\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\IpGeolocation\Exception\DbUpdateException; use Shlinkio\Shlink\IpGeolocation\Exception\MissingLicenseException; -use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdaterInterface; use Symfony\Component\Lock\LockFactory; @@ -41,8 +40,7 @@ public function __construct( * @throws GeolocationDbUpdateFailedException */ public function checkDbUpdate( - callable|null $beforeDownload = null, - callable|null $handleProgress = null, + GeolocationDownloadProgressHandlerInterface|null $downloadProgressHandler = null, ): GeolocationResult { if (! $this->trackingOptions->isGeolocationRelevant()) { return GeolocationResult::CHECK_SKIPPED; @@ -52,7 +50,7 @@ public function checkDbUpdate( $lock->acquire(true); // Block until lock is released try { - return $this->downloadIfNeeded($beforeDownload, $handleProgress); + return $this->downloadIfNeeded($downloadProgressHandler); } finally { $lock->release(); } @@ -61,15 +59,16 @@ public function checkDbUpdate( /** * @throws GeolocationDbUpdateFailedException */ - private function downloadIfNeeded(callable|null $beforeDownload, callable|null $handleProgress): GeolocationResult - { + private function downloadIfNeeded( + GeolocationDownloadProgressHandlerInterface|null $downloadProgressHandler, + ): GeolocationResult { if (! $this->dbUpdater->databaseFileExists()) { - return $this->downloadNewDb($beforeDownload, $handleProgress, olderDbExists: false); + return $this->downloadNewDb($downloadProgressHandler, olderDbExists: false); } $meta = ($this->geoLiteDbReaderFactory)()->metadata(); if ($this->buildIsTooOld($meta)) { - return $this->downloadNewDb($beforeDownload, $handleProgress, olderDbExists: true); + return $this->downloadNewDb($downloadProgressHandler, olderDbExists: true); } return GeolocationResult::DB_IS_UP_TO_DATE; @@ -106,32 +105,23 @@ private function resolveBuildTimestamp(Metadata $meta): int * @throws GeolocationDbUpdateFailedException */ private function downloadNewDb( - callable|null $beforeDownload, - callable|null $handleProgress, + GeolocationDownloadProgressHandlerInterface|null $downloadProgressHandler, bool $olderDbExists, ): GeolocationResult { - if ($beforeDownload !== null) { - $beforeDownload($olderDbExists); - } + $downloadProgressHandler?->beforeDownload($olderDbExists); try { - $this->dbUpdater->downloadFreshCopy($this->wrapHandleProgressCallback($handleProgress, $olderDbExists)); + $this->dbUpdater->downloadFreshCopy( + static fn (int $total, int $downloaded) + => $downloadProgressHandler?->handleProgress($total, $downloaded, $olderDbExists), + ); return $olderDbExists ? GeolocationResult::DB_UPDATED : GeolocationResult::DB_CREATED; } catch (MissingLicenseException) { return GeolocationResult::LICENSE_MISSING; - } catch (DbUpdateException | WrongIpException $e) { + } catch (DbUpdateException $e) { throw $olderDbExists ? GeolocationDbUpdateFailedException::withOlderDb($e) : GeolocationDbUpdateFailedException::withoutOlderDb($e); } } - - private function wrapHandleProgressCallback(callable|null $handleProgress, bool $olderDbExists): callable|null - { - if ($handleProgress === null) { - return null; - } - - return static fn (int $total, int $downloaded) => $handleProgress($total, $downloaded, $olderDbExists); - } } diff --git a/module/Core/src/Geolocation/GeolocationDbUpdaterInterface.php b/module/Core/src/Geolocation/GeolocationDbUpdaterInterface.php index 1e583e203..d7322e863 100644 --- a/module/Core/src/Geolocation/GeolocationDbUpdaterInterface.php +++ b/module/Core/src/Geolocation/GeolocationDbUpdaterInterface.php @@ -12,7 +12,6 @@ interface GeolocationDbUpdaterInterface * @throws GeolocationDbUpdateFailedException */ public function checkDbUpdate( - callable|null $beforeDownload = null, - callable|null $handleProgress = null, + GeolocationDownloadProgressHandlerInterface|null $downloadProgressHandler = null, ): GeolocationResult; } diff --git a/module/Core/src/Geolocation/GeolocationDownloadProgressHandlerInterface.php b/module/Core/src/Geolocation/GeolocationDownloadProgressHandlerInterface.php new file mode 100644 index 000000000..74cb90ae4 --- /dev/null +++ b/module/Core/src/Geolocation/GeolocationDownloadProgressHandlerInterface.php @@ -0,0 +1,21 @@ +