From 6208f6f0d5e4b983bfaf4bde65bd0db4bb86edbd Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 12 Oct 2017 11:28:45 +0200 Subject: [PATCH] Improved Exception management to be more specific --- config/container.php | 2 +- .../Command/Shortcode/ResolveUrlCommand.php | 5 ++++ .../Shortcode/ResolveUrlCommandTest.php | 5 ++-- module/Core/src/Action/PreviewAction.php | 10 +++++--- module/Core/src/Action/QrCodeAction.php | 7 +++--- module/Core/src/Action/RedirectAction.php | 9 ++++--- module/Core/src/Service/UrlShortener.php | 25 ++++++++++--------- .../src/Service/UrlShortenerInterface.php | 6 +++-- module/Core/test/Action/PreviewActionTest.php | 4 ++- module/Core/test/Action/QrCodeActionTest.php | 6 +++-- .../Core/test/Action/RedirectActionTest.php | 19 ++------------ module/Rest/src/Action/ResolveUrlAction.php | 14 +++++------ .../Rest/test/Action/ResolveUrlActionTest.php | 3 ++- 13 files changed, 60 insertions(+), 55 deletions(-) diff --git a/config/container.php b/config/container.php index 85cb0886..126351ae 100644 --- a/config/container.php +++ b/config/container.php @@ -11,7 +11,7 @@ require 'vendor/autoload.php'; // If the Dotenv class exists, load env vars and enable errors if (class_exists(Dotenv::class)) { error_reporting(E_ALL); - ini_set('display_errors', 1); + ini_set('display_errors', '1'); $dotenv = new Dotenv(__DIR__ . '/..'); $dotenv->load(); } diff --git a/module/CLI/src/Command/Shortcode/ResolveUrlCommand.php b/module/CLI/src/Command/Shortcode/ResolveUrlCommand.php index e5b53c23..fe79ee16 100644 --- a/module/CLI/src/Command/Shortcode/ResolveUrlCommand.php +++ b/module/CLI/src/Command/Shortcode/ResolveUrlCommand.php @@ -3,6 +3,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\Shortcode; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; use Symfony\Component\Console\Command\Command; @@ -81,6 +82,10 @@ class ResolveUrlCommand extends Command $output->writeln(sprintf('' . $this->translator->translate( 'Provided short code "%s" has an invalid format.' ) . '', $shortCode)); + } catch (EntityDoesNotExistException $e) { + $output->writeln(sprintf('' . $this->translator->translate( + 'Provided short code "%s" could not be found.' + ) . '', $shortCode)); } } } diff --git a/module/CLI/test/Command/Shortcode/ResolveUrlCommandTest.php b/module/CLI/test/Command/Shortcode/ResolveUrlCommandTest.php index c1fef098..da0eadc3 100644 --- a/module/CLI/test/Command/Shortcode/ResolveUrlCommandTest.php +++ b/module/CLI/test/Command/Shortcode/ResolveUrlCommandTest.php @@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\CLI\Command\Shortcode; use PHPUnit\Framework\TestCase; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\Shortcode\ResolveUrlCommand; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortener; use Symfony\Component\Console\Application; @@ -57,7 +58,7 @@ class ResolveUrlCommandTest extends TestCase public function incorrectShortCodeOutputsErrorMessage() { $shortCode = 'abc123'; - $this->urlShortener->shortCodeToUrl($shortCode)->willReturn(null) + $this->urlShortener->shortCodeToUrl($shortCode)->willThrow(EntityDoesNotExistException::class) ->shouldBeCalledTimes(1); $this->commandTester->execute([ @@ -65,7 +66,7 @@ class ResolveUrlCommandTest extends TestCase 'shortCode' => $shortCode, ]); $output = $this->commandTester->getDisplay(); - $this->assertEquals('No URL found for short code "' . $shortCode . '"' . PHP_EOL, $output); + $this->assertEquals('Provided short code "' . $shortCode . '" could not be found.' . PHP_EOL, $output); } /** diff --git a/module/Core/src/Action/PreviewAction.php b/module/Core/src/Action/PreviewAction.php index 054da045..788c3c2d 100644 --- a/module/Core/src/Action/PreviewAction.php +++ b/module/Core/src/Action/PreviewAction.php @@ -7,8 +7,10 @@ use Interop\Http\ServerMiddleware\DelegateInterface; use Interop\Http\ServerMiddleware\MiddlewareInterface; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; +use Shlinkio\Shlink\Common\Exception\PreviewGenerationException; use Shlinkio\Shlink\Common\Service\PreviewGeneratorInterface; use Shlinkio\Shlink\Common\Util\ResponseUtilsTrait; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; @@ -46,14 +48,14 @@ class PreviewAction implements MiddlewareInterface try { $url = $this->urlShortener->shortCodeToUrl($shortCode); - if (! isset($url)) { - return $delegate->process($request); - } - $imagePath = $this->previewGenerator->generatePreview($url); return $this->generateImageResponse($imagePath); } catch (InvalidShortCodeException $e) { return $delegate->process($request); + } catch (EntityDoesNotExistException $e) { + return $delegate->process($request); + } catch (PreviewGenerationException $e) { + return $delegate->process($request); } } } diff --git a/module/Core/src/Action/QrCodeAction.php b/module/Core/src/Action/QrCodeAction.php index a6e20af8..dfd7eaf2 100644 --- a/module/Core/src/Action/QrCodeAction.php +++ b/module/Core/src/Action/QrCodeAction.php @@ -11,6 +11,7 @@ use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; use Shlinkio\Shlink\Common\Response\QrCodeResponse; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; use Zend\Expressive\Router\RouterInterface; @@ -55,12 +56,12 @@ class QrCodeAction implements MiddlewareInterface $shortCode = $request->getAttribute('shortCode'); try { $shortUrl = $this->urlShortener->shortCodeToUrl($shortCode); - if ($shortUrl === null) { - return $delegate->process($request); - } } catch (InvalidShortCodeException $e) { $this->logger->warning('Tried to create a QR code with an invalid short code' . PHP_EOL . $e); return $delegate->process($request); + } catch (EntityDoesNotExistException $e) { + $this->logger->warning('Tried to create a QR code with a not found short code' . PHP_EOL . $e); + return $delegate->process($request); } $path = $this->router->generateUri('long-url-redirect', ['shortCode' => $shortCode]); diff --git a/module/Core/src/Action/RedirectAction.php b/module/Core/src/Action/RedirectAction.php index ae58da1b..c8d35b4a 100644 --- a/module/Core/src/Action/RedirectAction.php +++ b/module/Core/src/Action/RedirectAction.php @@ -9,8 +9,11 @@ use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; +use Zend\Diactoros\Response\HtmlResponse; use Zend\Diactoros\Response\RedirectResponse; class RedirectAction implements MiddlewareInterface @@ -66,9 +69,9 @@ class RedirectAction implements MiddlewareInterface // Return a redirect response to the long URL. // Use a temporary redirect to make sure browsers always hit the server for analytics purposes return new RedirectResponse($longUrl); - } catch (\Exception $e) { - // In case of error, dispatch 404 error - $this->logger->error('Error redirecting to long URL.' . PHP_EOL . $e); + } catch (InvalidShortCodeException $e) { + return $delegate->process($request); + } catch (EntityDoesNotExistException $e) { return $delegate->process($request); } } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index df4ee8ed..b30b649a 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -11,6 +11,7 @@ use GuzzleHttp\Exception\GuzzleException; use Psr\Http\Message\UriInterface; use Shlinkio\Shlink\Common\Exception\RuntimeException; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Util\TagManagerTrait; @@ -142,10 +143,11 @@ class UrlShortener implements UrlShortenerInterface * Tries to find the mapped URL for provided short code. Returns null if not found * * @param string $shortCode - * @return string|null + * @return string * @throws InvalidShortCodeException + * @throws EntityDoesNotExistException */ - public function shortCodeToUrl($shortCode) + public function shortCodeToUrl($shortCode): string { $cacheKey = sprintf('%s_longUrl', $shortCode); // Check if the short code => URL map is already cached @@ -158,17 +160,16 @@ class UrlShortener implements UrlShortenerInterface throw InvalidShortCodeException::fromCharset($shortCode, $this->chars); } - /** @var ShortUrl $shortUrl */ - $shortUrl = $this->em->getRepository(ShortUrl::class)->findOneBy([ - 'shortCode' => $shortCode, - ]); - // Cache the shortcode - if (isset($shortUrl)) { - $url = $shortUrl->getOriginalUrl(); - $this->cache->save($cacheKey, $url); - return $url; + $criteria = ['shortCode' => $shortCode]; + /** @var ShortUrl|null $shortUrl */ + $shortUrl = $this->em->getRepository(ShortUrl::class)->findOneBy($criteria); + if ($shortUrl === null) { + throw EntityDoesNotExistException::createFromEntityAndConditions(ShortUrl::class, $criteria); } - return null; + // Cache the shortcode + $url = $shortUrl->getOriginalUrl(); + $this->cache->save($cacheKey, $url); + return $url; } } diff --git a/module/Core/src/Service/UrlShortenerInterface.php b/module/Core/src/Service/UrlShortenerInterface.php index c677ec4e..3cd2c35c 100644 --- a/module/Core/src/Service/UrlShortenerInterface.php +++ b/module/Core/src/Service/UrlShortenerInterface.php @@ -5,6 +5,7 @@ namespace Shlinkio\Shlink\Core\Service; use Psr\Http\Message\UriInterface; use Shlinkio\Shlink\Common\Exception\RuntimeException; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; @@ -25,8 +26,9 @@ interface UrlShortenerInterface * Tries to find the mapped URL for provided short code. Returns null if not found * * @param string $shortCode - * @return string|null + * @return string * @throws InvalidShortCodeException + * @throws EntityDoesNotExistException */ - public function shortCodeToUrl($shortCode); + public function shortCodeToUrl($shortCode): string; } diff --git a/module/Core/test/Action/PreviewActionTest.php b/module/Core/test/Action/PreviewActionTest.php index 5521e677..d791a100 100644 --- a/module/Core/test/Action/PreviewActionTest.php +++ b/module/Core/test/Action/PreviewActionTest.php @@ -9,6 +9,7 @@ use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Common\Service\PreviewGenerator; use Shlinkio\Shlink\Core\Action\PreviewAction; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortener; use ShlinkioTest\Shlink\Common\Util\TestUtils; @@ -42,7 +43,8 @@ class PreviewActionTest extends TestCase public function invalidShortCodeFallsBackToNextMiddleware() { $shortCode = 'abc123'; - $this->urlShortener->shortCodeToUrl($shortCode)->willReturn(null)->shouldBeCalledTimes(1); + $this->urlShortener->shortCodeToUrl($shortCode)->willThrow(EntityDoesNotExistException::class) + ->shouldBeCalledTimes(1); $delegate = $this->prophesize(DelegateInterface::class); $delegate->process(Argument::cetera())->shouldBeCalledTimes(1); diff --git a/module/Core/test/Action/QrCodeActionTest.php b/module/Core/test/Action/QrCodeActionTest.php index 85f908e2..ba8adc2f 100644 --- a/module/Core/test/Action/QrCodeActionTest.php +++ b/module/Core/test/Action/QrCodeActionTest.php @@ -10,6 +10,7 @@ use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Common\Response\QrCodeResponse; use Shlinkio\Shlink\Core\Action\QrCodeAction; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortener; use Zend\Diactoros\ServerRequestFactory; @@ -42,7 +43,8 @@ class QrCodeActionTest extends TestCase public function aNotFoundShortCodeWillDelegateIntoNextMiddleware() { $shortCode = 'abc123'; - $this->urlShortener->shortCodeToUrl($shortCode)->willReturn(null)->shouldBeCalledTimes(1); + $this->urlShortener->shortCodeToUrl($shortCode)->willThrow(EntityDoesNotExistException::class) + ->shouldBeCalledTimes(1); $delegate = $this->prophesize(DelegateInterface::class); $this->action->process( @@ -77,7 +79,7 @@ class QrCodeActionTest extends TestCase public function aCorrectRequestReturnsTheQrCodeResponse() { $shortCode = 'abc123'; - $this->urlShortener->shortCodeToUrl($shortCode)->willReturn(new ShortUrl())->shouldBeCalledTimes(1); + $this->urlShortener->shortCodeToUrl($shortCode)->willReturn('')->shouldBeCalledTimes(1); $delegate = $this->prophesize(DelegateInterface::class); $resp = $this->action->process( diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index e2cc700a..49d3baec 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -8,6 +8,7 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Action\RedirectAction; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Service\UrlShortener; use Shlinkio\Shlink\Core\Service\VisitsTracker; use ShlinkioTest\Shlink\Common\Util\TestUtils; @@ -58,23 +59,7 @@ class RedirectActionTest extends TestCase public function nextMiddlewareIsInvokedIfLongUrlIsNotFound() { $shortCode = 'abc123'; - $this->urlShortener->shortCodeToUrl($shortCode)->willReturn(null) - ->shouldBeCalledTimes(1); - $delegate = $this->prophesize(DelegateInterface::class); - - $request = ServerRequestFactory::fromGlobals()->withAttribute('shortCode', $shortCode); - $this->action->process($request, $delegate->reveal()); - - $delegate->process($request)->shouldHaveBeenCalledTimes(1); - } - - /** - * @test - */ - public function nextMiddlewareIsInvokedIfAnExceptionIsThrown() - { - $shortCode = 'abc123'; - $this->urlShortener->shortCodeToUrl($shortCode)->willThrow(\Exception::class) + $this->urlShortener->shortCodeToUrl($shortCode)->willThrow(EntityDoesNotExistException::class) ->shouldBeCalledTimes(1); $delegate = $this->prophesize(DelegateInterface::class); diff --git a/module/Rest/src/Action/ResolveUrlAction.php b/module/Rest/src/Action/ResolveUrlAction.php index f20d4222..1abf5626 100644 --- a/module/Rest/src/Action/ResolveUrlAction.php +++ b/module/Rest/src/Action/ResolveUrlAction.php @@ -7,6 +7,7 @@ use Interop\Http\ServerMiddleware\DelegateInterface; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; use Shlinkio\Shlink\Rest\Util\RestUtils; @@ -46,13 +47,6 @@ class ResolveUrlAction extends AbstractRestAction try { $longUrl = $this->urlShortener->shortCodeToUrl($shortCode); - if ($longUrl === null) { - return new JsonResponse([ - 'error' => RestUtils::INVALID_ARGUMENT_ERROR, - 'message' => sprintf($this->translator->translate('No URL found for short code "%s"'), $shortCode), - ], self::STATUS_NOT_FOUND); - } - return new JsonResponse([ 'longUrl' => $longUrl, ]); @@ -65,6 +59,12 @@ class ResolveUrlAction extends AbstractRestAction $shortCode ), ], self::STATUS_BAD_REQUEST); + } catch (EntityDoesNotExistException $e) { + $this->logger->warning('Provided short code couldn\'t be found.' . PHP_EOL . $e); + return new JsonResponse([ + 'error' => RestUtils::INVALID_ARGUMENT_ERROR, + 'message' => sprintf($this->translator->translate('No URL found for short code "%s"'), $shortCode), + ], self::STATUS_NOT_FOUND); } catch (\Exception $e) { $this->logger->error('Unexpected error while resolving the URL behind a short code.' . PHP_EOL . $e); return new JsonResponse([ diff --git a/module/Rest/test/Action/ResolveUrlActionTest.php b/module/Rest/test/Action/ResolveUrlActionTest.php index 48c17b20..884fae55 100644 --- a/module/Rest/test/Action/ResolveUrlActionTest.php +++ b/module/Rest/test/Action/ResolveUrlActionTest.php @@ -5,6 +5,7 @@ namespace ShlinkioTest\Shlink\Rest\Action; use PHPUnit\Framework\TestCase; use Prophecy\Prophecy\ObjectProphecy; +use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortener; use Shlinkio\Shlink\Rest\Action\ResolveUrlAction; @@ -36,7 +37,7 @@ class ResolveUrlActionTest extends TestCase public function incorrectShortCodeReturnsError() { $shortCode = 'abc123'; - $this->urlShortener->shortCodeToUrl($shortCode)->willReturn(null) + $this->urlShortener->shortCodeToUrl($shortCode)->willThrow(EntityDoesNotExistException::class) ->shouldBeCalledTimes(1); $request = ServerRequestFactory::fromGlobals()->withAttribute('shortCode', $shortCode);