From ea1b285d5253273b90887109093a354f8783c723 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 29 Jan 2019 13:05:26 +0100 Subject: [PATCH 01/11] Little refactopring on tests config file --- config/test/test_config.global.php | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/config/test/test_config.global.php b/config/test/test_config.global.php index 3de2584e..10778bf7 100644 --- a/config/test/test_config.global.php +++ b/config/test/test_config.global.php @@ -7,8 +7,12 @@ use GuzzleHttp\Client; use Zend\ConfigAggregator\ConfigAggregator; use Zend\ServiceManager\Factory\InvokableFactory; use function realpath; +use function sprintf; use function sys_get_temp_dir; +$swooleTestingHost = '127.0.0.1'; +$swooleTestingPort = 9999; + return [ 'debug' => true, @@ -23,8 +27,8 @@ return [ 'zend-expressive-swoole' => [ 'swoole-http-server' => [ - 'port' => 9999, - 'host' => '127.0.0.1', + 'host' => $swooleTestingHost, + 'port' => $swooleTestingPort, 'process-name' => 'shlink_test', 'options' => [ 'pid_file' => sys_get_temp_dir() . '/shlink-test-swoole.pid', @@ -33,11 +37,13 @@ return [ ], 'dependencies' => [ + 'services' => [ + 'shlink_test_api_client' => new Client([ + 'base_uri' => sprintf('http://%s:%s/', $swooleTestingHost, $swooleTestingPort), + ]), + ], 'factories' => [ Common\TestHelper::class => InvokableFactory::class, - 'shlink_test_api_client' => function () { - return new Client(['base_uri' => 'http://localhost:9999/']); - }, ], ], From 57566095310131fd45826439a60e78ad2de45e02 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 29 Jan 2019 13:20:46 +0100 Subject: [PATCH 02/11] Deleted deprecated constant --- module/CLI/src/Command/Config/GenerateCharsetCommand.php | 6 +++--- module/Core/src/Service/UrlShortener.php | 2 -- .../src/Config/Plugin/UrlShortenerConfigCustomizer.php | 4 ++-- .../src/Action/ShortUrl/AbstractCreateShortUrlAction.php | 8 ++++---- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/module/CLI/src/Command/Config/GenerateCharsetCommand.php b/module/CLI/src/Command/Config/GenerateCharsetCommand.php index 5fe417d7..34e23195 100644 --- a/module/CLI/src/Command/Config/GenerateCharsetCommand.php +++ b/module/CLI/src/Command/Config/GenerateCharsetCommand.php @@ -3,7 +3,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\Config; -use Shlinkio\Shlink\Core\Service\UrlShortener; +use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; @@ -22,13 +22,13 @@ class GenerateCharsetCommand extends Command ->setDescription(sprintf( 'Generates a character set sample just by shuffling the default one, "%s". ' . 'Then it can be set in the SHORTCODE_CHARS environment variable', - UrlShortener::DEFAULT_CHARS + UrlShortenerOptions::DEFAULT_CHARS )); } protected function execute(InputInterface $input, OutputInterface $output): void { - $charSet = str_shuffle(UrlShortener::DEFAULT_CHARS); + $charSet = str_shuffle(UrlShortenerOptions::DEFAULT_CHARS); (new SymfonyStyle($input, $output))->success(sprintf('Character set: "%s"', $charSet)); } } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index c02e8f70..24a7ced4 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -29,8 +29,6 @@ class UrlShortener implements UrlShortenerInterface { use TagManagerTrait; - /** @deprecated */ - public const DEFAULT_CHARS = UrlShortenerOptions::DEFAULT_CHARS; private const ID_INCREMENT = 200000; /** @var ClientInterface */ diff --git a/module/Installer/src/Config/Plugin/UrlShortenerConfigCustomizer.php b/module/Installer/src/Config/Plugin/UrlShortenerConfigCustomizer.php index 0e5d4a00..0ef0c8d6 100644 --- a/module/Installer/src/Config/Plugin/UrlShortenerConfigCustomizer.php +++ b/module/Installer/src/Config/Plugin/UrlShortenerConfigCustomizer.php @@ -3,7 +3,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Installer\Config\Plugin; -use Shlinkio\Shlink\Core\Service\UrlShortener; +use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Installer\Model\CustomizableAppConfig; use Shlinkio\Shlink\Installer\Util\AskUtilsTrait; use Symfony\Component\Console\Style\SymfonyStyle; @@ -66,7 +66,7 @@ class UrlShortenerConfigCustomizer implements ConfigCustomizerInterface case self::CHARS: return $io->ask( 'Character set for generated short codes (leave empty to autogenerate one)' - ) ?: str_shuffle(UrlShortener::DEFAULT_CHARS); + ) ?: str_shuffle(UrlShortenerOptions::DEFAULT_CHARS); case self::VALIDATE_URL: return $io->confirm('Do you want to validate long urls by 200 HTTP status code on response'); case self::ENABLE_NOT_FOUND_REDIRECTION: diff --git a/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php index 33849c23..4e90e4e2 100644 --- a/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php @@ -38,15 +38,11 @@ abstract class AbstractCreateShortUrlAction extends AbstractRestAction /** * @param Request $request * @return Response - * @throws \InvalidArgumentException */ public function handle(Request $request): Response { try { $shortUrlData = $this->buildShortUrlData($request); - $shortUrlMeta = $shortUrlData->getMeta(); - $longUrl = $shortUrlData->getLongUrl(); - $customSlug = $shortUrlMeta->getCustomSlug(); } catch (InvalidArgumentException $e) { $this->logger->warning('Provided data is invalid. {e}', ['e' => $e]); return new JsonResponse([ @@ -55,6 +51,10 @@ abstract class AbstractCreateShortUrlAction extends AbstractRestAction ], self::STATUS_BAD_REQUEST); } + $longUrl = $shortUrlData->getLongUrl(); + $shortUrlMeta = $shortUrlData->getMeta(); + $customSlug = $shortUrlMeta->getCustomSlug(); + try { $shortUrl = $this->urlShortener->urlToShortCode( $longUrl, From d61f5faf5905a61043a364322eb9155a68e429f5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 29 Jan 2019 13:55:47 +0100 Subject: [PATCH 03/11] 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); From 4c46aaead8f46f7c44c2aeb9872572a7cbda46f8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 30 Jan 2019 18:28:07 +0100 Subject: [PATCH 04/11] Improved API tests and added test for short URLs creation --- config/test/bootstrap_api_tests.php | 8 +- config/test/test_config.global.php | 1 + module/Common/test-db/ApiTest/ApiTestCase.php | 38 ++++-- module/Common/test-db/TestHelper.php | 5 +- .../Action/CreateShortUrlActionTest.php | 121 ++++++++++++++++++ .../Middleware/AuthenticationTest.php | 48 +++---- 6 files changed, 176 insertions(+), 45 deletions(-) create mode 100644 module/Rest/test-api/Action/CreateShortUrlActionTest.php diff --git a/config/test/bootstrap_api_tests.php b/config/test/bootstrap_api_tests.php index c39042fa..a016f93f 100644 --- a/config/test/bootstrap_api_tests.php +++ b/config/test/bootstrap_api_tests.php @@ -17,10 +17,10 @@ if (! file_exists('.env')) { $container = require __DIR__ . '/../container.php'; $testHelper = $container->get(TestHelper::class); $config = $container->get('config'); +$em = $container->get(EntityManager::class); $testHelper->createTestDb(); - -$em = $container->get(EntityManager::class); -$testHelper->seedFixtures($em, $config['data_fixtures'] ?? []); - ApiTest\ApiTestCase::setApiClient($container->get('shlink_test_api_client')); +ApiTest\ApiTestCase::setSeedFixturesCallback(function () use ($testHelper, $em, $config) { + $testHelper->seedFixtures($em, $config['data_fixtures'] ?? []); +}); diff --git a/config/test/test_config.global.php b/config/test/test_config.global.php index 10778bf7..dd794fe4 100644 --- a/config/test/test_config.global.php +++ b/config/test/test_config.global.php @@ -40,6 +40,7 @@ return [ 'services' => [ 'shlink_test_api_client' => new Client([ 'base_uri' => sprintf('http://%s:%s/', $swooleTestingHost, $swooleTestingPort), + 'http_errors' => false, ]), ], 'factories' => [ diff --git a/module/Common/test-db/ApiTest/ApiTestCase.php b/module/Common/test-db/ApiTest/ApiTestCase.php index 29959837..65006bb0 100644 --- a/module/Common/test-db/ApiTest/ApiTestCase.php +++ b/module/Common/test-db/ApiTest/ApiTestCase.php @@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\Common\ApiTest; use Fig\Http\Message\RequestMethodInterface; use Fig\Http\Message\StatusCodeInterface; use GuzzleHttp\ClientInterface; +use GuzzleHttp\RequestOptions; use PHPUnit\Framework\TestCase; use Psr\Http\Message\ResponseInterface; use Shlinkio\Shlink\Rest\Authentication\Plugin\ApiKeyHeaderPlugin; @@ -14,32 +15,40 @@ use function sprintf; abstract class ApiTestCase extends TestCase implements StatusCodeInterface, RequestMethodInterface { - private const PATH_PREFX = '/rest/v1'; + private const REST_PATH_PREFX = '/rest/v1'; /** @var ClientInterface */ private static $client; + /** @var callable */ + private static $seedFixtures; public static function setApiClient(ClientInterface $client): void { self::$client = $client; } - /** - * @throws \GuzzleHttp\Exception\GuzzleException - */ - protected function callApi(string $method, string $uri, array $options = []): ResponseInterface + public static function setSeedFixturesCallback(callable $seedFixtures): void { - return self::$client->request($method, sprintf('%s%s', self::PATH_PREFX, $uri), $options); + self::$seedFixtures = $seedFixtures; + } + + public function setUp(): void + { + if (self::$seedFixtures) { + (self::$seedFixtures)(); + } + } + + protected function callApi(string $method, string $uri, array $options = []): ResponseInterface + { + return self::$client->request($method, sprintf('%s%s', self::REST_PATH_PREFX, $uri), $options); } - /** - * @throws \GuzzleHttp\Exception\GuzzleException - */ protected function callApiWithKey(string $method, string $uri, array $options = []): ResponseInterface { - $headers = $options['headers'] ?? []; + $headers = $options[RequestOptions::HEADERS] ?? []; $headers[ApiKeyHeaderPlugin::HEADER_NAME] = 'valid_api_key'; - $options['headers'] = $headers; + $options[RequestOptions::HEADERS] = $headers; return $this->callApi($method, $uri, $options); } @@ -48,4 +57,11 @@ abstract class ApiTestCase extends TestCase implements StatusCodeInterface, Requ { return json_decode((string) $resp->getBody()); } + + protected function callShortUrl(string $shortCode): ResponseInterface + { + return self::$client->request(self::METHOD_GET, sprintf('/%s', $shortCode), [ + RequestOptions::ALLOW_REDIRECTS => false, + ]); + } } diff --git a/module/Common/test-db/TestHelper.php b/module/Common/test-db/TestHelper.php index fc24657d..fba79a58 100644 --- a/module/Common/test-db/TestHelper.php +++ b/module/Common/test-db/TestHelper.php @@ -5,6 +5,7 @@ namespace ShlinkioTest\Shlink\Common; use Doctrine\Common\DataFixtures\Executor\ORMExecutor; use Doctrine\Common\DataFixtures\Loader; +use Doctrine\Common\DataFixtures\Purger\ORMPurger; use Doctrine\ORM\EntityManagerInterface; use Symfony\Component\Process\Process; use function file_exists; @@ -38,7 +39,7 @@ class TestHelper $loader->loadFromDirectory($path); } - $executor = new ORMExecutor($em); - $executor->execute($loader->getFixtures(), true); + $executor = new ORMExecutor($em, new ORMPurger()); + $executor->execute($loader->getFixtures()); } } diff --git a/module/Rest/test-api/Action/CreateShortUrlActionTest.php b/module/Rest/test-api/Action/CreateShortUrlActionTest.php new file mode 100644 index 00000000..2307d657 --- /dev/null +++ b/module/Rest/test-api/Action/CreateShortUrlActionTest.php @@ -0,0 +1,121 @@ +createShortUrl(); + + $this->assertEquals(self::STATUS_OK, $statusCode); + foreach ($expectedKeys as $key) { + $this->assertArrayHasKey($key, $payload); + } + } + + /** + * @test + */ + public function createsNewShortUrlWithCustomSlug() + { + [$statusCode, $payload] = $this->createShortUrl(['customSlug' => 'my cool slug']); + + $this->assertEquals(self::STATUS_OK, $statusCode); + $this->assertEquals('my-cool-slug', $payload['shortCode']); + } + + /** + * @test + */ + public function createsNewShortUrlWithTags() + { + [$statusCode, $payload] = $this->createShortUrl(['tags' => ['foo', 'bar', 'baz']]); + + $this->assertEquals(self::STATUS_OK, $statusCode); + $this->assertEquals(['foo', 'bar', 'baz'], $payload['tags']); + } + + /** + * @test + * @dataProvider provideMaxVisits + */ + public function createsNewShortUrlWithVisitsLimit(int $maxVisits) + { + [$statusCode, ['shortCode' => $shortCode]] = $this->createShortUrl(['maxVisits' => $maxVisits]); + + $this->assertEquals(self::STATUS_OK, $statusCode); + + // Last request to the short URL will return a 404, and the rest, a 302 + for ($i = 0; $i < $maxVisits; $i++) { + $this->assertEquals(self::STATUS_FOUND, $this->callShortUrl($shortCode)->getStatusCode()); + } + $lastResp = $this->callShortUrl($shortCode); + $this->assertEquals(self::STATUS_NOT_FOUND, $lastResp->getStatusCode()); + } + + public function provideMaxVisits(): array + { + return [ + [1], + [5], + [3], + ]; + } + + /** + * @test + */ + public function createsShortUrlWithValidSince() + { + [$statusCode, ['shortCode' => $shortCode]] = $this->createShortUrl([ + 'validSince' => Chronos::now()->addDay()->toAtomString(), + ]); + + $this->assertEquals(self::STATUS_OK, $statusCode); + + // Request to the short URL will return a 404 since ist' not valid yet + $lastResp = $this->callShortUrl($shortCode); + $this->assertEquals(self::STATUS_NOT_FOUND, $lastResp->getStatusCode()); + } + + /** + * @test + */ + public function createsShortUrlWithValidUntil() + { + [$statusCode, ['shortCode' => $shortCode]] = $this->createShortUrl([ + 'validUntil' => Chronos::now()->subDay()->toAtomString(), + ]); + + $this->assertEquals(self::STATUS_OK, $statusCode); + + // Request to the short URL will return a 404 since it's no longer valid + $lastResp = $this->callShortUrl($shortCode); + $this->assertEquals(self::STATUS_NOT_FOUND, $lastResp->getStatusCode()); + } + + /** + * @return array { + * @var int $statusCode + * @var array $payload + * } + */ + private function createShortUrl(array $body = []): array + { + $body['longUrl'] = 'https://app.shlink.io'; + $resp = $this->callApiWithKey(self::METHOD_POST, '/short-urls', [RequestOptions::JSON => $body]); + $payload = $this->getJsonResponsePayload($resp); + + return [$resp->getStatusCode(), $payload]; + } +} diff --git a/module/Rest/test-api/Middleware/AuthenticationTest.php b/module/Rest/test-api/Middleware/AuthenticationTest.php index c87b3c01..09ea3506 100644 --- a/module/Rest/test-api/Middleware/AuthenticationTest.php +++ b/module/Rest/test-api/Middleware/AuthenticationTest.php @@ -3,13 +3,11 @@ declare(strict_types=1); namespace ShlinkioApiTest\Shlink\Rest\Middleware; -use GuzzleHttp\Exception\ClientException; use Shlinkio\Shlink\Rest\Authentication\Plugin\ApiKeyHeaderPlugin; use Shlinkio\Shlink\Rest\Authentication\RequestToHttpAuthPlugin; use Shlinkio\Shlink\Rest\Util\RestUtils; use ShlinkioTest\Shlink\Common\ApiTest\ApiTestCase; use function implode; -use function Shlinkio\Shlink\Common\json_decode; use function sprintf; class AuthenticationTest extends ApiTestCase @@ -19,21 +17,18 @@ class AuthenticationTest extends ApiTestCase */ public function authorizationErrorIsReturnedIfNoApiKeyIsSent() { - try { - $this->callApi(self::METHOD_GET, '/short-codes'); - } catch (ClientException $e) { - ['error' => $error, 'message' => $message] = $this->getJsonResponsePayload($e->getResponse()); + $resp = $this->callApi(self::METHOD_GET, '/short-codes'); + ['error' => $error, 'message' => $message] = $this->getJsonResponsePayload($resp); - $this->assertEquals(self::STATUS_UNAUTHORIZED, $e->getCode()); - $this->assertEquals(RestUtils::INVALID_AUTHORIZATION_ERROR, $error); - $this->assertEquals( - sprintf( - 'Expected one of the following authentication headers, but none were provided, ["%s"]', - implode('", "', RequestToHttpAuthPlugin::SUPPORTED_AUTH_HEADERS) - ), - $message - ); - } + $this->assertEquals(self::STATUS_UNAUTHORIZED, $resp->getStatusCode()); + $this->assertEquals(RestUtils::INVALID_AUTHORIZATION_ERROR, $error); + $this->assertEquals( + sprintf( + 'Expected one of the following authentication headers, but none were provided, ["%s"]', + implode('", "', RequestToHttpAuthPlugin::SUPPORTED_AUTH_HEADERS) + ), + $message + ); } /** @@ -42,19 +37,16 @@ class AuthenticationTest extends ApiTestCase */ public function apiKeyErrorIsReturnedWhenProvidedApiKeyIsInvalid(string $apiKey) { - try { - $this->callApi(self::METHOD_GET, '/short-codes', [ - 'headers' => [ - ApiKeyHeaderPlugin::HEADER_NAME => $apiKey, - ], - ]); - } catch (ClientException $e) { - ['error' => $error, 'message' => $message] = json_decode((string) $e->getResponse()->getBody()); + $resp = $this->callApi(self::METHOD_GET, '/short-codes', [ + 'headers' => [ + ApiKeyHeaderPlugin::HEADER_NAME => $apiKey, + ], + ]); + ['error' => $error, 'message' => $message] = $this->getJsonResponsePayload($resp); - $this->assertEquals(self::STATUS_UNAUTHORIZED, $e->getCode()); - $this->assertEquals(RestUtils::INVALID_API_KEY_ERROR, $error); - $this->assertEquals('Provided API key does not exist or is invalid.', $message); - } + $this->assertEquals(self::STATUS_UNAUTHORIZED, $resp->getStatusCode()); + $this->assertEquals(RestUtils::INVALID_API_KEY_ERROR, $error); + $this->assertEquals('Provided API key does not exist or is invalid.', $message); } public function provideInvalidApiKeys(): array From 49668547d72afa91344a54e4278c0d6d35204099 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 30 Jan 2019 18:29:27 +0100 Subject: [PATCH 05/11] Fixed typo --- module/Common/test-db/ApiTest/ApiTestCase.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/module/Common/test-db/ApiTest/ApiTestCase.php b/module/Common/test-db/ApiTest/ApiTestCase.php index 65006bb0..4dedab6b 100644 --- a/module/Common/test-db/ApiTest/ApiTestCase.php +++ b/module/Common/test-db/ApiTest/ApiTestCase.php @@ -15,7 +15,7 @@ use function sprintf; abstract class ApiTestCase extends TestCase implements StatusCodeInterface, RequestMethodInterface { - private const REST_PATH_PREFX = '/rest/v1'; + private const REST_PATH_PREFIX = '/rest/v1'; /** @var ClientInterface */ private static $client; @@ -41,7 +41,7 @@ abstract class ApiTestCase extends TestCase implements StatusCodeInterface, Requ protected function callApi(string $method, string $uri, array $options = []): ResponseInterface { - return self::$client->request($method, sprintf('%s%s', self::REST_PATH_PREFX, $uri), $options); + return self::$client->request($method, sprintf('%s%s', self::REST_PATH_PREFIX, $uri), $options); } protected function callApiWithKey(string $method, string $uri, array $options = []): ResponseInterface From 594e7da256640c6b1321a10f18f0b95962a6d0b6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 2 Feb 2019 09:54:41 +0100 Subject: [PATCH 06/11] Created new findIfExists meta param --- .../src/Validation/InputFactoryTrait.php | 35 +++++++++++++++++++ module/Core/src/Model/ShortUrlMeta.php | 16 +++++++-- .../Core/src/Validation/InputFactoryTrait.php | 20 ----------- .../Validation/ShortUrlMetaInputFilter.php | 16 +++++---- 4 files changed, 58 insertions(+), 29 deletions(-) create mode 100644 module/Common/src/Validation/InputFactoryTrait.php delete mode 100644 module/Core/src/Validation/InputFactoryTrait.php diff --git a/module/Common/src/Validation/InputFactoryTrait.php b/module/Common/src/Validation/InputFactoryTrait.php new file mode 100644 index 00000000..71d92818 --- /dev/null +++ b/module/Common/src/Validation/InputFactoryTrait.php @@ -0,0 +1,35 @@ +setRequired($required) + ->getFilterChain()->attach(new Filter\StripTags()) + ->attach(new Filter\StringTrim()); + return $input; + } + + private function createBooleanInput(string $name, bool $required = true): Input + { + $input = $this->createInput($name, $required); + $input->getFilterChain()->attach(new Filter\Boolean()); + $input->getValidatorChain()->attach(new Validator\NotEmpty(['type' => [ + Validator\NotEmpty::OBJECT, + Validator\NotEmpty::SPACE, + Validator\NotEmpty::NULL, + Validator\NotEmpty::EMPTY_ARRAY, + Validator\NotEmpty::STRING, + ]])); + + return $input; + } +} diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index 35e450a0..2e7c0208 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -18,6 +18,8 @@ final class ShortUrlMeta private $customSlug; /** @var int|null */ private $maxVisits; + /** @var bool */ + private $findIfExists; // Force named constructors private function __construct() @@ -45,21 +47,25 @@ final class ShortUrlMeta * @param string|Chronos|null $validUntil * @param string|null $customSlug * @param int|null $maxVisits + * @param bool|null $findIfExists * @throws ValidationException */ public static function createFromParams( $validSince = null, $validUntil = null, $customSlug = null, - $maxVisits = null + $maxVisits = null, + $findIfExists = null ): self { - // We do not type hint the arguments because that will be done by the validation process + // We do not type hint the arguments because that will be done by the validation process and we would get a + // type error if any of them do not match $instance = new self(); $instance->validate([ ShortUrlMetaInputFilter::VALID_SINCE => $validSince, ShortUrlMetaInputFilter::VALID_UNTIL => $validUntil, ShortUrlMetaInputFilter::CUSTOM_SLUG => $customSlug, ShortUrlMetaInputFilter::MAX_VISITS => $maxVisits, + ShortUrlMetaInputFilter::FIND_IF_EXISTS => $findIfExists, ]); return $instance; } @@ -80,6 +86,7 @@ final class ShortUrlMeta $this->customSlug = $inputFilter->getValue(ShortUrlMetaInputFilter::CUSTOM_SLUG); $this->maxVisits = $inputFilter->getValue(ShortUrlMetaInputFilter::MAX_VISITS); $this->maxVisits = $this->maxVisits !== null ? (int) $this->maxVisits : null; + $this->findIfExists = (bool) $inputFilter->getValue(ShortUrlMetaInputFilter::FIND_IF_EXISTS); } /** @@ -139,6 +146,11 @@ final class ShortUrlMeta return $this->maxVisits !== null; } + public function findIfExists(): bool + { + return $this->findIfExists; + } + public function withCustomSlug(string $customSlug): self { $clone = clone $this; diff --git a/module/Core/src/Validation/InputFactoryTrait.php b/module/Core/src/Validation/InputFactoryTrait.php deleted file mode 100644 index 4e514694..00000000 --- a/module/Core/src/Validation/InputFactoryTrait.php +++ /dev/null @@ -1,20 +0,0 @@ -setRequired($required) - ->getFilterChain()->attach(new StripTags()) - ->attach(new StringTrim()); - return $input; - } -} diff --git a/module/Core/src/Validation/ShortUrlMetaInputFilter.php b/module/Core/src/Validation/ShortUrlMetaInputFilter.php index 80e14953..34632d49 100644 --- a/module/Core/src/Validation/ShortUrlMetaInputFilter.php +++ b/module/Core/src/Validation/ShortUrlMetaInputFilter.php @@ -4,10 +4,9 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Validation; use DateTime; -use Zend\I18n\Validator\IsInt; +use Shlinkio\Shlink\Common\Validation\InputFactoryTrait; use Zend\InputFilter\InputFilter; -use Zend\Validator\Date; -use Zend\Validator\GreaterThan; +use Zend\Validator; class ShortUrlMetaInputFilter extends InputFilter { @@ -17,6 +16,7 @@ class ShortUrlMetaInputFilter extends InputFilter public const VALID_UNTIL = 'validUntil'; public const CUSTOM_SLUG = 'customSlug'; public const MAX_VISITS = 'maxVisits'; + public const FIND_IF_EXISTS = 'findIfExists'; public function __construct(?array $data = null) { @@ -29,18 +29,20 @@ class ShortUrlMetaInputFilter extends InputFilter private function initialize(): void { $validSince = $this->createInput(self::VALID_SINCE, false); - $validSince->getValidatorChain()->attach(new Date(['format' => DateTime::ATOM])); + $validSince->getValidatorChain()->attach(new Validator\Date(['format' => DateTime::ATOM])); $this->add($validSince); $validUntil = $this->createInput(self::VALID_UNTIL, false); - $validUntil->getValidatorChain()->attach(new Date(['format' => DateTime::ATOM])); + $validUntil->getValidatorChain()->attach(new Validator\Date(['format' => DateTime::ATOM])); $this->add($validUntil); $this->add($this->createInput(self::CUSTOM_SLUG, false)); $maxVisits = $this->createInput(self::MAX_VISITS, false); - $maxVisits->getValidatorChain()->attach(new IsInt()) - ->attach(new GreaterThan(['min' => 1, 'inclusive' => true])); + $maxVisits->getValidatorChain()->attach(new Validator\Digits()) + ->attach(new Validator\GreaterThan(['min' => 1, 'inclusive' => true])); $this->add($maxVisits); + + $this->add($this->createBooleanInput(self::FIND_IF_EXISTS, false)); } } From 772494f46f4718a804e721ca8a7b96cef1ffc597 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Feb 2019 08:17:22 +0100 Subject: [PATCH 07/11] Moved process of sluggifying custom slug to a filter --- config/autoload/slugify.global.php | 23 -------------- .../Common/src/Validation/SluggerFilter.php | 31 +++++++++++++++++++ module/Core/config/dependencies.config.php | 2 +- module/Core/src/Model/ShortUrlMeta.php | 8 ----- module/Core/src/Service/UrlShortener.php | 24 ++++---------- .../Validation/ShortUrlMetaInputFilter.php | 8 +++-- module/Core/test/Service/UrlShortenerTest.php | 29 +---------------- 7 files changed, 44 insertions(+), 81 deletions(-) delete mode 100644 config/autoload/slugify.global.php create mode 100644 module/Common/src/Validation/SluggerFilter.php diff --git a/config/autoload/slugify.global.php b/config/autoload/slugify.global.php deleted file mode 100644 index 1c3a3f96..00000000 --- a/config/autoload/slugify.global.php +++ /dev/null @@ -1,23 +0,0 @@ - [ - 'lowercase' => false, - ], - - 'dependencies' => [ - 'factories' => [ - Slugify::class => ConfigAbstractFactory::class, - ], - ], - - ConfigAbstractFactory::class => [ - Slugify::class => ['config.slugify_options'], - ], - -]; diff --git a/module/Common/src/Validation/SluggerFilter.php b/module/Common/src/Validation/SluggerFilter.php new file mode 100644 index 00000000..587695f9 --- /dev/null +++ b/module/Common/src/Validation/SluggerFilter.php @@ -0,0 +1,31 @@ +slugger = $slugger ?: new Slugify\Slugify(['lowercase' => false]); + } + + /** + * Returns the result of filtering $value + * + * @param mixed $value + * @throws Exception\RuntimeException If filtering $value is impossible + * @return mixed + */ + public function filter($value) + { + return $value ? $this->slugger->slugify($value) : $value; + } +} diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index bb068e65..a7a9d26b 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -46,7 +46,7 @@ return [ Options\NotFoundShortUrlOptions::class => ['config.url_shortener.not_found_short_url'], Options\UrlShortenerOptions::class => ['config.url_shortener'], - Service\UrlShortener::class => ['httpClient', 'em', Options\UrlShortenerOptions::class, Slugify::class], + Service\UrlShortener::class => ['httpClient', 'em', Options\UrlShortenerOptions::class], Service\VisitsTracker::class => ['em'], Service\ShortUrlService::class => ['em'], Service\VisitService::class => ['em'], diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index 2e7c0208..84e7a1fa 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -150,12 +150,4 @@ final class ShortUrlMeta { return $this->findIfExists; } - - 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 96355c58..e6e63b50 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 Cocur\Slugify\SlugifyInterface; use Doctrine\ORM\EntityManagerInterface; use GuzzleHttp\ClientInterface; use GuzzleHttp\Exception\GuzzleException; @@ -34,21 +33,14 @@ class UrlShortener implements UrlShortenerInterface private $httpClient; /** @var EntityManagerInterface */ private $em; - /** @var SlugifyInterface */ - private $slugger; /** @var UrlShortenerOptions */ private $options; - public function __construct( - ClientInterface $httpClient, - EntityManagerInterface $em, - UrlShortenerOptions $options, - SlugifyInterface $slugger - ) { + public function __construct(ClientInterface $httpClient, EntityManagerInterface $em, UrlShortenerOptions $options) + { $this->httpClient = $httpClient; $this->em = $em; $this->options = $options; - $this->slugger = $slugger; } /** @@ -63,7 +55,7 @@ class UrlShortener implements UrlShortenerInterface if ($this->options->isUrlValidationEnabled()) { $this->checkUrlExists($url); } - $meta = $this->processCustomSlug($meta); + $this->verifyCustomSlug($meta); // Transactionally insert the short url, then generate the short code and finally update the short code try { @@ -123,15 +115,13 @@ class UrlShortener implements UrlShortenerInterface return $chars[(int) $id] . $code; } - private function processCustomSlug(ShortUrlMeta $meta): ?ShortUrlMeta + private function verifyCustomSlug(ShortUrlMeta $meta): void { if (! $meta->hasCustomSlug()) { - return $meta; + return; } - // 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()); + $customSlug = $meta->getCustomSlug(); /** @var ShortUrlRepository $repo */ $repo = $this->em->getRepository(ShortUrl::class); @@ -139,8 +129,6 @@ class UrlShortener implements UrlShortenerInterface if ($shortUrlsCount > 0) { throw NonUniqueSlugException::fromSlug($customSlug); } - - return $meta->withCustomSlug($customSlug); } /** diff --git a/module/Core/src/Validation/ShortUrlMetaInputFilter.php b/module/Core/src/Validation/ShortUrlMetaInputFilter.php index 34632d49..fad63f6c 100644 --- a/module/Core/src/Validation/ShortUrlMetaInputFilter.php +++ b/module/Core/src/Validation/ShortUrlMetaInputFilter.php @@ -4,13 +4,13 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Validation; use DateTime; -use Shlinkio\Shlink\Common\Validation\InputFactoryTrait; +use Shlinkio\Shlink\Common\Validation; use Zend\InputFilter\InputFilter; use Zend\Validator; class ShortUrlMetaInputFilter extends InputFilter { - use InputFactoryTrait; + use Validation\InputFactoryTrait; public const VALID_SINCE = 'validSince'; public const VALID_UNTIL = 'validUntil'; @@ -36,7 +36,9 @@ class ShortUrlMetaInputFilter extends InputFilter $validUntil->getValidatorChain()->attach(new Validator\Date(['format' => DateTime::ATOM])); $this->add($validUntil); - $this->add($this->createInput(self::CUSTOM_SLUG, false)); + $customSlug = $this->createInput(self::CUSTOM_SLUG, false); + $customSlug->getFilterChain()->attach(new Validation\SluggerFilter()); + $this->add($customSlug); $maxVisits = $this->createInput(self::MAX_VISITS, false); $maxVisits->getValidatorChain()->attach(new Validator\Digits()) diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index fb625202..350775db 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -3,7 +3,6 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Service; -use Cocur\Slugify\SlugifyInterface; use Doctrine\DBAL\Connection; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\ORMException; @@ -31,8 +30,6 @@ class UrlShortenerTest extends TestCase private $em; /** @var ObjectProphecy */ private $httpClient; - /** @var ObjectProphecy */ - private $slugger; public function setUp() { @@ -54,8 +51,6 @@ class UrlShortenerTest extends TestCase $repo->count(Argument::any())->willReturn(0); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - $this->slugger = $this->prophesize(SlugifyInterface::class); - $this->setUrlShortener(false); } @@ -64,8 +59,7 @@ class UrlShortenerTest extends TestCase $this->urlShortener = new UrlShortener( $this->httpClient->reveal(), $this->em->reveal(), - new UrlShortenerOptions(['validate_url' => $urlValidationEnabled]), - $this->slugger->reveal() + new UrlShortenerOptions(['validate_url' => $urlValidationEnabled]) ); } @@ -121,38 +115,17 @@ class UrlShortenerTest extends TestCase ); } - /** - * @test - */ - public function whenCustomSlugIsProvidedItIsUsed() - { - /** @var MethodProphecy $slugify */ - $slugify = $this->slugger->slugify('custom-slug')->willReturnArgument(); - - $this->urlShortener->urlToShortCode( - new Uri('http://foobar.com/12345/hello?foo=bar'), - [], - ShortUrlMeta::createFromRawData(['customSlug' => 'custom-slug']) - ); - - $slugify->shouldHaveBeenCalledOnce(); - } - /** * @test */ public function exceptionIsThrownWhenNonUniqueSlugIsProvided() { - /** @var MethodProphecy $slugify */ - $slugify = $this->slugger->slugify('custom-slug')->willReturnArgument(); - $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(); $countBySlug->shouldBeCalledOnce(); $getRepo->shouldBeCalled(); $this->expectException(NonUniqueSlugException::class); From c4fd8d5120e0d2601f23dda9f1954a3355c423c4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Feb 2019 09:40:32 +0100 Subject: [PATCH 08/11] Implemented feature to optionally return an existing short url when all provided params match an existing one --- .../Common/src/Validation/SluggerFilter.php | 2 +- module/Core/config/dependencies.config.php | 1 - .../src/Exception/InvalidUrlException.php | 2 +- module/Core/src/Model/ShortUrlMeta.php | 14 +-- module/Core/src/Service/UrlShortener.php | 94 ++++++++++++++----- module/Core/test/Service/UrlShortenerTest.php | 89 ++++++++++++++++-- 6 files changed, 160 insertions(+), 42 deletions(-) diff --git a/module/Common/src/Validation/SluggerFilter.php b/module/Common/src/Validation/SluggerFilter.php index 587695f9..9387e85a 100644 --- a/module/Common/src/Validation/SluggerFilter.php +++ b/module/Common/src/Validation/SluggerFilter.php @@ -26,6 +26,6 @@ class SluggerFilter implements FilterInterface */ public function filter($value) { - return $value ? $this->slugger->slugify($value) : $value; + return ! empty($value) ? $this->slugger->slugify($value) : null; } } diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index a7a9d26b..960b74ac 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -3,7 +3,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core; -use Cocur\Slugify\Slugify; use Doctrine\Common\Cache\Cache; use Shlinkio\Shlink\Common\Service\PreviewGenerator; use Shlinkio\Shlink\Core\Response\NotFoundHandler; diff --git a/module/Core/src/Exception/InvalidUrlException.php b/module/Core/src/Exception/InvalidUrlException.php index 6a94b548..4291f0ec 100644 --- a/module/Core/src/Exception/InvalidUrlException.php +++ b/module/Core/src/Exception/InvalidUrlException.php @@ -8,7 +8,7 @@ use function sprintf; class InvalidUrlException extends RuntimeException { - public static function fromUrl($url, Throwable $previous = null) + public static function fromUrl(string $url, Throwable $previous = null) { $code = $previous !== null ? $previous->getCode() : -1; return new static(sprintf('Provided URL "%s" is not an existing and valid URL', $url), $code, $previous); diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index 84e7a1fa..2eb6ebca 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -6,7 +6,6 @@ namespace Shlinkio\Shlink\Core\Model; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; -use function is_string; final class ShortUrlMeta { @@ -18,7 +17,7 @@ final class ShortUrlMeta private $customSlug; /** @var int|null */ private $maxVisits; - /** @var bool */ + /** @var bool|null */ private $findIfExists; // Force named constructors @@ -86,12 +85,11 @@ final class ShortUrlMeta $this->customSlug = $inputFilter->getValue(ShortUrlMetaInputFilter::CUSTOM_SLUG); $this->maxVisits = $inputFilter->getValue(ShortUrlMetaInputFilter::MAX_VISITS); $this->maxVisits = $this->maxVisits !== null ? (int) $this->maxVisits : null; - $this->findIfExists = (bool) $inputFilter->getValue(ShortUrlMetaInputFilter::FIND_IF_EXISTS); + $this->findIfExists = $inputFilter->getValue(ShortUrlMetaInputFilter::FIND_IF_EXISTS); } /** * @param string|Chronos|null $date - * @return Chronos|null */ private function parseDateField($date): ?Chronos { @@ -99,11 +97,7 @@ final class ShortUrlMeta return $date; } - if (is_string($date)) { - return Chronos::parse($date); - } - - return null; + return Chronos::parse($date); } public function getValidSince(): ?Chronos @@ -148,6 +142,6 @@ final class ShortUrlMeta public function findIfExists(): bool { - return $this->findIfExists; + return (bool) $this->findIfExists; } } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index e6e63b50..f224ab51 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -4,8 +4,10 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; use Doctrine\ORM\EntityManagerInterface; +use Fig\Http\Message\RequestMethodInterface; use GuzzleHttp\ClientInterface; use GuzzleHttp\Exception\GuzzleException; +use GuzzleHttp\RequestOptions; use Psr\Http\Message\UriInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; @@ -18,8 +20,12 @@ use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Throwable; +use function array_reduce; +use function count; use function floor; use function fmod; +use function Functional\contains; +use function Functional\invoke; use function preg_match; use function strlen; @@ -51,6 +57,14 @@ class UrlShortener implements UrlShortenerInterface */ public function urlToShortCode(UriInterface $url, array $tags, ShortUrlMeta $meta): ShortUrl { + $url = (string) $url; + + // First, check if a short URL exists for all provided params + $existingShortUrl = $this->findExistingShortUrlIfExists($url, $tags, $meta); + if ($existingShortUrl !== null) { + return $existingShortUrl; + } + // If the URL validation is enabled, check that the URL actually exists if ($this->options->isUrlValidationEnabled()) { $this->checkUrlExists($url); @@ -62,7 +76,7 @@ class UrlShortener implements UrlShortenerInterface $this->em->beginTransaction(); // First, create the short URL with an empty short code - $shortUrl = new ShortUrl((string) $url, $meta); + $shortUrl = new ShortUrl($url, $meta); $this->em->persist($shortUrl); $this->em->flush(); @@ -87,17 +101,71 @@ class UrlShortener implements UrlShortenerInterface } } - private function checkUrlExists(UriInterface $url): void + private function findExistingShortUrlIfExists(string $url, array $tags, ShortUrlMeta $meta): ?ShortUrl + { + if (! $meta->findIfExists()) { + return null; + } + + $criteria = ['longUrl' => $url]; + if ($meta->hasCustomSlug()) { + $criteria['shortCode'] = $meta->getCustomSlug(); + } + /** @var ShortUrl|null $shortUrl */ + $shortUrl = $this->em->getRepository(ShortUrl::class)->findOneBy($criteria); + if ($shortUrl === null) { + return null; + } + + if ($meta->hasMaxVisits() && $meta->getMaxVisits() !== $shortUrl->getMaxVisits()) { + return null; + } + if ($meta->hasValidSince() && ! $meta->getValidSince()->eq($shortUrl->getValidSince())) { + return null; + } + if ($meta->hasValidUntil() && ! $meta->getValidUntil()->eq($shortUrl->getValidUntil())) { + return null; + } + + $shortUrlTags = invoke($shortUrl->getTags(), '__toString'); + $hasAllTags = count($shortUrlTags) === count($tags) && array_reduce( + $tags, + function (bool $hasAllTags, string $tag) use ($shortUrlTags) { + return $hasAllTags && contains($shortUrlTags, $tag); + }, + true + ); + + return $hasAllTags ? $shortUrl : null; + } + + private function checkUrlExists(string $url): void { try { - $this->httpClient->request('GET', $url, ['allow_redirects' => [ - 'max' => 15, - ]]); + $this->httpClient->request(RequestMethodInterface::METHOD_GET, $url, [ + RequestOptions::ALLOW_REDIRECTS => ['max' => 15], + ]); } catch (GuzzleException $e) { throw InvalidUrlException::fromUrl($url, $e); } } + private function verifyCustomSlug(ShortUrlMeta $meta): void + { + if (! $meta->hasCustomSlug()) { + return; + } + + $customSlug = $meta->getCustomSlug(); + + /** @var ShortUrlRepository $repo */ + $repo = $this->em->getRepository(ShortUrl::class); + $shortUrlsCount = $repo->count(['shortCode' => $customSlug]); + if ($shortUrlsCount > 0) { + throw NonUniqueSlugException::fromSlug($customSlug); + } + } + private function convertAutoincrementIdToShortCode(float $id): string { $id += self::ID_INCREMENT; // Increment the Id so that the generated shortcode is not too short @@ -115,22 +183,6 @@ class UrlShortener implements UrlShortenerInterface return $chars[(int) $id] . $code; } - private function verifyCustomSlug(ShortUrlMeta $meta): void - { - if (! $meta->hasCustomSlug()) { - return; - } - - $customSlug = $meta->getCustomSlug(); - - /** @var ShortUrlRepository $repo */ - $repo = $this->em->getRepository(ShortUrl::class); - $shortUrlsCount = $repo->count(['shortCode' => $customSlug]); - if ($shortUrlsCount > 0) { - throw NonUniqueSlugException::fromSlug($customSlug); - } - } - /** * @throws InvalidShortCodeException * @throws EntityDoesNotExistException diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 350775db..48dae28e 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -3,6 +3,8 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Service; +use Cake\Chronos\Chronos; +use Doctrine\Common\Collections\ArrayCollection; use Doctrine\DBAL\Connection; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\ORMException; @@ -14,6 +16,7 @@ use Prophecy\Argument; use Prophecy\Prophecy\MethodProphecy; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; @@ -31,7 +34,7 @@ class UrlShortenerTest extends TestCase /** @var ObjectProphecy */ private $httpClient; - public function setUp() + public function setUp(): void { $this->httpClient = $this->prophesize(ClientInterface::class); @@ -54,7 +57,7 @@ class UrlShortenerTest extends TestCase $this->setUrlShortener(false); } - public function setUrlShortener(bool $urlValidationEnabled) + public function setUrlShortener(bool $urlValidationEnabled): void { $this->urlShortener = new UrlShortener( $this->httpClient->reveal(), @@ -66,7 +69,7 @@ class UrlShortenerTest extends TestCase /** * @test */ - public function urlIsProperlyShortened() + public function urlIsProperlyShortened(): void { // 10 -> 12C1c $shortUrl = $this->urlShortener->urlToShortCode( @@ -81,7 +84,7 @@ class UrlShortenerTest extends TestCase * @test * @expectedException \Shlinkio\Shlink\Core\Exception\RuntimeException */ - public function exceptionIsThrownWhenOrmThrowsException() + public function exceptionIsThrownWhenOrmThrowsException(): void { $conn = $this->prophesize(Connection::class); $conn->isTransactionActive()->willReturn(true); @@ -101,7 +104,7 @@ class UrlShortenerTest extends TestCase * @test * @expectedException \Shlinkio\Shlink\Core\Exception\InvalidUrlException */ - public function exceptionIsThrownWhenUrlDoesNotExist() + public function exceptionIsThrownWhenUrlDoesNotExist(): void { $this->setUrlShortener(true); @@ -118,7 +121,7 @@ class UrlShortenerTest extends TestCase /** * @test */ - public function exceptionIsThrownWhenNonUniqueSlugIsProvided() + public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void { $repo = $this->prophesize(ShortUrlRepository::class); $countBySlug = $repo->count(['shortCode' => 'custom-slug'])->willReturn(1); @@ -139,8 +142,78 @@ class UrlShortenerTest extends TestCase /** * @test + * @dataProvider provideExsitingShortUrls */ - public function shortCodeIsProperlyParsed() + public function existingShortUrlIsReturnedWhenRequested( + string $url, + array $tags, + ShortUrlMeta $meta, + ?ShortUrl $expected + ): void { + $repo = $this->prophesize(ShortUrlRepository::class); + $findExisting = $repo->findOneBy(Argument::any())->willReturn($expected); + $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + + $result = $this->urlShortener->urlToShortCode(new Uri($url), $tags, $meta); + + $this->assertSame($expected, $result); + $findExisting->shouldHaveBeenCalledOnce(); + $getRepo->shouldHaveBeenCalledOnce(); + } + + public function provideExsitingShortUrls(): array + { + $url = 'http://foo.com'; + + return [ + [$url, [], ShortUrlMeta::createFromRawData(['findIfExists' => true]), new ShortUrl($url)], + [$url, [], ShortUrlMeta::createFromRawData( + ['findIfExists' => true, 'customSlug' => 'foo'] + ), new ShortUrl($url)], + [ + $url, + ['foo', 'bar'], + ShortUrlMeta::createFromRawData(['findIfExists' => true]), + (new ShortUrl($url))->setTags(new ArrayCollection([new Tag('bar'), new Tag('foo')])), + ], + [ + $url, + [], + ShortUrlMeta::createFromRawData(['findIfExists' => true, 'maxVisits' => 3]), + new ShortUrl($url, ShortUrlMeta::createFromRawData(['maxVisits' => 3])), + ], + [ + $url, + [], + ShortUrlMeta::createFromRawData(['findIfExists' => true, 'validSince' => Chronos::parse('2017-01-01')]), + new ShortUrl($url, ShortUrlMeta::createFromRawData(['validSince' => Chronos::parse('2017-01-01')])), + ], + [ + $url, + [], + ShortUrlMeta::createFromRawData(['findIfExists' => true, 'validUntil' => Chronos::parse('2017-01-01')]), + new ShortUrl($url, ShortUrlMeta::createFromRawData(['validUntil' => Chronos::parse('2017-01-01')])), + ], + [ + $url, + ['baz', 'foo', 'bar'], + ShortUrlMeta::createFromRawData([ + 'findIfExists' => true, + 'validUntil' => Chronos::parse('2017-01-01'), + 'maxVisits' => 4, + ]), + (new ShortUrl($url, ShortUrlMeta::createFromRawData([ + 'validUntil' => Chronos::parse('2017-01-01'), + 'maxVisits' => 4, + ])))->setTags(new ArrayCollection([new Tag('foo'), new Tag('bar'), new Tag('baz')])), + ], + ]; + } + + /** + * @test + */ + public function shortCodeIsProperlyParsed(): void { // 12C1c -> 10 $shortCode = '12C1c'; @@ -159,7 +232,7 @@ class UrlShortenerTest extends TestCase * @test * @expectedException \Shlinkio\Shlink\Core\Exception\InvalidShortCodeException */ - public function invalidCharSetThrowsException() + public function invalidCharSetThrowsException(): void { $this->urlShortener->shortCodeToUrl('&/('); } From 810b25ff143b739b66ec9177a9983487693fa338 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Feb 2019 11:01:38 +0100 Subject: [PATCH 09/11] Added API tests covering creating short URLs with new findIfExists param --- .gitignore | 1 + config/autoload/dependencies.local.php.dist | 11 +++ config/test/bootstrap_api_tests.php | 2 +- config/test/bootstrap_db_tests.php | 3 +- config/test/test_config.global.php | 4 +- docs/swagger/paths/v1_short-urls.json | 4 + module/Common/test-db/TestHelper.php | 5 +- .../Action/ShortUrl/CreateShortUrlAction.php | 3 +- .../Action/CreateShortUrlActionTest.php | 86 +++++++++++++++++-- 9 files changed, 103 insertions(+), 16 deletions(-) diff --git a/.gitignore b/.gitignore index ce581c01..9c2b2223 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ composer.phar vendor/ .env data/database.sqlite +data/shlink-tests.db data/GeoLite2-City.mmdb docs/swagger-ui* docker-compose.override.yml diff --git a/config/autoload/dependencies.local.php.dist b/config/autoload/dependencies.local.php.dist index 5ce9874b..e7fb274f 100644 --- a/config/autoload/dependencies.local.php.dist +++ b/config/autoload/dependencies.local.php.dist @@ -1,12 +1,23 @@ [ 'lazy_services' => [ 'write_proxy_files' => false, ], + + 'initializers' => [ + function (ContainerInterface $container, $instance) { + if ($instance instanceof Log\LoggerAwareInterface) { + $instance->setLogger($container->get(Log\LoggerInterface::class)); + } + }, + ], ], ]; diff --git a/config/test/bootstrap_api_tests.php b/config/test/bootstrap_api_tests.php index a016f93f..41227dba 100644 --- a/config/test/bootstrap_api_tests.php +++ b/config/test/bootstrap_api_tests.php @@ -19,7 +19,7 @@ $testHelper = $container->get(TestHelper::class); $config = $container->get('config'); $em = $container->get(EntityManager::class); -$testHelper->createTestDb(); +$testHelper->createTestDb($config['entity_manager']['connection']['path']); ApiTest\ApiTestCase::setApiClient($container->get('shlink_test_api_client')); ApiTest\ApiTestCase::setSeedFixturesCallback(function () use ($testHelper, $em, $config) { $testHelper->seedFixtures($em, $config['data_fixtures'] ?? []); diff --git a/config/test/bootstrap_db_tests.php b/config/test/bootstrap_db_tests.php index 58bc2174..ee44849f 100644 --- a/config/test/bootstrap_db_tests.php +++ b/config/test/bootstrap_db_tests.php @@ -14,6 +14,7 @@ if (! file_exists('.env')) { /** @var ContainerInterface $container */ $container = require __DIR__ . '/../container.php'; +$config = $container->get('config'); -$container->get(TestHelper::class)->createTestDb(); +$container->get(TestHelper::class)->createTestDb($config['entity_manager']['connection']['path']); DbTest\DatabaseTestCase::setEntityManager($container->get('em')); diff --git a/config/test/test_config.global.php b/config/test/test_config.global.php index dd794fe4..a0f7bcb8 100644 --- a/config/test/test_config.global.php +++ b/config/test/test_config.global.php @@ -6,7 +6,6 @@ namespace ShlinkioTest\Shlink; use GuzzleHttp\Client; use Zend\ConfigAggregator\ConfigAggregator; use Zend\ServiceManager\Factory\InvokableFactory; -use function realpath; use function sprintf; use function sys_get_temp_dir; @@ -51,7 +50,8 @@ return [ 'entity_manager' => [ 'connection' => [ 'driver' => 'pdo_sqlite', - 'path' => realpath(sys_get_temp_dir()) . '/shlink-tests.db', + 'path' => sys_get_temp_dir() . '/shlink-tests.db', +// 'path' => __DIR__ . '/../../data/shlink-tests.db', ], ], diff --git a/docs/swagger/paths/v1_short-urls.json b/docs/swagger/paths/v1_short-urls.json index b58cd7f1..1bb59a9b 100644 --- a/docs/swagger/paths/v1_short-urls.json +++ b/docs/swagger/paths/v1_short-urls.json @@ -197,6 +197,10 @@ "maxVisits": { "description": "The maximum number of allowed visits for this short code", "type": "number" + }, + "findIfExists": { + "description": "Will force existing matching URL to be returned if found, instead of creating a new one", + "type": "boolean" } } } diff --git a/module/Common/test-db/TestHelper.php b/module/Common/test-db/TestHelper.php index fba79a58..89c8adca 100644 --- a/module/Common/test-db/TestHelper.php +++ b/module/Common/test-db/TestHelper.php @@ -9,15 +9,12 @@ use Doctrine\Common\DataFixtures\Purger\ORMPurger; use Doctrine\ORM\EntityManagerInterface; use Symfony\Component\Process\Process; use function file_exists; -use function realpath; -use function sys_get_temp_dir; use function unlink; class TestHelper { - public function createTestDb(): void + public function createTestDb(string $shlinkDbPath): void { - $shlinkDbPath = realpath(sys_get_temp_dir()) . '/shlink-tests.db'; if (file_exists($shlinkDbPath)) { unlink($shlinkDbPath); } diff --git a/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php index b49fa522..ff79818b 100644 --- a/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php @@ -35,7 +35,8 @@ class CreateShortUrlAction extends AbstractCreateShortUrlAction $this->getOptionalDate($postData, 'validSince'), $this->getOptionalDate($postData, 'validUntil'), $postData['customSlug'] ?? null, - isset($postData['maxVisits']) ? (int) $postData['maxVisits'] : null + $postData['maxVisits'] ?? null, + $postData['findIfExists'] ?? null ) ); } diff --git a/module/Rest/test-api/Action/CreateShortUrlActionTest.php b/module/Rest/test-api/Action/CreateShortUrlActionTest.php index 2307d657..aefb8d6f 100644 --- a/module/Rest/test-api/Action/CreateShortUrlActionTest.php +++ b/module/Rest/test-api/Action/CreateShortUrlActionTest.php @@ -12,7 +12,7 @@ class CreateShortUrlActionTest extends ApiTestCase /** * @test */ - public function createsNewShortUrlWhenOnlyLongUrlIsProvided() + public function createsNewShortUrlWhenOnlyLongUrlIsProvided(): void { $expectedKeys = ['shortCode', 'shortUrl', 'longUrl', 'dateCreated', 'visitsCount', 'tags']; [$statusCode, $payload] = $this->createShortUrl(); @@ -26,7 +26,7 @@ class CreateShortUrlActionTest extends ApiTestCase /** * @test */ - public function createsNewShortUrlWithCustomSlug() + public function createsNewShortUrlWithCustomSlug(): void { [$statusCode, $payload] = $this->createShortUrl(['customSlug' => 'my cool slug']); @@ -37,7 +37,7 @@ class CreateShortUrlActionTest extends ApiTestCase /** * @test */ - public function createsNewShortUrlWithTags() + public function createsNewShortUrlWithTags(): void { [$statusCode, $payload] = $this->createShortUrl(['tags' => ['foo', 'bar', 'baz']]); @@ -49,7 +49,7 @@ class CreateShortUrlActionTest extends ApiTestCase * @test * @dataProvider provideMaxVisits */ - public function createsNewShortUrlWithVisitsLimit(int $maxVisits) + public function createsNewShortUrlWithVisitsLimit(int $maxVisits): void { [$statusCode, ['shortCode' => $shortCode]] = $this->createShortUrl(['maxVisits' => $maxVisits]); @@ -75,7 +75,7 @@ class CreateShortUrlActionTest extends ApiTestCase /** * @test */ - public function createsShortUrlWithValidSince() + public function createsShortUrlWithValidSince(): void { [$statusCode, ['shortCode' => $shortCode]] = $this->createShortUrl([ 'validSince' => Chronos::now()->addDay()->toAtomString(), @@ -91,7 +91,7 @@ class CreateShortUrlActionTest extends ApiTestCase /** * @test */ - public function createsShortUrlWithValidUntil() + public function createsShortUrlWithValidUntil(): void { [$statusCode, ['shortCode' => $shortCode]] = $this->createShortUrl([ 'validUntil' => Chronos::now()->subDay()->toAtomString(), @@ -104,6 +104,76 @@ class CreateShortUrlActionTest extends ApiTestCase $this->assertEquals(self::STATUS_NOT_FOUND, $lastResp->getStatusCode()); } + /** + * @test + * @dataProvider provideMatchingBodies + */ + public function returnsAnExistingShortUrlWhenRequested(array $body): void + { + + [$firstStatusCode, ['shortCode' => $firstShortCode]] = $this->createShortUrl($body); + + $body['findIfExists'] = true; + [$secondStatusCode, ['shortCode' => $secondShortCode]] = $this->createShortUrl($body); + + $this->assertEquals(self::STATUS_OK, $firstStatusCode); + $this->assertEquals(self::STATUS_OK, $secondStatusCode); + $this->assertEquals($firstShortCode, $secondShortCode); + } + + public function provideMatchingBodies(): array + { + $longUrl = 'https://www.alejandrocelaya.com'; + + return [ + 'only long URL' => [['longUrl' => $longUrl]], + 'long URL and tags' => [['longUrl' => $longUrl, 'tags' => ['boo', 'far']]], + 'long URL custom slug' => [['longUrl' => $longUrl, 'customSlug' => 'my cool slug']], + 'several params' => [[ + 'longUrl' => $longUrl, + 'tags' => ['boo', 'far'], + 'validSince' => Chronos::now()->toAtomString(), + 'maxVisits' => 7, + ]], + ]; + } + + /** + * @test + */ + public function returnsErrorWhenRequestingReturnExistingButCustomSlugIsInUse(): void + { + $longUrl = 'https://www.alejandrocelaya.com'; + + [$firstStatusCode] = $this->createShortUrl(['longUrl' => $longUrl]); + [$secondStatusCode] = $this->createShortUrl([ + 'longUrl' => $longUrl, + 'customSlug' => 'custom', + 'findIfExists' => true, + ]); + + $this->assertEquals(self::STATUS_OK, $firstStatusCode); + $this->assertEquals(self::STATUS_BAD_REQUEST, $secondStatusCode); + } + + /** + * @test + */ + public function createsNewShortUrlIfRequestedToFindButThereIsNoMatch(): void + { + [$firstStatusCode, ['shortCode' => $firstShortCode]] = $this->createShortUrl([ + 'longUrl' => 'https://www.alejandrocelaya.com', + ]); + [$secondStatusCode, ['shortCode' => $secondShortCode]] = $this->createShortUrl([ + 'longUrl' => 'https://www.alejandrocelaya.com/projects', + 'findIfExists' => true, + ]); + + $this->assertEquals(self::STATUS_OK, $firstStatusCode); + $this->assertEquals(self::STATUS_OK, $secondStatusCode); + $this->assertNotEquals($firstShortCode, $secondShortCode); + } + /** * @return array { * @var int $statusCode @@ -112,7 +182,9 @@ class CreateShortUrlActionTest extends ApiTestCase */ private function createShortUrl(array $body = []): array { - $body['longUrl'] = 'https://app.shlink.io'; + if (! isset($body['longUrl'])) { + $body['longUrl'] = 'https://app.shlink.io'; + } $resp = $this->callApiWithKey(self::METHOD_POST, '/short-urls', [RequestOptions::JSON => $body]); $payload = $this->getJsonResponsePayload($resp); From a918113ba04315780f8e622825c1e65c3b54447b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Feb 2019 11:24:26 +0100 Subject: [PATCH 10/11] Documented new findIfExists flag --- CHANGELOG.md | 10 ++++++++++ docs/swagger/paths/v1_short-urls.json | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b99e6277..79412b1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this The status code can be `200 OK` in case of success or `503 Service Unavailable` in case of error, while the `status` property will be one of `pass` or `fail`, as defined in the [Health check RFC](https://inadarei.github.io/rfc-healthcheck/). +* [#279](https://github.com/shlinkio/shlink/issues/279) Added new `findIfExists` flag to the `[POST /short-url]` endpoint which will be used to return existing short URLs if they exist, instead of creating new ones. + + Thanks to this flag you won't need to remember if you created a short URL for a long one. It will just create it if needed or return the existing one if possible. + + The behavior might be a little bit counterintuitive when combined with other params. This is how the endpoint behaves when providing this new flag: + + * Only the long URL is provided: It will return the newest match or create a new short URL if none is found. + * Long url and custom slug are provided: It will return the short URL when both params match, return an error when the slug is in use for another long URL, or create a new short URL otherwise. + * Any of the above but including other params (tags, validSince, validUntil, maxVisits): It will behave the same as the previous two cases, but it will try to exactly match existing results using all the params. If any of them does not match, it will try to create a new short URL. + * [#336](https://github.com/shlinkio/shlink/issues/336) Added an API test suite which performs API calls to an actual instance of the web service. #### Changed diff --git a/docs/swagger/paths/v1_short-urls.json b/docs/swagger/paths/v1_short-urls.json index 1bb59a9b..e55b1162 100644 --- a/docs/swagger/paths/v1_short-urls.json +++ b/docs/swagger/paths/v1_short-urls.json @@ -151,7 +151,7 @@ "Short URLs" ], "summary": "Create short URL", - "description": "Creates a new short URL.

**Important note**: Before shlink v1.13, this endpoint used to use the `/short-codes` path instead of `/short-urls`. Both of them will keep working, while the first one is considered deprecated.", + "description": "Creates a new short URL.

**Important note**: Before shlink v1.13, this endpoint used to use the `/short-codes` path instead of `/short-urls`. Both of them will keep working, while the first one is considered deprecated.

**Param findIfExists:**: Starting with v1.16, this new param allows to force shlink to return existing short URLs when found based on provided params, instead of creating a new one. However, it might add complexity and have unexpected outputs.\n\nThese are the use cases:\n* Only the long URL is provided: It will return the newest match or create a new short URL if none is found.\n* Long url and custom slug are provided: It will return the short URL when both params match, return an error when the slug is in use for another long URL, or create a new short URL otherwise.\n* Any of the above but including other params (tags, validSince, validUntil, maxVisits): It will behave the same as the previous two cases, but it will try to exactly match existing results using all the params. If any of them does not match, it will try to create a new short URL.", "security": [ { "ApiKey": [] From 04d4d4a8d71de9520be3bf1555d50131c7d3b775 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Feb 2019 12:09:56 +0100 Subject: [PATCH 11/11] Updated GenerateShortUrlCommand to accept the findIfExists flag --- CHANGELOG.md | 2 +- .../ShortUrl/GenerateShortUrlCommand.php | 24 +++++++++++-------- .../ShortUrl/GenerateShortUrlCommandTest.php | 2 +- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79412b1f..308790a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this The status code can be `200 OK` in case of success or `503 Service Unavailable` in case of error, while the `status` property will be one of `pass` or `fail`, as defined in the [Health check RFC](https://inadarei.github.io/rfc-healthcheck/). -* [#279](https://github.com/shlinkio/shlink/issues/279) Added new `findIfExists` flag to the `[POST /short-url]` endpoint which will be used to return existing short URLs if they exist, instead of creating new ones. +* [#279](https://github.com/shlinkio/shlink/issues/279) Added new `findIfExists` flag to the `[POST /short-url]` REST endpoint and the `short-urls:generate` CLI command. It can be used to return existing short URLs when found, instead of creating new ones. Thanks to this flag you won't need to remember if you created a short URL for a long one. It will just create it if needed or return the existing one if possible. diff --git a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php index b187b97c..fcb4d305 100644 --- a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php @@ -16,8 +16,10 @@ use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; use Zend\Diactoros\Uri; -use function array_merge; -use function explode; +use function array_map; +use function Functional\curry; +use function Functional\flatten; +use function Functional\unique; use function sprintf; class GenerateShortUrlCommand extends Command @@ -77,6 +79,12 @@ class GenerateShortUrlCommand extends Command 'm', InputOption::VALUE_REQUIRED, 'This will limit the number of visits for this short URL.' + ) + ->addOption( + 'findIfExists', + 'f', + InputOption::VALUE_NONE, + 'This will force existing matching URL to be returned if found, instead of creating a new one.' ); } @@ -103,13 +111,8 @@ class GenerateShortUrlCommand extends Command return; } - $tags = $input->getOption('tags'); - $processedTags = []; - foreach ($tags as $key => $tag) { - $explodedTags = explode(',', $tag); - $processedTags = array_merge($processedTags, $explodedTags); - } - $tags = $processedTags; + $explodeWithComma = curry('explode')(','); + $tags = unique(flatten(array_map($explodeWithComma, $input->getOption('tags')))); $customSlug = $input->getOption('customSlug'); $maxVisits = $input->getOption('maxVisits'); @@ -121,7 +124,8 @@ class GenerateShortUrlCommand extends Command $this->getOptionalDate($input, 'validSince'), $this->getOptionalDate($input, 'validUntil'), $customSlug, - $maxVisits !== null ? (int) $maxVisits : null + $maxVisits !== null ? (int) $maxVisits : null, + $input->getOption('findIfExists') ) )->getShortCode(); $shortUrl = $this->buildShortUrl($this->domainConfig, $shortCode); diff --git a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php index 4698ed48..03db0d61 100644 --- a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php @@ -90,7 +90,7 @@ class GenerateShortUrlCommandTest extends TestCase $this->commandTester->execute([ 'command' => 'shortcode:generate', 'longUrl' => 'http://domain.com/foo/bar', - '--tags' => ['foo,bar', 'baz', 'boo,zar'], + '--tags' => ['foo,bar', 'baz', 'boo,zar,baz'], ]); $output = $this->commandTester->getDisplay();