From ab9042db2464031b8d0310bef5ac82e5a8928098 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 20:25:28 +0100 Subject: [PATCH] Ensured orphan visits are located ASAP when using swoole --- .../Core/config/event_dispatcher.config.php | 10 ++-- module/Core/src/Entity/Visit.php | 5 ++ .../{ShortUrlVisited.php => UrlVisited.php} | 2 +- ...ocateShortUrlVisit.php => LocateVisit.php} | 10 ++-- module/Core/src/Visit/VisitsTracker.php | 15 +++--- .../NotFoundTemplateHandlerTest.php | 1 - ...rtUrlVisitTest.php => LocateVisitTest.php} | 49 ++++++++++++------- module/Core/test/Visit/VisitsTrackerTest.php | 4 +- 8 files changed, 57 insertions(+), 39 deletions(-) rename module/Core/src/EventDispatcher/Event/{ShortUrlVisited.php => UrlVisited.php} (88%) rename module/Core/src/EventDispatcher/{LocateShortUrlVisit.php => LocateVisit.php} (93%) rename module/Core/test/EventDispatcher/{LocateShortUrlVisitTest.php => LocateVisitTest.php} (85%) diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index 66a23637..5c2c88e0 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/module/Core/config/event_dispatcher.config.php @@ -20,28 +20,28 @@ return [ ], ], 'async' => [ - EventDispatcher\Event\ShortUrlVisited::class => [ - EventDispatcher\LocateShortUrlVisit::class, + EventDispatcher\Event\UrlVisited::class => [ + EventDispatcher\LocateVisit::class, ], ], ], 'dependencies' => [ 'factories' => [ - EventDispatcher\LocateShortUrlVisit::class => ConfigAbstractFactory::class, + EventDispatcher\LocateVisit::class => ConfigAbstractFactory::class, EventDispatcher\NotifyVisitToWebHooks::class => ConfigAbstractFactory::class, EventDispatcher\NotifyVisitToMercure::class => ConfigAbstractFactory::class, ], 'delegators' => [ - EventDispatcher\LocateShortUrlVisit::class => [ + EventDispatcher\LocateVisit::class => [ EventDispatcher\CloseDbConnectionEventListenerDelegator::class, ], ], ], ConfigAbstractFactory::class => [ - EventDispatcher\LocateShortUrlVisit::class => [ + EventDispatcher\LocateVisit::class => [ IpLocationResolverInterface::class, 'em', 'Logger_Shlink', diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index d61e8af6..3bcee8e1 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -109,6 +109,11 @@ class Visit extends AbstractEntity implements JsonSerializable return $this; } + public function isOrphan(): bool + { + return $this->shortUrl === null; + } + public function jsonSerialize(): array { return [ diff --git a/module/Core/src/EventDispatcher/Event/ShortUrlVisited.php b/module/Core/src/EventDispatcher/Event/UrlVisited.php similarity index 88% rename from module/Core/src/EventDispatcher/Event/ShortUrlVisited.php rename to module/Core/src/EventDispatcher/Event/UrlVisited.php index f177721f..87b9e4cb 100644 --- a/module/Core/src/EventDispatcher/Event/ShortUrlVisited.php +++ b/module/Core/src/EventDispatcher/Event/UrlVisited.php @@ -4,7 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\EventDispatcher\Event; -final class ShortUrlVisited extends AbstractVisitEvent +final class UrlVisited extends AbstractVisitEvent { private ?string $originalIpAddress; diff --git a/module/Core/src/EventDispatcher/LocateShortUrlVisit.php b/module/Core/src/EventDispatcher/LocateVisit.php similarity index 93% rename from module/Core/src/EventDispatcher/LocateShortUrlVisit.php rename to module/Core/src/EventDispatcher/LocateVisit.php index 8b193578..5e3baf74 100644 --- a/module/Core/src/EventDispatcher/LocateShortUrlVisit.php +++ b/module/Core/src/EventDispatcher/LocateVisit.php @@ -11,7 +11,7 @@ use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdaterInterface; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; -use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlVisited; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; @@ -19,7 +19,7 @@ use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; use function sprintf; -class LocateShortUrlVisit +class LocateVisit { private IpLocationResolverInterface $ipLocationResolver; private EntityManagerInterface $em; @@ -41,7 +41,7 @@ class LocateShortUrlVisit $this->eventDispatcher = $eventDispatcher; } - public function __invoke(ShortUrlVisited $shortUrlVisited): void + public function __invoke(UrlVisited $shortUrlVisited): void { $visitId = $shortUrlVisited->visitId(); @@ -58,7 +58,9 @@ class LocateShortUrlVisit $this->locateVisit($visitId, $shortUrlVisited->originalIpAddress(), $visit); } - $this->eventDispatcher->dispatch(new VisitLocated($visitId)); + if (! $visit->isOrphan()) { + $this->eventDispatcher->dispatch(new VisitLocated($visitId)); + } } private function downloadOrUpdateGeoLiteDb(string $visitId): bool diff --git a/module/Core/src/Visit/VisitsTracker.php b/module/Core/src/Visit/VisitsTracker.php index af170cfc..48157e3b 100644 --- a/module/Core/src/Visit/VisitsTracker.php +++ b/module/Core/src/Visit/VisitsTracker.php @@ -8,7 +8,7 @@ use Doctrine\ORM; use Psr\EventDispatcher\EventDispatcherInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; -use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlVisited; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\Model\Visitor; class VisitsTracker implements VisitsTrackerInterface @@ -29,30 +29,29 @@ class VisitsTracker implements VisitsTrackerInterface public function track(ShortUrl $shortUrl, Visitor $visitor): void { - $visit = $this->trackVisit(Visit::forValidShortUrl($shortUrl, $visitor, $this->anonymizeRemoteAddr)); - $this->eventDispatcher->dispatch(new ShortUrlVisited($visit->getId(), $visitor->getRemoteAddress())); + $this->trackVisit(Visit::forValidShortUrl($shortUrl, $visitor, $this->anonymizeRemoteAddr), $visitor); } public function trackInvalidShortUrlVisit(Visitor $visitor): void { - $this->trackVisit(Visit::forInvalidShortUrl($visitor)); + $this->trackVisit(Visit::forInvalidShortUrl($visitor, $this->anonymizeRemoteAddr), $visitor); } public function trackBaseUrlVisit(Visitor $visitor): void { - $this->trackVisit(Visit::forBasePath($visitor)); + $this->trackVisit(Visit::forBasePath($visitor, $this->anonymizeRemoteAddr), $visitor); } public function trackRegularNotFoundVisit(Visitor $visitor): void { - $this->trackVisit(Visit::forRegularNotFound($visitor)); + $this->trackVisit(Visit::forRegularNotFound($visitor, $this->anonymizeRemoteAddr), $visitor); } - private function trackVisit(Visit $visit): Visit + private function trackVisit(Visit $visit, Visitor $visitor): void { $this->em->persist($visit); $this->em->flush(); - return $visit; + $this->eventDispatcher->dispatch(new UrlVisited($visit->getId(), $visitor->getRemoteAddress())); } } diff --git a/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php b/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php index b5c80de4..dcf42b54 100644 --- a/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php +++ b/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\ErrorHandler; -use Closure; use Laminas\Diactoros\Response; use Laminas\Diactoros\ServerRequestFactory; use Laminas\Diactoros\Uri; diff --git a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php b/module/Core/test/EventDispatcher/LocateVisitTest.php similarity index 85% rename from module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php rename to module/Core/test/EventDispatcher/LocateVisitTest.php index 03eef5f6..af21e3dc 100644 --- a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php +++ b/module/Core/test/EventDispatcher/LocateVisitTest.php @@ -17,19 +17,19 @@ use Shlinkio\Shlink\Common\Util\IpAddress; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; -use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlVisited; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; -use Shlinkio\Shlink\Core\EventDispatcher\LocateShortUrlVisit; +use Shlinkio\Shlink\Core\EventDispatcher\LocateVisit; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; -class LocateShortUrlVisitTest extends TestCase +class LocateVisitTest extends TestCase { use ProphecyTrait; - private LocateShortUrlVisit $locateVisit; + private LocateVisit $locateVisit; private ObjectProphecy $ipLocationResolver; private ObjectProphecy $em; private ObjectProphecy $logger; @@ -44,7 +44,7 @@ class LocateShortUrlVisitTest extends TestCase $this->dbUpdater = $this->prophesize(GeolocationDbUpdaterInterface::class); $this->eventDispatcher = $this->prophesize(EventDispatcherInterface::class); - $this->locateVisit = new LocateShortUrlVisit( + $this->locateVisit = new LocateVisit( $this->ipLocationResolver->reveal(), $this->em->reveal(), $this->logger->reveal(), @@ -56,7 +56,7 @@ class LocateShortUrlVisitTest extends TestCase /** @test */ public function invalidVisitLogsWarning(): void { - $event = new ShortUrlVisited('123'); + $event = new UrlVisited('123'); $findVisit = $this->em->find(Visit::class, '123')->willReturn(null); $logWarning = $this->logger->warning('Tried to locate visit with id "{visitId}", but it does not exist.', [ 'visitId' => 123, @@ -76,7 +76,7 @@ class LocateShortUrlVisitTest extends TestCase /** @test */ public function invalidAddressLogsWarning(): void { - $event = new ShortUrlVisited('123'); + $event = new UrlVisited('123'); $findVisit = $this->em->find(Visit::class, '123')->willReturn( Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), ); @@ -105,7 +105,7 @@ class LocateShortUrlVisitTest extends TestCase */ public function nonLocatableVisitsResolveToEmptyLocations(Visit $visit): void { - $event = new ShortUrlVisited('123'); + $event = new UrlVisited('123'); $findVisit = $this->em->find(Visit::class, '123')->willReturn($visit); $flush = $this->em->flush()->will(function (): void { }); @@ -136,12 +136,14 @@ class LocateShortUrlVisitTest extends TestCase * @test * @dataProvider provideIpAddresses */ - public function locatableVisitsResolveToLocation(string $anonymizedIpAddress, ?string $originalIpAddress): void - { - $ipAddr = $originalIpAddress ?? $anonymizedIpAddress; - $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr, '')); + public function locatableVisitsResolveToLocation( + Visit $visit, + ?string $originalIpAddress, + int $expectedDispatchCalls + ): void { + $ipAddr = $originalIpAddress ?? $visit->getRemoteAddr(); $location = new Location('', '', '', '', 0.0, 0.0, ''); - $event = new ShortUrlVisited('123', $originalIpAddress); + $event = new UrlVisited('123', $originalIpAddress); $findVisit = $this->em->find(Visit::class, '123')->willReturn($visit); $flush = $this->em->flush()->will(function (): void { @@ -157,13 +159,24 @@ class LocateShortUrlVisitTest extends TestCase $flush->shouldHaveBeenCalledOnce(); $resolveIp->shouldHaveBeenCalledOnce(); $this->logger->warning(Argument::cetera())->shouldNotHaveBeenCalled(); - $dispatch->shouldHaveBeenCalledOnce(); + $dispatch->shouldHaveBeenCalledTimes($expectedDispatchCalls); } public function provideIpAddresses(): iterable { - yield 'no original IP address' => ['1.2.3.0', null]; - yield 'original IP address' => ['1.2.3.0', '1.2.3.4']; + yield 'no original IP address' => [ + Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), + null, + 1, + ]; + yield 'original IP address' => [ + Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), + '1.2.3.4', + 1, + ]; + yield 'base url' => [Visit::forBasePath(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4', 0]; + yield 'invalid short url' => [Visit::forInvalidShortUrl(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4', 0]; + yield 'regular not found' => [Visit::forRegularNotFound(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4', 0]; } /** @test */ @@ -173,7 +186,7 @@ class LocateShortUrlVisitTest extends TestCase $ipAddr = '1.2.3.0'; $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr, '')); $location = new Location('', '', '', '', 0.0, 0.0, ''); - $event = new ShortUrlVisited('123'); + $event = new UrlVisited('123'); $findVisit = $this->em->find(Visit::class, '123')->willReturn($visit); $flush = $this->em->flush()->will(function (): void { @@ -204,7 +217,7 @@ class LocateShortUrlVisitTest extends TestCase $ipAddr = '1.2.3.0'; $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr, '')); $location = new Location('', '', '', '', 0.0, 0.0, ''); - $event = new ShortUrlVisited('123'); + $event = new UrlVisited('123'); $findVisit = $this->em->find(Visit::class, '123')->willReturn($visit); $flush = $this->em->flush()->will(function (): void { diff --git a/module/Core/test/Visit/VisitsTrackerTest.php b/module/Core/test/Visit/VisitsTrackerTest.php index a7ca98c3..2d8585d0 100644 --- a/module/Core/test/Visit/VisitsTrackerTest.php +++ b/module/Core/test/Visit/VisitsTrackerTest.php @@ -12,7 +12,7 @@ use Prophecy\Prophecy\ObjectProphecy; use Psr\EventDispatcher\EventDispatcherInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; -use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlVisited; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Visit\VisitsTracker; @@ -42,6 +42,6 @@ class VisitsTrackerTest extends TestCase $this->visitsTracker->track(ShortUrl::withLongUrl($shortCode), Visitor::emptyInstance()); - $this->eventDispatcher->dispatch(Argument::type(ShortUrlVisited::class))->shouldHaveBeenCalled(); + $this->eventDispatcher->dispatch(Argument::type(UrlVisited::class))->shouldHaveBeenCalled(); } }