diff --git a/composer.json b/composer.json index cd0d699a..164d17df 100644 --- a/composer.json +++ b/composer.json @@ -49,7 +49,7 @@ "predis/predis": "^1.1", "pugx/shortid-php": "^0.5", "ramsey/uuid": "^3.9", - "shlinkio/shlink-common": "^3.0", + "shlinkio/shlink-common": "dev-master#aafa221ec979271713f87e23f17f6a6b5ae5ee67 as 3.0.1", "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^1.4", "shlinkio/shlink-installer": "^4.3.2", diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index 29dbbd11..e885a283 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/module/Core/config/event_dispatcher.config.php @@ -29,6 +29,12 @@ return [ EventDispatcher\LocateShortUrlVisit::class => ConfigAbstractFactory::class, EventDispatcher\NotifyVisitToWebHooks::class => ConfigAbstractFactory::class, ], + + 'delegators' => [ + EventDispatcher\LocateShortUrlVisit::class => [ + EventDispatcher\CloseDbConnectionEventListenerDelegator::class, + ], + ], ], ConfigAbstractFactory::class => [ diff --git a/module/Core/src/EventDispatcher/CloseDbConnectionEventListener.php b/module/Core/src/EventDispatcher/CloseDbConnectionEventListener.php new file mode 100644 index 00000000..7f2c7297 --- /dev/null +++ b/module/Core/src/EventDispatcher/CloseDbConnectionEventListener.php @@ -0,0 +1,32 @@ +em = $em; + $this->wrapped = $wrapped; + } + + public function __invoke(object $event): void + { + $this->em->open(); + + try { + ($this->wrapped)($event); + } finally { + $this->em->getConnection()->close(); + $this->em->clear(); + } + } +} diff --git a/module/Core/src/EventDispatcher/CloseDbConnectionEventListenerDelegator.php b/module/Core/src/EventDispatcher/CloseDbConnectionEventListenerDelegator.php new file mode 100644 index 00000000..cbfc7208 --- /dev/null +++ b/module/Core/src/EventDispatcher/CloseDbConnectionEventListenerDelegator.php @@ -0,0 +1,24 @@ +get('em'); + + return new CloseDbConnectionEventListener($em, $wrapped); + } +} diff --git a/module/Core/src/EventDispatcher/LocateShortUrlVisit.php b/module/Core/src/EventDispatcher/LocateShortUrlVisit.php index ff382770..6abbe02b 100644 --- a/module/Core/src/EventDispatcher/LocateShortUrlVisit.php +++ b/module/Core/src/EventDispatcher/LocateShortUrlVisit.php @@ -9,7 +9,6 @@ use Psr\EventDispatcher\EventDispatcherInterface; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdaterInterface; -use Shlinkio\Shlink\Common\Doctrine\ReopeningEntityManager; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; @@ -42,35 +41,22 @@ class LocateShortUrlVisit public function __invoke(ShortUrlVisited $shortUrlVisited): void { - // FIXME Temporarily handling DB connection reset here to fix https://github.com/shlinkio/shlink/issues/717 - // Remove when https://github.com/shlinkio/shlink-event-dispatcher/issues/23 is implemented - if ($this->em instanceof ReopeningEntityManager) { - $this->em->open(); - } - $visitId = $shortUrlVisited->visitId(); - try { - /** @var Visit|null $visit */ - $visit = $this->em->find(Visit::class, $visitId); - if ($visit === null) { - $this->logger->warning('Tried to locate visit with id "{visitId}", but it does not exist.', [ - 'visitId' => $visitId, - ]); - return; - } - - if ($this->downloadOrUpdateGeoLiteDb($visitId)) { - $this->locateVisit($visitId, $shortUrlVisited->originalIpAddress(), $visit); - } - - $this->eventDispatcher->dispatch(new VisitLocated($visitId)); - } finally { - // FIXME Temporarily handling DB connection reset here to fix https://github.com/shlinkio/shlink/issues/717 - // Remove when https://github.com/shlinkio/shlink-event-dispatcher/issues/23 is implemented - $this->em->getConnection()->close(); - $this->em->clear(); + /** @var Visit|null $visit */ + $visit = $this->em->find(Visit::class, $visitId); + if ($visit === null) { + $this->logger->warning('Tried to locate visit with id "{visitId}", but it does not exist.', [ + 'visitId' => $visitId, + ]); + return; } + + if ($this->downloadOrUpdateGeoLiteDb($visitId)) { + $this->locateVisit($visitId, $shortUrlVisited->originalIpAddress(), $visit); + } + + $this->eventDispatcher->dispatch(new VisitLocated($visitId)); } private function downloadOrUpdateGeoLiteDb(string $visitId): bool diff --git a/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php b/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php index de07d5ef..b3923b86 100644 --- a/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php +++ b/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php @@ -9,6 +9,7 @@ use Doctrine\ORM\EntityManagerInterface; use Fig\Http\Message\RequestMethodInterface; use GuzzleHttp\ClientInterface; use GuzzleHttp\Promise\Promise; +use GuzzleHttp\Promise\PromiseInterface; use GuzzleHttp\RequestOptions; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Core\Entity\Visit; @@ -89,12 +90,14 @@ class NotifyVisitToWebHooks */ private function performRequests(array $requestOptions, string $visitId): array { - return map($this->webhooks, function (string $webhook) use ($requestOptions, $visitId) { - $promise = $this->httpClient->requestAsync(RequestMethodInterface::METHOD_POST, $webhook, $requestOptions); - return $promise->otherwise( - partial_left(Closure::fromCallable([$this, 'logWebhookFailure']), $webhook, $visitId), - ); - }); + $logWebhookFailure = Closure::fromCallable([$this, 'logWebhookFailure']); + + return map( + $this->webhooks, + fn (string $webhook): PromiseInterface => $this->httpClient + ->requestAsync(RequestMethodInterface::METHOD_POST, $webhook, $requestOptions) + ->otherwise(partial_left($logWebhookFailure, $webhook, $visitId)), + ); } private function logWebhookFailure(string $webhook, string $visitId, Throwable $e): void diff --git a/module/Core/test/EventDispatcher/CloseDbConnectionEventListenerDelegatorTest.php b/module/Core/test/EventDispatcher/CloseDbConnectionEventListenerDelegatorTest.php new file mode 100644 index 00000000..41fe94fa --- /dev/null +++ b/module/Core/test/EventDispatcher/CloseDbConnectionEventListenerDelegatorTest.php @@ -0,0 +1,43 @@ +container = $this->prophesize(ContainerInterface::class); + $this->delegator = new CloseDbConnectionEventListenerDelegator(); + } + + /** @test */ + public function properDependenciesArePassed(): void + { + $callbackInvoked = false; + $callback = function () use (&$callbackInvoked): callable { + $callbackInvoked = true; + + return function (): void { + }; + }; + + $em = $this->prophesize(ReopeningEntityManagerInterface::class); + $getEm = $this->container->get('em')->willReturn($em->reveal()); + + ($this->delegator)($this->container->reveal(), '', $callback); + + $this->assertTrue($callbackInvoked); + $getEm->shouldHaveBeenCalledOnce(); + } +} diff --git a/module/Core/test/EventDispatcher/CloseDbConnectionEventListenerTest.php b/module/Core/test/EventDispatcher/CloseDbConnectionEventListenerTest.php new file mode 100644 index 00000000..349e7724 --- /dev/null +++ b/module/Core/test/EventDispatcher/CloseDbConnectionEventListenerTest.php @@ -0,0 +1,75 @@ +em = $this->prophesize(ReopeningEntityManagerInterface::class); + } + + /** + * @test + * @dataProvider provideWrapped + */ + public function connectionIsOpenedBeforeAndClosedAfter(callable $wrapped, bool &$wrappedWasCalled): void + { + $conn = $this->prophesize(Connection::class); + $close = $conn->close()->will(function (): void { + }); + $getConn = $this->em->getConnection()->willReturn($conn->reveal()); + $clear = $this->em->clear()->will(function (): void { + }); + $open = $this->em->open()->will(function (): void { + }); + + $eventListener = new CloseDbConnectionEventListener($this->em->reveal(), $wrapped); + + try { + ($eventListener)(new stdClass()); + } catch (Throwable $e) { + // Ignore exceptions + } + + $this->assertTrue($wrappedWasCalled); + $close->shouldHaveBeenCalledOnce(); + $getConn->shouldHaveBeenCalledOnce(); + $clear->shouldHaveBeenCalledOnce(); + $open->shouldHaveBeenCalledOnce(); + } + + public function provideWrapped(): iterable + { + yield 'does not throw exception' => (function (): array { + $wrappedWasCalled = false; + $wrapped = function () use (&$wrappedWasCalled): void { + $wrappedWasCalled = true; + }; + + return [$wrapped, &$wrappedWasCalled]; + })(); + yield 'throws exception' => (function (): array { + $wrappedWasCalled = false; + $wrapped = function () use (&$wrappedWasCalled): void { + $wrappedWasCalled = true; + throw new RuntimeException('Some error'); + }; + + return [$wrapped, &$wrappedWasCalled]; + })(); + } +} diff --git a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php index e35e7921..087c0e0b 100644 --- a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php +++ b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\EventDispatcher; -use Doctrine\DBAL\Connection; use Doctrine\ORM\EntityManagerInterface; use PHPUnit\Framework\TestCase; use Prophecy\Argument; @@ -38,10 +37,6 @@ class LocateShortUrlVisitTest extends TestCase { $this->ipLocationResolver = $this->prophesize(IpLocationResolverInterface::class); $this->em = $this->prophesize(EntityManagerInterface::class); - $conn = $this->prophesize(Connection::class); - $this->em->getConnection()->willReturn($conn->reveal()); - $this->em->clear()->will(function (): void { - }); $this->logger = $this->prophesize(LoggerInterface::class); $this->dbUpdater = $this->prophesize(GeolocationDbUpdaterInterface::class); $this->eventDispatcher = $this->prophesize(EventDispatcherInterface::class); diff --git a/module/Rest/test/EventDispatcher/NotifyVisitToWebHooksTest.php b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php similarity index 98% rename from module/Rest/test/EventDispatcher/NotifyVisitToWebHooksTest.php rename to module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php index 369960e1..7a138960 100644 --- a/module/Rest/test/EventDispatcher/NotifyVisitToWebHooksTest.php +++ b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace ShlinkioTest\Shlink\Rest\EventDispatcher; +namespace ShlinkioTest\Shlink\Core\EventDispatcher; use Doctrine\ORM\EntityManagerInterface; use Exception;