Added logic to check if a short code is in use and regenerate it otherwise

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;

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;
+    }
+}

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)')

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;
 }

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();
+            $this->verifyShortCodeUniqueness($meta, $shortUrl);
-
-            // 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->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()) {
+        $shortCode = $shortUrlToBeCreated->getShortCode();
-            return;
-        }
-
-        $customSlug = $meta->getCustomSlug();
         $domain = $meta->getDomain();
 
         /** @var ShortUrlRepository $repo */
         $repo = $this->em->getRepository(ShortUrl::class);
-        $shortUrlsCount = $repo->slugIsInUse($customSlug, $domain);
+        $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domain);
-        if ($shortUrlsCount > 0) {
+
-            throw NonUniqueSlugException::fromSlug($customSlug, $domain);
+        if ($otherShortUrlsExist && $meta->hasCustomSlug()) {
+            throw NonUniqueSlugException::fromSlug($shortCode, $domain);
+        }
+
+        if ($otherShortUrlsExist) {
+            $shortUrlToBeCreated->regenerateShortCode();
+            $this->verifyShortCodeUniqueness($meta, $shortUrlToBeCreated);
         }
     }
 

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->assertTrue($this->repo->shortCodeIsInUse('my-cool-slug'));
-        $this->assertFalse($this->repo->slugIsInUse('my-cool-slug', 'doma.in'));
+        $this->assertFalse($this->repo->shortCodeIsInUse('my-cool-slug', 'doma.in'));
-        $this->assertFalse($this->repo->slugIsInUse('slug-not-in-use'));
+        $this->assertFalse($this->repo->shortCodeIsInUse('slug-not-in-use'));
-        $this->assertFalse($this->repo->slugIsInUse('another-slug'));
+        $this->assertFalse($this->repo->shortCodeIsInUse('another-slug'));
-        $this->assertFalse($this->repo->slugIsInUse('another-slug', 'example.com'));
+        $this->assertFalse($this->repo->shortCodeIsInUse('another-slug', 'example.com'));
-        $this->assertTrue($this->repo->slugIsInUse('another-slug', 'doma.in'));
+        $this->assertTrue($this->repo->shortCodeIsInUse('another-slug', 'doma.in'));
     }
 }

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->em->persist(Argument::cetera())->shouldNotHaveBeenCalled();
-            $this->assertSame($expected, $result);
+        $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']