diff --git a/.gitignore b/.gitignore index a5163a27..f01b1741 100644 --- a/.gitignore +++ b/.gitignore @@ -7,6 +7,7 @@ vendor/ data/database.sqlite data/shlink-tests.db data/GeoLite2-City.mmdb +data/GeoLite2-City.mmdb.* docs/swagger-ui* docker-compose.override.yml .phpunit.result.cache diff --git a/CHANGELOG.md b/CHANGELOG.md index bb9d03e8..be28d018 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this } ``` +* [#285](https://github.com/shlinkio/shlink/issues/285) Visit location resolution is now done asynchronously but in real time thanks to swoole task management. + + Now, when a short URL is visited, a task is enqueued to locate it. The user is immediately redirected to the long URL, and in the background, the visit is located, making stats to be available a couple of seconds after the visit without the requirement of cronjobs being run constantly. + + Sadly, this feature is not enabled when serving shlink via apache/nginx, where you should still rely on cronjobs. + #### Changed * *Nothing* diff --git a/README.md b/README.md index ee44552d..7afae708 100644 --- a/README.md +++ b/README.md @@ -188,7 +188,7 @@ There are a couple of time-consuming tasks that shlink expects you to do manuall Those tasks can be performed using shlink's CLI, so it should be easy to schedule them to be run in the background (for example, using cron jobs): -* Resolve IP address locations: `/path/to/shlink/bin/cli visit:locate` +* **For shlink older than 1.18.0 or not using swoole as the web server**: Resolve IP address locations: `/path/to/shlink/bin/cli visit:locate` If you don't run this command regularly, the stats will say all visits come from *unknown* locations. @@ -204,7 +204,7 @@ Those tasks can be performed using shlink's CLI, so it should be easy to schedul *Any of these commands accept the `-q` flag, which makes it not display any output. This is recommended when configuring the commands as cron jobs.* -In future versions, it is planed that, when using **swoole** to serve shlink, some of these tasks are automatically run without blocking the request and also, without having to configure cron jobs. Probably resolving IP locations and generating previews. +> In future versions, it is planed that, when using **swoole** to serve shlink, some of these tasks are automatically run without blocking the request and also, without having to configure cron jobs. Probably resolving IP locations and generating previews. ## Update to new version diff --git a/composer.json b/composer.json index 8f15d83e..e6e20989 100644 --- a/composer.json +++ b/composer.json @@ -29,6 +29,7 @@ "lstrojny/functional-php": "^1.8", "mikehaertl/phpwkhtmltopdf": "^2.2", "monolog/monolog": "^1.21", + "phly/phly-event-dispatcher": "^1.0", "shlinkio/shlink-installer": "^1.1", "symfony/console": "^4.2", "symfony/filesystem": "^4.2", @@ -52,6 +53,7 @@ "require-dev": { "devster/ubench": "^2.0", "doctrine/data-fixtures": "^1.3", + "eaglewu/swoole-ide-helper": "dev-master", "filp/whoops": "^2.0", "infection/infection": "^0.12.2", "phpstan/phpstan": "^0.11.2", @@ -69,10 +71,12 @@ "Shlinkio\\Shlink\\CLI\\": "module/CLI/src", "Shlinkio\\Shlink\\Rest\\": "module/Rest/src", "Shlinkio\\Shlink\\Core\\": "module/Core/src", - "Shlinkio\\Shlink\\Common\\": "module/Common/src" + "Shlinkio\\Shlink\\Common\\": "module/Common/src", + "Shlinkio\\Shlink\\EventDispatcher\\": "module/EventDispatcher/src" }, "files": [ - "module/Common/functions/functions.php" + "module/Common/functions/functions.php", + "module/EventDispatcher/functions/functions.php" ] }, "autoload-dev": { @@ -87,7 +91,8 @@ "ShlinkioTest\\Shlink\\Common\\": [ "module/Common/test", "module/Common/test-db" - ] + ], + "ShlinkioTest\\Shlink\\EventDispatcher\\": "module/EventDispatcher/test" } }, "scripts": { diff --git a/config/autoload/swoole.global.php b/config/autoload/swoole.global.php index 5f82f0bc..11a0304f 100644 --- a/config/autoload/swoole.global.php +++ b/config/autoload/swoole.global.php @@ -9,6 +9,11 @@ return [ 'swoole-http-server' => [ 'host' => '0.0.0.0', 'process-name' => 'shlink', + + 'options' => [ + 'worker_num' => 16, + 'task_worker_num' => 16, + ], ], ], diff --git a/config/config.php b/config/config.php index fcee89fb..f09bb9f6 100644 --- a/config/config.php +++ b/config/config.php @@ -20,6 +20,7 @@ return (new ConfigAggregator\ConfigAggregator([ Core\ConfigProvider::class, CLI\ConfigProvider::class, Rest\ConfigProvider::class, + EventDispatcher\ConfigProvider::class, new ConfigAggregator\PhpFileProvider('config/autoload/{{,*.}global,{,*.}local}.php'), new ConfigAggregator\ZendConfigProvider('config/params/{generated_config.php,*.config.{php,json}}'), env('APP_ENV') === 'test' diff --git a/config/test/test_config.global.php b/config/test/test_config.global.php index 31f53e28..24057833 100644 --- a/config/test/test_config.global.php +++ b/config/test/test_config.global.php @@ -70,6 +70,8 @@ return [ 'process-name' => 'shlink_test', 'options' => [ 'pid_file' => sys_get_temp_dir() . '/shlink-test-swoole.pid', + 'worker_num' => 1, + 'task_worker_num' => 1, ], ], ], diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index b56ff76e..0a90c1ff 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -48,7 +48,7 @@ return [ ], ConfigAbstractFactory::class => [ - GeolocationDbUpdater::class => [DbUpdater::class, Reader::class], + GeolocationDbUpdater::class => [DbUpdater::class, Reader::class, Lock\Factory::class], Command\ShortUrl\GenerateShortUrlCommand::class => [Service\UrlShortener::class, 'config.url_shortener.domain'], Command\ShortUrl\ResolveUrlCommand::class => [Service\UrlShortener::class], diff --git a/module/CLI/src/Util/GeolocationDbUpdater.php b/module/CLI/src/Util/GeolocationDbUpdater.php index 067a9561..34354a0e 100644 --- a/module/CLI/src/Util/GeolocationDbUpdater.php +++ b/module/CLI/src/Util/GeolocationDbUpdater.php @@ -9,18 +9,24 @@ use InvalidArgumentException; use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\Common\Exception\RuntimeException; use Shlinkio\Shlink\Common\IpGeolocation\GeoLite2\DbUpdaterInterface; +use Symfony\Component\Lock\Factory as Locker; class GeolocationDbUpdater implements GeolocationDbUpdaterInterface { + private const LOCK_NAME = 'geolocation-db-update'; + /** @var DbUpdaterInterface */ private $dbUpdater; /** @var Reader */ private $geoLiteDbReader; + /** @var Locker */ + private $locker; - public function __construct(DbUpdaterInterface $dbUpdater, Reader $geoLiteDbReader) + public function __construct(DbUpdaterInterface $dbUpdater, Reader $geoLiteDbReader, Locker $locker) { $this->dbUpdater = $dbUpdater; $this->geoLiteDbReader = $geoLiteDbReader; + $this->locker = $locker; } /** @@ -28,6 +34,9 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface */ public function checkDbUpdate(callable $mustBeUpdated = null, callable $handleProgress = null): void { + $lock = $this->locker->createLock(self::LOCK_NAME); + $lock->acquire(true); // Block until lock is released + try { $meta = $this->geoLiteDbReader->metadata(); if ($this->buildIsTooOld($meta->__get('buildEpoch'))) { @@ -36,6 +45,8 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface } catch (InvalidArgumentException $e) { // This is the exception thrown by the reader when the database file does not exist $this->downloadNewDb(false, $mustBeUpdated, $handleProgress); + } finally { + $lock->release(); } } 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/Common/src/Entity/AbstractEntity.php b/module/Common/src/Entity/AbstractEntity.php index b798a087..9358d2c5 100644 --- a/module/Common/src/Entity/AbstractEntity.php +++ b/module/Common/src/Entity/AbstractEntity.php @@ -20,6 +20,9 @@ abstract class AbstractEntity return $this->id; } + /** + * @internal + */ public function setId(string $id): self { $this->id = $id; diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 960b74ac..28ea8a56 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core; use Doctrine\Common\Cache\Cache; +use Psr\EventDispatcher\EventDispatcherInterface; use Shlinkio\Shlink\Common\Service\PreviewGenerator; use Shlinkio\Shlink\Core\Response\NotFoundHandler; use Zend\Expressive\Router\RouterInterface; @@ -46,7 +47,7 @@ return [ Options\UrlShortenerOptions::class => ['config.url_shortener'], Service\UrlShortener::class => ['httpClient', 'em', Options\UrlShortenerOptions::class], - Service\VisitsTracker::class => ['em'], + Service\VisitsTracker::class => ['em', EventDispatcherInterface::class], Service\ShortUrlService::class => ['em'], Service\VisitService::class => ['em'], Service\Tag\TagService::class => ['em'], diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php new file mode 100644 index 00000000..767142f1 --- /dev/null +++ b/module/Core/config/event_dispatcher.config.php @@ -0,0 +1,36 @@ + [ + 'regular' => [], + 'async' => [ + EventDispatcher\ShortUrlVisited::class => [ + EventDispatcher\LocateShortUrlVisit::class, + ], + ], + ], + + 'dependencies' => [ + 'factories' => [ + EventDispatcher\LocateShortUrlVisit::class => ConfigAbstractFactory::class, + ], + ], + + ConfigAbstractFactory::class => [ + EventDispatcher\LocateShortUrlVisit::class => [ + IpLocationResolverInterface::class, + 'em', + 'Logger_Shlink', + GeolocationDbUpdater::class, + ], + ], + +]; diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index 8910ae1d..58401088 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -65,6 +65,11 @@ class Visit extends AbstractEntity implements JsonSerializable return $this->visitLocation ?? new UnknownVisitLocation(); } + public function isLocatable(): bool + { + return $this->hasRemoteAddr() && $this->remoteAddr !== IpAddress::LOCALHOST; + } + public function locate(VisitLocation $visitLocation): self { $this->visitLocation = $visitLocation; diff --git a/module/Core/src/EventDispatcher/LocateShortUrlVisit.php b/module/Core/src/EventDispatcher/LocateShortUrlVisit.php new file mode 100644 index 00000000..de7eb360 --- /dev/null +++ b/module/Core/src/EventDispatcher/LocateShortUrlVisit.php @@ -0,0 +1,86 @@ +ipLocationResolver = $ipLocationResolver; + $this->em = $em; + $this->logger = $logger; + $this->dbUpdater = $dbUpdater; + } + + public function __invoke(ShortUrlVisited $shortUrlVisited): void + { + $visitId = $shortUrlVisited->visitId(); + + /** @var Visit|null $visit */ + $visit = $this->em->find(Visit::class, $visitId); + if ($visit === null) { + $this->logger->warning(sprintf('Tried to locate visit with id "%s", but it does not exist.', $visitId)); + return; + } + + try { + $this->dbUpdater->checkDbUpdate(function (bool $olderDbExists) { + $this->logger->notice(sprintf('%s GeoLite2 database...', $olderDbExists ? 'Updating' : 'Downloading')); + }); + } catch (GeolocationDbUpdateFailedException $e) { + if (! $e->olderDbExists()) { + $this->logger->error( + sprintf( + 'GeoLite2 database download failed. It is not possible to locate visit with id %s. {e}', + $visitId + ), + ['e' => $e] + ); + return; + } + + $this->logger->warning('GeoLite2 database update failed. Proceeding with old version. {e}', ['e' => $e]); + } + + try { + $location = $visit->isLocatable() + ? $this->ipLocationResolver->resolveIpLocation($visit->getRemoteAddr()) + : Location::emptyInstance(); + } catch (WrongIpException $e) { + $this->logger->warning( + sprintf('Tried to locate visit with id "%s", but its address seems to be wrong. {e}', $visitId), + ['e' => $e] + ); + return; + } + + $visit->locate(new VisitLocation($location)); + $this->em->flush($visit); + } +} diff --git a/module/Core/src/EventDispatcher/ShortUrlVisited.php b/module/Core/src/EventDispatcher/ShortUrlVisited.php new file mode 100644 index 00000000..0984f3e9 --- /dev/null +++ b/module/Core/src/EventDispatcher/ShortUrlVisited.php @@ -0,0 +1,27 @@ +visitId = $visitId; + } + + public function visitId(): string + { + return $this->visitId; + } + + public function jsonSerialize(): array + { + return ['visitId' => $this->visitId]; + } +} diff --git a/module/Core/src/Service/VisitsTracker.php b/module/Core/src/Service/VisitsTracker.php index 6b5b9873..e5723850 100644 --- a/module/Core/src/Service/VisitsTracker.php +++ b/module/Core/src/Service/VisitsTracker.php @@ -4,8 +4,10 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; use Doctrine\ORM; +use Psr\EventDispatcher\EventDispatcherInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\EventDispatcher\ShortUrlVisited; use Shlinkio\Shlink\Core\Exception\InvalidArgumentException; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; @@ -19,10 +21,13 @@ class VisitsTracker implements VisitsTrackerInterface { /** @var ORM\EntityManagerInterface */ private $em; + /** @var EventDispatcherInterface */ + private $eventDispatcher; - public function __construct(ORM\EntityManagerInterface $em) + public function __construct(ORM\EntityManagerInterface $em, EventDispatcherInterface $eventDispatcher) { $this->em = $em; + $this->eventDispatcher = $eventDispatcher; } /** @@ -41,6 +46,8 @@ class VisitsTracker implements VisitsTrackerInterface $em = $this->em; $em->persist($visit); $em->flush($visit); + + $this->eventDispatcher->dispatch(new ShortUrlVisited($visit->getId())); } /** diff --git a/module/Core/src/Visit/Model/UnknownVisitLocation.php b/module/Core/src/Visit/Model/UnknownVisitLocation.php index 118e38bd..1cfbe4b1 100644 --- a/module/Core/src/Visit/Model/UnknownVisitLocation.php +++ b/module/Core/src/Visit/Model/UnknownVisitLocation.php @@ -25,14 +25,7 @@ final class UnknownVisitLocation implements VisitLocationInterface return 'Unknown'; } - /** - * Specify data which should be serialized to JSON - * @link https://php.net/manual/en/jsonserializable.jsonserialize.php - * @return mixed data which can be serialized by json_encode, - * which is a value of any type other than a resource. - * @since 5.4.0 - */ - public function jsonSerialize() + public function jsonSerialize(): array { return [ 'countryCode' => 'Unknown', diff --git a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php new file mode 100644 index 00000000..ced0bd63 --- /dev/null +++ b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php @@ -0,0 +1,199 @@ +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->dbUpdater->reveal() + ); + } + + /** @test */ + public function invalidVisitLogsWarning(): void + { + $event = new ShortUrlVisited('123'); + $findVisit = $this->em->find(Visit::class, '123')->willReturn(null); + $logWarning = $this->logger->warning('Tried to locate visit with id "123", but it does not exist.'); + + ($this->locateVisit)($event); + + $findVisit->shouldHaveBeenCalledOnce(); + $this->em->flush(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->ipLocationResolver->resolveIpLocation(Argument::cetera())->shouldNotHaveBeenCalled(); + $logWarning->shouldHaveBeenCalled(); + } + + /** @test */ + public function invalidAddressLogsWarning(): void + { + $event = new ShortUrlVisited('123'); + $findVisit = $this->em->find(Visit::class, '123')->willReturn( + new Visit(new ShortUrl(''), new Visitor('', '', '1.2.3.4')) + ); + $resolveLocation = $this->ipLocationResolver->resolveIpLocation(Argument::cetera())->willThrow( + WrongIpException::class + ); + $logWarning = $this->logger->warning( + Argument::containingString('Tried to locate visit with id "123", but its address seems to be wrong.'), + Argument::type('array') + ); + + ($this->locateVisit)($event); + + $findVisit->shouldHaveBeenCalledOnce(); + $resolveLocation->shouldHaveBeenCalledOnce(); + $logWarning->shouldHaveBeenCalled(); + $this->em->flush(Argument::cetera())->shouldNotHaveBeenCalled(); + } + + /** + * @test + * @dataProvider provideNonLocatableVisits + */ + public function nonLocatableVisitsResolveToEmptyLocations(Visit $visit): void + { + $event = new ShortUrlVisited('123'); + $findVisit = $this->em->find(Visit::class, '123')->willReturn($visit); + $flush = $this->em->flush($visit)->will(function () { + }); + $resolveIp = $this->ipLocationResolver->resolveIpLocation(Argument::any()); + + ($this->locateVisit)($event); + + $this->assertEquals($visit->getVisitLocation(), new VisitLocation(Location::emptyInstance())); + $findVisit->shouldHaveBeenCalledOnce(); + $flush->shouldHaveBeenCalledOnce(); + $resolveIp->shouldNotHaveBeenCalled(); + $this->logger->warning(Argument::cetera())->shouldNotHaveBeenCalled(); + } + + public function provideNonLocatableVisits(): iterable + { + $shortUrl = new ShortUrl(''); + + yield 'null IP' => [new Visit($shortUrl, new Visitor('', '', null))]; + yield 'empty IP' => [new Visit($shortUrl, new Visitor('', '', ''))]; + yield 'localhost' => [new Visit($shortUrl, new Visitor('', '', IpAddress::LOCALHOST))]; + } + + /** @test */ + public function locatableVisitsResolveToLocation(): void + { + $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); + + ($this->locateVisit)($event); + + $this->assertEquals($visit->getVisitLocation(), new VisitLocation($location)); + $findVisit->shouldHaveBeenCalledOnce(); + $flush->shouldHaveBeenCalledOnce(); + $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(); + } +} diff --git a/module/Core/test/Service/VisitsTrackerTest.php b/module/Core/test/Service/VisitsTrackerTest.php index c76bfc1f..7caef547 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Service/VisitsTrackerTest.php @@ -9,9 +9,11 @@ use PHPUnit\Framework\Assert; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; +use Psr\EventDispatcher\EventDispatcherInterface; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\EventDispatcher\ShortUrlVisited; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Repository\VisitRepository; @@ -24,15 +26,19 @@ class VisitsTrackerTest extends TestCase private $visitsTracker; /** @var ObjectProphecy */ private $em; + /** @var EventDispatcherInterface */ + private $eventDispatcher; public function setUp(): void { $this->em = $this->prophesize(EntityManager::class); - $this->visitsTracker = new VisitsTracker($this->em->reveal()); + $this->eventDispatcher = $this->prophesize(EventDispatcherInterface::class); + + $this->visitsTracker = new VisitsTracker($this->em->reveal(), $this->eventDispatcher->reveal()); } /** @test */ - public function trackPersistsVisit() + public function trackPersistsVisit(): void { $shortCode = '123ABC'; $repo = $this->prophesize(EntityRepository::class); @@ -40,13 +46,18 @@ class VisitsTrackerTest extends TestCase $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); $this->em->persist(Argument::any())->shouldBeCalledOnce(); - $this->em->flush(Argument::type(Visit::class))->shouldBeCalledOnce(); + $this->em->flush(Argument::that(function (Visit $visit) { + $visit->setId('1'); + return $visit; + }))->shouldBeCalledOnce(); $this->visitsTracker->track($shortCode, Visitor::emptyInstance()); + + $this->eventDispatcher->dispatch(Argument::type(ShortUrlVisited::class))->shouldHaveBeenCalled(); } /** @test */ - public function trackedIpAddressGetsObfuscated() + public function trackedIpAddressGetsObfuscated(): void { $shortCode = '123ABC'; $repo = $this->prophesize(EntityRepository::class); @@ -58,13 +69,18 @@ class VisitsTrackerTest extends TestCase $visit = $args[0]; Assert::assertEquals('4.3.2.0', $visit->getRemoteAddr()); })->shouldBeCalledOnce(); - $this->em->flush(Argument::type(Visit::class))->shouldBeCalledOnce(); + $this->em->flush(Argument::that(function (Visit $visit) { + $visit->setId('1'); + return $visit; + }))->shouldBeCalledOnce(); $this->visitsTracker->track($shortCode, new Visitor('', '', '4.3.2.1')); + + $this->eventDispatcher->dispatch(Argument::type(ShortUrlVisited::class))->shouldHaveBeenCalled(); } /** @test */ - public function infoReturnsVisistForCertainShortCode() + public function infoReturnsVisitsForCertainShortCode(): void { $shortCode = '123ABC'; $repo = $this->prophesize(EntityRepository::class); diff --git a/module/EventDispatcher/config/event_dispatcher.config.php b/module/EventDispatcher/config/event_dispatcher.config.php new file mode 100644 index 00000000..336162b8 --- /dev/null +++ b/module/EventDispatcher/config/event_dispatcher.config.php @@ -0,0 +1,26 @@ + [ + 'regular' => [], + 'async' => [], + ], + + 'dependencies' => [ + 'factories' => [ + Phly\EventDispatcher::class => Phly\EventDispatcherFactory::class, + Psr\ListenerProviderInterface::class => Listener\ListenerProviderFactory::class, + ], + 'aliases' => [ + Psr\EventDispatcherInterface::class => Phly\EventDispatcher::class, + ], + ], + +]; diff --git a/module/EventDispatcher/config/task_runner.config.php b/module/EventDispatcher/config/task_runner.config.php new file mode 100644 index 00000000..a0a23db5 --- /dev/null +++ b/module/EventDispatcher/config/task_runner.config.php @@ -0,0 +1,21 @@ + [ + 'factories' => [ + Async\TaskRunner::class => Async\TaskRunnerFactory::class, + ], + 'delegators' => [ + HttpServer::class => [ + Async\TaskRunnerDelegator::class, + ], + ], + ], + +]; diff --git a/module/EventDispatcher/functions/functions.php b/module/EventDispatcher/functions/functions.php new file mode 100644 index 00000000..a1c93231 --- /dev/null +++ b/module/EventDispatcher/functions/functions.php @@ -0,0 +1,11 @@ +logger = $logger; + $this->container = $container; + } + + public function __invoke(HttpServer $server, int $taskId, int $fromId, $task): void + { + if (! $task instanceof TaskInterface) { + $this->logger->warning('Invalid task provided to task worker: {type}. Task ignored', [ + 'type' => is_object($task) ? get_class($task) : gettype($task), + ]); + $server->finish(''); + return; + } + + $this->logger->notice('Starting work on task {taskId}: {task}', [ + 'taskId' => $taskId, + 'task' => $task->toString(), + ]); + + try { + $task->run($this->container); + } catch (Throwable $e) { + $this->logger->error('Error processing task {taskId}: {e}', [ + 'taskId' => $taskId, + 'e' => $e, + ]); + } finally { + $server->finish(''); + } + } +} diff --git a/module/EventDispatcher/src/Async/TaskRunnerDelegator.php b/module/EventDispatcher/src/Async/TaskRunnerDelegator.php new file mode 100644 index 00000000..887313df --- /dev/null +++ b/module/EventDispatcher/src/Async/TaskRunnerDelegator.php @@ -0,0 +1,29 @@ +get(LoggerInterface::class); + + $server->on('task', $container->get(TaskRunner::class)); + $server->on('finish', function (HttpServer $server, int $taskId) use ($logger) { + $logger->notice('Task #{taskId} has finished processing', ['taskId' => $taskId]); + }); + + return $server; + } +} diff --git a/module/EventDispatcher/src/Async/TaskRunnerFactory.php b/module/EventDispatcher/src/Async/TaskRunnerFactory.php new file mode 100644 index 00000000..b50ee791 --- /dev/null +++ b/module/EventDispatcher/src/Async/TaskRunnerFactory.php @@ -0,0 +1,17 @@ +get(LoggerInterface::class); + return new TaskRunner($logger, $container); + } +} diff --git a/module/EventDispatcher/src/ConfigProvider.php b/module/EventDispatcher/src/ConfigProvider.php new file mode 100644 index 00000000..70b6ab2e --- /dev/null +++ b/module/EventDispatcher/src/ConfigProvider.php @@ -0,0 +1,15 @@ +regularListenerName = $regularListenerName; + $this->server = $server; + } + + public function __invoke(object $event): void + { + $this->server->task(new EventListenerTask($this->regularListenerName, $event)); + } +} diff --git a/module/EventDispatcher/src/Listener/EventListenerTask.php b/module/EventDispatcher/src/Listener/EventListenerTask.php new file mode 100644 index 00000000..bc1e3689 --- /dev/null +++ b/module/EventDispatcher/src/Listener/EventListenerTask.php @@ -0,0 +1,34 @@ +listenerName = $listenerName; + $this->event = $event; + } + + public function run(ContainerInterface $container): void + { + ($container->get($this->listenerName))($this->event); + } + + public function toString(): string + { + return sprintf('Listener -> "%s", Event -> "%s"', $this->listenerName, get_class($this->event)); + } +} diff --git a/module/EventDispatcher/src/Listener/ListenerProviderFactory.php b/module/EventDispatcher/src/Listener/ListenerProviderFactory.php new file mode 100644 index 00000000..1fb629f2 --- /dev/null +++ b/module/EventDispatcher/src/Listener/ListenerProviderFactory.php @@ -0,0 +1,53 @@ +has('config') ? $container->get('config') : []; + $events = $config['events'] ?? []; + $provider = new AttachableListenerProvider(); + + $this->registerListeners($events['regular'] ?? [], $container, $provider); + $this->registerListeners($events['async'] ?? [], $container, $provider, true); + + return $provider; + } + + private function registerListeners( + array $events, + ContainerInterface $container, + AttachableListenerProvider $provider, + bool $isAsync = false + ): void { + if (empty($events)) { + return; + } + + // Avoid registering async event listeners when the swoole server is not registered + if ($isAsync && ! $container->has(HttpServer::class)) { + return; + } + + foreach ($events as $eventName => $listeners) { + foreach ($listeners as $listenerName) { + $eventListener = $isAsync + ? asyncListener($container->get(HttpServer::class), $listenerName) + : lazyListener($container, $listenerName); + + $provider->listen($eventName, $eventListener); + } + } + } +} diff --git a/module/EventDispatcher/test/Async/TaskRunnerDelegatorTest.php b/module/EventDispatcher/test/Async/TaskRunnerDelegatorTest.php new file mode 100644 index 00000000..82c6b280 --- /dev/null +++ b/module/EventDispatcher/test/Async/TaskRunnerDelegatorTest.php @@ -0,0 +1,46 @@ +delegator = new TaskRunnerDelegator(); + } + + /** @test */ + public function serverIsFetchedFromCallbackAndDecorated(): void + { + $server = $this->createMock(HttpServer::class); + $server + ->expects($this->exactly(2)) + ->method('on'); + $callback = function () use ($server) { + return $server; + }; + + $container = $this->prophesize(ContainerInterface::class); + $getTaskRunner = $container->get(TaskRunner::class)->willReturn($this->prophesize(TaskRunner::class)->reveal()); + $getLogger = $container->get(LoggerInterface::class)->willReturn( + $this->prophesize(LoggerInterface::class)->reveal() + ); + + $result = ($this->delegator)($container->reveal(), '', $callback); + + $this->assertSame($server, $result); + $getTaskRunner->shouldHaveBeenCalledOnce(); + $getLogger->shouldHaveBeenCalledOnce(); + } +} diff --git a/module/EventDispatcher/test/Async/TaskRunnerFactoryTest.php b/module/EventDispatcher/test/Async/TaskRunnerFactoryTest.php new file mode 100644 index 00000000..418abe29 --- /dev/null +++ b/module/EventDispatcher/test/Async/TaskRunnerFactoryTest.php @@ -0,0 +1,48 @@ +factory = new TaskRunnerFactory(); + } + + /** @test */ + public function properlyCreatesService(): void + { + $loggerMock = $this->prophesize(LoggerInterface::class); + $logger = $loggerMock->reveal(); + $containerMock = $this->prophesize(ContainerInterface::class); + $getLogger = $containerMock->get(LoggerInterface::class)->willReturn($logger); + $container = $containerMock->reveal(); + + $taskRunner = ($this->factory)($container, ''); + $loggerProp = $this->getPropertyFromTaskRunner($taskRunner, 'logger'); + $containerProp = $this->getPropertyFromTaskRunner($taskRunner, 'container'); + + $this->assertSame($container, $containerProp); + $this->assertSame($logger, $loggerProp); + $getLogger->shouldHaveBeenCalledOnce(); + } + + private function getPropertyFromTaskRunner(TaskRunner $taskRunner, string $propertyName) + { + $ref = new ReflectionObject($taskRunner); + $prop = $ref->getProperty($propertyName); + $prop->setAccessible(true); + return $prop->getValue($taskRunner); + } +} diff --git a/module/EventDispatcher/test/Async/TaskRunnerTest.php b/module/EventDispatcher/test/Async/TaskRunnerTest.php new file mode 100644 index 00000000..0bf56d7c --- /dev/null +++ b/module/EventDispatcher/test/Async/TaskRunnerTest.php @@ -0,0 +1,107 @@ +logger = $this->prophesize(LoggerInterface::class); + $this->container = $this->prophesize(ContainerInterface::class); + $this->task = $this->prophesize(TaskInterface::class); + + $this->server = $this->createMock(HttpServer::class); + $this->server + ->expects($this->once()) + ->method('finish') + ->with(''); + + $this->taskRunner = new TaskRunner($this->logger->reveal(), $this->container->reveal()); + } + + /** @test */ + public function warningIsLoggedWhenProvidedTaskIsInvalid(): void + { + $logWarning = $this->logger->warning('Invalid task provided to task worker: {type}. Task ignored', [ + 'type' => 'string', + ]); + $logInfo = $this->logger->info(Argument::cetera()); + $logError = $this->logger->error(Argument::cetera()); + + ($this->taskRunner)($this->server, 1, 1, 'invalid_task'); + + $logWarning->shouldHaveBeenCalledOnce(); + $logInfo->shouldNotHaveBeenCalled(); + $logError->shouldNotHaveBeenCalled(); + } + + /** @test */ + public function properTasksAreRun(): void + { + $logWarning = $this->logger->warning(Argument::cetera()); + $logInfo = $this->logger->notice('Starting work on task {taskId}: {task}', [ + 'taskId' => 1, + 'task' => 'The task', + ]); + $logError = $this->logger->error(Argument::cetera()); + $taskToString = $this->task->toString()->willReturn('The task'); + $taskRun = $this->task->run($this->container->reveal())->will(function () { + }); + + ($this->taskRunner)($this->server, 1, 1, $this->task->reveal()); + + $logWarning->shouldNotHaveBeenCalled(); + $logInfo->shouldHaveBeenCalledOnce(); + $logError->shouldNotHaveBeenCalled(); + $taskToString->shouldHaveBeenCalledOnce(); + $taskRun->shouldHaveBeenCalledOnce(); + } + + /** @test */ + public function errorIsLoggedWhenTasksFail(): void + { + $e = new Exception('Error'); + + $logWarning = $this->logger->warning(Argument::cetera()); + $logInfo = $this->logger->notice('Starting work on task {taskId}: {task}', [ + 'taskId' => 1, + 'task' => 'The task', + ]); + $logError = $this->logger->error('Error processing task {taskId}: {e}', [ + 'taskId' => 1, + 'e' => $e, + ]); + $taskToString = $this->task->toString()->willReturn('The task'); + $taskRun = $this->task->run($this->container->reveal())->willThrow($e); + + ($this->taskRunner)($this->server, 1, 1, $this->task->reveal()); + + $logWarning->shouldNotHaveBeenCalled(); + $logInfo->shouldHaveBeenCalledOnce(); + $logError->shouldHaveBeenCalledOnce(); + $taskToString->shouldHaveBeenCalledOnce(); + $taskRun->shouldHaveBeenCalledOnce(); + } +} diff --git a/module/EventDispatcher/test/ConfigProviderTest.php b/module/EventDispatcher/test/ConfigProviderTest.php new file mode 100644 index 00000000..8c344467 --- /dev/null +++ b/module/EventDispatcher/test/ConfigProviderTest.php @@ -0,0 +1,27 @@ +configProvider = new ConfigProvider(); + } + + /** @test */ + public function configIsReturned(): void + { + $config = $this->configProvider->__invoke(); + + $this->assertArrayHasKey('dependencies', $config); + $this->assertArrayHasKey('events', $config); + } +} diff --git a/module/EventDispatcher/test/Listener/AsyncEventListenerTest.php b/module/EventDispatcher/test/Listener/AsyncEventListenerTest.php new file mode 100644 index 00000000..554528cd --- /dev/null +++ b/module/EventDispatcher/test/Listener/AsyncEventListenerTest.php @@ -0,0 +1,41 @@ +regularListenerName = 'the_regular_listener'; + $this->server = $this->createMock(HttpServer::class); + + $this->eventListener = new AsyncEventListener($this->server, $this->regularListenerName); + } + + /** @test */ + public function enqueuesTaskWhenInvoked(): void + { + $event = new stdClass(); + + $this->server + ->expects($this->once()) + ->method('task') + ->with(new EventListenerTask($this->regularListenerName, $event)); + + ($this->eventListener)($event); + } +} diff --git a/module/EventDispatcher/test/Listener/EventListenerTaskTest.php b/module/EventDispatcher/test/Listener/EventListenerTaskTest.php new file mode 100644 index 00000000..5cc6d5a9 --- /dev/null +++ b/module/EventDispatcher/test/Listener/EventListenerTaskTest.php @@ -0,0 +1,58 @@ +event = new stdClass(); + $this->listenerName = 'the_listener'; + + $this->task = new EventListenerTask($this->listenerName, $this->event); + } + + /** @test */ + public function toStringReturnsTheStringRepresentation(): void + { + $this->assertEquals( + sprintf('Listener -> "%s", Event -> "%s"', $this->listenerName, get_class($this->event)), + $this->task->toString() + ); + } + + /** @test */ + public function runInvokesContainerAndListenerWithEvent(): void + { + $invoked = false; + $container = $this->prophesize(ContainerInterface::class); + $listener = function (object $event) use (&$invoked) { + $invoked = true; + Assert::assertSame($event, $this->event); + }; + + $getListener = $container->get($this->listenerName)->willReturn($listener); + + $this->task->run($container->reveal()); + + $this->assertTrue($invoked); + $getListener->shouldHaveBeenCalledOnce(); + } +} diff --git a/module/EventDispatcher/test/Listener/ListenerProviderFactoryTest.php b/module/EventDispatcher/test/Listener/ListenerProviderFactoryTest.php new file mode 100644 index 00000000..f0c3b47f --- /dev/null +++ b/module/EventDispatcher/test/Listener/ListenerProviderFactoryTest.php @@ -0,0 +1,176 @@ +factory = new ListenerProviderFactory(); + } + + /** + * @test + * @dataProvider provideContainersWithoutEvents + */ + public function noListenersAreAttachedWhenNoConfigOrEventsAreRegistered(ContainerInterface $container): void + { + $provider = ($this->factory)($container, ''); + $listeners = $this->getListenersFromProvider($provider); + + $this->assertInstanceOf(AttachableListenerProvider::class, $provider); + $this->assertEmpty($listeners); + } + + public function provideContainersWithoutEvents(): iterable + { + yield 'no config' => [(function () { + $container = $this->prophesize(ContainerInterface::class); + $container->has('config')->willReturn(false); + + return $container->reveal(); + })()]; + yield 'no events' => [(function () { + $container = $this->prophesize(ContainerInterface::class); + $container->has('config')->willReturn(true); + $container->get('config')->willReturn([]); + + return $container->reveal(); + })()]; + } + + /** @test */ + public function configuredRegularEventsAreProperlyAttached(): void + { + $containerMock = $this->prophesize(ContainerInterface::class); + $containerMock->has('config')->willReturn(true); + $containerMock->get('config')->willReturn([ + 'events' => [ + 'regular' => [ + 'foo' => [ + 'bar', + 'baz', + ], + 'something' => [ + 'some_listener', + 'another_listener', + 'foobar', + ], + ], + ], + ]); + $container = $containerMock->reveal(); + + $provider = ($this->factory)($container, ''); + $listeners = $this->getListenersFromProvider($provider); + + $this->assertInstanceOf(AttachableListenerProvider::class, $provider); + $this->assertEquals([ + 'foo' => [ + lazyListener($container, 'bar'), + lazyListener($container, 'baz'), + ], + 'something' => [ + lazyListener($container, 'some_listener'), + lazyListener($container, 'another_listener'), + lazyListener($container, 'foobar'), + ], + ], $listeners); + } + + /** @test */ + public function configuredAsyncEventsAreProperlyAttached(): void + { + $server = $this->createMock(HttpServer::class); // Some weird errors are thrown if prophesize is used + + $containerMock = $this->prophesize(ContainerInterface::class); + $containerMock->has('config')->willReturn(true); + $containerMock->get('config')->willReturn([ + 'events' => [ + 'async' => [ + 'foo' => [ + 'bar', + 'baz', + ], + 'something' => [ + 'some_listener', + 'another_listener', + 'foobar', + ], + ], + ], + ]); + $containerMock->has(HttpServer::class)->willReturn(true); + $containerMock->get(HttpServer::class)->willReturn($server); + $container = $containerMock->reveal(); + + $provider = ($this->factory)($container, ''); + $listeners = $this->getListenersFromProvider($provider); + + $this->assertInstanceOf(AttachableListenerProvider::class, $provider); + $this->assertEquals([ + 'foo' => [ + asyncListener($server, 'bar'), + asyncListener($server, 'baz'), + ], + 'something' => [ + asyncListener($server, 'some_listener'), + asyncListener($server, 'another_listener'), + asyncListener($server, 'foobar'), + ], + ], $listeners); + } + + /** @test */ + public function ignoresAsyncEventsWhenServerIsNotRegistered(): void + { + $containerMock = $this->prophesize(ContainerInterface::class); + $containerMock->has('config')->willReturn(true); + $containerMock->get('config')->willReturn([ + 'events' => [ + 'async' => [ + 'foo' => [ + 'bar', + 'baz', + ], + 'something' => [ + 'some_listener', + 'another_listener', + 'foobar', + ], + ], + ], + ]); + $containerMock->has(HttpServer::class)->willReturn(false); + $container = $containerMock->reveal(); + + $provider = ($this->factory)($container, ''); + $listeners = $this->getListenersFromProvider($provider); + + $this->assertInstanceOf(AttachableListenerProvider::class, $provider); + $this->assertEmpty($listeners); + } + + private function getListenersFromProvider($provider): array + { + $ref = new ReflectionObject($provider); + $prop = $ref->getProperty('listeners'); + $prop->setAccessible(true); + + return $prop->getValue($provider); + } +} diff --git a/phpstan.neon b/phpstan.neon index 96f5d0c3..7756bc14 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -2,3 +2,4 @@ parameters: ignoreErrors: - '#League\\Plates\\callback#' - '#is not subtype of Throwable#' + - '#ObjectManager::flush()#' diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 8220a06e..259a51ae 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -18,8 +18,8 @@ ./module/CLI/test - - ./module/Installer/test + + ./module/EventDispatcher/test