diff --git a/CHANGELOG.md b/CHANGELOG.md index 74082f33..5091b0c6 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/module/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index 5f0d5c05..a7a4b2ca 100644 --- a/module/Rest/config/dependencies.config.php +++ b/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, + ], ], ]; diff --git a/module/Rest/config/routes.config.php b/module/Rest/config/routes.config.php index 16f83149..c9b42579 100644 --- a/module/Rest/config/routes.config.php +++ b/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]), ], ]; diff --git a/module/Rest/src/Action/MercureInfoAction.php b/module/Rest/src/Action/MercureInfoAction.php index d6710357..1454cbbc 100644 --- a/module/Rest/src/Action/MercureInfoAction.php +++ b/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), diff --git a/module/Rest/src/Exception/MercureException.php b/module/Rest/src/Exception/MercureException.php index 6c318e93..9435cb54 100644 --- a/module/Rest/src/Exception/MercureException.php +++ b/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; diff --git a/module/Rest/src/Middleware/Mercure/NotConfiguredMercureErrorHandler.php b/module/Rest/src/Middleware/Mercure/NotConfiguredMercureErrorHandler.php new file mode 100644 index 00000000..32a714b6 --- /dev/null +++ b/module/Rest/src/Middleware/Mercure/NotConfiguredMercureErrorHandler.php @@ -0,0 +1,34 @@ +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); + } + } +} diff --git a/module/Rest/test/Action/MercureInfoActionTest.php b/module/Rest/test/Action/MercureInfoActionTest.php index eca4177d..33083c79 100644 --- a/module/Rest/test/Action/MercureInfoActionTest.php +++ b/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']]; diff --git a/module/Rest/test/Middleware/Mercure/NotConfiguredMercureErrorHandlerTest.php b/module/Rest/test/Middleware/Mercure/NotConfiguredMercureErrorHandlerTest.php new file mode 100644 index 00000000..138c01f0 --- /dev/null +++ b/module/Rest/test/Middleware/Mercure/NotConfiguredMercureErrorHandlerTest.php @@ -0,0 +1,62 @@ +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(); + } +}