From 2f09ff456cf2e812dce8220bb1399b1e99ef0972 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Fri, 11 Oct 2019 09:14:25 +0200
Subject: [PATCH 1/5] Updated logic to generate random short codes, increasing
 entropy

---
 composer.json                                 |  1 +
 config/autoload/url-shortener.global.php      |  3 --
 .../Command/Config/GenerateCharsetCommand.php |  6 +--
 .../ShortUrl/GenerateShortUrlCommandTest.php  | 22 ++++----
 module/Core/src/Entity/ShortUrl.php           | 17 ++++---
 .../Core/src/Options/UrlShortenerOptions.php  | 14 -----
 module/Core/src/Service/UrlShortener.php      | 40 +--------------
 .../Repository/ShortUrlRepositoryTest.php     | 51 +++++++++----------
 .../ShortUrl/DeleteShortUrlServiceTest.php    | 36 ++++++-------
 module/Core/test/Service/UrlShortenerTest.php | 15 ++----
 .../Action/CreateShortUrlActionTest.php       |  2 +-
 .../test-api/Fixtures/ShortUrlsFixture.php    | 16 +++---
 .../ShortUrl/CreateShortUrlActionTest.php     | 17 ++++---
 13 files changed, 91 insertions(+), 149 deletions(-)

diff --git a/composer.json b/composer.json
index 3159566c..f488c757 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",
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/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('<fg=red;options=bold>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/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php
index f8ae2401..99e7c00a 100644
--- a/module/Core/src/Entity/ShortUrl.php
+++ b/module/Core/src/Entity/ShortUrl.php
@@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\Entity;
 use Cake\Chronos\Chronos;
 use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\Common\Collections\Collection;
+use PUGX\Shortid\Factory as ShortIdFactory;
 use Shlinkio\Shlink\Common\Entity\AbstractEntity;
 use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface;
 use Shlinkio\Shlink\Core\Domain\Resolver\SimpleDomainResolver;
@@ -20,6 +21,8 @@ use function Functional\invoke;
 
 class ShortUrl extends AbstractEntity
 {
+    private const BASE62 = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
+
     /** @var string */
     private $longUrl;
     /** @var string */
@@ -53,10 +56,15 @@ 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->shortCode = $meta->getCustomSlug() ?? $this->generateShortCode();
         $this->domain = ($domainResolver ?? new SimpleDomainResolver())->resolveDomain($meta->getDomain());
     }
 
+    private function generateShortCode(): string
+    {
+        return (new ShortIdFactory())->generate(6, self::BASE62)->serialize();
+    }
+
     public function getLongUrl(): string
     {
         return $this->longUrl;
@@ -67,13 +75,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;
diff --git a/module/Core/src/Options/UrlShortenerOptions.php b/module/Core/src/Options/UrlShortenerOptions.php
index 4523bc00..6816ba6e 100644
--- a/module/Core/src/Options/UrlShortenerOptions.php
+++ b/module/Core/src/Options/UrlShortenerOptions.php
@@ -8,26 +8,12 @@ use Zend\Stdlib\AbstractOptions;
 
 class UrlShortenerOptions extends AbstractOptions
 {
-    public const DEFAULT_CHARS = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
-
     // phpcs:disable
     protected $__strictMode__ = false;
     // phpcs:enable
 
-    private $shortcodeChars = self::DEFAULT_CHARS;
     private $validateUrl = true;
 
-    public function getChars(): string
-    {
-        return $this->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/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php
index 47ea6056..510f6d41 100644
--- a/module/Core/src/Service/UrlShortener.php
+++ b/module/Core/src/Service/UrlShortener.php
@@ -24,17 +24,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 */
@@ -77,16 +71,8 @@ class UrlShortener implements UrlShortenerInterface
 
             // First, create the short URL with an empty short code
             $shortUrl = new ShortUrl($url, $meta, new PersistenceDomainResolver($this->em));
-            $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->persist($shortUrl);
             $this->em->flush();
 
             $this->em->commit();
@@ -155,36 +141,12 @@ class UrlShortener implements UrlShortenerInterface
         }
     }
 
-    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);
-        }
-
-        return $chars[(int) $id] . $code;
-    }
-
     /**
      * @throws InvalidShortCodeException
      * @throws EntityDoesNotExistException
      */
     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..29c9860b 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();
@@ -174,13 +171,13 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
     /** @test */
     public function slugIsInUseLooksForShortUrlInProperSetOfTables(): 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();
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..55f2452c 100644
--- a/module/Core/test/Service/UrlShortenerTest.php
+++ b/module/Core/test/Service/UrlShortenerTest.php
@@ -17,7 +17,6 @@ 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;
@@ -74,13 +73,13 @@ 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 */
@@ -243,9 +242,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 +252,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 */

From 8f2e78c9466fd896d88a77b93a9639f6ff753c89 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Fri, 11 Oct 2019 09:35:09 +0200
Subject: [PATCH 2/5] Moved logic to generate random short codes to external
 function

---
 composer.json                       |  5 ++++-
 module/Core/functions/functions.php | 18 ++++++++++++++++++
 module/Core/src/Entity/ShortUrl.php | 11 ++---------
 3 files changed, 24 insertions(+), 10 deletions(-)
 create mode 100644 module/Core/functions/functions.php

diff --git a/composer.json b/composer.json
index f488c757..9869ac4f 100644
--- a/composer.json
+++ b/composer.json
@@ -78,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/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 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core;
+
+use PUGX\Shortid\Factory as ShortIdFactory;
+
+function generateRandomShortCode(int $length = 5): string
+{
+    static $shortIdFactory;
+    if ($shortIdFactory === null) {
+        $shortIdFactory = new ShortIdFactory();
+    }
+
+    $alphabet = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
+    return $shortIdFactory->generate($length, $alphabet)->serialize();
+}
diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php
index 99e7c00a..cebe1785 100644
--- a/module/Core/src/Entity/ShortUrl.php
+++ b/module/Core/src/Entity/ShortUrl.php
@@ -7,7 +7,6 @@ namespace Shlinkio\Shlink\Core\Entity;
 use Cake\Chronos\Chronos;
 use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\Common\Collections\Collection;
-use PUGX\Shortid\Factory as ShortIdFactory;
 use Shlinkio\Shlink\Common\Entity\AbstractEntity;
 use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface;
 use Shlinkio\Shlink\Core\Domain\Resolver\SimpleDomainResolver;
@@ -18,11 +17,10 @@ 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
 {
-    private const BASE62 = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
-
     /** @var string */
     private $longUrl;
     /** @var string */
@@ -56,15 +54,10 @@ class ShortUrl extends AbstractEntity
         $this->validSince = $meta->getValidSince();
         $this->validUntil = $meta->getValidUntil();
         $this->maxVisits = $meta->getMaxVisits();
-        $this->shortCode = $meta->getCustomSlug() ?? $this->generateShortCode();
+        $this->shortCode = $meta->getCustomSlug() ?? generateRandomShortCode();
         $this->domain = ($domainResolver ?? new SimpleDomainResolver())->resolveDomain($meta->getDomain());
     }
 
-    private function generateShortCode(): string
-    {
-        return (new ShortIdFactory())->generate(6, self::BASE62)->serialize();
-    }
-
     public function getLongUrl(): string
     {
         return $this->longUrl;

From 9538f474de1c96438984d38b409805984c190cae Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Fri, 11 Oct 2019 11:09:33 +0200
Subject: [PATCH 3/5] Added logic to check if a short code is in use and
 regenerate it otherwise

---
 module/Core/src/Entity/ShortUrl.php           | 23 +++++++++++
 .../ShortCodeCannotBeRegeneratedException.php | 29 +++++++++++++
 .../src/Repository/ShortUrlRepository.php     |  2 +-
 .../ShortUrlRepositoryInterface.php           |  2 +-
 module/Core/src/Service/UrlShortener.php      | 41 +++++++++----------
 .../Repository/ShortUrlRepositoryTest.php     | 14 +++----
 module/Core/test/Service/UrlShortenerTest.php | 21 ++++------
 7 files changed, 90 insertions(+), 42 deletions(-)
 create mode 100644 module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php

diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php
index cebe1785..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;
 
@@ -39,6 +40,8 @@ class ShortUrl extends AbstractEntity
     private $maxVisits;
     /** @var Domain|null */
     private $domain;
+    /** @var bool */
+    private $customSlugWasProvided;
 
     public function __construct(
         string $longUrl,
@@ -54,6 +57,7 @@ class ShortUrl extends AbstractEntity
         $this->validSince = $meta->getValidSince();
         $this->validUntil = $meta->getValidUntil();
         $this->maxVisits = $meta->getMaxVisits();
+        $this->customSlugWasProvided = $meta->hasCustomSlug();
         $this->shortCode = $meta->getCustomSlug() ?? generateRandomShortCode();
         $this->domain = ($domainResolver ?? new SimpleDomainResolver())->resolveDomain($meta->getDomain());
     }
@@ -103,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..e1df3068
--- /dev/null
+++ b/module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php
@@ -0,0 +1,29 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Exception;
+
+class ShortCodeCannotBeRegeneratedException extends RuntimeException
+{
+    /** @var @bool */
+    private $reasonIsSlug = false;
+
+    public static function forShortUrlWithCustomSlug(): self
+    {
+        $e = new self('The short code cannot be regenerated on ShortUrls where a custom slug was provided.');
+        $e->reasonIsSlug = true;
+
+        return $e;
+    }
+
+    public static function forShortUrlAlreadyPersisted(): self
+    {
+        return new self('The short code can be regenerated only on new ShortUrls which have not been persisted yet.');
+    }
+
+    public function reasonIsSlug(): bool
+    {
+        return $this->reasonIsSlug;
+    }
+}
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 510f6d41..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;
@@ -47,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
     {
@@ -63,28 +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));
-            $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags));
+            $this->verifyShortCodeUniqueness($meta, $shortUrl);
             $this->em->persist($shortUrl);
             $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
@@ -124,20 +121,22 @@ 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);
+
+        if ($otherShortUrlsExist && $meta->hasCustomSlug()) {
+            throw NonUniqueSlugException::fromSlug($shortCode, $domain);
+        }
+
+        if ($otherShortUrlsExist) {
+            $shortUrlToBeCreated->regenerateShortCode();
+            $this->verifyShortCodeUniqueness($meta, $shortUrlToBeCreated);
         }
     }
 
diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php
index 29c9860b..f3913ea2 100644
--- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php
+++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php
@@ -169,7 +169,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
     }
 
     /** @test */
-    public function slugIsInUseLooksForShortUrlInProperSetOfTables(): void
+    public function shortCodeIsInUseLooksForShortUrlInProperSetOfTables(): void
     {
         $shortUrlWithoutDomain = new ShortUrl('foo', ShortUrlMeta::createFromRawData(['customSlug' => 'my-cool-slug']));
         $this->getEntityManager()->persist($shortUrlWithoutDomain);
@@ -182,11 +182,11 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
 
         $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/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php
index 55f2452c..0185ef1a 100644
--- a/module/Core/test/Service/UrlShortenerTest.php
+++ b/module/Core/test/Service/UrlShortenerTest.php
@@ -19,7 +19,6 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\Entity\Tag;
 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;
@@ -55,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);
@@ -83,7 +82,7 @@ class UrlShortenerTest extends TestCase
     }
 
     /** @test */
-    public function exceptionIsThrownWhenOrmThrowsException(): void
+    public function transactionIsRolledBackAndExceptionRethrownWhenExceptionIsThrown(): void
     {
         $conn = $this->prophesize(Connection::class);
         $conn->isTransactionActive()->willReturn(true);
@@ -93,7 +92,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'),
             [],
@@ -122,11 +121,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);
 
@@ -145,26 +144,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']

From 5bd7b53e0acfd26b20b2df44aae2d8b4638db308 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Fri, 11 Oct 2019 11:28:53 +0200
Subject: [PATCH 4/5] Added more tests for new logics

---
 .../ShortCodeCannotBeRegeneratedException.php | 13 +----
 module/Core/test/Entity/ShortUrlTest.php      | 51 +++++++++++++++++++
 module/Core/test/Service/UrlShortenerTest.php | 32 ++++++++++++
 3 files changed, 84 insertions(+), 12 deletions(-)
 create mode 100644 module/Core/test/Entity/ShortUrlTest.php

diff --git a/module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php b/module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php
index e1df3068..fad5805d 100644
--- a/module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php
+++ b/module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php
@@ -6,24 +6,13 @@ namespace Shlinkio\Shlink\Core\Exception;
 
 class ShortCodeCannotBeRegeneratedException extends RuntimeException
 {
-    /** @var @bool */
-    private $reasonIsSlug = false;
-
     public static function forShortUrlWithCustomSlug(): self
     {
-        $e = new self('The short code cannot be regenerated on ShortUrls where a custom slug was provided.');
-        $e->reasonIsSlug = true;
-
-        return $e;
+        return new self('The short code cannot be regenerated on ShortUrls where a custom slug was provided.');
     }
 
     public static function forShortUrlAlreadyPersisted(): self
     {
         return new self('The short code can be regenerated only on new ShortUrls which have not been persisted yet.');
     }
-
-    public function reasonIsSlug(): bool
-    {
-        return $this->reasonIsSlug;
-    }
 }
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 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\Entity;
+
+use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\Core\Entity\ShortUrl;
+use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException;
+use Shlinkio\Shlink\Core\Model\ShortUrlMeta;
+
+class ShortUrlTest extends TestCase
+{
+    /**
+     * @test
+     * @dataProvider provideInvalidShortUrls
+     */
+    public function regenerateShortCodeThrowsExceptionIfStateIsInvalid(
+        ShortUrl $shortUrl,
+        string $expectedMessage
+    ): void {
+        $this->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/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php
index 0185ef1a..78e61a6a 100644
--- a/module/Core/test/Service/UrlShortenerTest.php
+++ b/module/Core/test/Service/UrlShortenerTest.php
@@ -81,6 +81,32 @@ class UrlShortenerTest extends TestCase
         $this->assertEquals('http://foobar.com/12345/hello?foo=bar', $shortUrl->getLongUrl());
     }
 
+    /** @test */
+    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
     {
@@ -190,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'],

From 740a65f88043742bbc1933c5e77dfd1ad5a3eb52 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Fri, 11 Oct 2019 11:41:14 +0200
Subject: [PATCH 5/5] Updated references to SHORTCODE_CHARS in docker docs

---
 .env.dist                                |  1 -
 CHANGELOG.md                             |  6 +++++-
 docker/README.md                         | 17 +++++++++--------
 docker/config/shlink_in_docker.local.php | 21 ++++-----------------
 4 files changed, 18 insertions(+), 27 deletions(-)

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/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(),
     ],