From 6f0afe269d63dfdcc9dd410171ed1537ebe8c6b6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Nov 2019 12:41:12 +0100 Subject: [PATCH] Moved InvalidShortCode exception handling to problem details --- config/autoload/error-handler.global.php | 8 ++ .../autoload/middleware-pipeline.global.php | 3 +- .../Exception/InvalidShortCodeException.php | 7 +- module/Rest/config/dependencies.config.php | 6 ++ .../Action/ShortUrl/EditShortUrlAction.php | 8 -- .../ShortUrl/EditShortUrlTagsAction.php | 14 +-- .../Rest/src/Action/Visit/GetVisitsAction.php | 5 -- module/Rest/src/ConfigProvider.php | 2 +- ...ardsCompatibleProblemDetailsMiddleware.php | 87 +++++++++++++++++++ .../src/Middleware/PathVersionMiddleware.php | 13 +-- .../ShortUrl/EditShortUrlActionTest.php | 26 +----- .../ShortUrl/EditShortUrlTagsActionTest.php | 19 +--- 12 files changed, 113 insertions(+), 85 deletions(-) create mode 100644 module/Rest/src/Middleware/BackwardsCompatibleProblemDetailsMiddleware.php diff --git a/config/autoload/error-handler.global.php b/config/autoload/error-handler.global.php index 4ef36e0a..964b90de 100644 --- a/config/autoload/error-handler.global.php +++ b/config/autoload/error-handler.global.php @@ -8,6 +8,14 @@ use Zend\Stratigility\Middleware\ErrorHandler; return [ + 'backwards_compatible_problem_details' => [ + 'default_type_fallbacks' => [ + 404 => 'NOT_FOUND', + 500 => 'INTERNAL_SERVER_ERROR', + ], + 'json_flags' => JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE | JSON_PRESERVE_ZERO_FRACTION, + ], + 'error_handler' => [ 'listeners' => [Logger\ErrorLogger::class], ], diff --git a/config/autoload/middleware-pipeline.global.php b/config/autoload/middleware-pipeline.global.php index 6013f56a..f3370a7a 100644 --- a/config/autoload/middleware-pipeline.global.php +++ b/config/autoload/middleware-pipeline.global.php @@ -13,19 +13,20 @@ return [ 'middleware_pipeline' => [ 'error-handler' => [ 'middleware' => [ + Expressive\Helper\ContentLengthMiddleware::class, ErrorHandler::class, ], ], 'error-handler-rest' => [ 'path' => '/rest', 'middleware' => [ + Rest\Middleware\BackwardsCompatibleProblemDetailsMiddleware::class, ProblemDetails\ProblemDetailsMiddleware::class, ], ], 'pre-routing' => [ 'middleware' => [ - Expressive\Helper\ContentLengthMiddleware::class, Common\Middleware\CloseDbConnectionMiddleware::class, ], ], diff --git a/module/Core/src/Exception/InvalidShortCodeException.php b/module/Core/src/Exception/InvalidShortCodeException.php index 77240aca..d75cdad7 100644 --- a/module/Core/src/Exception/InvalidShortCodeException.php +++ b/module/Core/src/Exception/InvalidShortCodeException.php @@ -14,20 +14,17 @@ class InvalidShortCodeException extends RuntimeException implements ProblemDetai { use CommonProblemDetailsExceptionTrait; - public const TITLE = 'Invalid short code'; + private const TITLE = 'Invalid short code'; public const TYPE = 'INVALID_SHORTCODE'; public static function fromNotFoundShortCode(string $shortCode): self { $e = new self(sprintf('No URL found for short code "%s"', $shortCode)); + $e->detail = $e->getMessage(); $e->title = self::TITLE; $e->type = self::TYPE; $e->status = StatusCodeInterface::STATUS_NOT_FOUND; - $e->additional = [ - 'error' => $e->type, - 'message' => $e->detail, - ]; return $e; } diff --git a/module/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index 98a26bc1..17c30642 100644 --- a/module/Rest/config/dependencies.config.php +++ b/module/Rest/config/dependencies.config.php @@ -39,6 +39,7 @@ return [ Middleware\BodyParserMiddleware::class => InvokableFactory::class, Middleware\CrossDomainMiddleware::class => InvokableFactory::class, Middleware\PathVersionMiddleware::class => InvokableFactory::class, + Middleware\BackwardsCompatibleProblemDetailsMiddleware::class => ConfigAbstractFactory::class, Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class => InvokableFactory::class, Middleware\ShortUrl\ShortCodePathMiddleware::class => InvokableFactory::class, ], @@ -75,6 +76,11 @@ return [ Action\Tag\DeleteTagsAction::class => [Service\Tag\TagService::class, LoggerInterface::class], Action\Tag\CreateTagsAction::class => [Service\Tag\TagService::class, LoggerInterface::class], Action\Tag\UpdateTagAction::class => [Service\Tag\TagService::class, LoggerInterface::class], + + Middleware\BackwardsCompatibleProblemDetailsMiddleware::class => [ + 'config.backwards_compatible_problem_details.default_type_fallbacks', + 'config.backwards_compatible_problem_details.json_flags', + ], ], ]; diff --git a/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php b/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php index fe609e03..8391db8a 100644 --- a/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php @@ -15,8 +15,6 @@ use Shlinkio\Shlink\Rest\Util\RestUtils; use Zend\Diactoros\Response\EmptyResponse; use Zend\Diactoros\Response\JsonResponse; -use function sprintf; - class EditShortUrlAction extends AbstractRestAction { protected const ROUTE_PATH = '/short-urls/{shortCode}'; @@ -51,12 +49,6 @@ class EditShortUrlAction extends AbstractRestAction ShortUrlMeta::createFromRawData($postData) ); return new EmptyResponse(); - } catch (Exception\InvalidShortCodeException $e) { - $this->logger->warning('Provided data is invalid. {e}', ['e' => $e]); - return new JsonResponse([ - 'error' => RestUtils::getRestErrorCodeFromException($e), - 'message' => sprintf('No URL found for short code "%s"', $shortCode), - ], self::STATUS_NOT_FOUND); } catch (Exception\ValidationException $e) { $this->logger->warning('Provided data is invalid. {e}', ['e' => $e]); return new JsonResponse([ diff --git a/module/Rest/src/Action/ShortUrl/EditShortUrlTagsAction.php b/module/Rest/src/Action/ShortUrl/EditShortUrlTagsAction.php index 4daa0fac..9d82c608 100644 --- a/module/Rest/src/Action/ShortUrl/EditShortUrlTagsAction.php +++ b/module/Rest/src/Action/ShortUrl/EditShortUrlTagsAction.php @@ -7,14 +7,11 @@ namespace Shlinkio\Shlink\Rest\Action\ShortUrl; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; -use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\ShortUrlServiceInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; use Shlinkio\Shlink\Rest\Util\RestUtils; use Zend\Diactoros\Response\JsonResponse; -use function sprintf; - class EditShortUrlTagsAction extends AbstractRestAction { protected const ROUTE_PATH = '/short-urls/{shortCode}/tags'; @@ -47,14 +44,7 @@ class EditShortUrlTagsAction extends AbstractRestAction } $tags = $bodyParams['tags']; - try { - $shortUrl = $this->shortUrlService->setTagsByShortCode($shortCode, $tags); - return new JsonResponse(['tags' => $shortUrl->getTags()->toArray()]); - } catch (InvalidShortCodeException $e) { - return new JsonResponse([ - 'error' => RestUtils::getRestErrorCodeFromException($e), - 'message' => sprintf('No URL found for short code "%s"', $shortCode), - ], self::STATUS_NOT_FOUND); - } + $shortUrl = $this->shortUrlService->setTagsByShortCode($shortCode, $tags); + return new JsonResponse(['tags' => $shortUrl->getTags()->toArray()]); } } diff --git a/module/Rest/src/Action/Visit/GetVisitsAction.php b/module/Rest/src/Action/Visit/GetVisitsAction.php index 68912be2..ed614ae7 100644 --- a/module/Rest/src/Action/Visit/GetVisitsAction.php +++ b/module/Rest/src/Action/Visit/GetVisitsAction.php @@ -33,11 +33,6 @@ class GetVisitsAction extends AbstractRestAction $this->visitsTracker = $visitsTracker; } - /** - * @param Request $request - * @return Response - * @throws \InvalidArgumentException - */ public function handle(Request $request): Response { $shortCode = $request->getAttribute('shortCode'); diff --git a/module/Rest/src/ConfigProvider.php b/module/Rest/src/ConfigProvider.php index 11e6f811..d942cf51 100644 --- a/module/Rest/src/ConfigProvider.php +++ b/module/Rest/src/ConfigProvider.php @@ -11,7 +11,7 @@ use function sprintf; class ConfigProvider { - private const ROUTES_PREFIX = '/rest/v{version:1}'; + private const ROUTES_PREFIX = '/rest/v{version:1|2}'; public function __invoke() { diff --git a/module/Rest/src/Middleware/BackwardsCompatibleProblemDetailsMiddleware.php b/module/Rest/src/Middleware/BackwardsCompatibleProblemDetailsMiddleware.php new file mode 100644 index 00000000..9857465b --- /dev/null +++ b/module/Rest/src/Middleware/BackwardsCompatibleProblemDetailsMiddleware.php @@ -0,0 +1,87 @@ + 'type', + 'message' => 'detail', + ]; + + /** @var array */ + private $defaultTypeFallbacks; + /** @var int */ + private $jsonFlags; + + public function __construct(array $defaultTypeFallbacks, int $jsonFlags) + { + $this->defaultTypeFallbacks = $defaultTypeFallbacks; + $this->jsonFlags = $jsonFlags; + } + + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface + { + $resp = $handler->handle($request); + + if ($resp->getHeaderLine('Content-type') !== 'application/problem+json') { + return $resp; + } + + try { + $body = (string) $resp->getBody(); + $payload = json_decode($body); + } catch (Throwable $e) { + return $resp; + } + + $payload = $this->mapStandardErrorTypes($payload, $resp->getStatusCode()); + + if ($this->isVersionOne($request)) { + $payload = $this->makePayloadBackwardsCompatible($payload); + } + + return new JsonResponse($payload, $resp->getStatusCode(), $resp->getHeaders(), $this->jsonFlags); + } + + private function mapStandardErrorTypes(array $payload, int $respStatusCode): array + { + $type = $payload['type'] ?? ''; + if (strpos($type, 'https://httpstatus.es') === 0) { + $payload['type'] = $this->defaultTypeFallbacks[$respStatusCode] ?? $type; + } + + return $payload; + } + + /** @deprecated When Shlink 2 is released, do not chekc the version */ + private function isVersionOne(ServerRequestInterface $request): bool + { + $uri = $request->getUri(); + $path = $uri->getPath(); + + return strpos($path, '/v') === false || strpos($path, '/v1') === 0; + } + + /** @deprecated When Shlink v2 is released, do not map old fields */ + private function makePayloadBackwardsCompatible(array $payload): array + { + return reduce_left(self::BACKWARDS_COMPATIBLE_FIELDS, function (string $newKey, string $oldKey, $c, $acc) { + $acc[$oldKey] = $acc[$newKey]; + return $acc; + }, $payload); + } +} diff --git a/module/Rest/src/Middleware/PathVersionMiddleware.php b/module/Rest/src/Middleware/PathVersionMiddleware.php index c08c8dbb..84921710 100644 --- a/module/Rest/src/Middleware/PathVersionMiddleware.php +++ b/module/Rest/src/Middleware/PathVersionMiddleware.php @@ -15,23 +15,12 @@ class PathVersionMiddleware implements MiddlewareInterface { // TODO The /health endpoint needs this middleware in order to work without the version. // Take it into account if this middleware is ever removed. - - /** - * Process an incoming server request and return a response, optionally delegating - * to the next middleware component to create the response. - * - * @param Request $request - * @param RequestHandlerInterface $handler - * - * @return Response - * @throws \InvalidArgumentException - */ public function process(Request $request, RequestHandlerInterface $handler): Response { $uri = $request->getUri(); $path = $uri->getPath(); - // If the path does not begin with the version number, prepend v1 by default for BC compatibility purposes + // If the path does not begin with the version number, prepend v1 by default for BC purposes if (strpos($path, '/v') !== 0) { $request = $request->withUri($uri->withPath('/v1' . $uri->getPath())); } diff --git a/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php index d8e62c20..55fcc2d7 100644 --- a/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php @@ -8,7 +8,6 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; -use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\ShortUrlServiceInterface; use Shlinkio\Shlink\Rest\Action\ShortUrl\EditShortUrlAction; use Shlinkio\Shlink\Rest\Util\RestUtils; @@ -29,7 +28,7 @@ class EditShortUrlActionTest extends TestCase } /** @test */ - public function invalidDataReturnsError() + public function invalidDataReturnsError(): void { $request = (new ServerRequest())->withParsedBody([ 'maxVisits' => 'invalid', @@ -45,28 +44,7 @@ class EditShortUrlActionTest extends TestCase } /** @test */ - public function incorrectShortCodeReturnsError() - { - $request = (new ServerRequest())->withAttribute('shortCode', 'abc123') - ->withParsedBody([ - 'maxVisits' => 5, - ]); - $updateMeta = $this->shortUrlService->updateMetadataByShortCode(Argument::cetera())->willThrow( - InvalidShortCodeException::class - ); - - /** @var JsonResponse $resp */ - $resp = $this->action->handle($request); - $payload = $resp->getPayload(); - - $this->assertEquals(404, $resp->getStatusCode()); - $this->assertEquals(RestUtils::INVALID_SHORTCODE_ERROR, $payload['error']); - $this->assertEquals('No URL found for short code "abc123"', $payload['message']); - $updateMeta->shouldHaveBeenCalled(); - } - - /** @test */ - public function correctShortCodeReturnsSuccess() + public function correctShortCodeReturnsSuccess(): void { $request = (new ServerRequest())->withAttribute('shortCode', 'abc123') ->withParsedBody([ diff --git a/module/Rest/test/Action/ShortUrl/EditShortUrlTagsActionTest.php b/module/Rest/test/Action/ShortUrl/EditShortUrlTagsActionTest.php index d56bbaf1..5c0ec628 100644 --- a/module/Rest/test/Action/ShortUrl/EditShortUrlTagsActionTest.php +++ b/module/Rest/test/Action/ShortUrl/EditShortUrlTagsActionTest.php @@ -7,7 +7,6 @@ namespace ShlinkioTest\Shlink\Rest\Action\ShortUrl; use PHPUnit\Framework\TestCase; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; -use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\ShortUrlService; use Shlinkio\Shlink\Rest\Action\ShortUrl\EditShortUrlTagsAction; use Zend\Diactoros\ServerRequest; @@ -26,28 +25,14 @@ class EditShortUrlTagsActionTest extends TestCase } /** @test */ - public function notProvidingTagsReturnsError() + public function notProvidingTagsReturnsError(): void { $response = $this->action->handle((new ServerRequest())->withAttribute('shortCode', 'abc123')); $this->assertEquals(400, $response->getStatusCode()); } /** @test */ - public function anInvalidShortCodeReturnsNotFound() - { - $shortCode = 'abc123'; - $this->shortUrlService->setTagsByShortCode($shortCode, [])->willThrow(InvalidShortCodeException::class) - ->shouldBeCalledOnce(); - - $response = $this->action->handle( - (new ServerRequest())->withAttribute('shortCode', 'abc123') - ->withParsedBody(['tags' => []]) - ); - $this->assertEquals(404, $response->getStatusCode()); - } - - /** @test */ - public function tagsListIsReturnedIfCorrectShortCodeIsProvided() + public function tagsListIsReturnedIfCorrectShortCodeIsProvided(): void { $shortCode = 'abc123'; $this->shortUrlService->setTagsByShortCode($shortCode, [])->willReturn(new ShortUrl(''))