From 91698034e7284763da51cc2bda75390bdb389feb Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 13 Jul 2019 12:04:21 +0200 Subject: [PATCH 01/15] Added event dispatcher to track when a short URL is visited --- composer.json | 1 + config/autoload/events.global.php | 26 ++++ config/config.php | 2 + module/Common/src/Entity/AbstractEntity.php | 3 + .../ListenerProviderFactory.php | 28 +++++ .../EventDispatcher/SwooleEventDispatcher.php | 43 +++++++ .../ListenerProviderFactoryTest.php | 99 ++++++++++++++++ .../SwooleEventDispatcherTest.php | 41 +++++++ module/Core/config/dependencies.config.php | 3 +- .../Core/config/event_dispatcher.config.php | 25 ++++ module/Core/src/Entity/Visit.php | 5 + .../EventDispatcher/LocateShortUrlVisit.php | 52 ++++++++ .../src/EventDispatcher/ShortUrlVisited.php | 20 ++++ module/Core/src/Service/VisitsTracker.php | 9 +- .../LocateShortUrlVisitTest.php | 111 ++++++++++++++++++ .../Core/test/Service/VisitsTrackerTest.php | 28 ++++- 16 files changed, 488 insertions(+), 8 deletions(-) create mode 100644 config/autoload/events.global.php create mode 100644 module/Common/src/EventDispatcher/ListenerProviderFactory.php create mode 100644 module/Common/src/EventDispatcher/SwooleEventDispatcher.php create mode 100644 module/Common/test/EventDispatcher/ListenerProviderFactoryTest.php create mode 100644 module/Common/test/EventDispatcher/SwooleEventDispatcherTest.php create mode 100644 module/Core/config/event_dispatcher.config.php create mode 100644 module/Core/src/EventDispatcher/LocateShortUrlVisit.php create mode 100644 module/Core/src/EventDispatcher/ShortUrlVisited.php create mode 100644 module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php diff --git a/composer.json b/composer.json index 8f15d83e..ba72716d 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", diff --git a/config/autoload/events.global.php b/config/autoload/events.global.php new file mode 100644 index 00000000..e2d3561e --- /dev/null +++ b/config/autoload/events.global.php @@ -0,0 +1,26 @@ + [ + 'factories' => [ + Common\EventDispatcher\SwooleEventDispatcher::class => ConfigAbstractFactory::class, + Psr\ListenerProviderInterface::class => Common\EventDispatcher\ListenerProviderFactory::class, + ], + 'aliases' => [ + Psr\EventDispatcherInterface::class => Common\EventDispatcher\SwooleEventDispatcher::class, + ], + ], + + ConfigAbstractFactory::class => [ + Common\EventDispatcher\SwooleEventDispatcher::class => [Phly\EventDispatcher::class], + ], + +]; diff --git a/config/config.php b/config/config.php index fcee89fb..4ad8e8bb 100644 --- a/config/config.php +++ b/config/config.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink; use Acelaya\ExpressiveErrorHandler; +use Phly\EventDispatcher; use Zend\ConfigAggregator; use Zend\Expressive; @@ -16,6 +17,7 @@ return (new ConfigAggregator\ConfigAggregator([ Expressive\Plates\ConfigProvider::class, Expressive\Swoole\ConfigProvider::class, ExpressiveErrorHandler\ConfigProvider::class, + EventDispatcher\ConfigProvider::class, Common\ConfigProvider::class, Core\ConfigProvider::class, CLI\ConfigProvider::class, 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/Common/src/EventDispatcher/ListenerProviderFactory.php b/module/Common/src/EventDispatcher/ListenerProviderFactory.php new file mode 100644 index 00000000..a69f019a --- /dev/null +++ b/module/Common/src/EventDispatcher/ListenerProviderFactory.php @@ -0,0 +1,28 @@ +has('config') ? $container->get('config') : []; + $events = $config['events'] ?? []; + $provider = new AttachableListenerProvider(); + + foreach ($events as $eventName => $listeners) { + foreach ($listeners as $listenerName) { + $provider->listen($eventName, lazyListener($container, $listenerName)); + } + } + + return $provider; + } +} diff --git a/module/Common/src/EventDispatcher/SwooleEventDispatcher.php b/module/Common/src/EventDispatcher/SwooleEventDispatcher.php new file mode 100644 index 00000000..9fada852 --- /dev/null +++ b/module/Common/src/EventDispatcher/SwooleEventDispatcher.php @@ -0,0 +1,43 @@ +innerDispatcher = $innerDispatcher; + $this->isSwoole = $isSwoole ?? PHP_SAPI === 'cli' && extension_loaded('swoole'); + } + + /** + * Provide all relevant listeners with an event to process. + * + * @param object $event + * The object to process. + * + * @return object + * The Event that was passed, now modified by listeners. + */ + public function dispatch(object $event) + { + // Do not really dispatch the event if the app is not being run with swoole + if (! $this->isSwoole) { + return $event; + } + + return $this->innerDispatcher->dispatch($event); + } +} diff --git a/module/Common/test/EventDispatcher/ListenerProviderFactoryTest.php b/module/Common/test/EventDispatcher/ListenerProviderFactoryTest.php new file mode 100644 index 00000000..9834c4cd --- /dev/null +++ b/module/Common/test/EventDispatcher/ListenerProviderFactoryTest.php @@ -0,0 +1,99 @@ +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 configuredEventsAreProperlyAttached(): void + { + $containerMock = $this->prophesize(ContainerInterface::class); + $containerMock->has('config')->willReturn(true); + $containerMock->get('config')->willReturn([ + 'events' => [ + '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); + } + + private function getListenersFromProvider($provider): array + { + $ref = new ReflectionObject($provider); + $prop = $ref->getProperty('listeners'); + $prop->setAccessible(true); + + return $prop->getValue($provider); + } +} diff --git a/module/Common/test/EventDispatcher/SwooleEventDispatcherTest.php b/module/Common/test/EventDispatcher/SwooleEventDispatcherTest.php new file mode 100644 index 00000000..50bc8a26 --- /dev/null +++ b/module/Common/test/EventDispatcher/SwooleEventDispatcherTest.php @@ -0,0 +1,41 @@ +innerDispatcher = $this->prophesize(EventDispatcherInterface::class); + } + + /** + * @test + * @dataProvider provideIsSwoole + */ + public function callsInnerDispatcherOnlyWhenInSwooleContext(bool $isSwoole, int $expectedCalls): void + { + $dispatcher = new SwooleEventDispatcher($this->innerDispatcher->reveal(), $isSwoole); + $event = new stdClass(); + + $dispatcher->dispatch($event); + + $this->innerDispatcher->dispatch($event)->shouldHaveBeenCalledTimes($expectedCalls); + } + + public function provideIsSwoole(): iterable + { + yield 'with swoole' => [true, 1]; + yield 'without swoole' => [false, 0]; + } +} 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..fb9be0c4 --- /dev/null +++ b/module/Core/config/event_dispatcher.config.php @@ -0,0 +1,25 @@ + [ + EventDispatcher\ShortUrlVisited::class => [ + EventDispatcher\LocateShortUrlVisit::class, + ], + ], + + 'dependencies' => [ + EventDispatcher\LocateShortUrlVisit::class => ConfigAbstractFactory::class, + ], + + ConfigAbstractFactory::class => [ + EventDispatcher\LocateShortUrlVisit::class => [IpLocationResolverInterface::class, 'em', 'Logger_Shlink'], + ], + +]; 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..defa0bf8 --- /dev/null +++ b/module/Core/src/EventDispatcher/LocateShortUrlVisit.php @@ -0,0 +1,52 @@ +ipLocationResolver = $ipLocationResolver; + $this->em = $em; + $this->logger = $logger; + } + + 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; + } + + $location = $visit->isLocatable() + ? $this->ipLocationResolver->resolveIpLocation($visit->getRemoteAddr()) + : Location::emptyInstance(); + + $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..7874bbc6 --- /dev/null +++ b/module/Core/src/EventDispatcher/ShortUrlVisited.php @@ -0,0 +1,20 @@ +visitId = $visitId; + } + + public function visitId(): string + { + return $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/test/EventDispatcher/LocateShortUrlVisitTest.php b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php new file mode 100644 index 00000000..7dfaacf0 --- /dev/null +++ b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php @@ -0,0 +1,111 @@ +ipLocationResolver = $this->prophesize(IpLocationResolverInterface::class); + $this->em = $this->prophesize(EntityManagerInterface::class); + $this->logger = $this->prophesize(LoggerInterface::class); + + $this->locateVisit = new LocateShortUrlVisit( + $this->ipLocationResolver->reveal(), + $this->em->reveal(), + $this->logger->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 + * @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(); + } +} 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); From 4380b6271518cd98bdb6a47c9e5bd09d6c29da85 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 13 Jul 2019 15:43:54 +0200 Subject: [PATCH 02/15] Fixed event handler not being properly registered as a service --- .../Core/config/event_dispatcher.config.php | 4 +++- .../EventDispatcher/LocateShortUrlVisit.php | 15 +++++++++--- .../LocateShortUrlVisitTest.php | 24 +++++++++++++++++++ phpstan.neon | 1 + 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index fb9be0c4..34ef9e78 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/module/Core/config/event_dispatcher.config.php @@ -15,7 +15,9 @@ return [ ], 'dependencies' => [ - EventDispatcher\LocateShortUrlVisit::class => ConfigAbstractFactory::class, + 'factories' => [ + EventDispatcher\LocateShortUrlVisit::class => ConfigAbstractFactory::class, + ], ], ConfigAbstractFactory::class => [ diff --git a/module/Core/src/EventDispatcher/LocateShortUrlVisit.php b/module/Core/src/EventDispatcher/LocateShortUrlVisit.php index defa0bf8..fa6aaa09 100644 --- a/module/Core/src/EventDispatcher/LocateShortUrlVisit.php +++ b/module/Core/src/EventDispatcher/LocateShortUrlVisit.php @@ -5,6 +5,7 @@ namespace Shlinkio\Shlink\Core\EventDispatcher; use Doctrine\ORM\EntityManagerInterface; use Psr\Log\LoggerInterface; +use Shlinkio\Shlink\Common\Exception\WrongIpException; use Shlinkio\Shlink\Common\IpGeolocation\IpLocationResolverInterface; use Shlinkio\Shlink\Common\IpGeolocation\Model\Location; use Shlinkio\Shlink\Core\Entity\Visit; @@ -42,9 +43,17 @@ class LocateShortUrlVisit return; } - $location = $visit->isLocatable() - ? $this->ipLocationResolver->resolveIpLocation($visit->getRemoteAddr()) - : Location::emptyInstance(); + 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/test/EventDispatcher/LocateShortUrlVisitTest.php b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php index 7dfaacf0..b40ede09 100644 --- a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php +++ b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php @@ -8,6 +8,7 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Psr\Log\LoggerInterface; +use Shlinkio\Shlink\Common\Exception\WrongIpException; use Shlinkio\Shlink\Common\IpGeolocation\IpLocationResolverInterface; use Shlinkio\Shlink\Common\IpGeolocation\Model\Location; use Shlinkio\Shlink\Common\Util\IpAddress; @@ -57,6 +58,29 @@ class LocateShortUrlVisitTest extends TestCase $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 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()#' From 0dfadcbb4a7b5b6854154357bed563b023d343ea Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 14 Jul 2019 10:46:31 +0200 Subject: [PATCH 03/15] Added package to delegate the execution of event listeners to a swoole task worker --- composer.json | 1 + config/autoload/swoole.global.php | 5 +++++ config/config.php | 2 ++ module/Core/config/event_dispatcher.config.php | 6 ++++++ module/Core/src/EventDispatcher/ShortUrlVisited.php | 9 ++++++++- module/Core/src/Visit/Model/UnknownVisitLocation.php | 9 +-------- 6 files changed, 23 insertions(+), 9 deletions(-) diff --git a/composer.json b/composer.json index ba72716d..a93106c5 100644 --- a/composer.json +++ b/composer.json @@ -30,6 +30,7 @@ "mikehaertl/phpwkhtmltopdf": "^2.2", "monolog/monolog": "^1.21", "phly/phly-event-dispatcher": "^1.0", + "phly/phly-swoole-taskworker": "^1.1", "shlinkio/shlink-installer": "^1.1", "symfony/console": "^4.2", "symfony/filesystem": "^4.2", 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 4ad8e8bb..48f638af 100644 --- a/config/config.php +++ b/config/config.php @@ -5,6 +5,7 @@ namespace Shlinkio\Shlink; use Acelaya\ExpressiveErrorHandler; use Phly\EventDispatcher; +use Phly\Swoole\TaskWorker; use Zend\ConfigAggregator; use Zend\Expressive; @@ -18,6 +19,7 @@ return (new ConfigAggregator\ConfigAggregator([ Expressive\Swoole\ConfigProvider::class, ExpressiveErrorHandler\ConfigProvider::class, EventDispatcher\ConfigProvider::class, + TaskWorker\ConfigProvider::class, Common\ConfigProvider::class, Core\ConfigProvider::class, CLI\ConfigProvider::class, diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index 34ef9e78..406e574f 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/module/Core/config/event_dispatcher.config.php @@ -3,6 +3,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core; +use Phly\Swoole\TaskWorker\DeferredListenerDelegator; use Shlinkio\Shlink\Common\IpGeolocation\IpLocationResolverInterface; use Zend\ServiceManager\AbstractFactory\ConfigAbstractFactory; @@ -18,6 +19,11 @@ return [ 'factories' => [ EventDispatcher\LocateShortUrlVisit::class => ConfigAbstractFactory::class, ], + 'delegators' => [ + EventDispatcher\LocateShortUrlVisit::class => [ + DeferredListenerDelegator::class, + ], + ], ], ConfigAbstractFactory::class => [ diff --git a/module/Core/src/EventDispatcher/ShortUrlVisited.php b/module/Core/src/EventDispatcher/ShortUrlVisited.php index 7874bbc6..0984f3e9 100644 --- a/module/Core/src/EventDispatcher/ShortUrlVisited.php +++ b/module/Core/src/EventDispatcher/ShortUrlVisited.php @@ -3,7 +3,9 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\EventDispatcher; -final class ShortUrlVisited +use JsonSerializable; + +final class ShortUrlVisited implements JsonSerializable { /** @var string */ private $visitId; @@ -17,4 +19,9 @@ final class ShortUrlVisited { return $this->visitId; } + + public function jsonSerialize(): array + { + return ['visitId' => $this->visitId]; + } } 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', From bccc1774149ca15d465aeae609504893640569bc Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 18 Jul 2019 19:07:07 +0200 Subject: [PATCH 04/15] Created task running system based on event listener which are transparently cast into tasks --- composer.json | 1 - config/autoload/event_dispatcher.global.php | 20 +++++++ config/autoload/events.global.php | 26 --------- config/config.php | 3 - module/Common/config/task_runner.config.php | 21 +++++++ module/Common/functions/functions.php | 7 +++ .../EventDispatcher/AsyncEventListener.php | 25 +++++++++ .../ListenerProviderFactory.php | 31 +++++++++-- .../EventDispatcher/SwooleEventDispatcher.php | 43 --------------- module/Common/src/EventDispatcher/Task.php | 33 +++++++++++ .../Common/src/EventDispatcher/TaskRunner.php | 55 +++++++++++++++++++ .../EventDispatcher/TaskRunnerDelegator.php | 29 ++++++++++ .../src/EventDispatcher/TaskRunnerFactory.php | 17 ++++++ .../SwooleEventDispatcherTest.php | 41 -------------- .../Core/config/event_dispatcher.config.php | 13 ++--- 15 files changed, 238 insertions(+), 127 deletions(-) create mode 100644 config/autoload/event_dispatcher.global.php delete mode 100644 config/autoload/events.global.php create mode 100644 module/Common/config/task_runner.config.php create mode 100644 module/Common/src/EventDispatcher/AsyncEventListener.php delete mode 100644 module/Common/src/EventDispatcher/SwooleEventDispatcher.php create mode 100644 module/Common/src/EventDispatcher/Task.php create mode 100644 module/Common/src/EventDispatcher/TaskRunner.php create mode 100644 module/Common/src/EventDispatcher/TaskRunnerDelegator.php create mode 100644 module/Common/src/EventDispatcher/TaskRunnerFactory.php delete mode 100644 module/Common/test/EventDispatcher/SwooleEventDispatcherTest.php diff --git a/composer.json b/composer.json index a93106c5..ba72716d 100644 --- a/composer.json +++ b/composer.json @@ -30,7 +30,6 @@ "mikehaertl/phpwkhtmltopdf": "^2.2", "monolog/monolog": "^1.21", "phly/phly-event-dispatcher": "^1.0", - "phly/phly-swoole-taskworker": "^1.1", "shlinkio/shlink-installer": "^1.1", "symfony/console": "^4.2", "symfony/filesystem": "^4.2", diff --git a/config/autoload/event_dispatcher.global.php b/config/autoload/event_dispatcher.global.php new file mode 100644 index 00000000..53964152 --- /dev/null +++ b/config/autoload/event_dispatcher.global.php @@ -0,0 +1,20 @@ + [ + 'factories' => [ + Psr\ListenerProviderInterface::class => Common\EventDispatcher\ListenerProviderFactory::class, + ], + 'aliases' => [ + Psr\EventDispatcherInterface::class => Phly\EventDispatcher::class, + ], + ], + +]; diff --git a/config/autoload/events.global.php b/config/autoload/events.global.php deleted file mode 100644 index e2d3561e..00000000 --- a/config/autoload/events.global.php +++ /dev/null @@ -1,26 +0,0 @@ - [ - 'factories' => [ - Common\EventDispatcher\SwooleEventDispatcher::class => ConfigAbstractFactory::class, - Psr\ListenerProviderInterface::class => Common\EventDispatcher\ListenerProviderFactory::class, - ], - 'aliases' => [ - Psr\EventDispatcherInterface::class => Common\EventDispatcher\SwooleEventDispatcher::class, - ], - ], - - ConfigAbstractFactory::class => [ - Common\EventDispatcher\SwooleEventDispatcher::class => [Phly\EventDispatcher::class], - ], - -]; diff --git a/config/config.php b/config/config.php index 48f638af..e10d3c1e 100644 --- a/config/config.php +++ b/config/config.php @@ -5,10 +5,8 @@ namespace Shlinkio\Shlink; use Acelaya\ExpressiveErrorHandler; use Phly\EventDispatcher; -use Phly\Swoole\TaskWorker; use Zend\ConfigAggregator; use Zend\Expressive; - use function Shlinkio\Shlink\Common\env; return (new ConfigAggregator\ConfigAggregator([ @@ -19,7 +17,6 @@ return (new ConfigAggregator\ConfigAggregator([ Expressive\Swoole\ConfigProvider::class, ExpressiveErrorHandler\ConfigProvider::class, EventDispatcher\ConfigProvider::class, - TaskWorker\ConfigProvider::class, Common\ConfigProvider::class, Core\ConfigProvider::class, CLI\ConfigProvider::class, diff --git a/module/Common/config/task_runner.config.php b/module/Common/config/task_runner.config.php new file mode 100644 index 00000000..8f0e688e --- /dev/null +++ b/module/Common/config/task_runner.config.php @@ -0,0 +1,21 @@ + [ + 'factories' => [ + EventDispatcher\TaskRunner::class => EventDispatcher\TaskRunnerFactory::class, + ], + 'delegators' => [ + HttpServer::class => [ + EventDispatcher\TaskRunnerDelegator::class, + ], + ], + ], + +]; diff --git a/module/Common/functions/functions.php b/module/Common/functions/functions.php index 485df607..17856af2 100644 --- a/module/Common/functions/functions.php +++ b/module/Common/functions/functions.php @@ -3,6 +3,8 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Common; +use Swoole\Http\Server as HttpServer; + use const JSON_ERROR_NONE; use function getenv; @@ -59,3 +61,8 @@ function json_decode(string $json, int $depth = 512, int $options = 0): array return $data; } + +function asyncListener(HttpServer $server, string $regularListenerName): EventDispatcher\AsyncEventListener +{ + return new EventDispatcher\AsyncEventListener($server, $regularListenerName); +} diff --git a/module/Common/src/EventDispatcher/AsyncEventListener.php b/module/Common/src/EventDispatcher/AsyncEventListener.php new file mode 100644 index 00000000..0b65d083 --- /dev/null +++ b/module/Common/src/EventDispatcher/AsyncEventListener.php @@ -0,0 +1,25 @@ +regularListenerName = $regularListenerName; + $this->server = $server; + } + + public function __invoke(object $event): void + { + $this->server->task(new Task($this->regularListenerName, $event)); + } +} diff --git a/module/Common/src/EventDispatcher/ListenerProviderFactory.php b/module/Common/src/EventDispatcher/ListenerProviderFactory.php index a69f019a..3d9de084 100644 --- a/module/Common/src/EventDispatcher/ListenerProviderFactory.php +++ b/module/Common/src/EventDispatcher/ListenerProviderFactory.php @@ -5,9 +5,11 @@ namespace Shlinkio\Shlink\Common\EventDispatcher; use Interop\Container\ContainerInterface; use Phly\EventDispatcher\ListenerProvider\AttachableListenerProvider; +use Swoole\Http\Server as HttpServer; use Zend\ServiceManager\Factory\FactoryInterface; use function Phly\EventDispatcher\lazyListener; +use function Shlinkio\Shlink\Common\asyncListener; class ListenerProviderFactory implements FactoryInterface { @@ -17,12 +19,31 @@ class ListenerProviderFactory implements FactoryInterface $events = $config['events'] ?? []; $provider = new AttachableListenerProvider(); - foreach ($events as $eventName => $listeners) { - foreach ($listeners as $listenerName) { - $provider->listen($eventName, lazyListener($container, $listenerName)); - } - } + $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 { + // 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/Common/src/EventDispatcher/SwooleEventDispatcher.php b/module/Common/src/EventDispatcher/SwooleEventDispatcher.php deleted file mode 100644 index 9fada852..00000000 --- a/module/Common/src/EventDispatcher/SwooleEventDispatcher.php +++ /dev/null @@ -1,43 +0,0 @@ -innerDispatcher = $innerDispatcher; - $this->isSwoole = $isSwoole ?? PHP_SAPI === 'cli' && extension_loaded('swoole'); - } - - /** - * Provide all relevant listeners with an event to process. - * - * @param object $event - * The object to process. - * - * @return object - * The Event that was passed, now modified by listeners. - */ - public function dispatch(object $event) - { - // Do not really dispatch the event if the app is not being run with swoole - if (! $this->isSwoole) { - return $event; - } - - return $this->innerDispatcher->dispatch($event); - } -} diff --git a/module/Common/src/EventDispatcher/Task.php b/module/Common/src/EventDispatcher/Task.php new file mode 100644 index 00000000..f12abb55 --- /dev/null +++ b/module/Common/src/EventDispatcher/Task.php @@ -0,0 +1,33 @@ +regularListenerName = $regularListenerName; + $this->event = $event; + } + + public function __invoke(ContainerInterface $container) + { + ($container->get($this->regularListenerName))($this->event); + } + + public function toString(): string + { + return sprintf('Listener -> "%s", Event -> "%s"', $this->regularListenerName, get_class($this->event)); + } +} diff --git a/module/Common/src/EventDispatcher/TaskRunner.php b/module/Common/src/EventDispatcher/TaskRunner.php new file mode 100644 index 00000000..7329307e --- /dev/null +++ b/module/Common/src/EventDispatcher/TaskRunner.php @@ -0,0 +1,55 @@ +logger = $logger; + $this->container = $container; + } + + public function __invoke(HttpServer $server, int $taskId, int $fromId, $task): void + { + if (! $task instanceof Task) { + $this->logger->error('Invalid task provided to task worker: {type}', [ + '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($this->container); + } catch (Throwable $e) { + $this->logger->error('Error processing task {taskId}: {e}', [ + 'taskId' => $taskId, + 'e' => $e, + ]); + } finally { + // Notify the server that processing of the task has finished: + $server->finish(''); + } + } +} diff --git a/module/Common/src/EventDispatcher/TaskRunnerDelegator.php b/module/Common/src/EventDispatcher/TaskRunnerDelegator.php new file mode 100644 index 00000000..bb7f138c --- /dev/null +++ b/module/Common/src/EventDispatcher/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/Common/src/EventDispatcher/TaskRunnerFactory.php b/module/Common/src/EventDispatcher/TaskRunnerFactory.php new file mode 100644 index 00000000..b24eebdb --- /dev/null +++ b/module/Common/src/EventDispatcher/TaskRunnerFactory.php @@ -0,0 +1,17 @@ +get(LoggerInterface::class); + return new TaskRunner($logger, $container); + } +} diff --git a/module/Common/test/EventDispatcher/SwooleEventDispatcherTest.php b/module/Common/test/EventDispatcher/SwooleEventDispatcherTest.php deleted file mode 100644 index 50bc8a26..00000000 --- a/module/Common/test/EventDispatcher/SwooleEventDispatcherTest.php +++ /dev/null @@ -1,41 +0,0 @@ -innerDispatcher = $this->prophesize(EventDispatcherInterface::class); - } - - /** - * @test - * @dataProvider provideIsSwoole - */ - public function callsInnerDispatcherOnlyWhenInSwooleContext(bool $isSwoole, int $expectedCalls): void - { - $dispatcher = new SwooleEventDispatcher($this->innerDispatcher->reveal(), $isSwoole); - $event = new stdClass(); - - $dispatcher->dispatch($event); - - $this->innerDispatcher->dispatch($event)->shouldHaveBeenCalledTimes($expectedCalls); - } - - public function provideIsSwoole(): iterable - { - yield 'with swoole' => [true, 1]; - yield 'without swoole' => [false, 0]; - } -} diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index 406e574f..ab34fe99 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/module/Core/config/event_dispatcher.config.php @@ -3,15 +3,17 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core; -use Phly\Swoole\TaskWorker\DeferredListenerDelegator; use Shlinkio\Shlink\Common\IpGeolocation\IpLocationResolverInterface; use Zend\ServiceManager\AbstractFactory\ConfigAbstractFactory; return [ 'events' => [ - EventDispatcher\ShortUrlVisited::class => [ - EventDispatcher\LocateShortUrlVisit::class, + 'regular' => [], + 'async' => [ + EventDispatcher\ShortUrlVisited::class => [ + EventDispatcher\LocateShortUrlVisit::class, + ], ], ], @@ -19,11 +21,6 @@ return [ 'factories' => [ EventDispatcher\LocateShortUrlVisit::class => ConfigAbstractFactory::class, ], - 'delegators' => [ - EventDispatcher\LocateShortUrlVisit::class => [ - DeferredListenerDelegator::class, - ], - ], ], ConfigAbstractFactory::class => [ From d086131630556a080d8947bac5792e7d16015e1d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 19 Jul 2019 19:54:39 +0200 Subject: [PATCH 05/15] Moved all event-dispatching stuff to its own module --- composer.json | 10 ++++--- config/autoload/event_dispatcher.global.php | 20 -------------- config/config.php | 4 +-- module/Common/functions/functions.php | 7 ----- .../config/event_dispatcher.config.php | 27 +++++++++++++++++++ .../config/task_runner.config.php | 6 ++--- .../EventDispatcher/functions/functions.php | 11 ++++++++ .../src/Async}/Task.php | 2 +- .../src/Async}/TaskRunner.php | 2 +- .../src/Async}/TaskRunnerDelegator.php | 2 +- .../src/Async}/TaskRunnerFactory.php | 2 +- module/EventDispatcher/src/ConfigProvider.php | 15 +++++++++++ .../src/Listener}/AsyncEventListener.php | 3 ++- .../src/Listener}/ListenerProviderFactory.php | 4 +-- .../Listener}/ListenerProviderFactoryTest.php | 4 +-- phpunit.xml.dist | 4 +-- 16 files changed, 77 insertions(+), 46 deletions(-) delete mode 100644 config/autoload/event_dispatcher.global.php create mode 100644 module/EventDispatcher/config/event_dispatcher.config.php rename module/{Common => EventDispatcher}/config/task_runner.config.php (56%) create mode 100644 module/EventDispatcher/functions/functions.php rename module/{Common/src/EventDispatcher => EventDispatcher/src/Async}/Task.php (93%) rename module/{Common/src/EventDispatcher => EventDispatcher/src/Async}/TaskRunner.php (96%) rename module/{Common/src/EventDispatcher => EventDispatcher/src/Async}/TaskRunnerDelegator.php (94%) rename module/{Common/src/EventDispatcher => EventDispatcher/src/Async}/TaskRunnerFactory.php (89%) create mode 100644 module/EventDispatcher/src/ConfigProvider.php rename module/{Common/src/EventDispatcher => EventDispatcher/src/Listener}/AsyncEventListener.php (84%) rename module/{Common/src/EventDispatcher => EventDispatcher/src/Listener}/ListenerProviderFactory.php (93%) rename module/{Common/test/EventDispatcher => EventDispatcher/test/Listener}/ListenerProviderFactoryTest.php (95%) diff --git a/composer.json b/composer.json index ba72716d..e6e20989 100644 --- a/composer.json +++ b/composer.json @@ -53,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", @@ -70,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": { @@ -88,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/event_dispatcher.global.php b/config/autoload/event_dispatcher.global.php deleted file mode 100644 index 53964152..00000000 --- a/config/autoload/event_dispatcher.global.php +++ /dev/null @@ -1,20 +0,0 @@ - [ - 'factories' => [ - Psr\ListenerProviderInterface::class => Common\EventDispatcher\ListenerProviderFactory::class, - ], - 'aliases' => [ - Psr\EventDispatcherInterface::class => Phly\EventDispatcher::class, - ], - ], - -]; diff --git a/config/config.php b/config/config.php index e10d3c1e..f09bb9f6 100644 --- a/config/config.php +++ b/config/config.php @@ -4,9 +4,9 @@ declare(strict_types=1); namespace Shlinkio\Shlink; use Acelaya\ExpressiveErrorHandler; -use Phly\EventDispatcher; use Zend\ConfigAggregator; use Zend\Expressive; + use function Shlinkio\Shlink\Common\env; return (new ConfigAggregator\ConfigAggregator([ @@ -16,11 +16,11 @@ return (new ConfigAggregator\ConfigAggregator([ Expressive\Plates\ConfigProvider::class, Expressive\Swoole\ConfigProvider::class, ExpressiveErrorHandler\ConfigProvider::class, - EventDispatcher\ConfigProvider::class, Common\ConfigProvider::class, 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/module/Common/functions/functions.php b/module/Common/functions/functions.php index 17856af2..485df607 100644 --- a/module/Common/functions/functions.php +++ b/module/Common/functions/functions.php @@ -3,8 +3,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Common; -use Swoole\Http\Server as HttpServer; - use const JSON_ERROR_NONE; use function getenv; @@ -61,8 +59,3 @@ function json_decode(string $json, int $depth = 512, int $options = 0): array return $data; } - -function asyncListener(HttpServer $server, string $regularListenerName): EventDispatcher\AsyncEventListener -{ - return new EventDispatcher\AsyncEventListener($server, $regularListenerName); -} diff --git a/module/EventDispatcher/config/event_dispatcher.config.php b/module/EventDispatcher/config/event_dispatcher.config.php new file mode 100644 index 00000000..9930d9e3 --- /dev/null +++ b/module/EventDispatcher/config/event_dispatcher.config.php @@ -0,0 +1,27 @@ + [ + '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/Common/config/task_runner.config.php b/module/EventDispatcher/config/task_runner.config.php similarity index 56% rename from module/Common/config/task_runner.config.php rename to module/EventDispatcher/config/task_runner.config.php index 8f0e688e..a0a23db5 100644 --- a/module/Common/config/task_runner.config.php +++ b/module/EventDispatcher/config/task_runner.config.php @@ -1,7 +1,7 @@ [ 'factories' => [ - EventDispatcher\TaskRunner::class => EventDispatcher\TaskRunnerFactory::class, + Async\TaskRunner::class => Async\TaskRunnerFactory::class, ], 'delegators' => [ HttpServer::class => [ - EventDispatcher\TaskRunnerDelegator::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 @@ + ./module/CLI/test - - ./module/Installer/test + + ./module/EventDispatcher/test From af40e8de5c062d28f1903e33e16747c8605cff33 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 19 Jul 2019 20:28:28 +0200 Subject: [PATCH 06/15] Improved ListenerProviderFactoryTest --- config/test/test_config.global.php | 2 + .../config/event_dispatcher.config.php | 1 - .../src/Listener/ListenerProviderFactory.php | 4 + .../Listener/ListenerProviderFactoryTest.php | 95 +++++++++++++++++-- 4 files changed, 92 insertions(+), 10 deletions(-) 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/EventDispatcher/config/event_dispatcher.config.php b/module/EventDispatcher/config/event_dispatcher.config.php index 9930d9e3..336162b8 100644 --- a/module/EventDispatcher/config/event_dispatcher.config.php +++ b/module/EventDispatcher/config/event_dispatcher.config.php @@ -5,7 +5,6 @@ namespace Shlinkio\Shlink\EventDispatcher; use Phly\EventDispatcher as Phly; use Psr\EventDispatcher as Psr; -use Shlinkio\Shlink\Common; return [ diff --git a/module/EventDispatcher/src/Listener/ListenerProviderFactory.php b/module/EventDispatcher/src/Listener/ListenerProviderFactory.php index 7ad48508..1fb629f2 100644 --- a/module/EventDispatcher/src/Listener/ListenerProviderFactory.php +++ b/module/EventDispatcher/src/Listener/ListenerProviderFactory.php @@ -31,6 +31,10 @@ class ListenerProviderFactory implements FactoryInterface 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; diff --git a/module/EventDispatcher/test/Listener/ListenerProviderFactoryTest.php b/module/EventDispatcher/test/Listener/ListenerProviderFactoryTest.php index 165bac37..f0c3b47f 100644 --- a/module/EventDispatcher/test/Listener/ListenerProviderFactoryTest.php +++ b/module/EventDispatcher/test/Listener/ListenerProviderFactoryTest.php @@ -8,8 +8,10 @@ use Phly\EventDispatcher\ListenerProvider\AttachableListenerProvider; use PHPUnit\Framework\TestCase; use ReflectionObject; use Shlinkio\Shlink\EventDispatcher\Listener\ListenerProviderFactory; +use Swoole\Http\Server as HttpServer; use function Phly\EventDispatcher\lazyListener; +use function Shlinkio\Shlink\EventDispatcher\asyncListener; class ListenerProviderFactoryTest extends TestCase { @@ -52,20 +54,22 @@ class ListenerProviderFactoryTest extends TestCase } /** @test */ - public function configuredEventsAreProperlyAttached(): void + public function configuredRegularEventsAreProperlyAttached(): void { $containerMock = $this->prophesize(ContainerInterface::class); $containerMock->has('config')->willReturn(true); $containerMock->get('config')->willReturn([ 'events' => [ - 'foo' => [ - 'bar', - 'baz', - ], - 'something' => [ - 'some_listener', - 'another_listener', - 'foobar', + 'regular' => [ + 'foo' => [ + 'bar', + 'baz', + ], + 'something' => [ + 'some_listener', + 'another_listener', + 'foobar', + ], ], ], ]); @@ -88,6 +92,79 @@ class ListenerProviderFactoryTest extends TestCase ], $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); From af4ee8f7ec173c9e5832dfc5a5f4a3e3ded71019 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 19 Jul 2019 20:59:06 +0200 Subject: [PATCH 07/15] Created TaskRunnerTest --- module/EventDispatcher/src/Async/Task.php | 33 ------ .../src/Async/TaskInterface.php | 13 +++ .../EventDispatcher/src/Async/TaskRunner.php | 7 +- .../src/Listener/AsyncEventListener.php | 3 +- .../src/Listener/EventListenerTask.php | 34 ++++++ .../test/Async/TaskRunnerTest.php | 107 ++++++++++++++++++ 6 files changed, 158 insertions(+), 39 deletions(-) delete mode 100644 module/EventDispatcher/src/Async/Task.php create mode 100644 module/EventDispatcher/src/Async/TaskInterface.php create mode 100644 module/EventDispatcher/src/Listener/EventListenerTask.php create mode 100644 module/EventDispatcher/test/Async/TaskRunnerTest.php diff --git a/module/EventDispatcher/src/Async/Task.php b/module/EventDispatcher/src/Async/Task.php deleted file mode 100644 index 7a44faa2..00000000 --- a/module/EventDispatcher/src/Async/Task.php +++ /dev/null @@ -1,33 +0,0 @@ -regularListenerName = $regularListenerName; - $this->event = $event; - } - - public function __invoke(ContainerInterface $container) - { - ($container->get($this->regularListenerName))($this->event); - } - - public function toString(): string - { - return sprintf('Listener -> "%s", Event -> "%s"', $this->regularListenerName, get_class($this->event)); - } -} diff --git a/module/EventDispatcher/src/Async/TaskInterface.php b/module/EventDispatcher/src/Async/TaskInterface.php new file mode 100644 index 00000000..b9f20486 --- /dev/null +++ b/module/EventDispatcher/src/Async/TaskInterface.php @@ -0,0 +1,13 @@ +logger->error('Invalid task provided to task worker: {type}', [ + 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(''); @@ -41,14 +41,13 @@ class TaskRunner ]); try { - $task($this->container); + $task->run($this->container); } catch (Throwable $e) { $this->logger->error('Error processing task {taskId}: {e}', [ 'taskId' => $taskId, 'e' => $e, ]); } finally { - // Notify the server that processing of the task has finished: $server->finish(''); } } diff --git a/module/EventDispatcher/src/Listener/AsyncEventListener.php b/module/EventDispatcher/src/Listener/AsyncEventListener.php index 28132ae8..7f59221a 100644 --- a/module/EventDispatcher/src/Listener/AsyncEventListener.php +++ b/module/EventDispatcher/src/Listener/AsyncEventListener.php @@ -3,7 +3,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\EventDispatcher\Listener; -use Shlinkio\Shlink\EventDispatcher\Async\Task; use Swoole\Http\Server as HttpServer; class AsyncEventListener @@ -21,6 +20,6 @@ class AsyncEventListener public function __invoke(object $event): void { - $this->server->task(new Task($this->regularListenerName, $event)); + $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/test/Async/TaskRunnerTest.php b/module/EventDispatcher/test/Async/TaskRunnerTest.php new file mode 100644 index 00000000..a78a741b --- /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(); + } +} From 7e8126a42180accd4fbeff6d1b0a381c6fca3365 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 19 Jul 2019 21:06:34 +0200 Subject: [PATCH 08/15] Added AsyncEventListenerTest --- .../test/Async/TaskRunnerTest.php | 2 +- .../test/Listener/AsyncEventListenerTest.php | 41 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 module/EventDispatcher/test/Listener/AsyncEventListenerTest.php diff --git a/module/EventDispatcher/test/Async/TaskRunnerTest.php b/module/EventDispatcher/test/Async/TaskRunnerTest.php index a78a741b..0bf56d7c 100644 --- a/module/EventDispatcher/test/Async/TaskRunnerTest.php +++ b/module/EventDispatcher/test/Async/TaskRunnerTest.php @@ -21,7 +21,7 @@ class TaskRunnerTest extends TestCase private $logger; /** @var ObjectProphecy */ private $container; - /** @var ObjectProphecy */ + /** @var HttpServer */ private $server; /** @var ObjectProphecy */ private $task; 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); + } +} From bc99ee6ebe1d7101a4487e3c3f69c15ca7c9cc81 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 19 Jul 2019 21:16:09 +0200 Subject: [PATCH 09/15] Created EventListenerTaskTest --- .../test/Listener/EventListenerTaskTest.php | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 module/EventDispatcher/test/Listener/EventListenerTaskTest.php 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(); + } +} From 37e286df48aeb6306482780b93270898c0bf81e2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 20 Jul 2019 10:47:12 +0200 Subject: [PATCH 10/15] Created more tests --- .../test/Async/TaskRunnerDelegatorTest.php | 46 ++++++++++++++++++ .../test/Async/TaskRunnerFactoryTest.php | 48 +++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 module/EventDispatcher/test/Async/TaskRunnerDelegatorTest.php create mode 100644 module/EventDispatcher/test/Async/TaskRunnerFactoryTest.php 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); + } +} From e0e522c3f5439f71c5e7d83d056974db7db5a916 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 20 Jul 2019 11:21:00 +0200 Subject: [PATCH 11/15] Updated LocateShortUrlVisit listener so that it updates geolite db is needed --- .gitignore | 1 + .../Core/config/event_dispatcher.config.php | 8 +++++- .../EventDispatcher/LocateShortUrlVisit.php | 27 ++++++++++++++++++- 3 files changed, 34 insertions(+), 2 deletions(-) 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/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index ab34fe99..767142f1 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/module/Core/config/event_dispatcher.config.php @@ -3,6 +3,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core; +use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdater; use Shlinkio\Shlink\Common\IpGeolocation\IpLocationResolverInterface; use Zend\ServiceManager\AbstractFactory\ConfigAbstractFactory; @@ -24,7 +25,12 @@ return [ ], ConfigAbstractFactory::class => [ - EventDispatcher\LocateShortUrlVisit::class => [IpLocationResolverInterface::class, 'em', 'Logger_Shlink'], + EventDispatcher\LocateShortUrlVisit::class => [ + IpLocationResolverInterface::class, + 'em', + 'Logger_Shlink', + GeolocationDbUpdater::class, + ], ], ]; diff --git a/module/Core/src/EventDispatcher/LocateShortUrlVisit.php b/module/Core/src/EventDispatcher/LocateShortUrlVisit.php index fa6aaa09..de7eb360 100644 --- a/module/Core/src/EventDispatcher/LocateShortUrlVisit.php +++ b/module/Core/src/EventDispatcher/LocateShortUrlVisit.php @@ -5,6 +5,8 @@ namespace Shlinkio\Shlink\Core\EventDispatcher; use Doctrine\ORM\EntityManagerInterface; 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; @@ -21,15 +23,19 @@ class LocateShortUrlVisit private $em; /** @var LoggerInterface */ private $logger; + /** @var GeolocationDbUpdaterInterface */ + private $dbUpdater; public function __construct( IpLocationResolverInterface $ipLocationResolver, EntityManagerInterface $em, - LoggerInterface $logger + LoggerInterface $logger, + GeolocationDbUpdaterInterface $dbUpdater ) { $this->ipLocationResolver = $ipLocationResolver; $this->em = $em; $this->logger = $logger; + $this->dbUpdater = $dbUpdater; } public function __invoke(ShortUrlVisited $shortUrlVisited): void @@ -43,6 +49,25 @@ class LocateShortUrlVisit 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()) From f28540a53e316687743aceefee99788726509e21 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 20 Jul 2019 11:30:26 +0200 Subject: [PATCH 12/15] Updated GeolocationDbUpdater so that it handles a lock which prevents the db to be updated in parallel --- module/CLI/config/dependencies.config.php | 2 +- module/CLI/src/Util/GeolocationDbUpdater.php | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) 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..0d5c4b3e 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,11 @@ 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; + } + try { $meta = $this->geoLiteDbReader->metadata(); if ($this->buildIsTooOld($meta->__get('buildEpoch'))) { @@ -36,6 +47,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(); } } From a1c7e7d5da5b0330bcd3778a81cfe7afdba1416b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 20 Jul 2019 12:11:07 +0200 Subject: [PATCH 13/15] Updated tests --- module/CLI/src/Util/GeolocationDbUpdater.php | 4 +- .../test/Util/GeolocationDbUpdaterTest.php | 16 ++++- .../LocateShortUrlVisitTest.php | 66 ++++++++++++++++++- 3 files changed, 81 insertions(+), 5 deletions(-) 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(); + } } From 4c76df91cec88609ff774e4fab262199f546f791 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 20 Jul 2019 12:16:31 +0200 Subject: [PATCH 14/15] Added ConfigProviderTest for EventDispatcher module --- .../test/ConfigProviderTest.php | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 module/EventDispatcher/test/ConfigProviderTest.php 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); + } +} From 89e4ed5573fa155ae34f7115c44e7497a679bb0b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 20 Jul 2019 12:27:28 +0200 Subject: [PATCH 15/15] Update docs --- CHANGELOG.md | 6 ++++++ README.md | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) 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