From 32b3c72bdff6e91e089522e244dcba91a0e1232d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Nov 2019 23:45:40 +0100 Subject: [PATCH] Converted ValidationException into a problem details exception --- .../src/Exception/ValidationException.php | 23 ++++++++++++++-- .../Exception/ValidationExceptionTest.php | 3 ++- .../ShortUrl/AbstractCreateShortUrlAction.php | 16 +---------- .../Action/ShortUrl/EditShortUrlAction.php | 27 ++----------------- module/Rest/src/Util/RestUtils.php | 6 ++--- .../ShortUrl/CreateShortUrlActionTest.php | 17 +++++------- .../ShortUrl/EditShortUrlActionTest.php | 11 +++----- .../SingleStepCreateShortUrlActionTest.php | 22 +++++---------- 8 files changed, 47 insertions(+), 78 deletions(-) diff --git a/module/Core/src/Exception/ValidationException.php b/module/Core/src/Exception/ValidationException.php index 70dfe0d0..510a31b0 100644 --- a/module/Core/src/Exception/ValidationException.php +++ b/module/Core/src/Exception/ValidationException.php @@ -4,8 +4,11 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Exception; +use Fig\Http\Message\StatusCodeInterface; use Throwable; use Zend\InputFilter\InputFilterInterface; +use Zend\ProblemDetails\Exception\CommonProblemDetailsExceptionTrait; +use Zend\ProblemDetails\Exception\ProblemDetailsExceptionInterface; use function Functional\reduce_left; use function is_array; @@ -14,8 +17,13 @@ use function sprintf; use const PHP_EOL; -class ValidationException extends RuntimeException +class ValidationException extends RuntimeException implements ProblemDetailsExceptionInterface { + use CommonProblemDetailsExceptionTrait; + + private const TITLE = 'Invalid data'; + public const TYPE = 'INVALID_ARGUMENT'; + /** @var array */ private $invalidElements; @@ -36,7 +44,18 @@ class ValidationException extends RuntimeException public static function fromArray(array $invalidData, ?Throwable $prev = null): self { - return new self('Provided data is not valid', $invalidData, -1, $prev); + $status = StatusCodeInterface::STATUS_BAD_REQUEST; + $e = new self('Provided data is not valid', $invalidData, $status, $prev); + + $e->detail = $e->getMessage(); + $e->title = self::TITLE; + $e->type = self::TYPE; + $e->status = StatusCodeInterface::STATUS_BAD_REQUEST; + $e->additional = [ + 'invalidElements' => $invalidData, + ]; + + return $e; } public function getInvalidElements(): array diff --git a/module/Core/test/Exception/ValidationExceptionTest.php b/module/Core/test/Exception/ValidationExceptionTest.php index bd7855e2..eb47caed 100644 --- a/module/Core/test/Exception/ValidationExceptionTest.php +++ b/module/Core/test/Exception/ValidationExceptionTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Exception; +use Fig\Http\Message\StatusCodeInterface; use LogicException; use PHPUnit\Framework\TestCase; use RuntimeException; @@ -67,7 +68,7 @@ EOT; $this->assertEquals($invalidData, $e->getInvalidElements()); $this->assertEquals('Provided data is not valid', $e->getMessage()); - $this->assertEquals(-1, $e->getCode()); + $this->assertEquals(StatusCodeInterface::STATUS_BAD_REQUEST, $e->getCode()); $this->assertEquals($prev, $e->getPrevious()); $this->assertStringContainsString($expectedStringRepresentation, (string) $e); $getMessages->shouldHaveBeenCalledOnce(); diff --git a/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php index 38daf905..af392067 100644 --- a/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php @@ -12,7 +12,6 @@ use Shlinkio\Shlink\Core\Model\CreateShortUrlData; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; use Shlinkio\Shlink\Core\Transformer\ShortUrlDataTransformer; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; -use Shlinkio\Shlink\Rest\Util\RestUtils; use Zend\Diactoros\Response\JsonResponse; abstract class AbstractCreateShortUrlAction extends AbstractRestAction @@ -32,22 +31,9 @@ abstract class AbstractCreateShortUrlAction extends AbstractRestAction $this->domainConfig = $domainConfig; } - /** - * @param Request $request - * @return Response - */ public function handle(Request $request): Response { - try { - $shortUrlData = $this->buildShortUrlData($request); - } catch (ValidationException $e) { - $this->logger->warning('Provided data is invalid. {e}', ['e' => $e]); - return new JsonResponse([ - 'error' => RestUtils::INVALID_ARGUMENT_ERROR, - 'message' => $e->getMessage(), - ], self::STATUS_BAD_REQUEST); - } - + $shortUrlData = $this->buildShortUrlData($request); $longUrl = $shortUrlData->getLongUrl(); $tags = $shortUrlData->getTags(); $shortUrlMeta = $shortUrlData->getMeta(); diff --git a/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php b/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php index 8391db8a..33796f7d 100644 --- a/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php @@ -7,13 +7,10 @@ namespace Shlinkio\Shlink\Rest\Action\ShortUrl; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Log\LoggerInterface; -use Shlinkio\Shlink\Core\Exception; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Service\ShortUrlServiceInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; -use Shlinkio\Shlink\Rest\Util\RestUtils; use Zend\Diactoros\Response\EmptyResponse; -use Zend\Diactoros\Response\JsonResponse; class EditShortUrlAction extends AbstractRestAction { @@ -29,32 +26,12 @@ class EditShortUrlAction extends AbstractRestAction $this->shortUrlService = $shortUrlService; } - /** - * Process an incoming server request and return a response, optionally delegating - * to the next middleware component to create the response. - * - * @param ServerRequestInterface $request - * - * @return ResponseInterface - * @throws \InvalidArgumentException - */ public function handle(ServerRequestInterface $request): ResponseInterface { $postData = (array) $request->getParsedBody(); $shortCode = $request->getAttribute('shortCode', ''); - try { - $this->shortUrlService->updateMetadataByShortCode( - $shortCode, - ShortUrlMeta::createFromRawData($postData) - ); - return new EmptyResponse(); - } catch (Exception\ValidationException $e) { - $this->logger->warning('Provided data is invalid. {e}', ['e' => $e]); - return new JsonResponse([ - 'error' => RestUtils::getRestErrorCodeFromException($e), - 'message' => 'Provided data is invalid.', - ], self::STATUS_BAD_REQUEST); - } + $this->shortUrlService->updateMetadataByShortCode($shortCode, ShortUrlMeta::createFromRawData($postData)); + return new EmptyResponse(); } } diff --git a/module/Rest/src/Util/RestUtils.php b/module/Rest/src/Util/RestUtils.php index 24398eb9..096db51a 100644 --- a/module/Rest/src/Util/RestUtils.php +++ b/module/Rest/src/Util/RestUtils.php @@ -6,19 +6,19 @@ namespace Shlinkio\Shlink\Rest\Util; use Shlinkio\Shlink\Common\Exception as Common; use Shlinkio\Shlink\Core\Exception as Core; -use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Rest\Exception as Rest; use Throwable; class RestUtils { /** @deprecated */ - public const INVALID_SHORTCODE_ERROR = ShortUrlNotFoundException::TYPE; + public const INVALID_SHORTCODE_ERROR = Core\ShortUrlNotFoundException::TYPE; // FIXME Should be INVALID_SHORT_URL_DELETION public const INVALID_SHORTCODE_DELETION_ERROR = 'INVALID_SHORTCODE_DELETION'; /** @deprecated */ public const INVALID_URL_ERROR = Core\InvalidUrlException::TYPE; - public const INVALID_ARGUMENT_ERROR = 'INVALID_ARGUMENT'; + /** @deprecated */ + public const INVALID_ARGUMENT_ERROR = Core\ValidationException::TYPE; /** @deprecated */ public const INVALID_SLUG_ERROR = Core\NonUniqueSlugException::TYPE; public const INVALID_CREDENTIALS_ERROR = 'INVALID_CREDENTIALS'; diff --git a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php index c4f8970d..92a3c2aa 100644 --- a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php @@ -8,10 +8,9 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Service\UrlShortener; use Shlinkio\Shlink\Rest\Action\ShortUrl\CreateShortUrlAction; -use Shlinkio\Shlink\Rest\Util\RestUtils; -use Zend\Diactoros\Response\JsonResponse; use Zend\Diactoros\ServerRequest; use Zend\Diactoros\Uri; @@ -38,8 +37,8 @@ class CreateShortUrlActionTest extends TestCase /** @test */ public function missingLongUrlParamReturnsError(): void { - $response = $this->action->handle(new ServerRequest()); - $this->assertEquals(400, $response->getStatusCode()); + $this->expectException(ValidationException::class); + $this->action->handle(new ServerRequest()); } /** @test */ @@ -71,13 +70,11 @@ class CreateShortUrlActionTest extends TestCase 'longUrl' => 'http://www.domain.com/foo/bar', 'domain' => $domain, ]); - /** @var JsonResponse $response */ - $response = $this->action->handle($request); - $payload = $response->getPayload(); - $this->assertEquals(400, $response->getStatusCode()); - $this->assertEquals(RestUtils::INVALID_ARGUMENT_ERROR, $payload['error']); - $urlToShortCode->shouldNotHaveBeenCalled(); + $this->expectException(ValidationException::class); + $urlToShortCode->shouldNotBeCalled(); + + $this->action->handle($request); } public function provideInvalidDomains(): iterable diff --git a/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php index 55fcc2d7..63aa1ffb 100644 --- a/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php @@ -8,6 +8,7 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Service\ShortUrlServiceInterface; use Shlinkio\Shlink\Rest\Action\ShortUrl\EditShortUrlAction; use Shlinkio\Shlink\Rest\Util\RestUtils; @@ -28,19 +29,15 @@ class EditShortUrlActionTest extends TestCase } /** @test */ - public function invalidDataReturnsError(): void + public function invalidDataThrowsError(): void { $request = (new ServerRequest())->withParsedBody([ 'maxVisits' => 'invalid', ]); - /** @var JsonResponse $resp */ - $resp = $this->action->handle($request); - $payload = $resp->getPayload(); + $this->expectException(ValidationException::class); - $this->assertEquals(400, $resp->getStatusCode()); - $this->assertEquals(RestUtils::INVALID_ARGUMENT_ERROR, $payload['error']); - $this->assertEquals('Provided data is invalid.', $payload['message']); + $this->action->handle($request); } /** @test */ diff --git a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php index 8bebc6c5..b0d8c65d 100644 --- a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php @@ -10,11 +10,11 @@ use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Psr\Http\Message\UriInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; use Shlinkio\Shlink\Rest\Action\ShortUrl\SingleStepCreateShortUrlAction; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; -use Zend\Diactoros\Response\JsonResponse; use Zend\Diactoros\ServerRequest; class SingleStepCreateShortUrlActionTest extends TestCase @@ -47,14 +47,10 @@ class SingleStepCreateShortUrlActionTest extends TestCase $request = (new ServerRequest())->withQueryParams(['apiKey' => 'abc123']); $findApiKey = $this->apiKeyService->check('abc123')->willReturn(false); - /** @var JsonResponse $resp */ - $resp = $this->action->handle($request); - $payload = $resp->getPayload(); + $this->expectException(ValidationException::class); + $findApiKey->shouldBeCalledOnce(); - $this->assertEquals(400, $resp->getStatusCode()); - $this->assertEquals('INVALID_ARGUMENT', $payload['error']); - $this->assertEquals('Provided data is not valid', $payload['message']); - $findApiKey->shouldHaveBeenCalled(); + $this->action->handle($request); } /** @test */ @@ -63,14 +59,10 @@ class SingleStepCreateShortUrlActionTest extends TestCase $request = (new ServerRequest())->withQueryParams(['apiKey' => 'abc123']); $findApiKey = $this->apiKeyService->check('abc123')->willReturn(true); - /** @var JsonResponse $resp */ - $resp = $this->action->handle($request); - $payload = $resp->getPayload(); + $this->expectException(ValidationException::class); + $findApiKey->shouldBeCalledOnce(); - $this->assertEquals(400, $resp->getStatusCode()); - $this->assertEquals('INVALID_ARGUMENT', $payload['error']); - $this->assertEquals('Provided data is not valid', $payload['message']); - $findApiKey->shouldHaveBeenCalled(); + $this->action->handle($request); } /** @test */