Merge pull request #724 from acelaya-forks/feature/clean-tasks

Created decorator for database connection closing and reopening for s…

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",

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 => [

module/Core/src/EventDispatcher/CloseDbConnectionEventListener.php

@@ -0,0 +1,32 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\EventDispatcher;
+
+use Shlinkio\Shlink\Common\Doctrine\ReopeningEntityManagerInterface;
+
+class CloseDbConnectionEventListener
+{
+    private ReopeningEntityManagerInterface $em;
+    /** @var callable */
+    private $wrapped;
+
+    public function __construct(ReopeningEntityManagerInterface $em, callable $wrapped)
+    {
+        $this->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();
+        }
+    }
+}

module/Core/src/EventDispatcher/CloseDbConnectionEventListenerDelegator.php

@@ -0,0 +1,24 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\EventDispatcher;
+
+use Psr\Container\ContainerInterface;
+use Shlinkio\Shlink\Common\Doctrine\ReopeningEntityManagerInterface;
+
+class CloseDbConnectionEventListenerDelegator
+{
+    public function __invoke(
+        ContainerInterface $container,
+        string $name,
+        callable $callback
+    ): CloseDbConnectionEventListener {
+        /** @var callable $wrapped */
+        $wrapped = $callback();
+        /** @var ReopeningEntityManagerInterface $em */
+        $em = $container->get('em');
+
+        return new CloseDbConnectionEventListener($em, $wrapped);
+    }
+}

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,15 +41,8 @@ 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) {
@@ -65,12 +57,6 @@ class LocateShortUrlVisit
         }
 
         $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();
-        }
     }
 
     private function downloadOrUpdateGeoLiteDb(string $visitId): bool

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) {
+        $logWebhookFailure = Closure::fromCallable([$this, 'logWebhookFailure']);
-            $promise = $this->httpClient->requestAsync(RequestMethodInterface::METHOD_POST, $webhook, $requestOptions);
+
-            return $promise->otherwise(
+        return map(
-                partial_left(Closure::fromCallable([$this, 'logWebhookFailure']), $webhook, $visitId),
+            $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

module/Core/test/EventDispatcher/CloseDbConnectionEventListenerDelegatorTest.php

@@ -0,0 +1,43 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioApiTest\Shlink\Rest\EventDispatcher;
+
+use PHPUnit\Framework\TestCase;
+use Prophecy\Prophecy\ObjectProphecy;
+use Psr\Container\ContainerInterface;
+use Shlinkio\Shlink\Common\Doctrine\ReopeningEntityManagerInterface;
+use Shlinkio\Shlink\Core\EventDispatcher\CloseDbConnectionEventListenerDelegator;
+
+class CloseDbConnectionEventListenerDelegatorTest extends TestCase
+{
+    private CloseDbConnectionEventListenerDelegator $delegator;
+    private ObjectProphecy $container;
+
+    public function setUp(): void
+    {
+        $this->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();
+    }
+}

module/Core/test/EventDispatcher/CloseDbConnectionEventListenerTest.php

@@ -0,0 +1,75 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioApiTest\Shlink\Core\EventDispatcher;
+
+use Doctrine\DBAL\Connection;
+use PHPUnit\Framework\TestCase;
+use Prophecy\Prophecy\ObjectProphecy;
+use RuntimeException;
+use Shlinkio\Shlink\Common\Doctrine\ReopeningEntityManagerInterface;
+use Shlinkio\Shlink\Core\EventDispatcher\CloseDbConnectionEventListener;
+use stdClass;
+use Throwable;
+
+class CloseDbConnectionEventListenerTest extends TestCase
+{
+    private ObjectProphecy $em;
+
+    public function setUp(): void
+    {
+        $this->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];
+        })();
+    }
+}

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);

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;