diff --git a/module/CLI/src/Util/GeolocationDbUpdater.php b/module/CLI/src/Util/GeolocationDbUpdater.php index 0d5c4b3e..34354a0e 100644 --- a/module/CLI/src/Util/GeolocationDbUpdater.php +++ b/module/CLI/src/Util/GeolocationDbUpdater.php @@ -35,9 +35,7 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface public function checkDbUpdate(callable $mustBeUpdated = null, callable $handleProgress = null): void { $lock = $this->locker->createLock(self::LOCK_NAME); - if (! $lock->acquire()) { - return; - } + $lock->acquire(true); // Block until lock is released try { $meta = $this->geoLiteDbReader->metadata(); diff --git a/module/CLI/test/Util/GeolocationDbUpdaterTest.php b/module/CLI/test/Util/GeolocationDbUpdaterTest.php index 3ab69aa7..e12dcfd9 100644 --- a/module/CLI/test/Util/GeolocationDbUpdaterTest.php +++ b/module/CLI/test/Util/GeolocationDbUpdaterTest.php @@ -8,11 +8,13 @@ use GeoIp2\Database\Reader; use InvalidArgumentException; use MaxMind\Db\Reader\Metadata; use PHPUnit\Framework\TestCase; +use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdater; use Shlinkio\Shlink\Common\Exception\RuntimeException; use Shlinkio\Shlink\Common\IpGeolocation\GeoLite2\DbUpdaterInterface; +use Symfony\Component\Lock; use Throwable; use function Functional\map; @@ -26,15 +28,27 @@ class GeolocationDbUpdaterTest extends TestCase private $dbUpdater; /** @var ObjectProphecy */ private $geoLiteDbReader; + /** @var ObjectProphecy */ + private $locker; + /** @var ObjectProphecy */ + private $lock; public function setUp(): void { $this->dbUpdater = $this->prophesize(DbUpdaterInterface::class); $this->geoLiteDbReader = $this->prophesize(Reader::class); + $this->locker = $this->prophesize(Lock\Factory::class); + $this->lock = $this->prophesize(Lock\LockInterface::class); + $this->lock->acquire(true)->willReturn(true); + $this->lock->release()->will(function () { + }); + $this->locker->createLock(Argument::type('string'))->willReturn($this->lock->reveal()); + $this->geolocationDbUpdater = new GeolocationDbUpdater( $this->dbUpdater->reveal(), - $this->geoLiteDbReader->reveal() + $this->geoLiteDbReader->reveal(), + $this->locker->reveal() ); } diff --git a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php index b40ede09..ced0bd63 100644 --- a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php +++ b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php @@ -8,6 +8,8 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Psr\Log\LoggerInterface; +use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; +use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdaterInterface; use Shlinkio\Shlink\Common\Exception\WrongIpException; use Shlinkio\Shlink\Common\IpGeolocation\IpLocationResolverInterface; use Shlinkio\Shlink\Common\IpGeolocation\Model\Location; @@ -18,6 +20,7 @@ use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\EventDispatcher\LocateShortUrlVisit; use Shlinkio\Shlink\Core\EventDispatcher\ShortUrlVisited; use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Visit\Model\UnknownVisitLocation; class LocateShortUrlVisitTest extends TestCase { @@ -29,17 +32,21 @@ class LocateShortUrlVisitTest extends TestCase private $em; /** @var ObjectProphecy */ private $logger; + /** @var ObjectProphecy */ + private $dbUpdater; public function setUp(): void { $this->ipLocationResolver = $this->prophesize(IpLocationResolverInterface::class); $this->em = $this->prophesize(EntityManagerInterface::class); $this->logger = $this->prophesize(LoggerInterface::class); + $this->dbUpdater = $this->prophesize(GeolocationDbUpdaterInterface::class); $this->locateVisit = new LocateShortUrlVisit( $this->ipLocationResolver->reveal(), $this->em->reveal(), - $this->logger->reveal() + $this->logger->reveal(), + $this->dbUpdater->reveal() ); } @@ -132,4 +139,61 @@ class LocateShortUrlVisitTest extends TestCase $resolveIp->shouldHaveBeenCalledOnce(); $this->logger->warning(Argument::cetera())->shouldNotHaveBeenCalled(); } + + /** @test */ + public function errorWhenUpdatingGeoliteWithExistingCopyLogsWarning(): void + { + $e = GeolocationDbUpdateFailedException::create(true); + $ipAddr = '1.2.3.0'; + $visit = new Visit(new ShortUrl(''), new Visitor('', '', $ipAddr)); + $location = new Location('', '', '', '', 0.0, 0.0, ''); + $event = new ShortUrlVisited('123'); + + $findVisit = $this->em->find(Visit::class, '123')->willReturn($visit); + $flush = $this->em->flush($visit)->will(function () { + }); + $resolveIp = $this->ipLocationResolver->resolveIpLocation($ipAddr)->willReturn($location); + $checkUpdateDb = $this->dbUpdater->checkDbUpdate(Argument::cetera())->willThrow($e); + + ($this->locateVisit)($event); + + $this->assertEquals($visit->getVisitLocation(), new VisitLocation($location)); + $findVisit->shouldHaveBeenCalledOnce(); + $flush->shouldHaveBeenCalledOnce(); + $resolveIp->shouldHaveBeenCalledOnce(); + $checkUpdateDb->shouldHaveBeenCalledOnce(); + $this->logger->warning( + 'GeoLite2 database update failed. Proceeding with old version. {e}', + ['e' => $e] + )->shouldHaveBeenCalledOnce(); + } + + /** @test */ + public function errorWhenDownloadingGeoliteCancelsLocation(): void + { + $e = GeolocationDbUpdateFailedException::create(false); + $ipAddr = '1.2.3.0'; + $visit = new Visit(new ShortUrl(''), new Visitor('', '', $ipAddr)); + $location = new Location('', '', '', '', 0.0, 0.0, ''); + $event = new ShortUrlVisited('123'); + + $findVisit = $this->em->find(Visit::class, '123')->willReturn($visit); + $flush = $this->em->flush($visit)->will(function () { + }); + $resolveIp = $this->ipLocationResolver->resolveIpLocation($ipAddr)->willReturn($location); + $checkUpdateDb = $this->dbUpdater->checkDbUpdate(Argument::cetera())->willThrow($e); + $logError = $this->logger->error( + 'GeoLite2 database download failed. It is not possible to locate visit with id 123. {e}', + ['e' => $e] + ); + + ($this->locateVisit)($event); + + $this->assertEquals($visit->getVisitLocation(), new UnknownVisitLocation()); + $findVisit->shouldHaveBeenCalledOnce(); + $flush->shouldNotHaveBeenCalled(); + $resolveIp->shouldNotHaveBeenCalled(); + $checkUpdateDb->shouldHaveBeenCalledOnce(); + $logError->shouldHaveBeenCalledOnce(); + } }