diff --git a/.env.dist b/.env.dist index 16ad75bc..daf74cbb 100644 --- a/.env.dist +++ b/.env.dist @@ -3,7 +3,6 @@ APP_ENV= SECRET_KEY= SHORTENED_URL_SCHEMA= SHORTENED_URL_HOSTNAME= -SHORTCODE_CHARS= # Database DB_USER= diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d4ec601..cda60fac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Added -* *Nothing* +* [#491](https://github.com/shlinkio/shlink/issues/491) Added improved short code generation logic. + + Now, short codes are truly random, which removes the guessability factor existing in previous versions. + + Generated short codes have 5 characters, and shlink makes sure they keep unique, while making it backwards-compatible. #### Changed diff --git a/composer.json b/composer.json index 3159566c..9869ac4f 100644 --- a/composer.json +++ b/composer.json @@ -33,6 +33,7 @@ "ocramius/proxy-manager": "~2.2.2", "phly/phly-event-dispatcher": "^1.0", "predis/predis": "^1.1", + "pugx/shortid-php": "^0.5", "shlinkio/shlink-common": "^2.0", "shlinkio/shlink-event-dispatcher": "^1.0", "shlinkio/shlink-installer": "^2.1", @@ -77,7 +78,10 @@ "Shlinkio\\Shlink\\Rest\\": "module/Rest/src", "Shlinkio\\Shlink\\Core\\": "module/Core/src", "Shlinkio\\Shlink\\PreviewGenerator\\": "module/PreviewGenerator/src/" - } + }, + "files": [ + "module/Core/functions/functions.php" + ] }, "autoload-dev": { "psr-4": { diff --git a/config/autoload/url-shortener.global.php b/config/autoload/url-shortener.global.php index 4bbd2c31..3d3689ea 100644 --- a/config/autoload/url-shortener.global.php +++ b/config/autoload/url-shortener.global.php @@ -2,8 +2,6 @@ declare(strict_types=1); -use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; - use function Shlinkio\Shlink\Common\env; return [ @@ -13,7 +11,6 @@ return [ 'schema' => env('SHORTENED_URL_SCHEMA', 'http'), 'hostname' => env('SHORTENED_URL_HOSTNAME'), ], - 'shortcode_chars' => env('SHORTCODE_CHARS', UrlShortenerOptions::DEFAULT_CHARS), 'validate_url' => true, 'not_found_short_url' => [ 'enable_redirection' => false, diff --git a/docker/README.md b/docker/README.md index aff3d997..eca69d36 100644 --- a/docker/README.md +++ b/docker/README.md @@ -92,7 +92,6 @@ This is the complete list of supported env vars: * `SHORT_DOMAIN_HOST`: The custom short domain used for this shlink instance. For example **doma.in**. * `SHORT_DOMAIN_SCHEMA`: Either **http** or **https**. -* `SHORTCODE_CHARS`: A charset to use when building short codes. Only needed when using more than one shlink instance ([Multi instance considerations](#multi-instance-considerations)). * `DB_DRIVER`: **sqlite** (which is the default value), **mysql**, **maria** or **postgres**. * `DB_NAME`: The database name to be used when using an external database driver. Defaults to **shlink**. * `DB_USER`: The username credential to be used when using an external database driver. @@ -112,6 +111,8 @@ This is the complete list of supported env vars: In the future, these redis servers could be used for other caching operations performed by shlink. +* `SHORTCODE_CHARS`: **Ignored when using Shlink 1.20 or newer**. A charset to use when building short codes. Only needed when using more than one shlink instance ([Multi instance considerations](#multi-instance-considerations)). + An example using all env vars could look like this: ```bash @@ -178,7 +179,13 @@ docker run --name shlink -p 8080:8080 -v ${PWD}/my/config/dir:/etc/shlink/config These are some considerations to take into account when running multiple instances of shlink. -* The first time shlink is run, it generates a charset used to generate short codes, which is a shuffled base62 charset. +* Some operations performed by Shlink should never be run more than once at the same time (like creating the database for the first time, or downloading the GeoLite2 database). For this reason, Shlink uses a locking system. + + However, these locks are locally scoped to each Shlink instance by default. + + You can (and should) make the locks to be shared by all Shlink instances by using a redis server/cluster. Just define the `REDIS_SERVERS` env var with the list of servers. + +* **Ignore this if using Shlink 1.20 or newer**. The first time shlink is run, it generates a charset used to generate short codes, which is a shuffled base62 charset. If you are using several shlink instances, you will probably want all of them to use the same charset. @@ -186,12 +193,6 @@ These are some considerations to take into account when running multiple instanc If you don't do this, each shlink instance will use a different charset. However this shouldn't be a problem in practice, since the chances to get a collision will be very low. -* Some operations performed by Shlink should never be run more than once at the same time (like creating the database for the first time, or downloading the GeoLite2 database). For this reason, Shlink uses a locking system. - - However, these locks are locally scoped to each Shlink instance by default. - - You can (and should) make the locks to be shared by all Shlink instances by using a redis server/cluster. Just define the `REDIS_SERVERS` env var with the list of servers. - ## Versions Versions of this image match the shlink version it contains. diff --git a/docker/config/shlink_in_docker.local.php b/docker/config/shlink_in_docker.local.php index d0744358..5477e892 100644 --- a/docker/config/shlink_in_docker.local.php +++ b/docker/config/shlink_in_docker.local.php @@ -32,17 +32,15 @@ $helper = new class { 'postgres' => '5432', ]; - /** @var string */ - private $charset; /** @var string */ private $secretKey; public function __construct() { - [$this->charset, $this->secretKey] = $this->initShlinkKeys(); + [, $this->secretKey] = $this->initShlinkSecretKey(); } - private function initShlinkKeys(): array + private function initShlinkSecretKey(): array { $keysFile = sprintf('%s/shlink.keys', sys_get_temp_dir()); if (file_exists($keysFile)) { @@ -50,29 +48,19 @@ $helper = new class { } $keys = [ - env('SHORTCODE_CHARS', $this->generateShortcodeChars()), - env('SECRET_KEY', $this->generateSecretKey()), + '', // This was the SHORTCODE_CHARS. Kept as empty string for BC + env('SECRET_KEY', $this->generateSecretKey()), // Deprecated ]; file_put_contents($keysFile, implode(',', $keys)); return $keys; } - private function generateShortcodeChars(): string - { - return str_shuffle(self::BASE62); - } - private function generateSecretKey(): string { return substr(str_shuffle(self::BASE62), 0, 32); } - public function getShortcodeChars(): string - { - return $this->charset; - } - public function getSecretKey(): string { return $this->secretKey; @@ -137,7 +125,6 @@ return [ 'schema' => env('SHORT_DOMAIN_SCHEMA', 'http'), 'hostname' => env('SHORT_DOMAIN_HOST', ''), ], - 'shortcode_chars' => $helper->getShortcodeChars(), 'validate_url' => (bool) env('VALIDATE_URLS', true), 'not_found_short_url' => $helper->getNotFoundConfig(), ], diff --git a/module/CLI/src/Command/Config/GenerateCharsetCommand.php b/module/CLI/src/Command/Config/GenerateCharsetCommand.php index 924c45fc..a285c7f0 100644 --- a/module/CLI/src/Command/Config/GenerateCharsetCommand.php +++ b/module/CLI/src/Command/Config/GenerateCharsetCommand.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\Config; use Shlinkio\Shlink\CLI\Util\ExitCodes; -use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; @@ -18,6 +17,7 @@ use function str_shuffle; class GenerateCharsetCommand extends Command { public const NAME = 'config:generate-charset'; + private const DEFAULT_CHARS = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'; protected function configure(): void { @@ -26,14 +26,14 @@ class GenerateCharsetCommand extends Command ->setDescription(sprintf( '[DEPRECATED] Generates a character set sample just by shuffling the default one, "%s". ' . 'Then it can be set in the SHORTCODE_CHARS environment variable', - UrlShortenerOptions::DEFAULT_CHARS + self::DEFAULT_CHARS )) ->setHelp('This command is deprecated. Better leave shlink generate the charset.'); } protected function execute(InputInterface $input, OutputInterface $output): ?int { - $charSet = str_shuffle(UrlShortenerOptions::DEFAULT_CHARS); + $charSet = str_shuffle(self::DEFAULT_CHARS); (new SymfonyStyle($input, $output))->success(sprintf('Character set: "%s"', $charSet)); return ExitCodes::EXIT_SUCCESS; } diff --git a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php index 0ac05688..d83bd042 100644 --- a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php @@ -20,6 +20,11 @@ use Symfony\Component\Console\Tester\CommandTester; class GenerateShortUrlCommandTest extends TestCase { + private const DOMAIN_CONFIG = [ + 'schema' => 'http', + 'hostname' => 'foo.com', + ]; + /** @var CommandTester */ private $commandTester; /** @var ObjectProphecy */ @@ -28,10 +33,7 @@ class GenerateShortUrlCommandTest extends TestCase public function setUp(): void { $this->urlShortener = $this->prophesize(UrlShortener::class); - $command = new GenerateShortUrlCommand($this->urlShortener->reveal(), [ - 'schema' => 'http', - 'hostname' => 'foo.com', - ]); + $command = new GenerateShortUrlCommand($this->urlShortener->reveal(), self::DOMAIN_CONFIG); $app = new Application(); $app->add($command); $this->commandTester = new CommandTester($command); @@ -40,9 +42,8 @@ class GenerateShortUrlCommandTest extends TestCase /** @test */ public function properShortCodeIsCreatedIfLongUrlIsCorrect(): void { - $urlToShortCode = $this->urlShortener->urlToShortCode(Argument::cetera())->willReturn( - (new ShortUrl(''))->setShortCode('abc123') - ); + $shortUrl = new ShortUrl(''); + $urlToShortCode = $this->urlShortener->urlToShortCode(Argument::cetera())->willReturn($shortUrl); $this->commandTester->execute([ 'longUrl' => 'http://domain.com/foo/bar', @@ -51,7 +52,7 @@ class GenerateShortUrlCommandTest extends TestCase $output = $this->commandTester->getDisplay(); $this->assertEquals(ExitCodes::EXIT_SUCCESS, $this->commandTester->getStatusCode()); - $this->assertStringContainsString('http://foo.com/abc123', $output); + $this->assertStringContainsString($shortUrl->toString(self::DOMAIN_CONFIG), $output); $urlToShortCode->shouldHaveBeenCalledOnce(); } @@ -86,6 +87,7 @@ class GenerateShortUrlCommandTest extends TestCase /** @test */ public function properlyProcessesProvidedTags(): void { + $shortUrl = new ShortUrl(''); $urlToShortCode = $this->urlShortener->urlToShortCode( Argument::type(UriInterface::class), Argument::that(function (array $tags) { @@ -93,7 +95,7 @@ class GenerateShortUrlCommandTest extends TestCase return $tags; }), Argument::cetera() - )->willReturn((new ShortUrl(''))->setShortCode('abc123')); + )->willReturn($shortUrl); $this->commandTester->execute([ 'longUrl' => 'http://domain.com/foo/bar', @@ -102,7 +104,7 @@ class GenerateShortUrlCommandTest extends TestCase $output = $this->commandTester->getDisplay(); $this->assertEquals(ExitCodes::EXIT_SUCCESS, $this->commandTester->getStatusCode()); - $this->assertStringContainsString('http://foo.com/abc123', $output); + $this->assertStringContainsString($shortUrl->toString(self::DOMAIN_CONFIG), $output); $urlToShortCode->shouldHaveBeenCalledOnce(); } } diff --git a/module/Core/functions/functions.php b/module/Core/functions/functions.php new file mode 100644 index 00000000..92d40fe1 --- /dev/null +++ b/module/Core/functions/functions.php @@ -0,0 +1,18 @@ +generate($length, $alphabet)->serialize(); +} diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index f8ae2401..6cf6378a 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -10,6 +10,7 @@ use Doctrine\Common\Collections\Collection; use Shlinkio\Shlink\Common\Entity\AbstractEntity; use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface; use Shlinkio\Shlink\Core\Domain\Resolver\SimpleDomainResolver; +use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Zend\Diactoros\Uri; @@ -17,6 +18,7 @@ use function array_reduce; use function count; use function Functional\contains; use function Functional\invoke; +use function Shlinkio\Shlink\Core\generateRandomShortCode; class ShortUrl extends AbstractEntity { @@ -38,6 +40,8 @@ class ShortUrl extends AbstractEntity private $maxVisits; /** @var Domain|null */ private $domain; + /** @var bool */ + private $customSlugWasProvided; public function __construct( string $longUrl, @@ -53,7 +57,8 @@ class ShortUrl extends AbstractEntity $this->validSince = $meta->getValidSince(); $this->validUntil = $meta->getValidUntil(); $this->maxVisits = $meta->getMaxVisits(); - $this->shortCode = $meta->getCustomSlug() ?? ''; // TODO logic to calculate short code should be passed somehow + $this->customSlugWasProvided = $meta->hasCustomSlug(); + $this->shortCode = $meta->getCustomSlug() ?? generateRandomShortCode(); $this->domain = ($domainResolver ?? new SimpleDomainResolver())->resolveDomain($meta->getDomain()); } @@ -67,13 +72,6 @@ class ShortUrl extends AbstractEntity return $this->shortCode; } - // TODO Short code is currently calculated based on the ID, so a setter is needed - public function setShortCode(string $shortCode): self - { - $this->shortCode = $shortCode; - return $this; - } - public function getDateCreated(): Chronos { return $this->dateCreated; @@ -109,6 +107,25 @@ class ShortUrl extends AbstractEntity } } + /** + * @throws ShortCodeCannotBeRegeneratedException + */ + public function regenerateShortCode(): self + { + // In ShortUrls where a custom slug was provided, do nothing + if ($this->customSlugWasProvided) { + throw ShortCodeCannotBeRegeneratedException::forShortUrlWithCustomSlug(); + } + + // The short code can be regenerated only on ShortUrl which have not been persisted yet + if ($this->id !== null) { + throw ShortCodeCannotBeRegeneratedException::forShortUrlAlreadyPersisted(); + } + + $this->shortCode = generateRandomShortCode(); + return $this; + } + public function getValidSince(): ?Chronos { return $this->validSince; diff --git a/module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php b/module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php new file mode 100644 index 00000000..fad5805d --- /dev/null +++ b/module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php @@ -0,0 +1,18 @@ +shortcodeChars; - } - - protected function setShortcodeChars(string $shortcodeChars): self - { - $this->shortcodeChars = empty($shortcodeChars) ? self::DEFAULT_CHARS : $shortcodeChars; - return $this; - } - public function isUrlValidationEnabled(): bool { return $this->validateUrl; diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index 1638fb33..47f8f985 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -155,7 +155,7 @@ DQL; return $shortUrl !== null && ! $shortUrl->maxVisitsReached() ? $shortUrl : null; } - public function slugIsInUse(string $slug, ?string $domain = null): bool + public function shortCodeIsInUse(string $slug, ?string $domain = null): bool { $qb = $this->getEntityManager()->createQueryBuilder(); $qb->select('COUNT(DISTINCT s.id)') diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index 0535d979..da5cef61 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -29,5 +29,5 @@ interface ShortUrlRepositoryInterface extends ObjectRepository public function findOneByShortCode(string $shortCode, ?string $domain = null): ?ShortUrl; - public function slugIsInUse(string $slug, ?string $domain): bool; + public function shortCodeIsInUse(string $slug, ?string $domain): bool; } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index 47ea6056..f282ba54 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -16,7 +16,6 @@ use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; 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; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; @@ -24,17 +23,11 @@ use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Throwable; use function array_reduce; -use function floor; -use function fmod; -use function preg_match; -use function strlen; class UrlShortener implements UrlShortenerInterface { use TagManagerTrait; - private const ID_INCREMENT = 200000; - /** @var ClientInterface */ private $httpClient; /** @var EntityManagerInterface */ @@ -53,7 +46,7 @@ class UrlShortener implements UrlShortenerInterface * @param string[] $tags * @throws NonUniqueSlugException * @throws InvalidUrlException - * @throws RuntimeException + * @throws Throwable */ public function urlToShortCode(UriInterface $url, array $tags, ShortUrlMeta $meta): ShortUrl { @@ -69,36 +62,26 @@ class UrlShortener implements UrlShortenerInterface if ($this->options->isUrlValidationEnabled()) { $this->checkUrlExists($url); } - $this->verifyCustomSlug($meta); - // Transactionally insert the short url, then generate the short code and finally update the short code + $this->em->beginTransaction(); + $shortUrl = new ShortUrl($url, $meta, new PersistenceDomainResolver($this->em)); + $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); + try { - $this->em->beginTransaction(); - - // First, create the short URL with an empty short code - $shortUrl = new ShortUrl($url, $meta, new PersistenceDomainResolver($this->em)); + $this->verifyShortCodeUniqueness($meta, $shortUrl); $this->em->persist($shortUrl); $this->em->flush(); - - // 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(); - return $shortUrl; } catch (Throwable $e) { if ($this->em->getConnection()->isTransactionActive()) { $this->em->rollback(); $this->em->close(); } - throw new RuntimeException('An error occurred while persisting the short URL', -1, $e); + throw $e; } + + return $shortUrl; } private function findExistingShortUrlIfExists(string $url, array $tags, ShortUrlMeta $meta): ?ShortUrl @@ -138,38 +121,23 @@ class UrlShortener implements UrlShortenerInterface } } - private function verifyCustomSlug(ShortUrlMeta $meta): void + private function verifyShortCodeUniqueness(ShortUrlMeta $meta, ShortUrl $shortUrlToBeCreated): void { - if (! $meta->hasCustomSlug()) { - return; - } - - $customSlug = $meta->getCustomSlug(); + $shortCode = $shortUrlToBeCreated->getShortCode(); $domain = $meta->getDomain(); /** @var ShortUrlRepository $repo */ $repo = $this->em->getRepository(ShortUrl::class); - $shortUrlsCount = $repo->slugIsInUse($customSlug, $domain); - if ($shortUrlsCount > 0) { - throw NonUniqueSlugException::fromSlug($customSlug, $domain); - } - } + $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domain); - private function convertAutoincrementIdToShortCode(float $id): string - { - $id += self::ID_INCREMENT; // Increment the Id so that the generated shortcode is not too short - $chars = $this->options->getChars(); - - $length = strlen($chars); - $code = ''; - - while ($id > 0) { - // Determine the value of the next higher character in the short code and prepend it - $code = $chars[(int) fmod($id, $length)] . $code; - $id = floor($id / $length); + if ($otherShortUrlsExist && $meta->hasCustomSlug()) { + throw NonUniqueSlugException::fromSlug($shortCode, $domain); } - return $chars[(int) $id] . $code; + if ($otherShortUrlsExist) { + $shortUrlToBeCreated->regenerateShortCode(); + $this->verifyShortCodeUniqueness($meta, $shortUrlToBeCreated); + } } /** @@ -178,13 +146,6 @@ class UrlShortener implements UrlShortenerInterface */ public function shortCodeToUrl(string $shortCode, ?string $domain = null): ShortUrl { - $chars = $this->options->getChars(); - - // Validate short code format - if (! preg_match('|[' . $chars . ']+|', $shortCode)) { - throw InvalidShortCodeException::fromCharset($shortCode, $chars); - } - /** @var ShortUrlRepository $shortUrlRepo */ $shortUrlRepo = $this->em->getRepository(ShortUrl::class); $shortUrl = $shortUrlRepo->findOneByShortCode($shortCode, $domain); diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index e6d511fa..f3913ea2 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -37,37 +37,41 @@ class ShortUrlRepositoryTest extends DatabaseTestCase /** @test */ public function findOneByShortCodeReturnsProperData(): void { - $regularOne = new ShortUrl('foo'); - $regularOne->setShortCode('foo'); + $regularOne = new ShortUrl('foo', ShortUrlMeta::createFromParams(null, null, 'foo')); $this->getEntityManager()->persist($regularOne); - $notYetValid = new ShortUrl('bar', ShortUrlMeta::createFromParams(Chronos::now()->addMonth())); - $notYetValid->setShortCode('bar_very_long_text'); + $notYetValid = new ShortUrl( + 'bar', + ShortUrlMeta::createFromParams(Chronos::now()->addMonth(), null, 'bar_very_long_text') + ); $this->getEntityManager()->persist($notYetValid); - $expired = new ShortUrl('expired', ShortUrlMeta::createFromParams(null, Chronos::now()->subMonth())); - $expired->setShortCode('expired'); + $expired = new ShortUrl('expired', ShortUrlMeta::createFromParams(null, Chronos::now()->subMonth(), 'expired')); $this->getEntityManager()->persist($expired); - $allVisitsComplete = new ShortUrl('baz', ShortUrlMeta::createFromRawData(['maxVisits' => 3])); + $allVisitsComplete = new ShortUrl('baz', ShortUrlMeta::createFromRawData([ + 'maxVisits' => 3, + 'customSlug' => 'baz', + ])); $visits = []; for ($i = 0; $i < 3; $i++) { $visit = new Visit($allVisitsComplete, Visitor::emptyInstance()); $this->getEntityManager()->persist($visit); $visits[] = $visit; } - $allVisitsComplete->setShortCode('baz') - ->setVisits(new ArrayCollection($visits)); + $allVisitsComplete->setVisits(new ArrayCollection($visits)); $this->getEntityManager()->persist($allVisitsComplete); - $withDomain = new ShortUrl('foo', ShortUrlMeta::createFromRawData(['domain' => 'example.com'])); - $withDomain->setShortCode('domain-short-code'); + $withDomain = new ShortUrl('foo', ShortUrlMeta::createFromRawData([ + 'domain' => 'example.com', + 'customSlug' => 'domain-short-code', + ])); $this->getEntityManager()->persist($withDomain); $withDomainDuplicatingRegular = new ShortUrl('foo_with_domain', ShortUrlMeta::createFromRawData([ 'domain' => 'doma.in', + 'customSlug' => 'foo', ])); - $withDomainDuplicatingRegular->setShortCode('foo'); $this->getEntityManager()->persist($withDomainDuplicatingRegular); $this->getEntityManager()->flush(); @@ -96,9 +100,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase { $count = 5; for ($i = 0; $i < $count; $i++) { - $this->getEntityManager()->persist( - (new ShortUrl((string) $i))->setShortCode((string) $i) - ); + $this->getEntityManager()->persist(new ShortUrl((string) $i)); } $this->getEntityManager()->flush(); @@ -112,19 +114,16 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->getEntityManager()->persist($tag); $foo = new ShortUrl('foo'); - $foo->setShortCode('foo') - ->setTags(new ArrayCollection([$tag])); + $foo->setTags(new ArrayCollection([$tag])); $this->getEntityManager()->persist($foo); $bar = new ShortUrl('bar'); $visit = new Visit($bar, Visitor::emptyInstance()); $this->getEntityManager()->persist($visit); - $bar->setShortCode('bar_very_long_text') - ->setVisits(new ArrayCollection([$visit])); + $bar->setVisits(new ArrayCollection([$visit])); $this->getEntityManager()->persist($bar); $foo2 = new ShortUrl('foo_2'); - $foo2->setShortCode('foo_2'); $this->getEntityManager()->persist($foo2); $this->getEntityManager()->flush(); @@ -155,9 +154,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase { $urls = ['a', 'z', 'c', 'b']; foreach ($urls as $url) { - $this->getEntityManager()->persist( - (new ShortUrl($url))->setShortCode($url) - ); + $this->getEntityManager()->persist(new ShortUrl($url)); } $this->getEntityManager()->flush(); @@ -172,24 +169,24 @@ class ShortUrlRepositoryTest extends DatabaseTestCase } /** @test */ - public function slugIsInUseLooksForShortUrlInProperSetOfTables(): void + public function shortCodeIsInUseLooksForShortUrlInProperSetOfTables(): void { - $shortUrlWithoutDomain = (new ShortUrl('foo'))->setShortCode('my-cool-slug'); + $shortUrlWithoutDomain = new ShortUrl('foo', ShortUrlMeta::createFromRawData(['customSlug' => 'my-cool-slug'])); $this->getEntityManager()->persist($shortUrlWithoutDomain); - $shortUrlWithDomain = (new ShortUrl( + $shortUrlWithDomain = new ShortUrl( 'foo', - ShortUrlMeta::createFromRawData(['domain' => 'doma.in']) - ))->setShortCode('another-slug'); + ShortUrlMeta::createFromRawData(['domain' => 'doma.in', 'customSlug' => 'another-slug']) + ); $this->getEntityManager()->persist($shortUrlWithDomain); $this->getEntityManager()->flush(); - $this->assertTrue($this->repo->slugIsInUse('my-cool-slug')); - $this->assertFalse($this->repo->slugIsInUse('my-cool-slug', 'doma.in')); - $this->assertFalse($this->repo->slugIsInUse('slug-not-in-use')); - $this->assertFalse($this->repo->slugIsInUse('another-slug')); - $this->assertFalse($this->repo->slugIsInUse('another-slug', 'example.com')); - $this->assertTrue($this->repo->slugIsInUse('another-slug', 'doma.in')); + $this->assertTrue($this->repo->shortCodeIsInUse('my-cool-slug')); + $this->assertFalse($this->repo->shortCodeIsInUse('my-cool-slug', 'doma.in')); + $this->assertFalse($this->repo->shortCodeIsInUse('slug-not-in-use')); + $this->assertFalse($this->repo->shortCodeIsInUse('another-slug')); + $this->assertFalse($this->repo->shortCodeIsInUse('another-slug', 'example.com')); + $this->assertTrue($this->repo->shortCodeIsInUse('another-slug', 'doma.in')); } } diff --git a/module/Core/test/Entity/ShortUrlTest.php b/module/Core/test/Entity/ShortUrlTest.php new file mode 100644 index 00000000..98404ca1 --- /dev/null +++ b/module/Core/test/Entity/ShortUrlTest.php @@ -0,0 +1,51 @@ +expectException(ShortCodeCannotBeRegeneratedException::class); + $this->expectExceptionMessage($expectedMessage); + + $shortUrl->regenerateShortCode(); + } + + public function provideInvalidShortUrls(): iterable + { + yield 'with custom slug' => [ + new ShortUrl('', ShortUrlMeta::createFromRawData(['customSlug' => 'custom-slug'])), + 'The short code cannot be regenerated on ShortUrls where a custom slug was provided.', + ]; + yield 'already persisted' => [ + (new ShortUrl(''))->setId('1'), + 'The short code can be regenerated only on new ShortUrls which have not been persisted yet.', + ]; + } + + /** @test */ + public function regenerateShortCodeProperlyChangesTheValueOnValidShortUrls(): void + { + $shortUrl = new ShortUrl(''); + $firstShortCode = $shortUrl->getShortCode(); + + $shortUrl->regenerateShortCode(); + $secondShortCode = $shortUrl->getShortCode(); + + $this->assertNotEquals($firstShortCode, $secondShortCode); + } +} diff --git a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php index f96b6f33..cbb99c57 100644 --- a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php @@ -19,20 +19,21 @@ use Shlinkio\Shlink\Core\Service\ShortUrl\DeleteShortUrlService; use function Functional\map; use function range; +use function sprintf; class DeleteShortUrlServiceTest extends TestCase { - /** @var DeleteShortUrlService */ - private $service; /** @var ObjectProphecy */ private $em; + /** @var string */ + private $shortCode; public function setUp(): void { - $shortUrl = (new ShortUrl(''))->setShortCode('abc123') - ->setVisits(new ArrayCollection(map(range(0, 10), function () { - return new Visit(new ShortUrl(''), Visitor::emptyInstance()); - }))); + $shortUrl = (new ShortUrl(''))->setVisits(new ArrayCollection(map(range(0, 10), function () { + return new Visit(new ShortUrl(''), Visitor::emptyInstance()); + }))); + $this->shortCode = $shortUrl->getShortCode(); $this->em = $this->prophesize(EntityManagerInterface::class); @@ -42,55 +43,56 @@ class DeleteShortUrlServiceTest extends TestCase } /** @test */ - public function deleteByShortCodeThrowsExceptionWhenThresholdIsReached() + public function deleteByShortCodeThrowsExceptionWhenThresholdIsReached(): void { $service = $this->createService(); $this->expectException(DeleteShortUrlException::class); - $this->expectExceptionMessage( - 'Impossible to delete short URL with short code "abc123" since it has more than "5" visits.' - ); + $this->expectExceptionMessage(sprintf( + 'Impossible to delete short URL with short code "%s" since it has more than "5" visits.', + $this->shortCode + )); - $service->deleteByShortCode('abc123'); + $service->deleteByShortCode($this->shortCode); } /** @test */ - public function deleteByShortCodeDeletesUrlWhenThresholdIsReachedButExplicitlyIgnored() + public function deleteByShortCodeDeletesUrlWhenThresholdIsReachedButExplicitlyIgnored(): void { $service = $this->createService(); $remove = $this->em->remove(Argument::type(ShortUrl::class))->willReturn(null); $flush = $this->em->flush()->willReturn(null); - $service->deleteByShortCode('abc123', true); + $service->deleteByShortCode($this->shortCode, true); $remove->shouldHaveBeenCalledOnce(); $flush->shouldHaveBeenCalledOnce(); } /** @test */ - public function deleteByShortCodeDeletesUrlWhenThresholdIsReachedButCheckIsDisabled() + public function deleteByShortCodeDeletesUrlWhenThresholdIsReachedButCheckIsDisabled(): void { $service = $this->createService(false); $remove = $this->em->remove(Argument::type(ShortUrl::class))->willReturn(null); $flush = $this->em->flush()->willReturn(null); - $service->deleteByShortCode('abc123'); + $service->deleteByShortCode($this->shortCode); $remove->shouldHaveBeenCalledOnce(); $flush->shouldHaveBeenCalledOnce(); } /** @test */ - public function deleteByShortCodeDeletesUrlWhenThresholdIsNotReached() + public function deleteByShortCodeDeletesUrlWhenThresholdIsNotReached(): void { $service = $this->createService(true, 100); $remove = $this->em->remove(Argument::type(ShortUrl::class))->willReturn(null); $flush = $this->em->flush()->willReturn(null); - $service->deleteByShortCode('abc123'); + $service->deleteByShortCode($this->shortCode); $remove->shouldHaveBeenCalledOnce(); $flush->shouldHaveBeenCalledOnce(); diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 3e75e5b4..78e61a6a 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -17,10 +17,8 @@ use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Tag; -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; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; @@ -56,7 +54,7 @@ class UrlShortenerTest extends TestCase $shortUrl->setId('10'); }); $repo = $this->prophesize(ShortUrlRepository::class); - $repo->slugIsInUse(Argument::cetera())->willReturn(false); + $repo->shortCodeIsInUse(Argument::cetera())->willReturn(false); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->setUrlShortener(false); @@ -74,17 +72,43 @@ class UrlShortenerTest extends TestCase /** @test */ public function urlIsProperlyShortened(): void { - // 10 -> 0Q1Y $shortUrl = $this->urlShortener->urlToShortCode( new Uri('http://foobar.com/12345/hello?foo=bar'), [], ShortUrlMeta::createEmpty() ); - $this->assertEquals('0Q1Y', $shortUrl->getShortCode()); + + $this->assertEquals('http://foobar.com/12345/hello?foo=bar', $shortUrl->getLongUrl()); } /** @test */ - public function exceptionIsThrownWhenOrmThrowsException(): void + public function shortCodeIsRegeneratedIfAlreadyInUse(): void + { + $callIndex = 0; + $expectedCalls = 3; + $repo = $this->prophesize(ShortUrlRepository::class); + $shortCodeIsInUse = $repo->shortCodeIsInUse(Argument::cetera())->will( + function () use (&$callIndex, $expectedCalls) { + $callIndex++; + return $callIndex < $expectedCalls; + } + ); + $repo->findBy(Argument::cetera())->willReturn([]); + $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + + $shortUrl = $this->urlShortener->urlToShortCode( + new Uri('http://foobar.com/12345/hello?foo=bar'), + [], + ShortUrlMeta::createEmpty() + ); + + $this->assertEquals('http://foobar.com/12345/hello?foo=bar', $shortUrl->getLongUrl()); + $getRepo->shouldBeCalledTimes($expectedCalls); + $shortCodeIsInUse->shouldBeCalledTimes($expectedCalls); + } + + /** @test */ + public function transactionIsRolledBackAndExceptionRethrownWhenExceptionIsThrown(): void { $conn = $this->prophesize(Connection::class); $conn->isTransactionActive()->willReturn(true); @@ -94,7 +118,7 @@ class UrlShortenerTest extends TestCase $this->em->flush()->willThrow(new ORMException()); - $this->expectException(RuntimeException::class); + $this->expectException(ORMException::class); $this->urlShortener->urlToShortCode( new Uri('http://foobar.com/12345/hello?foo=bar'), [], @@ -123,11 +147,11 @@ class UrlShortenerTest extends TestCase public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void { $repo = $this->prophesize(ShortUrlRepository::class); - $slugIsInUse = $repo->slugIsInUse('custom-slug', null)->willReturn(true); + $shortCodeIsInUse = $repo->shortCodeIsInUse('custom-slug', null)->willReturn(true); $repo->findBy(Argument::cetera())->willReturn([]); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - $slugIsInUse->shouldBeCalledOnce(); + $shortCodeIsInUse->shouldBeCalledOnce(); $getRepo->shouldBeCalled(); $this->expectException(NonUniqueSlugException::class); @@ -146,26 +170,24 @@ class UrlShortenerTest extends TestCase string $url, array $tags, ShortUrlMeta $meta, - ?ShortUrl $expected + ShortUrl $expected ): void { $repo = $this->prophesize(ShortUrlRepository::class); - $findExisting = $repo->findBy(Argument::any())->willReturn($expected !== null ? [$expected] : []); + $findExisting = $repo->findBy(Argument::any())->willReturn([$expected]); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlShortener->urlToShortCode(new Uri($url), $tags, $meta); $findExisting->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); - if ($expected) { - $this->assertSame($expected, $result); - } + $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->assertSame($expected, $result); } public function provideExistingShortUrls(): iterable { $url = 'http://foo.com'; - yield [$url, [], ShortUrlMeta::createFromRawData(['findIfExists' => true]), null]; yield [$url, [], ShortUrlMeta::createFromRawData(['findIfExists' => true]), new ShortUrl($url)]; yield [$url, [], ShortUrlMeta::createFromRawData( ['findIfExists' => true, 'customSlug' => 'foo'] @@ -194,6 +216,12 @@ class UrlShortenerTest extends TestCase ShortUrlMeta::createFromRawData(['findIfExists' => true, 'validUntil' => Chronos::parse('2017-01-01')]), new ShortUrl($url, ShortUrlMeta::createFromRawData(['validUntil' => Chronos::parse('2017-01-01')])), ]; + yield [ + $url, + [], + ShortUrlMeta::createFromRawData(['findIfExists' => true, 'domain' => 'example.com']), + new ShortUrl($url, ShortUrlMeta::createFromRawData(['domain' => 'example.com'])), + ]; yield [ $url, ['baz', 'foo', 'bar'], @@ -243,9 +271,8 @@ class UrlShortenerTest extends TestCase /** @test */ public function shortCodeIsProperlyParsed(): void { - $shortCode = '12C1c'; $shortUrl = new ShortUrl('expected_url'); - $shortUrl->setShortCode($shortCode); + $shortCode = $shortUrl->getShortCode(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); $repo->findOneByShortCode($shortCode, null)->willReturn($shortUrl); @@ -254,11 +281,4 @@ class UrlShortenerTest extends TestCase $url = $this->urlShortener->shortCodeToUrl($shortCode); $this->assertSame($shortUrl, $url); } - - /** @test */ - public function invalidCharSetThrowsException(): void - { - $this->expectException(InvalidShortCodeException::class); - $this->urlShortener->shortCodeToUrl('&/('); - } } diff --git a/module/Rest/test-api/Action/CreateShortUrlActionTest.php b/module/Rest/test-api/Action/CreateShortUrlActionTest.php index ed6a66d8..c3f7a4e4 100644 --- a/module/Rest/test-api/Action/CreateShortUrlActionTest.php +++ b/module/Rest/test-api/Action/CreateShortUrlActionTest.php @@ -90,7 +90,7 @@ class CreateShortUrlActionTest extends ApiTestCase $this->assertEquals(self::STATUS_OK, $statusCode); - // Request to the short URL will return a 404 since ist' not valid yet + // Request to the short URL will return a 404 since it's not valid yet $lastResp = $this->callShortUrl($shortCode); $this->assertEquals(self::STATUS_NOT_FOUND, $lastResp->getStatusCode()); } diff --git a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php index 4a9de19b..253b0032 100644 --- a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php +++ b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php @@ -20,13 +20,15 @@ class ShortUrlsFixture extends AbstractFixture */ public function load(ObjectManager $manager): void { - $abcShortUrl = $this->setShortUrlDate(new ShortUrl('https://shlink.io'))->setShortCode('abc123'); + $abcShortUrl = $this->setShortUrlDate( + new ShortUrl('https://shlink.io', ShortUrlMeta::createFromRawData(['customSlug' => 'abc123'])) + ); $manager->persist($abcShortUrl); $defShortUrl = $this->setShortUrlDate(new ShortUrl( 'https://blog.alejandrocelaya.com/2017/12/09/acmailer-7-0-the-most-important-release-in-a-long-time/', - ShortUrlMeta::createFromParams(Chronos::parse('2020-05-01')) - ))->setShortCode('def456'); + ShortUrlMeta::createFromParams(Chronos::parse('2020-05-01'), null, 'def456') + )); $manager->persist($defShortUrl); $customShortUrl = $this->setShortUrlDate(new ShortUrl( @@ -37,14 +39,14 @@ class ShortUrlsFixture extends AbstractFixture $withDomainShortUrl = $this->setShortUrlDate(new ShortUrl( 'https://blog.alejandrocelaya.com/2019/04/27/considerations-to-properly-use-open-source-software-projects/', - ShortUrlMeta::createFromRawData(['domain' => 'example.com']) - ))->setShortCode('ghi789'); + ShortUrlMeta::createFromRawData(['domain' => 'example.com', 'customSlug' => 'ghi789']) + )); $manager->persist($withDomainShortUrl); $withDomainAndSlugShortUrl = $this->setShortUrlDate(new ShortUrl( 'https://google.com', - ShortUrlMeta::createFromRawData(['domain' => 'some-domain.com']) - ))->setShortCode('custom-with-domain'); + ShortUrlMeta::createFromRawData(['domain' => 'some-domain.com', 'customSlug' => 'custom-with-domain']) + )); $manager->persist($withDomainAndSlugShortUrl); $manager->flush(); diff --git a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php index 7175b794..8b00229e 100644 --- a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php @@ -22,6 +22,11 @@ use function strpos; class CreateShortUrlActionTest extends TestCase { + private const DOMAIN_CONFIG = [ + 'schema' => 'http', + 'hostname' => 'foo.com', + ]; + /** @var CreateShortUrlAction */ private $action; /** @var ObjectProphecy */ @@ -30,10 +35,7 @@ class CreateShortUrlActionTest extends TestCase public function setUp(): void { $this->urlShortener = $this->prophesize(UrlShortener::class); - $this->action = new CreateShortUrlAction($this->urlShortener->reveal(), [ - 'schema' => 'http', - 'hostname' => 'foo.com', - ]); + $this->action = new CreateShortUrlAction($this->urlShortener->reveal(), self::DOMAIN_CONFIG); } /** @test */ @@ -46,10 +48,9 @@ class CreateShortUrlActionTest extends TestCase /** @test */ public function properShortcodeConversionReturnsData(): void { + $shortUrl = new ShortUrl(''); $this->urlShortener->urlToShortCode(Argument::type(Uri::class), Argument::type('array'), Argument::cetera()) - ->willReturn( - (new ShortUrl(''))->setShortCode('abc123') - ) + ->willReturn($shortUrl) ->shouldBeCalledOnce(); $request = (new ServerRequest())->withParsedBody([ @@ -57,7 +58,7 @@ class CreateShortUrlActionTest extends TestCase ]); $response = $this->action->handle($request); $this->assertEquals(200, $response->getStatusCode()); - $this->assertTrue(strpos($response->getBody()->getContents(), 'http://foo.com/abc123') > 0); + $this->assertTrue(strpos($response->getBody()->getContents(), $shortUrl->toString(self::DOMAIN_CONFIG)) > 0); } /** @test */