From d61f5faf5905a61043a364322eb9155a68e429f5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 29 Jan 2019 13:55:47 +0100 Subject: [PATCH] Refactored UrlShortener public method to receibe DTOs instead of primitive params --- .../ShortUrl/GenerateShortUrlCommand.php | 11 ++-- module/Core/src/Model/CreateShortUrlData.php | 4 +- module/Core/src/Model/ShortUrlMeta.php | 8 +++ module/Core/src/Service/UrlShortener.php | 53 +++++++++---------- .../src/Service/UrlShortenerInterface.php | 14 ++--- module/Core/test/Service/UrlShortenerTest.php | 40 ++++++++------ .../ShortUrl/AbstractCreateShortUrlAction.php | 11 +--- .../ShortUrl/CreateShortUrlActionTest.php | 5 +- .../SingleStepCreateShortUrlActionTest.php | 6 +-- 9 files changed, 74 insertions(+), 78 deletions(-) diff --git a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php index 65b417ac..b187b97c 100644 --- a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\CLI\Command\ShortUrl; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; +use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; use Shlinkio\Shlink\Core\Util\ShortUrlBuilderTrait; use Symfony\Component\Console\Command\Command; @@ -116,10 +117,12 @@ class GenerateShortUrlCommand extends Command $shortCode = $this->urlShortener->urlToShortCode( new Uri($longUrl), $tags, - $this->getOptionalDate($input, 'validSince'), - $this->getOptionalDate($input, 'validUntil'), - $customSlug, - $maxVisits !== null ? (int) $maxVisits : null + ShortUrlMeta::createFromParams( + $this->getOptionalDate($input, 'validSince'), + $this->getOptionalDate($input, 'validUntil'), + $customSlug, + $maxVisits !== null ? (int) $maxVisits : null + ) )->getShortCode(); $shortUrl = $this->buildShortUrl($this->domainConfig, $shortCode); diff --git a/module/Core/src/Model/CreateShortUrlData.php b/module/Core/src/Model/CreateShortUrlData.php index fa2fbde9..adf1683e 100644 --- a/module/Core/src/Model/CreateShortUrlData.php +++ b/module/Core/src/Model/CreateShortUrlData.php @@ -17,11 +17,11 @@ final class CreateShortUrlData public function __construct( UriInterface $longUrl, array $tags = [], - ShortUrlMeta $meta = null + ?ShortUrlMeta $meta = null ) { $this->longUrl = $longUrl; $this->tags = $tags; - $this->meta = $meta ?? ShortUrlMeta::createFromParams(); + $this->meta = $meta ?? ShortUrlMeta::createEmpty(); } /** diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index 32df4376..35e450a0 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -138,4 +138,12 @@ final class ShortUrlMeta { return $this->maxVisits !== null; } + + public function withCustomSlug(string $customSlug): self + { + $clone = clone $this; + $clone->customSlug = $customSlug; + + return $clone; + } } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index 24a7ced4..96355c58 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -3,7 +3,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; -use Cake\Chronos\Chronos; use Cocur\Slugify\SlugifyInterface; use Doctrine\ORM\EntityManagerInterface; use GuzzleHttp\ClientInterface; @@ -53,41 +52,35 @@ class UrlShortener implements UrlShortenerInterface } /** + * @param string[] $tags * @throws NonUniqueSlugException * @throws InvalidUrlException * @throws RuntimeException */ - public function urlToShortCode( - UriInterface $url, - array $tags = [], - ?Chronos $validSince = null, - ?Chronos $validUntil = null, - ?string $customSlug = null, - ?int $maxVisits = null - ): ShortUrl { + public function urlToShortCode(UriInterface $url, array $tags, ShortUrlMeta $meta): ShortUrl + { // If the URL validation is enabled, check that the URL actually exists if ($this->options->isUrlValidationEnabled()) { $this->checkUrlExists($url); } - $customSlug = $this->processCustomSlug($customSlug); + $meta = $this->processCustomSlug($meta); // Transactionally insert the short url, then generate the short code and finally update the short code try { $this->em->beginTransaction(); // First, create the short URL with an empty short code - $shortUrl = new ShortUrl( - (string) $url, - ShortUrlMeta::createFromParams($validSince, $validUntil, null, $maxVisits) - ); + $shortUrl = new ShortUrl((string) $url, $meta); $this->em->persist($shortUrl); $this->em->flush(); - // Generate the short code and persist it - // TODO Somehow provide the logic to calculate the shortCode to avoid the need of a setter - $shortCode = $customSlug ?? $this->convertAutoincrementIdToShortCode((float) $shortUrl->getId()); - $shortUrl->setShortCode($shortCode) - ->setTags($this->tagNamesToEntities($this->em, $tags)); + // Generate the short code and persist it if no custom slug was provided + if (! $meta->hasCustomSlug()) { + // TODO Somehow provide the logic to calculate the shortCode to avoid the need of a setter + $shortCode = $this->convertAutoincrementIdToShortCode((float) $shortUrl->getId()); + $shortUrl->setShortCode($shortCode); + } + $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); $this->em->flush(); $this->em->commit(); @@ -130,25 +123,27 @@ class UrlShortener implements UrlShortenerInterface return $chars[(int) $id] . $code; } - private function processCustomSlug(?string $customSlug): ?string + private function processCustomSlug(ShortUrlMeta $meta): ?ShortUrlMeta { - if ($customSlug === null) { - return null; + if (! $meta->hasCustomSlug()) { + return $meta; } - // If a custom slug was provided, make sure it's unique - $customSlug = $this->slugger->slugify($customSlug); - $shortUrl = $this->em->getRepository(ShortUrl::class)->findOneBy(['shortCode' => $customSlug]); - if ($shortUrl !== null) { + // FIXME If the slug was generated while filtering the value originally, we would not need an immutable setter + // in ShortUrlMeta + $customSlug = $this->slugger->slugify($meta->getCustomSlug()); + + /** @var ShortUrlRepository $repo */ + $repo = $this->em->getRepository(ShortUrl::class); + $shortUrlsCount = $repo->count(['shortCode' => $customSlug]); + if ($shortUrlsCount > 0) { throw NonUniqueSlugException::fromSlug($customSlug); } - return $customSlug; + return $meta->withCustomSlug($customSlug); } /** - * Tries to find the mapped URL for provided short code. Returns null if not found - * * @throws InvalidShortCodeException * @throws EntityDoesNotExistException */ diff --git a/module/Core/src/Service/UrlShortenerInterface.php b/module/Core/src/Service/UrlShortenerInterface.php index ff9780d9..314b6de2 100644 --- a/module/Core/src/Service/UrlShortenerInterface.php +++ b/module/Core/src/Service/UrlShortenerInterface.php @@ -3,7 +3,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; -use Cake\Chronos\Chronos; use Psr\Http\Message\UriInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; @@ -11,26 +10,19 @@ use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; use Shlinkio\Shlink\Core\Exception\RuntimeException; +use Shlinkio\Shlink\Core\Model\ShortUrlMeta; interface UrlShortenerInterface { /** + * @param string[] $tags * @throws NonUniqueSlugException * @throws InvalidUrlException * @throws RuntimeException */ - public function urlToShortCode( - UriInterface $url, - array $tags = [], - ?Chronos $validSince = null, - ?Chronos $validUntil = null, - ?string $customSlug = null, - ?int $maxVisits = null - ): ShortUrl; + public function urlToShortCode(UriInterface $url, array $tags, ShortUrlMeta $meta): ShortUrl; /** - * Tries to find the mapped URL for provided short code. Returns null if not found - * * @throws InvalidShortCodeException * @throws EntityDoesNotExistException */ diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 19896441..fb625202 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Service; use Cocur\Slugify\SlugifyInterface; -use Doctrine\Common\Persistence\ObjectRepository; use Doctrine\DBAL\Connection; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\ORMException; @@ -17,7 +16,9 @@ use Prophecy\Prophecy\MethodProphecy; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; +use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; +use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\UrlShortener; use Zend\Diactoros\Uri; @@ -49,8 +50,8 @@ class UrlShortenerTest extends TestCase $shortUrl = $arguments[0]; $shortUrl->setId('10'); }); - $repo = $this->prophesize(ObjectRepository::class); - $repo->findOneBy(Argument::any())->willReturn(null); + $repo = $this->prophesize(ShortUrlRepository::class); + $repo->count(Argument::any())->willReturn(0); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->slugger = $this->prophesize(SlugifyInterface::class); @@ -74,7 +75,11 @@ class UrlShortenerTest extends TestCase public function urlIsProperlyShortened() { // 10 -> 12C1c - $shortUrl = $this->urlShortener->urlToShortCode(new Uri('http://foobar.com/12345/hello?foo=bar')); + $shortUrl = $this->urlShortener->urlToShortCode( + new Uri('http://foobar.com/12345/hello?foo=bar'), + [], + ShortUrlMeta::createEmpty() + ); $this->assertEquals('12C1c', $shortUrl->getShortCode()); } @@ -91,7 +96,11 @@ class UrlShortenerTest extends TestCase $this->em->close()->shouldBeCalledOnce(); $this->em->flush()->willThrow(new ORMException()); - $this->urlShortener->urlToShortCode(new Uri('http://foobar.com/12345/hello?foo=bar')); + $this->urlShortener->urlToShortCode( + new Uri('http://foobar.com/12345/hello?foo=bar'), + [], + ShortUrlMeta::createEmpty() + ); } /** @@ -105,7 +114,11 @@ class UrlShortenerTest extends TestCase $this->httpClient->request(Argument::cetera())->willThrow( new ClientException('', $this->prophesize(Request::class)->reveal()) ); - $this->urlShortener->urlToShortCode(new Uri('http://foobar.com/12345/hello?foo=bar')); + $this->urlShortener->urlToShortCode( + new Uri('http://foobar.com/12345/hello?foo=bar'), + [], + ShortUrlMeta::createEmpty() + ); } /** @@ -119,9 +132,7 @@ class UrlShortenerTest extends TestCase $this->urlShortener->urlToShortCode( new Uri('http://foobar.com/12345/hello?foo=bar'), [], - null, - null, - 'custom-slug' + ShortUrlMeta::createFromRawData(['customSlug' => 'custom-slug']) ); $slugify->shouldHaveBeenCalledOnce(); @@ -135,24 +146,21 @@ class UrlShortenerTest extends TestCase /** @var MethodProphecy $slugify */ $slugify = $this->slugger->slugify('custom-slug')->willReturnArgument(); - $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - /** @var MethodProphecy $findBySlug */ - $findBySlug = $repo->findOneBy(['shortCode' => 'custom-slug'])->willReturn(new ShortUrl('')); + $repo = $this->prophesize(ShortUrlRepository::class); + $countBySlug = $repo->count(['shortCode' => 'custom-slug'])->willReturn(1); $repo->findOneBy(Argument::cetera())->willReturn(null); /** @var MethodProphecy $getRepo */ $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $slugify->shouldBeCalledOnce(); - $findBySlug->shouldBeCalledOnce(); + $countBySlug->shouldBeCalledOnce(); $getRepo->shouldBeCalled(); $this->expectException(NonUniqueSlugException::class); $this->urlShortener->urlToShortCode( new Uri('http://foobar.com/12345/hello?foo=bar'), [], - null, - null, - 'custom-slug' + ShortUrlMeta::createFromRawData(['customSlug' => 'custom-slug']) ); } diff --git a/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php index 4e90e4e2..a9d9dcfa 100644 --- a/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php @@ -53,17 +53,9 @@ abstract class AbstractCreateShortUrlAction extends AbstractRestAction $longUrl = $shortUrlData->getLongUrl(); $shortUrlMeta = $shortUrlData->getMeta(); - $customSlug = $shortUrlMeta->getCustomSlug(); try { - $shortUrl = $this->urlShortener->urlToShortCode( - $longUrl, - $shortUrlData->getTags(), - $shortUrlMeta->getValidSince(), - $shortUrlMeta->getValidUntil(), - $customSlug, - $shortUrlMeta->getMaxVisits() - ); + $shortUrl = $this->urlShortener->urlToShortCode($longUrl, $shortUrlData->getTags(), $shortUrlMeta); $transformer = new ShortUrlDataTransformer($this->domainConfig); return new JsonResponse($transformer->transform($shortUrl)); @@ -74,6 +66,7 @@ abstract class AbstractCreateShortUrlAction extends AbstractRestAction 'message' => sprintf('Provided URL %s is invalid. Try with a different one.', $longUrl), ], self::STATUS_BAD_REQUEST); } catch (NonUniqueSlugException $e) { + $customSlug = $shortUrlMeta->getCustomSlug(); $this->logger->warning('Provided non-unique slug. {e}', ['e' => $e]); return new JsonResponse([ 'error' => RestUtils::getRestErrorCodeFromException($e), diff --git a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php index cc9a06a2..7c84388d 100644 --- a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php @@ -10,6 +10,7 @@ use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; +use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Service\UrlShortener; use Shlinkio\Shlink\Rest\Action\ShortUrl\CreateShortUrlAction; use Shlinkio\Shlink\Rest\Util\RestUtils; @@ -86,9 +87,7 @@ class CreateShortUrlActionTest extends TestCase $this->urlShortener->urlToShortCode( Argument::type(Uri::class), Argument::type('array'), - null, - null, - 'foo', + ShortUrlMeta::createFromRawData(['customSlug' => 'foo']), Argument::cetera() )->willThrow(NonUniqueSlugException::class)->shouldBeCalledOnce(); diff --git a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php index 2735e633..7a7dc4d7 100644 --- a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php @@ -9,6 +9,7 @@ use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Psr\Http\Message\UriInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; +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; @@ -91,10 +92,7 @@ class SingleStepCreateShortUrlActionTest extends TestCase return $argument; }), [], - null, - null, - null, - null + ShortUrlMeta::createEmpty() )->willReturn(new ShortUrl('')); $resp = $this->action->handle($request);