From c7a621cb31fdbe0f85875b57398e143ecdb4308b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 23 Oct 2021 13:22:42 +0200 Subject: [PATCH 1/2] Removed transactionality when dispatching async events, as they run in different processes with different db connections --- CHANGELOG.md | 17 +++++++++++++++++ composer.json | 2 +- .../src/Command/Db/CreateDatabaseCommand.php | 2 +- module/Core/src/Visit/VisitsTracker.php | 8 +++----- module/Core/test/Visit/VisitsTrackerTest.php | 7 ------- module/Rest/src/Action/HealthAction.php | 2 +- module/Rest/test/Action/HealthActionTest.php | 6 +++--- 7 files changed, 26 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5201b2c9..baa31e0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,23 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). +## [Unreleased] +### Added +* *Nothing* + +### Changed +* *Nothing* + +### Deprecated +* *Nothing* + +### Removed +* *Nothing* + +### Fixed +* [#1210](https://github.com/shlinkio/shlink/issues/1210) Fixed real time updates not being notified. + + ## [2.9.1] - 2021-10-11 ### Added * *Nothing* diff --git a/composer.json b/composer.json index aea53f62..aa13a847 100644 --- a/composer.json +++ b/composer.json @@ -18,7 +18,7 @@ "akrabat/ip-address-middleware": "^2.0", "cakephp/chronos": "^2.2", "cocur/slugify": "^4.0", - "doctrine/migrations": "^3.2", + "doctrine/migrations": "^3.2 <3.3", "doctrine/orm": "^2.9", "endroid/qr-code": "^4.2", "geoip2/geoip2": "^2.11", diff --git a/module/CLI/src/Command/Db/CreateDatabaseCommand.php b/module/CLI/src/Command/Db/CreateDatabaseCommand.php index 100dc49d..428140e5 100644 --- a/module/CLI/src/Command/Db/CreateDatabaseCommand.php +++ b/module/CLI/src/Command/Db/CreateDatabaseCommand.php @@ -71,7 +71,7 @@ class CreateDatabaseCommand extends AbstractDatabaseCommand $databases = $schemaManager->listDatabases(); $shlinkDatabase = $this->regularConn->getDatabase(); - if (! contains($databases, $shlinkDatabase)) { + if ($shlinkDatabase !== null && ! contains($databases, $shlinkDatabase)) { $schemaManager->createDatabase($shlinkDatabase); } } diff --git a/module/Core/src/Visit/VisitsTracker.php b/module/Core/src/Visit/VisitsTracker.php index d5d0dc8e..f4e5bf92 100644 --- a/module/Core/src/Visit/VisitsTracker.php +++ b/module/Core/src/Visit/VisitsTracker.php @@ -69,11 +69,9 @@ class VisitsTracker implements VisitsTrackerInterface } $visit = $createVisit($visitor->normalizeForTrackingOptions($this->options)); - $this->em->transactional(function () use ($visit, $visitor): void { - $this->em->persist($visit); - $this->em->flush(); + $this->em->persist($visit); + $this->em->flush(); - $this->eventDispatcher->dispatch(new UrlVisited($visit->getId(), $visitor->getRemoteAddress())); - }); + $this->eventDispatcher->dispatch(new UrlVisited($visit->getId(), $visitor->getRemoteAddress())); } } diff --git a/module/Core/test/Visit/VisitsTrackerTest.php b/module/Core/test/Visit/VisitsTrackerTest.php index 45188f6c..904f92d1 100644 --- a/module/Core/test/Visit/VisitsTrackerTest.php +++ b/module/Core/test/Visit/VisitsTrackerTest.php @@ -29,10 +29,6 @@ class VisitsTrackerTest extends TestCase public function setUp(): void { $this->em = $this->prophesize(EntityManager::class); - $this->em->transactional(Argument::any())->will(function (array $args) { - [$callback] = $args; - return $callback(); - }); $this->eventDispatcher = $this->prophesize(EventDispatcherInterface::class); $this->options = new TrackingOptions(); @@ -52,7 +48,6 @@ class VisitsTrackerTest extends TestCase $this->visitsTracker->{$method}(...$args); $persist->shouldHaveBeenCalledOnce(); - $this->em->transactional(Argument::cetera())->shouldHaveBeenCalledOnce(); $this->em->flush()->shouldHaveBeenCalledOnce(); $this->eventDispatcher->dispatch(Argument::type(UrlVisited::class))->shouldHaveBeenCalled(); } @@ -68,7 +63,6 @@ class VisitsTrackerTest extends TestCase $this->visitsTracker->{$method}(...$args); $this->eventDispatcher->dispatch(Argument::cetera())->shouldNotHaveBeenCalled(); - $this->em->transactional(Argument::cetera())->shouldNotHaveBeenCalled(); $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); $this->em->flush()->shouldNotHaveBeenCalled(); } @@ -92,7 +86,6 @@ class VisitsTrackerTest extends TestCase $this->visitsTracker->{$method}(Visitor::emptyInstance()); $this->eventDispatcher->dispatch(Argument::cetera())->shouldNotHaveBeenCalled(); - $this->em->transactional(Argument::cetera())->shouldNotHaveBeenCalled(); $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); $this->em->flush()->shouldNotHaveBeenCalled(); } diff --git a/module/Rest/src/Action/HealthAction.php b/module/Rest/src/Action/HealthAction.php index 5f9d052c..e2b0d828 100644 --- a/module/Rest/src/Action/HealthAction.php +++ b/module/Rest/src/Action/HealthAction.php @@ -32,7 +32,7 @@ class HealthAction extends AbstractRestAction public function handle(ServerRequestInterface $request): ResponseInterface { try { - $connected = $this->em->getConnection()->ping(); + $connected = $this->em->getConnection()->isConnected(); } catch (Throwable) { $connected = false; } diff --git a/module/Rest/test/Action/HealthActionTest.php b/module/Rest/test/Action/HealthActionTest.php index bdfc9ccd..59307e40 100644 --- a/module/Rest/test/Action/HealthActionTest.php +++ b/module/Rest/test/Action/HealthActionTest.php @@ -34,7 +34,7 @@ class HealthActionTest extends TestCase /** @test */ public function passResponseIsReturnedWhenConnectionSucceeds(): void { - $ping = $this->conn->ping()->willReturn(true); + $ping = $this->conn->isConnected()->willReturn(true); /** @var JsonResponse $resp */ $resp = $this->action->handle(new ServerRequest()); @@ -54,7 +54,7 @@ class HealthActionTest extends TestCase /** @test */ public function failResponseIsReturnedWhenConnectionFails(): void { - $ping = $this->conn->ping()->willReturn(false); + $ping = $this->conn->isConnected()->willReturn(false); /** @var JsonResponse $resp */ $resp = $this->action->handle(new ServerRequest()); @@ -74,7 +74,7 @@ class HealthActionTest extends TestCase /** @test */ public function failResponseIsReturnedWhenConnectionThrowsException(): void { - $ping = $this->conn->ping()->willThrow(Exception::class); + $ping = $this->conn->isConnected()->willThrow(Exception::class); /** @var JsonResponse $resp */ $resp = $this->action->handle(new ServerRequest()); From a4fde0f9e6b26edd612b2e40ff24cef5aff70c68 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 23 Oct 2021 13:36:27 +0200 Subject: [PATCH 2/2] Changed mechanism to determine if connection to database worked for health endpoint --- module/Rest/src/Action/HealthAction.php | 4 +- module/Rest/test/Action/HealthActionTest.php | 40 +++++++------------- 2 files changed, 16 insertions(+), 28 deletions(-) diff --git a/module/Rest/src/Action/HealthAction.php b/module/Rest/src/Action/HealthAction.php index e2b0d828..462eb345 100644 --- a/module/Rest/src/Action/HealthAction.php +++ b/module/Rest/src/Action/HealthAction.php @@ -32,7 +32,9 @@ class HealthAction extends AbstractRestAction public function handle(ServerRequestInterface $request): ResponseInterface { try { - $connected = $this->em->getConnection()->isConnected(); + $connection = $this->em->getConnection(); + $connection->executeQuery($connection->getDatabasePlatform()->getDummySelectSQL()); + $connected = true; } catch (Throwable) { $connected = false; } diff --git a/module/Rest/test/Action/HealthActionTest.php b/module/Rest/test/Action/HealthActionTest.php index 59307e40..1fbad63d 100644 --- a/module/Rest/test/Action/HealthActionTest.php +++ b/module/Rest/test/Action/HealthActionTest.php @@ -5,11 +5,13 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Rest\Action; use Doctrine\DBAL\Connection; +use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\ORM\EntityManagerInterface; use Exception; use Laminas\Diactoros\Response\JsonResponse; use Laminas\Diactoros\ServerRequest; use PHPUnit\Framework\TestCase; +use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Options\AppOptions; @@ -25,6 +27,12 @@ class HealthActionTest extends TestCase public function setUp(): void { $this->conn = $this->prophesize(Connection::class); + $this->conn->executeQuery(Argument::cetera())->will(function (): void { + }); + $dbPlatform = $this->prophesize(AbstractPlatform::class); + $dbPlatform->getDummySelectSQL()->willReturn(''); + $this->conn->getDatabasePlatform()->willReturn($dbPlatform->reveal()); + $em = $this->prophesize(EntityManagerInterface::class); $em->getConnection()->willReturn($this->conn->reveal()); @@ -32,10 +40,8 @@ class HealthActionTest extends TestCase } /** @test */ - public function passResponseIsReturnedWhenConnectionSucceeds(): void + public function passResponseIsReturnedWhenDummyQuerySucceeds(): void { - $ping = $this->conn->isConnected()->willReturn(true); - /** @var JsonResponse $resp */ $resp = $this->action->handle(new ServerRequest()); $payload = $resp->getPayload(); @@ -48,13 +54,13 @@ class HealthActionTest extends TestCase 'project' => 'https://github.com/shlinkio/shlink', ], $payload['links']); self::assertEquals('application/health+json', $resp->getHeaderLine('Content-type')); - $ping->shouldHaveBeenCalledOnce(); + $this->conn->executeQuery(Argument::cetera())->shouldHaveBeenCalledOnce(); } /** @test */ - public function failResponseIsReturnedWhenConnectionFails(): void + public function failResponseIsReturnedWhenDummyQueryThrowsException(): void { - $ping = $this->conn->isConnected()->willReturn(false); + $executeQuery = $this->conn->executeQuery(Argument::cetera())->willThrow(Exception::class); /** @var JsonResponse $resp */ $resp = $this->action->handle(new ServerRequest()); @@ -68,26 +74,6 @@ class HealthActionTest extends TestCase 'project' => 'https://github.com/shlinkio/shlink', ], $payload['links']); self::assertEquals('application/health+json', $resp->getHeaderLine('Content-type')); - $ping->shouldHaveBeenCalledOnce(); - } - - /** @test */ - public function failResponseIsReturnedWhenConnectionThrowsException(): void - { - $ping = $this->conn->isConnected()->willThrow(Exception::class); - - /** @var JsonResponse $resp */ - $resp = $this->action->handle(new ServerRequest()); - $payload = $resp->getPayload(); - - self::assertEquals(503, $resp->getStatusCode()); - self::assertEquals('fail', $payload['status']); - self::assertEquals('1.2.3', $payload['version']); - self::assertEquals([ - 'about' => 'https://shlink.io', - 'project' => 'https://github.com/shlinkio/shlink', - ], $payload['links']); - self::assertEquals('application/health+json', $resp->getHeaderLine('Content-type')); - $ping->shouldHaveBeenCalledOnce(); + $executeQuery->shouldHaveBeenCalledOnce(); } }