From 6ddb60d04750643dfd397a719bf4cf4b82e7d24b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 21 Nov 2019 20:05:06 +0100 Subject: [PATCH] Improved ValidationException to avoid polluting the message with invalid data but keeping it on the string representation --- .../src/Exception/ValidationException.php | 41 +++++++++++-------- .../Exception/ValidationExceptionTest.php | 8 ++-- .../ShortUrl/AbstractCreateShortUrlAction.php | 6 +-- .../Action/ShortUrl/CreateShortUrlAction.php | 30 ++++++-------- .../SingleStepCreateShortUrlAction.php | 13 +++--- .../SingleStepCreateShortUrlActionTest.php | 10 ++--- 6 files changed, 55 insertions(+), 53 deletions(-) diff --git a/module/Core/src/Exception/ValidationException.php b/module/Core/src/Exception/ValidationException.php index 1b767594..70dfe0d0 100644 --- a/module/Core/src/Exception/ValidationException.php +++ b/module/Core/src/Exception/ValidationException.php @@ -34,24 +34,34 @@ class ValidationException extends RuntimeException return static::fromArray($inputFilter->getMessages(), $prev); } - private static function fromArray(array $invalidData, ?Throwable $prev = null): self + public static function fromArray(array $invalidData, ?Throwable $prev = null): self { - return new self( - sprintf( - 'Provided data is not valid. These are the messages:%s%s%s', - PHP_EOL, - self::formMessagesToString($invalidData), - PHP_EOL - ), - $invalidData, - -1, - $prev + return new self('Provided data is not valid', $invalidData, -1, $prev); + } + + public function getInvalidElements(): array + { + return $this->invalidElements; + } + + public function __toString(): string + { + return sprintf( + '%s %s in %s:%s%s%sStack trace:%s%s', + __CLASS__, + $this->getMessage(), + $this->getFile(), + $this->getLine(), + $this->invalidElementsToString(), + PHP_EOL, + PHP_EOL, + $this->getTraceAsString() ); } - private static function formMessagesToString(array $messages = []): string + private function invalidElementsToString(): string { - return reduce_left($messages, function ($messageSet, $name, $_, string $acc) { + return reduce_left($this->invalidElements, function ($messageSet, string $name, $_, string $acc) { return $acc . sprintf( "\n '%s' => %s", $name, @@ -59,9 +69,4 @@ class ValidationException extends RuntimeException ); }, ''); } - - public function getInvalidElements(): array - { - return $this->invalidElements; - } } diff --git a/module/Core/test/Exception/ValidationExceptionTest.php b/module/Core/test/Exception/ValidationExceptionTest.php index 5cab422c..bd7855e2 100644 --- a/module/Core/test/Exception/ValidationExceptionTest.php +++ b/module/Core/test/Exception/ValidationExceptionTest.php @@ -55,12 +55,9 @@ class ValidationExceptionTest extends TestCase 'something' => ['baz', 'foo'], ]; $barValue = print_r(['baz', 'foo'], true); - $expectedMessage = << bar 'something' => {$barValue} - EOT; $inputFilter = $this->prophesize(InputFilterInterface::class); @@ -69,9 +66,10 @@ EOT; $e = ValidationException::fromInputFilter($inputFilter->reveal()); $this->assertEquals($invalidData, $e->getInvalidElements()); - $this->assertEquals($expectedMessage, $e->getMessage()); + $this->assertEquals('Provided data is not valid', $e->getMessage()); $this->assertEquals(-1, $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 f3eaee19..bc0d50ef 100644 --- a/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php @@ -7,9 +7,9 @@ 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\InvalidArgumentException; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; +use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Model\CreateShortUrlData; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; use Shlinkio\Shlink\Core\Transformer\ShortUrlDataTransformer; @@ -44,7 +44,7 @@ abstract class AbstractCreateShortUrlAction extends AbstractRestAction { try { $shortUrlData = $this->buildShortUrlData($request); - } catch (InvalidArgumentException $e) { + } catch (ValidationException $e) { $this->logger->warning('Provided data is invalid. {e}', ['e' => $e]); return new JsonResponse([ 'error' => RestUtils::INVALID_ARGUMENT_ERROR, @@ -79,7 +79,7 @@ abstract class AbstractCreateShortUrlAction extends AbstractRestAction /** * @param Request $request * @return CreateShortUrlData - * @throws InvalidArgumentException + * @throws ValidationException */ abstract protected function buildShortUrlData(Request $request): CreateShortUrlData; } diff --git a/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php index 3b0a0b61..9c20de6c 100644 --- a/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php @@ -6,7 +6,6 @@ namespace Shlinkio\Shlink\Rest\Action\ShortUrl; use Cake\Chronos\Chronos; use Psr\Http\Message\ServerRequestInterface as Request; -use Shlinkio\Shlink\Core\Exception\InvalidArgumentException; use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Model\CreateShortUrlData; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; @@ -20,30 +19,27 @@ class CreateShortUrlAction extends AbstractCreateShortUrlAction /** * @param Request $request * @return CreateShortUrlData - * @throws InvalidArgumentException - * @throws \InvalidArgumentException + * @throws ValidationException */ protected function buildShortUrlData(Request $request): CreateShortUrlData { $postData = (array) $request->getParsedBody(); if (! isset($postData['longUrl'])) { - throw new InvalidArgumentException('A URL was not provided'); + throw ValidationException::fromArray([ + 'longUrl' => 'A URL was not provided', + ]); } - try { - $meta = ShortUrlMeta::createFromParams( - $this->getOptionalDate($postData, 'validSince'), - $this->getOptionalDate($postData, 'validUntil'), - $postData['customSlug'] ?? null, - $postData['maxVisits'] ?? null, - $postData['findIfExists'] ?? null, - $postData['domain'] ?? null - ); + $meta = ShortUrlMeta::createFromParams( + $this->getOptionalDate($postData, 'validSince'), + $this->getOptionalDate($postData, 'validUntil'), + $postData['customSlug'] ?? null, + $postData['maxVisits'] ?? null, + $postData['findIfExists'] ?? null, + $postData['domain'] ?? null + ); - return new CreateShortUrlData(new Uri($postData['longUrl']), (array) ($postData['tags'] ?? []), $meta); - } catch (ValidationException $e) { - throw new InvalidArgumentException('Provided meta data is not valid', -1, $e); - } + return new CreateShortUrlData(new Uri($postData['longUrl']), (array) ($postData['tags'] ?? []), $meta); } private function getOptionalDate(array $postData, string $fieldName): ?Chronos diff --git a/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php index 327df46e..834c6b12 100644 --- a/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php @@ -6,7 +6,7 @@ namespace Shlinkio\Shlink\Rest\Action\ShortUrl; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; -use Shlinkio\Shlink\Core\Exception\InvalidArgumentException; +use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Model\CreateShortUrlData; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; @@ -33,19 +33,22 @@ class SingleStepCreateShortUrlAction extends AbstractCreateShortUrlAction /** * @param Request $request * @return CreateShortUrlData - * @throws \InvalidArgumentException - * @throws InvalidArgumentException + * @throws ValidationException */ protected function buildShortUrlData(Request $request): CreateShortUrlData { $query = $request->getQueryParams(); if (! $this->apiKeyService->check($query['apiKey'] ?? '')) { - throw new InvalidArgumentException('No API key was provided or it is not valid'); + throw ValidationException::fromArray([ + 'apiKey' => 'No API key was provided or it is not valid', + ]); } if (! isset($query['longUrl'])) { - throw new InvalidArgumentException('A URL was not provided'); + throw ValidationException::fromArray([ + 'longUrl' => 'A URL was not provided', + ]); } return new CreateShortUrlData(new Uri($query['longUrl'])); diff --git a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php index f3452e9a..8bebc6c5 100644 --- a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php @@ -42,7 +42,7 @@ class SingleStepCreateShortUrlActionTest extends TestCase } /** @test */ - public function errorResponseIsReturnedIfInvalidApiKeyIsProvided() + public function errorResponseIsReturnedIfInvalidApiKeyIsProvided(): void { $request = (new ServerRequest())->withQueryParams(['apiKey' => 'abc123']); $findApiKey = $this->apiKeyService->check('abc123')->willReturn(false); @@ -53,12 +53,12 @@ class SingleStepCreateShortUrlActionTest extends TestCase $this->assertEquals(400, $resp->getStatusCode()); $this->assertEquals('INVALID_ARGUMENT', $payload['error']); - $this->assertEquals('No API key was provided or it is not valid', $payload['message']); + $this->assertEquals('Provided data is not valid', $payload['message']); $findApiKey->shouldHaveBeenCalled(); } /** @test */ - public function errorResponseIsReturnedIfNoUrlIsProvided() + public function errorResponseIsReturnedIfNoUrlIsProvided(): void { $request = (new ServerRequest())->withQueryParams(['apiKey' => 'abc123']); $findApiKey = $this->apiKeyService->check('abc123')->willReturn(true); @@ -69,12 +69,12 @@ class SingleStepCreateShortUrlActionTest extends TestCase $this->assertEquals(400, $resp->getStatusCode()); $this->assertEquals('INVALID_ARGUMENT', $payload['error']); - $this->assertEquals('A URL was not provided', $payload['message']); + $this->assertEquals('Provided data is not valid', $payload['message']); $findApiKey->shouldHaveBeenCalled(); } /** @test */ - public function properDataIsPassedWhenGeneratingShortCode() + public function properDataIsPassedWhenGeneratingShortCode(): void { $request = (new ServerRequest())->withQueryParams([ 'apiKey' => 'abc123',