From 06c0a94b31ffed7a19f8ac4d2ad583f34a73047b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 10 Dec 2024 10:58:08 +0100 Subject: [PATCH 1/5] Move GeolocationDbUpdater from CLI to Core module --- module/CLI/config/dependencies.config.php | 15 ++------------- .../Command/Visit/DownloadGeoLiteDbCommand.php | 6 +++--- .../Visit/DownloadGeoLiteDbCommandTest.php | 6 +++--- .../GeolocationDbUpdateFailedExceptionTest.php | 2 +- .../CLI/test/GeoLite/GeolocationDbUpdaterTest.php | 6 +++--- module/Core/config/dependencies.config.php | 11 +++++++++++ module/Core/config/event_dispatcher.config.php | 2 +- .../Core/src/EventDispatcher/UpdateGeoLiteDb.php | 4 ++-- .../GeolocationDbUpdateFailedException.php | 2 +- .../src/Geolocation}/GeolocationDbUpdater.php | 4 ++-- .../GeolocationDbUpdaterInterface.php | 4 ++-- .../src/Geolocation}/GeolocationResult.php | 2 +- .../test/EventDispatcher/UpdateGeoLiteDbTest.php | 4 ++-- 13 files changed, 34 insertions(+), 34 deletions(-) rename module/{CLI => Core}/src/Exception/GeolocationDbUpdateFailedException.php (97%) rename module/{CLI/src/GeoLite => Core/src/Geolocation}/GeolocationDbUpdater.php (97%) rename module/{CLI/src/GeoLite => Core/src/Geolocation}/GeolocationDbUpdaterInterface.php (72%) rename module/{CLI/src/GeoLite => Core/src/Geolocation}/GeolocationResult.php (77%) diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 76e7c4f54..5df74822a 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -7,9 +7,9 @@ use Laminas\ServiceManager\AbstractFactory\ConfigAbstractFactory; use Laminas\ServiceManager\Factory\InvokableFactory; use Shlinkio\Shlink\Common\Doctrine\NoDbNameConnectionFactory; -use Shlinkio\Shlink\Core\Config\Options\TrackingOptions; use Shlinkio\Shlink\Core\Config\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Domain\DomainService; +use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdater; use Shlinkio\Shlink\Core\Matomo; use Shlinkio\Shlink\Core\RedirectRule\ShortUrlRedirectRuleService; use Shlinkio\Shlink\Core\ShortUrl; @@ -17,15 +17,11 @@ use Shlinkio\Shlink\Core\Tag\TagService; use Shlinkio\Shlink\Core\Visit; use Shlinkio\Shlink\Installer\Factory\ProcessHelperFactory; -use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdater; -use Shlinkio\Shlink\IpGeolocation\GeoLite2\GeoLite2ReaderFactory; use Shlinkio\Shlink\Rest\Service\ApiKeyService; use Symfony\Component\Console as SymfonyCli; use Symfony\Component\Lock\LockFactory; use Symfony\Component\Process\PhpExecutableFinder; -use const Shlinkio\Shlink\LOCAL_LOCK_FACTORY; - return [ 'dependencies' => [ @@ -34,7 +30,6 @@ SymfonyCli\Helper\ProcessHelper::class => ProcessHelperFactory::class, PhpExecutableFinder::class => InvokableFactory::class, - GeoLite\GeolocationDbUpdater::class => ConfigAbstractFactory::class, RedirectRule\RedirectRuleHandler::class => InvokableFactory::class, Util\ProcessRunner::class => ConfigAbstractFactory::class, @@ -82,12 +77,6 @@ ], ConfigAbstractFactory::class => [ - GeoLite\GeolocationDbUpdater::class => [ - DbUpdater::class, - GeoLite2ReaderFactory::class, - LOCAL_LOCK_FACTORY, - TrackingOptions::class, - ], Util\ProcessRunner::class => [SymfonyCli\Helper\ProcessHelper::class], ApiKey\RoleResolver::class => [DomainService::class, UrlShortenerOptions::class], @@ -107,7 +96,7 @@ Command\ShortUrl\DeleteShortUrlVisitsCommand::class => [ShortUrl\ShortUrlVisitsDeleter::class], Command\ShortUrl\DeleteExpiredShortUrlsCommand::class => [ShortUrl\DeleteShortUrlService::class], - Command\Visit\DownloadGeoLiteDbCommand::class => [GeoLite\GeolocationDbUpdater::class], + Command\Visit\DownloadGeoLiteDbCommand::class => [GeolocationDbUpdater::class], Command\Visit\LocateVisitsCommand::class => [ Visit\Geolocation\VisitLocator::class, Visit\Geolocation\VisitToLocationHelper::class, diff --git a/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php b/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php index 0fdd8ae38..f6110d07a 100644 --- a/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php +++ b/module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php @@ -4,10 +4,10 @@ namespace Shlinkio\Shlink\CLI\Command\Visit; -use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; -use Shlinkio\Shlink\CLI\GeoLite\GeolocationDbUpdaterInterface; -use Shlinkio\Shlink\CLI\GeoLite\GeolocationResult; use Shlinkio\Shlink\CLI\Util\ExitCode; +use Shlinkio\Shlink\Core\Exception\GeolocationDbUpdateFailedException; +use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdaterInterface; +use Shlinkio\Shlink\Core\Geolocation\GeolocationResult; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Helper\ProgressBar; use Symfony\Component\Console\Input\InputInterface; diff --git a/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php b/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php index 4d2754f8f..2b477f031 100644 --- a/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php +++ b/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php @@ -9,10 +9,10 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\CLI\Command\Visit\DownloadGeoLiteDbCommand; -use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; -use Shlinkio\Shlink\CLI\GeoLite\GeolocationDbUpdaterInterface; -use Shlinkio\Shlink\CLI\GeoLite\GeolocationResult; use Shlinkio\Shlink\CLI\Util\ExitCode; +use Shlinkio\Shlink\Core\Exception\GeolocationDbUpdateFailedException; +use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdaterInterface; +use Shlinkio\Shlink\Core\Geolocation\GeolocationResult; use ShlinkioTest\Shlink\CLI\Util\CliTestUtils; use Symfony\Component\Console\Tester\CommandTester; diff --git a/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php b/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php index 519ddf02e..a1d6db657 100644 --- a/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php +++ b/module/CLI/test/Exception/GeolocationDbUpdateFailedExceptionTest.php @@ -9,7 +9,7 @@ use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; use RuntimeException; -use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; +use Shlinkio\Shlink\Core\Exception\GeolocationDbUpdateFailedException; use Throwable; class GeolocationDbUpdateFailedExceptionTest extends TestCase diff --git a/module/CLI/test/GeoLite/GeolocationDbUpdaterTest.php b/module/CLI/test/GeoLite/GeolocationDbUpdaterTest.php index 038d570c9..dc1d614c5 100644 --- a/module/CLI/test/GeoLite/GeolocationDbUpdaterTest.php +++ b/module/CLI/test/GeoLite/GeolocationDbUpdaterTest.php @@ -11,10 +11,10 @@ use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; -use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; -use Shlinkio\Shlink\CLI\GeoLite\GeolocationDbUpdater; -use Shlinkio\Shlink\CLI\GeoLite\GeolocationResult; use Shlinkio\Shlink\Core\Config\Options\TrackingOptions; +use Shlinkio\Shlink\Core\Exception\GeolocationDbUpdateFailedException; +use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdater; +use Shlinkio\Shlink\Core\Geolocation\GeolocationResult; use Shlinkio\Shlink\IpGeolocation\Exception\DbUpdateException; use Shlinkio\Shlink\IpGeolocation\Exception\MissingLicenseException; use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdaterInterface; diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 4844e6d5c..b16a4c5cc 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -9,12 +9,16 @@ use Psr\EventDispatcher\EventDispatcherInterface; use Shlinkio\Shlink\Common\Doctrine\EntityRepositoryFactory; use Shlinkio\Shlink\Core\Config\Options\NotFoundRedirectOptions; +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; +use const Shlinkio\Shlink\LOCAL_LOCK_FACTORY; + return [ 'dependencies' => [ @@ -103,6 +107,7 @@ EventDispatcher\PublishingUpdatesGenerator::class => ConfigAbstractFactory::class, + Geolocation\GeolocationDbUpdater::class => ConfigAbstractFactory::class, Geolocation\Middleware\IpGeolocationMiddleware::class => ConfigAbstractFactory::class, Importer\ImportedLinksProcessor::class => ConfigAbstractFactory::class, @@ -240,6 +245,12 @@ EventDispatcher\PublishingUpdatesGenerator::class => [ShortUrl\Transformer\ShortUrlDataTransformer::class], + GeolocationDbUpdater::class => [ + DbUpdater::class, + GeoLite2ReaderFactory::class, + LOCAL_LOCK_FACTORY, + Config\Options\TrackingOptions::class, + ], Geolocation\Middleware\IpGeolocationMiddleware::class => [ IpLocationResolverInterface::class, DbUpdater::class, diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index 4e130fcf9..39efa3cb7 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/module/Core/config/event_dispatcher.config.php @@ -6,11 +6,11 @@ use Laminas\ServiceManager\AbstractFactory\ConfigAbstractFactory; use Psr\EventDispatcher\EventDispatcherInterface; -use Shlinkio\Shlink\CLI\GeoLite\GeolocationDbUpdater; use Shlinkio\Shlink\Common\Cache\RedisPublishingHelper; use Shlinkio\Shlink\Common\Mercure\MercureHubPublishingHelper; use Shlinkio\Shlink\Common\Mercure\MercureOptions; use Shlinkio\Shlink\Common\RabbitMq\RabbitMqPublishingHelper; +use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdater; use Shlinkio\Shlink\Core\Matomo\MatomoOptions; use Shlinkio\Shlink\Core\Visit\Geolocation\VisitLocator; use Shlinkio\Shlink\Core\Visit\Geolocation\VisitToLocationHelper; diff --git a/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php b/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php index 4e4720c5e..7f14cc24e 100644 --- a/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php +++ b/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php @@ -6,9 +6,9 @@ use Psr\EventDispatcher\EventDispatcherInterface; use Psr\Log\LoggerInterface; -use Shlinkio\Shlink\CLI\GeoLite\GeolocationDbUpdaterInterface; -use Shlinkio\Shlink\CLI\GeoLite\GeolocationResult; use Shlinkio\Shlink\Core\EventDispatcher\Event\GeoLiteDbCreated; +use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdaterInterface; +use Shlinkio\Shlink\Core\Geolocation\GeolocationResult; use Throwable; use function sprintf; diff --git a/module/CLI/src/Exception/GeolocationDbUpdateFailedException.php b/module/Core/src/Exception/GeolocationDbUpdateFailedException.php similarity index 97% rename from module/CLI/src/Exception/GeolocationDbUpdateFailedException.php rename to module/Core/src/Exception/GeolocationDbUpdateFailedException.php index ee31ac82e..f3c3f65fd 100644 --- a/module/CLI/src/Exception/GeolocationDbUpdateFailedException.php +++ b/module/Core/src/Exception/GeolocationDbUpdateFailedException.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Shlinkio\Shlink\CLI\Exception; +namespace Shlinkio\Shlink\Core\Exception; use RuntimeException; use Throwable; diff --git a/module/CLI/src/GeoLite/GeolocationDbUpdater.php b/module/Core/src/Geolocation/GeolocationDbUpdater.php similarity index 97% rename from module/CLI/src/GeoLite/GeolocationDbUpdater.php rename to module/Core/src/Geolocation/GeolocationDbUpdater.php index 0d3d96db4..73515a45a 100644 --- a/module/CLI/src/GeoLite/GeolocationDbUpdater.php +++ b/module/Core/src/Geolocation/GeolocationDbUpdater.php @@ -2,14 +2,14 @@ declare(strict_types=1); -namespace Shlinkio\Shlink\CLI\GeoLite; +namespace Shlinkio\Shlink\Core\Geolocation; use Cake\Chronos\Chronos; use Closure; use GeoIp2\Database\Reader; use MaxMind\Db\Reader\Metadata; -use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\Core\Config\Options\TrackingOptions; +use Shlinkio\Shlink\Core\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\IpGeolocation\Exception\DbUpdateException; use Shlinkio\Shlink\IpGeolocation\Exception\MissingLicenseException; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; diff --git a/module/CLI/src/GeoLite/GeolocationDbUpdaterInterface.php b/module/Core/src/Geolocation/GeolocationDbUpdaterInterface.php similarity index 72% rename from module/CLI/src/GeoLite/GeolocationDbUpdaterInterface.php rename to module/Core/src/Geolocation/GeolocationDbUpdaterInterface.php index ba0f0e708..1e583e203 100644 --- a/module/CLI/src/GeoLite/GeolocationDbUpdaterInterface.php +++ b/module/Core/src/Geolocation/GeolocationDbUpdaterInterface.php @@ -2,9 +2,9 @@ declare(strict_types=1); -namespace Shlinkio\Shlink\CLI\GeoLite; +namespace Shlinkio\Shlink\Core\Geolocation; -use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; +use Shlinkio\Shlink\Core\Exception\GeolocationDbUpdateFailedException; interface GeolocationDbUpdaterInterface { diff --git a/module/CLI/src/GeoLite/GeolocationResult.php b/module/Core/src/Geolocation/GeolocationResult.php similarity index 77% rename from module/CLI/src/GeoLite/GeolocationResult.php rename to module/Core/src/Geolocation/GeolocationResult.php index 859768864..3b472d09c 100644 --- a/module/CLI/src/GeoLite/GeolocationResult.php +++ b/module/Core/src/Geolocation/GeolocationResult.php @@ -1,6 +1,6 @@ Date: Wed, 11 Dec 2024 08:27:50 +0100 Subject: [PATCH 2/5] 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 @@ + Date: Wed, 11 Dec 2024 08:35:24 +0100 Subject: [PATCH 3/5] Fix UpdateGeoLiteDbTest --- .../EventDispatcher/UpdateGeoLiteDbTest.php | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/module/Core/test/EventDispatcher/UpdateGeoLiteDbTest.php b/module/Core/test/EventDispatcher/UpdateGeoLiteDbTest.php index 55662d5a5..5288aa1bf 100644 --- a/module/Core/test/EventDispatcher/UpdateGeoLiteDbTest.php +++ b/module/Core/test/EventDispatcher/UpdateGeoLiteDbTest.php @@ -14,6 +14,7 @@ use Shlinkio\Shlink\Core\EventDispatcher\Event\GeoLiteDbCreated; use Shlinkio\Shlink\Core\EventDispatcher\UpdateGeoLiteDb; use Shlinkio\Shlink\Core\Geolocation\GeolocationDbUpdaterInterface; +use Shlinkio\Shlink\Core\Geolocation\GeolocationDownloadProgressHandlerInterface; use Shlinkio\Shlink\Core\Geolocation\GeolocationResult; use function array_map; @@ -51,11 +52,11 @@ public function exceptionWhileUpdatingDbLogsError(): void } #[Test, DataProvider('provideFlags')] - public function noticeMessageIsPrintedWhenFirstCallbackIsInvoked(bool $oldDbExists, string $expectedMessage): void + public function noticeMessageIsPrintedWhenDownloadIsStarted(bool $oldDbExists, string $expectedMessage): void { $this->dbUpdater->expects($this->once())->method('checkDbUpdate')->withAnyParameters()->willReturnCallback( - function (callable $firstCallback) use ($oldDbExists): GeolocationResult { - $firstCallback($oldDbExists); + function (GeolocationDownloadProgressHandlerInterface $handler) use ($oldDbExists): GeolocationResult { + $handler->beforeDownload($oldDbExists); return GeolocationResult::DB_IS_UP_TO_DATE; }, ); @@ -73,18 +74,24 @@ public static function provideFlags(): iterable } #[Test, DataProvider('provideDownloaded')] - public function noticeMessageIsPrintedWhenSecondCallbackIsInvoked( + public function noticeMessageIsPrintedWhenDownloadIsFinished( int $total, int $downloaded, bool $oldDbExists, string|null $expectedMessage, ): void { $this->dbUpdater->expects($this->once())->method('checkDbUpdate')->withAnyParameters()->willReturnCallback( - function ($_, callable $secondCallback) use ($total, $downloaded, $oldDbExists): GeolocationResult { + function ( + GeolocationDownloadProgressHandlerInterface $handler, + ) use ( + $total, + $downloaded, + $oldDbExists, + ): GeolocationResult { // Invoke several times to ensure the log is printed only once - $secondCallback($total, $downloaded, $oldDbExists); - $secondCallback($total, $downloaded, $oldDbExists); - $secondCallback($total, $downloaded, $oldDbExists); + $handler->handleProgress($total, $downloaded, $oldDbExists); + $handler->handleProgress($total, $downloaded, $oldDbExists); + $handler->handleProgress($total, $downloaded, $oldDbExists); return GeolocationResult::DB_UPDATED; }, From 84d12f681108811c5c1df65fc6df0a85d3b1bd31 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 11 Dec 2024 08:47:13 +0100 Subject: [PATCH 4/5] Move GeolocationDbUpdaterTest to Core module --- .../test/Geolocation}/GeolocationDbUpdaterTest.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) rename module/{CLI/test/GeoLite => Core/test/Geolocation}/GeolocationDbUpdaterTest.php (98%) diff --git a/module/CLI/test/GeoLite/GeolocationDbUpdaterTest.php b/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php similarity index 98% rename from module/CLI/test/GeoLite/GeolocationDbUpdaterTest.php rename to module/Core/test/Geolocation/GeolocationDbUpdaterTest.php index 5a4518010..5c76747b1 100644 --- a/module/CLI/test/GeoLite/GeolocationDbUpdaterTest.php +++ b/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php @@ -2,9 +2,10 @@ declare(strict_types=1); -namespace ShlinkioTest\Shlink\CLI\GeoLite; +namespace ShlinkioTest\Shlink\Core\Geolocation; use Cake\Chronos\Chronos; +use Closure; use GeoIp2\Database\Reader; use MaxMind\Db\Reader\Metadata; use PHPUnit\Framework\Attributes\DataProvider; @@ -80,7 +81,7 @@ public function exceptionIsThrownWhenOlderDbDoesNotExistAndDownloadFails(): void $this->dbUpdater->expects($this->once())->method('databaseFileExists')->willReturn(false); $this->dbUpdater->expects($this->once())->method('downloadFreshCopy')->with( - $this->isNull(), + $this->isInstanceOf(Closure::class), )->willThrowException($prev); $this->geoLiteDbReader->expects($this->never())->method('metadata'); @@ -101,7 +102,7 @@ public function exceptionIsThrownWhenOlderDbIsTooOldAndDownloadFails(int $days): $prev = new DbUpdateException(''); $this->dbUpdater->expects($this->once())->method('databaseFileExists')->willReturn(true); $this->dbUpdater->expects($this->once())->method('downloadFreshCopy')->with( - $this->isNull(), + $this->isInstanceOf(Closure::class), )->willThrowException($prev); $this->geoLiteDbReader->expects($this->once())->method('metadata')->with()->willReturn( $this->buildMetaWithBuildEpoch(Chronos::now()->subDays($days)->getTimestamp()), From 2ede615da8c181b19b2019064cf210661aad359d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 11 Dec 2024 08:50:56 +0100 Subject: [PATCH 5/5] Fix DownloadGeoLiteDbCommandTest --- .../Command/Visit/DownloadGeoLiteDbCommandTest.php | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php b/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php index 2b477f031..7fa46a05a 100644 --- a/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php +++ b/module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php @@ -12,6 +12,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 ShlinkioTest\Shlink\CLI\Util\CliTestUtils; use Symfony\Component\Console\Tester\CommandTester; @@ -36,9 +37,9 @@ public function showsProperMessageWhenGeoLiteUpdateFails( int $expectedExitCode, ): void { $this->dbUpdater->expects($this->once())->method('checkDbUpdate')->withAnyParameters()->willReturnCallback( - function (callable $beforeDownload, callable $handleProgress) use ($olderDbExists): void { - $beforeDownload($olderDbExists); - $handleProgress(100, 50); + function (GeolocationDownloadProgressHandlerInterface $handler) use ($olderDbExists): void { + $handler->beforeDownload($olderDbExists); + $handler->handleProgress(100, 50, $olderDbExists); throw $olderDbExists ? GeolocationDbUpdateFailedException::withOlderDb() @@ -105,8 +106,8 @@ public function printsExpectedMessageWhenNoErrorOccurs(callable $checkUpdateBeha public static function provideSuccessParams(): iterable { yield 'up to date db' => [fn () => GeolocationResult::CHECK_SKIPPED, '[INFO] GeoLite2 db file is up to date.']; - yield 'outdated db' => [function (callable $beforeDownload): GeolocationResult { - $beforeDownload(true); + yield 'outdated db' => [function (GeolocationDownloadProgressHandlerInterface $handler): GeolocationResult { + $handler->beforeDownload(true); return GeolocationResult::DB_CREATED; }, '[OK] GeoLite2 db file properly downloaded.']; }