From a28ef1f1769ab28ed976bbdaad575fd097daa692 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 25 Nov 2019 19:15:46 +0100 Subject: [PATCH] Converted EntityDoesNotExistException into a problem details exception renamed as TagNotFoundException --- docs/swagger/paths/v1_short-urls_shorten.json | 4 +-- .../CLI/src/Command/Tag/RenameTagCommand.php | 4 +-- .../test/Command/Tag/RenameTagCommandTest.php | 4 +-- .../Exception/EntityDoesNotExistException.php | 30 ------------------ .../src/Exception/TagNotFoundException.php | 31 +++++++++++++++++++ module/Core/src/Service/Tag/TagService.php | 19 ++++-------- .../src/Service/Tag/TagServiceInterface.php | 12 ++----- .../Core/test/Service/Tag/TagServiceTest.php | 4 +-- .../Rest/src/Action/Tag/UpdateTagAction.php | 26 +++++----------- .../Rest/src/Action/Visit/GetVisitsAction.php | 21 +++---------- .../src/Exception/AuthenticationException.php | 1 + module/Rest/src/Util/RestUtils.php | 5 ++- .../test-api/Action/GetVisitsActionTest.php | 2 +- .../ShortUrl/EditShortUrlActionTest.php | 2 -- .../test/Action/Tag/UpdateTagActionTest.php | 22 +++---------- .../test/Action/Visit/GetVisitsActionTest.php | 13 -------- module/Rest/test/Util/RestUtilsTest.php | 2 +- 17 files changed, 70 insertions(+), 132 deletions(-) delete mode 100644 module/Core/src/Exception/EntityDoesNotExistException.php create mode 100644 module/Core/src/Exception/TagNotFoundException.php diff --git a/docs/swagger/paths/v1_short-urls_shorten.json b/docs/swagger/paths/v1_short-urls_shorten.json index 803d77d5..d0c3c4c7 100644 --- a/docs/swagger/paths/v1_short-urls_shorten.json +++ b/docs/swagger/paths/v1_short-urls_shorten.json @@ -112,10 +112,10 @@ }, "examples": { "application/json": { - "error": "UNKNOWN_ERROR", + "error": "INTERNAL_SERVER_ERROR", "message": "Unexpected error occurred" }, - "text/plain": "UNKNOWN_ERROR" + "text/plain": "INTERNAL_SERVER_ERROR" } } } diff --git a/module/CLI/src/Command/Tag/RenameTagCommand.php b/module/CLI/src/Command/Tag/RenameTagCommand.php index 3e9f021b..a7002f60 100644 --- a/module/CLI/src/Command/Tag/RenameTagCommand.php +++ b/module/CLI/src/Command/Tag/RenameTagCommand.php @@ -5,7 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\Tag; use Shlinkio\Shlink\CLI\Util\ExitCodes; -use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\TagNotFoundException; use Shlinkio\Shlink\Core\Service\Tag\TagServiceInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -47,7 +47,7 @@ class RenameTagCommand extends Command $this->tagService->renameTag($oldName, $newName); $io->success('Tag properly renamed.'); return ExitCodes::EXIT_SUCCESS; - } catch (EntityDoesNotExistException $e) { + } catch (TagNotFoundException $e) { $io->error(sprintf('A tag with name "%s" was not found', $oldName)); return ExitCodes::EXIT_FAILURE; } diff --git a/module/CLI/test/Command/Tag/RenameTagCommandTest.php b/module/CLI/test/Command/Tag/RenameTagCommandTest.php index 228d854d..4fc2aaad 100644 --- a/module/CLI/test/Command/Tag/RenameTagCommandTest.php +++ b/module/CLI/test/Command/Tag/RenameTagCommandTest.php @@ -8,7 +8,7 @@ use PHPUnit\Framework\TestCase; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\Tag\RenameTagCommand; use Shlinkio\Shlink\Core\Entity\Tag; -use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\TagNotFoundException; use Shlinkio\Shlink\Core\Service\Tag\TagServiceInterface; use Symfony\Component\Console\Application; use Symfony\Component\Console\Tester\CommandTester; @@ -38,7 +38,7 @@ class RenameTagCommandTest extends TestCase { $oldName = 'foo'; $newName = 'bar'; - $renameTag = $this->tagService->renameTag($oldName, $newName)->willThrow(EntityDoesNotExistException::class); + $renameTag = $this->tagService->renameTag($oldName, $newName)->willThrow(TagNotFoundException::class); $this->commandTester->execute([ 'oldName' => $oldName, diff --git a/module/Core/src/Exception/EntityDoesNotExistException.php b/module/Core/src/Exception/EntityDoesNotExistException.php deleted file mode 100644 index 9214aa47..00000000 --- a/module/Core/src/Exception/EntityDoesNotExistException.php +++ /dev/null @@ -1,30 +0,0 @@ - $value) { - $result[] = sprintf('"%s" => "%s"', $key, $value); - } - - return implode(', ', $result); - } -} diff --git a/module/Core/src/Exception/TagNotFoundException.php b/module/Core/src/Exception/TagNotFoundException.php new file mode 100644 index 00000000..6ff36bb2 --- /dev/null +++ b/module/Core/src/Exception/TagNotFoundException.php @@ -0,0 +1,31 @@ +detail = $e->getMessage(); + $e->title = self::TITLE; + $e->type = self::TYPE; + $e->status = StatusCodeInterface::STATUS_NOT_FOUND; + + return $e; + } +} diff --git a/module/Core/src/Service/Tag/TagService.php b/module/Core/src/Service/Tag/TagService.php index d5ac562e..40bdbfcd 100644 --- a/module/Core/src/Service/Tag/TagService.php +++ b/module/Core/src/Service/Tag/TagService.php @@ -7,7 +7,7 @@ namespace Shlinkio\Shlink\Core\Service\Tag; use Doctrine\Common\Collections\Collection; use Doctrine\ORM; use Shlinkio\Shlink\Core\Entity\Tag; -use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\TagNotFoundException; use Shlinkio\Shlink\Core\Repository\TagRepository; use Shlinkio\Shlink\Core\Util\TagManagerTrait; @@ -35,8 +35,7 @@ class TagService implements TagServiceInterface } /** - * @param array $tagNames - * @return void + * @param string[] $tagNames */ public function deleteTags(array $tagNames): void { @@ -60,23 +59,17 @@ class TagService implements TagServiceInterface } /** - * @param string $oldName - * @param string $newName - * @return Tag - * @throws EntityDoesNotExistException - * @throws ORM\OptimisticLockException + * @throws TagNotFoundException */ - public function renameTag($oldName, $newName): Tag + public function renameTag(string $oldName, string $newName): Tag { - $criteria = ['name' => $oldName]; /** @var Tag|null $tag */ - $tag = $this->em->getRepository(Tag::class)->findOneBy($criteria); + $tag = $this->em->getRepository(Tag::class)->findOneBy(['name' => $oldName]); if ($tag === null) { - throw EntityDoesNotExistException::createFromEntityAndConditions(Tag::class, $criteria); + throw TagNotFoundException::fromTag($oldName); } $tag->rename($newName); - $this->em->flush(); return $tag; diff --git a/module/Core/src/Service/Tag/TagServiceInterface.php b/module/Core/src/Service/Tag/TagServiceInterface.php index 1eb11112..dec01e31 100644 --- a/module/Core/src/Service/Tag/TagServiceInterface.php +++ b/module/Core/src/Service/Tag/TagServiceInterface.php @@ -6,7 +6,7 @@ namespace Shlinkio\Shlink\Core\Service\Tag; use Doctrine\Common\Collections\Collection; use Shlinkio\Shlink\Core\Entity\Tag; -use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\TagNotFoundException; interface TagServiceInterface { @@ -17,23 +17,17 @@ interface TagServiceInterface /** * @param string[] $tagNames - * @return void */ public function deleteTags(array $tagNames): void; /** - * Provided a list of tag names, creates all that do not exist yet - * * @param string[] $tagNames * @return Collection|Tag[] */ public function createTags(array $tagNames): Collection; /** - * @param string $oldName - * @param string $newName - * @return Tag - * @throws EntityDoesNotExistException + * @throws TagNotFoundException */ - public function renameTag($oldName, $newName): Tag; + public function renameTag(string $oldName, string $newName): Tag; } diff --git a/module/Core/test/Service/Tag/TagServiceTest.php b/module/Core/test/Service/Tag/TagServiceTest.php index 0dd63ff5..0131d9e9 100644 --- a/module/Core/test/Service/Tag/TagServiceTest.php +++ b/module/Core/test/Service/Tag/TagServiceTest.php @@ -10,7 +10,7 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\Tag; -use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\TagNotFoundException; use Shlinkio\Shlink\Core\Repository\TagRepository; use Shlinkio\Shlink\Core\Service\Tag\TagService; @@ -83,7 +83,7 @@ class TagServiceTest extends TestCase $find->shouldBeCalled(); $getRepo->shouldBeCalled(); - $this->expectException(EntityDoesNotExistException::class); + $this->expectException(TagNotFoundException::class); $this->service->renameTag('foo', 'bar'); } diff --git a/module/Rest/src/Action/Tag/UpdateTagAction.php b/module/Rest/src/Action/Tag/UpdateTagAction.php index 64ec7a21..175ed0a0 100644 --- a/module/Rest/src/Action/Tag/UpdateTagAction.php +++ b/module/Rest/src/Action/Tag/UpdateTagAction.php @@ -7,14 +7,10 @@ namespace Shlinkio\Shlink\Rest\Action\Tag; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Log\LoggerInterface; -use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Service\Tag\TagServiceInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; -use Shlinkio\Shlink\Rest\Util\RestUtils; use Zend\Diactoros\Response\EmptyResponse; -use Zend\Diactoros\Response\JsonResponse; - -use function sprintf; class UpdateTagAction extends AbstractRestAction { @@ -43,21 +39,13 @@ class UpdateTagAction extends AbstractRestAction { $body = $request->getParsedBody(); if (! isset($body['oldName'], $body['newName'])) { - return new JsonResponse([ - 'error' => RestUtils::INVALID_ARGUMENT_ERROR, - 'message' => - 'You have to provide both \'oldName\' and \'newName\' params in order to properly rename the tag', - ], self::STATUS_BAD_REQUEST); + throw ValidationException::fromArray([ + 'oldName' => 'oldName is required', + 'newName' => 'newName is required', + ]); } - try { - $this->tagService->renameTag($body['oldName'], $body['newName']); - return new EmptyResponse(); - } catch (EntityDoesNotExistException $e) { - return new JsonResponse([ - 'error' => RestUtils::NOT_FOUND_ERROR, - 'message' => sprintf('It was not possible to find a tag with name %s', $body['oldName']), - ], self::STATUS_NOT_FOUND); - } + $this->tagService->renameTag($body['oldName'], $body['newName']); + return new EmptyResponse(); } } diff --git a/module/Rest/src/Action/Visit/GetVisitsAction.php b/module/Rest/src/Action/Visit/GetVisitsAction.php index 68d50533..ca8c2838 100644 --- a/module/Rest/src/Action/Visit/GetVisitsAction.php +++ b/module/Rest/src/Action/Visit/GetVisitsAction.php @@ -8,15 +8,11 @@ use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Common\Paginator\Util\PaginatorUtilsTrait; -use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; -use Shlinkio\Shlink\Rest\Util\RestUtils; use Zend\Diactoros\Response\JsonResponse; -use function sprintf; - class GetVisitsAction extends AbstractRestAction { use PaginatorUtilsTrait; @@ -36,19 +32,10 @@ class GetVisitsAction extends AbstractRestAction public function handle(Request $request): Response { $shortCode = $request->getAttribute('shortCode'); + $visits = $this->visitsTracker->info($shortCode, VisitsParams::fromRawData($request->getQueryParams())); - try { - $visits = $this->visitsTracker->info($shortCode, VisitsParams::fromRawData($request->getQueryParams())); - - return new JsonResponse([ - 'visits' => $this->serializePaginator($visits), - ]); - } catch (ShortUrlNotFoundException $e) { - $this->logger->warning('Provided nonexistent short code {e}', ['e' => $e]); - return new JsonResponse([ - 'error' => RestUtils::INVALID_ARGUMENT_ERROR, // FIXME Wrong code. Use correct one in "type" - 'message' => sprintf('Provided short code %s does not exist', $shortCode), - ], self::STATUS_NOT_FOUND); - } + return new JsonResponse([ + 'visits' => $this->serializePaginator($visits), + ]); } } diff --git a/module/Rest/src/Exception/AuthenticationException.php b/module/Rest/src/Exception/AuthenticationException.php index 1613bac5..3e60edb6 100644 --- a/module/Rest/src/Exception/AuthenticationException.php +++ b/module/Rest/src/Exception/AuthenticationException.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Rest\Exception; use Throwable; +/** @deprecated */ class AuthenticationException extends RuntimeException { public static function expiredJWT(?Throwable $prev = null): self diff --git a/module/Rest/src/Util/RestUtils.php b/module/Rest/src/Util/RestUtils.php index 97ca0d0b..3f4f1161 100644 --- a/module/Rest/src/Util/RestUtils.php +++ b/module/Rest/src/Util/RestUtils.php @@ -21,11 +21,14 @@ class RestUtils public const INVALID_ARGUMENT_ERROR = Core\ValidationException::TYPE; /** @deprecated */ public const INVALID_SLUG_ERROR = Core\NonUniqueSlugException::TYPE; + /** @deprecated */ public const INVALID_CREDENTIALS_ERROR = 'INVALID_CREDENTIALS'; public const INVALID_AUTH_TOKEN_ERROR = 'INVALID_AUTH_TOKEN'; public const INVALID_AUTHORIZATION_ERROR = 'INVALID_AUTHORIZATION'; public const INVALID_API_KEY_ERROR = 'INVALID_API_KEY'; - public const NOT_FOUND_ERROR = 'NOT_FOUND'; + /** @deprecated */ + public const NOT_FOUND_ERROR = Core\TagNotFoundException::TYPE; + /** @deprecated */ public const UNKNOWN_ERROR = 'UNKNOWN_ERROR'; /** @deprecated */ diff --git a/module/Rest/test-api/Action/GetVisitsActionTest.php b/module/Rest/test-api/Action/GetVisitsActionTest.php index 150c3066..d2c5f6eb 100644 --- a/module/Rest/test-api/Action/GetVisitsActionTest.php +++ b/module/Rest/test-api/Action/GetVisitsActionTest.php @@ -16,6 +16,6 @@ class GetVisitsActionTest extends ApiTestCase ['error' => $error] = $this->getJsonResponsePayload($resp); $this->assertEquals(self::STATUS_NOT_FOUND, $resp->getStatusCode()); - $this->assertEquals(RestUtils::INVALID_ARGUMENT_ERROR, $error); + $this->assertEquals(RestUtils::INVALID_SHORTCODE_ERROR, $error); } } diff --git a/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php index 63aa1ffb..ff86117e 100644 --- a/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php @@ -11,8 +11,6 @@ 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; -use Zend\Diactoros\Response\JsonResponse; use Zend\Diactoros\ServerRequest; class EditShortUrlActionTest extends TestCase diff --git a/module/Rest/test/Action/Tag/UpdateTagActionTest.php b/module/Rest/test/Action/Tag/UpdateTagActionTest.php index 9ded1716..3b068add 100644 --- a/module/Rest/test/Action/Tag/UpdateTagActionTest.php +++ b/module/Rest/test/Action/Tag/UpdateTagActionTest.php @@ -7,7 +7,7 @@ namespace ShlinkioTest\Shlink\Rest\Action\Tag; use PHPUnit\Framework\TestCase; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\Tag; -use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Service\Tag\TagServiceInterface; use Shlinkio\Shlink\Rest\Action\Tag\UpdateTagAction; use Zend\Diactoros\ServerRequest; @@ -32,9 +32,10 @@ class UpdateTagActionTest extends TestCase public function whenInvalidParamsAreProvidedAnErrorIsReturned(array $bodyParams): void { $request = (new ServerRequest())->withParsedBody($bodyParams); - $resp = $this->action->handle($request); - $this->assertEquals(400, $resp->getStatusCode()); + $this->expectException(ValidationException::class); + + $this->action->handle($request); } public function provideParams(): iterable @@ -44,21 +45,6 @@ class UpdateTagActionTest extends TestCase yield 'no params' => [[]]; } - /** @test */ - public function requestingInvalidTagReturnsError(): void - { - $request = (new ServerRequest())->withParsedBody([ - 'oldName' => 'foo', - 'newName' => 'bar', - ]); - $rename = $this->tagService->renameTag('foo', 'bar')->willThrow(EntityDoesNotExistException::class); - - $resp = $this->action->handle($request); - - $this->assertEquals(404, $resp->getStatusCode()); - $rename->shouldHaveBeenCalled(); - } - /** @test */ public function correctInvocationRenamesTag(): void { diff --git a/module/Rest/test/Action/Visit/GetVisitsActionTest.php b/module/Rest/test/Action/Visit/GetVisitsActionTest.php index cb38dd85..80e06085 100644 --- a/module/Rest/test/Action/Visit/GetVisitsActionTest.php +++ b/module/Rest/test/Action/Visit/GetVisitsActionTest.php @@ -9,7 +9,6 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Common\Util\DateRange; -use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTracker; use Shlinkio\Shlink\Rest\Action\Visit\GetVisitsAction; @@ -42,18 +41,6 @@ class GetVisitsActionTest extends TestCase $this->assertEquals(200, $response->getStatusCode()); } - /** @test */ - public function providingInvalidShortCodeReturnsError(): void - { - $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, Argument::type(VisitsParams::class))->willThrow( - ShortUrlNotFoundException::class - )->shouldBeCalledOnce(); - - $response = $this->action->handle((new ServerRequest())->withAttribute('shortCode', $shortCode)); - $this->assertEquals(404, $response->getStatusCode()); - } - /** @test */ public function paramsAreReadFromQuery(): void { diff --git a/module/Rest/test/Util/RestUtilsTest.php b/module/Rest/test/Util/RestUtilsTest.php index d8a45181..6097cdc6 100644 --- a/module/Rest/test/Util/RestUtilsTest.php +++ b/module/Rest/test/Util/RestUtilsTest.php @@ -6,8 +6,8 @@ namespace ShlinkioTest\Shlink\Rest\Util; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; -use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; +use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\Rest\Exception\AuthenticationException; use Shlinkio\Shlink\Rest\Util\RestUtils;