Merge pull request #1386 from acelaya-forks/feature/mercure-error

Feature/mercure error

CHANGELOG.md

@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 
 ### Changed
 * [#1359](https://github.com/shlinkio/shlink/issues/1359) Hidden database commands.
+* [#1385](https://github.com/shlinkio/shlink/issues/1385) Prevented a big error message from being logged when using Shlink without mercure.
 
 ### Deprecated
 * [#1340](https://github.com/shlinkio/shlink/issues/1340) Deprecated webhooks. New events will only be added to other real-time updates approaches, and webhooks will be completely removed in Shlink 4.0.0.

module/Rest/config/dependencies.config.php

@@ -6,7 +6,9 @@ namespace Shlinkio\Shlink\Rest;
 
 use Laminas\ServiceManager\AbstractFactory\ConfigAbstractFactory;
 use Laminas\ServiceManager\Factory\InvokableFactory;
+use Mezzio\ProblemDetails\ProblemDetailsResponseFactory;
 use Mezzio\Router\Middleware\ImplicitOptionsMiddleware;
+use Psr\Log\LoggerInterface;
 use Shlinkio\Shlink\Common\Mercure\LcobucciJwtProvider;
 use Shlinkio\Shlink\Core\Domain\DomainService;
 use Shlinkio\Shlink\Core\Options;
@@ -49,6 +51,7 @@ return [
             Middleware\ShortUrl\DropDefaultDomainFromRequestMiddleware::class => ConfigAbstractFactory::class,
             Middleware\ShortUrl\DefaultShortCodesLengthMiddleware::class => ConfigAbstractFactory::class,
             Middleware\ShortUrl\OverrideDomainMiddleware::class => ConfigAbstractFactory::class,
+            Middleware\Mercure\NotConfiguredMercureErrorHandler::class => ConfigAbstractFactory::class,
         ],
     ],
 
@@ -90,6 +93,10 @@ return [
             'config.url_shortener.default_short_codes_length',
         ],
         Middleware\ShortUrl\OverrideDomainMiddleware::class => [DomainService::class],
+        Middleware\Mercure\NotConfiguredMercureErrorHandler::class => [
+            ProblemDetailsResponseFactory::class,
+            LoggerInterface::class,
+        ],
     ],
 
 ];

module/Rest/config/routes.config.php

@@ -4,6 +4,8 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Rest;
 
+use Shlinkio\Shlink\Rest\Middleware\Mercure\NotConfiguredMercureErrorHandler;
+
 $contentNegotiationMiddleware = Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class;
 $dropDomainMiddleware = Middleware\ShortUrl\DropDefaultDomainFromRequestMiddleware::class;
 $overrideDomainMiddleware = Middleware\ShortUrl\OverrideDomainMiddleware::class;
@@ -46,7 +48,7 @@ return [
         Action\Domain\ListDomainsAction::getRouteDef(),
         Action\Domain\DomainRedirectsAction::getRouteDef(),
 
-        Action\MercureInfoAction::getRouteDef(),
+        Action\MercureInfoAction::getRouteDef([NotConfiguredMercureErrorHandler::class]),
     ],
 
 ];

module/Rest/src/Action/MercureInfoAction.php

@@ -10,7 +10,6 @@ use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Shlinkio\Shlink\Common\Mercure\JwtProviderInterface;
 use Shlinkio\Shlink\Rest\Exception\MercureException;
-use Throwable;
 
 use function sprintf;
 
@@ -32,12 +31,7 @@ class MercureInfoAction extends AbstractRestAction
 
         $days = $this->mercureConfig['jwt_days_duration'] ?? 1;
         $expiresAt = Chronos::now()->addDays($days);
-
-        try {
-            $jwt = $this->jwtProvider->buildSubscriptionToken($expiresAt);
-        } catch (Throwable $e) {
-            throw MercureException::mercureNotConfigured($e);
-        }
+        $jwt = $this->jwtProvider->buildSubscriptionToken($expiresAt);
 
         return new JsonResponse([
             'mercureHubUrl' => sprintf('%s/.well-known/mercure', $hubUrl),

module/Rest/src/Exception/MercureException.php

@@ -7,7 +7,6 @@ namespace Shlinkio\Shlink\Rest\Exception;
 use Fig\Http\Message\StatusCodeInterface;
 use Mezzio\ProblemDetails\Exception\CommonProblemDetailsExceptionTrait;
 use Mezzio\ProblemDetails\Exception\ProblemDetailsExceptionInterface;
-use Throwable;
 
 class MercureException extends RuntimeException implements ProblemDetailsExceptionInterface
 {
@@ -16,9 +15,9 @@ class MercureException extends RuntimeException implements ProblemDetailsExcepti
     private const TITLE = 'Mercure integration not configured';
     private const TYPE = 'MERCURE_NOT_CONFIGURED';
 
-    public static function mercureNotConfigured(?Throwable $prev = null): self
+    public static function mercureNotConfigured(): self
     {
-        $e = new self('This Shlink instance is not integrated with a mercure hub.', 1, $prev);
+        $e = new self('This Shlink instance is not integrated with a mercure hub.');
 
         $e->detail = $e->getMessage();
         $e->title = self::TITLE;

module/Rest/src/Middleware/Mercure/NotConfiguredMercureErrorHandler.php

@@ -0,0 +1,34 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Rest\Middleware\Mercure;
+
+use Mezzio\ProblemDetails\ProblemDetailsResponseFactory;
+use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Server\MiddlewareInterface;
+use Psr\Http\Server\RequestHandlerInterface;
+use Psr\Log\LoggerInterface;
+use Shlinkio\Shlink\Rest\Exception\MercureException;
+
+class NotConfiguredMercureErrorHandler implements MiddlewareInterface
+{
+    public function __construct(private ProblemDetailsResponseFactory $respFactory, private LoggerInterface $logger)
+    {
+    }
+
+    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
+    {
+        try {
+            return $handler->handle($request);
+        } catch (MercureException $e) {
+            // Throwing this kind of exception makes a big error trace to be logged, for anyone who has decided to not
+            // use mercure.
+            // It happens every time the shlink-web-client is opened, so this mitigates the problem by just logging a
+            // simple warning, and casting the exception to a response on the fly.
+            $this->logger->warning($e->getMessage());
+            return $this->respFactory->createResponseFromThrowable($request, $e);
+        }
+    }
+}

module/Rest/test/Action/MercureInfoActionTest.php

@@ -11,7 +11,6 @@ use PHPUnit\Framework\TestCase;
 use Prophecy\Argument;
 use Prophecy\PhpUnit\ProphecyTrait;
 use Prophecy\Prophecy\ObjectProphecy;
-use RuntimeException;
 use Shlinkio\Shlink\Common\Mercure\JwtProviderInterface;
 use Shlinkio\Shlink\Rest\Action\MercureInfoAction;
 use Shlinkio\Shlink\Rest\Exception\MercureException;
@@ -49,24 +48,6 @@ class MercureInfoActionTest extends TestCase
         yield 'host is null' => [['public_hub_url' => null]];
     }
 
-    /**
-     * @test
-     * @dataProvider provideValidConfigs
-     */
-    public function throwsExceptionWhenBuildingTokenFails(array $mercureConfig): void
-    {
-        $buildToken = $this->provider->buildSubscriptionToken(Argument::any())->willThrow(
-            new RuntimeException('Error'),
-        );
-
-        $action = new MercureInfoAction($this->provider->reveal(), $mercureConfig);
-
-        $this->expectException(MercureException::class);
-        $buildToken->shouldBeCalledOnce();
-
-        $action->handle(ServerRequestFactory::fromGlobals());
-    }
-
     public function provideValidConfigs(): iterable
     {
         yield 'days not defined' => [['public_hub_url' => 'http://foobar.com']];

module/Rest/test/Middleware/Mercure/NotConfiguredMercureErrorHandlerTest.php

@@ -0,0 +1,62 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Rest\Middleware\Mercure;
+
+use Laminas\Diactoros\Response;
+use Laminas\Diactoros\ServerRequestFactory;
+use Mezzio\ProblemDetails\ProblemDetailsResponseFactory;
+use PHPUnit\Framework\TestCase;
+use Prophecy\Argument;
+use Prophecy\PhpUnit\ProphecyTrait;
+use Prophecy\Prophecy\ObjectProphecy;
+use Psr\Http\Server\RequestHandlerInterface;
+use Psr\Log\LoggerInterface;
+use Shlinkio\Shlink\Rest\Exception\MercureException;
+use Shlinkio\Shlink\Rest\Middleware\Mercure\NotConfiguredMercureErrorHandler;
+
+class NotConfiguredMercureErrorHandlerTest extends TestCase
+{
+    use ProphecyTrait;
+
+    private NotConfiguredMercureErrorHandler $middleware;
+    private ObjectProphecy $respFactory;
+    private ObjectProphecy $logger;
+    private ObjectProphecy $handler;
+
+    protected function setUp(): void
+    {
+        $this->respFactory = $this->prophesize(ProblemDetailsResponseFactory::class);
+        $this->logger = $this->prophesize(LoggerInterface::class);
+        $this->middleware = new NotConfiguredMercureErrorHandler($this->respFactory->reveal(), $this->logger->reveal());
+        $this->handler = $this->prophesize(RequestHandlerInterface::class);
+    }
+
+    /** @test */
+    public function requestHandlerIsInvokedWhenNotErrorOccurs(): void
+    {
+        $req = ServerRequestFactory::fromGlobals();
+        $handle = $this->handler->handle($req)->willReturn(new Response());
+
+        $this->middleware->process($req, $this->handler->reveal());
+
+        $handle->shouldHaveBeenCalledOnce();
+        $this->logger->warning(Argument::cetera())->shouldNotHaveBeenCalled();
+        $this->respFactory->createResponseFromThrowable(Argument::cetera())->shouldNotHaveBeenCalled();
+    }
+
+    /** @test */
+    public function exceptionIsParsedToResponse(): void
+    {
+        $req = ServerRequestFactory::fromGlobals();
+        $handle = $this->handler->handle($req)->willThrow(MercureException::mercureNotConfigured());
+        $createResp = $this->respFactory->createResponseFromThrowable(Argument::cetera())->willReturn(new Response());
+
+        $this->middleware->process($req, $this->handler->reveal());
+
+        $handle->shouldHaveBeenCalledOnce();
+        $createResp->shouldHaveBeenCalledOnce();
+        $this->logger->warning(Argument::cetera())->shouldHaveBeenCalledOnce();
+    }
+}