Moved short code uniqueness checks to external helper class that is used in UrlShortener and ImportedLinksProcessor

module/Core/config/dependencies.config.php

@@ -32,6 +32,7 @@ return [
             Tag\TagService::class => ConfigAbstractFactory::class,
             Service\ShortUrl\DeleteShortUrlService::class => ConfigAbstractFactory::class,
             Service\ShortUrl\ShortUrlResolver::class => ConfigAbstractFactory::class,
+            Service\ShortUrl\ShortCodeHelper::class => ConfigAbstractFactory::class,
             Domain\DomainService::class => ConfigAbstractFactory::class,
 
             Util\UrlValidator::class => ConfigAbstractFactory::class,
@@ -61,7 +62,12 @@ return [
         Options\NotFoundRedirectOptions::class => ['config.not_found_redirects'],
         Options\UrlShortenerOptions::class => ['config.url_shortener'],
 
-        Service\UrlShortener::class => [Util\UrlValidator::class, 'em', Resolver\PersistenceDomainResolver::class],
+        Service\UrlShortener::class => [
+            Util\UrlValidator::class,
+            'em',
+            Resolver\PersistenceDomainResolver::class,
+            Service\ShortUrl\ShortCodeHelper::class,
+        ],
         Service\VisitsTracker::class => [
             'em',
             EventDispatcherInterface::class,
@@ -77,6 +83,7 @@ return [
             Service\ShortUrl\ShortUrlResolver::class,
         ],
         Service\ShortUrl\ShortUrlResolver::class => ['em'],
+        Service\ShortUrl\ShortCodeHelper::class => ['em'],
         Domain\DomainService::class => ['em'],
 
         Util\UrlValidator::class => ['httpClient', Options\UrlShortenerOptions::class],
@@ -104,7 +111,11 @@ return [
 
         Mercure\MercureUpdatesGenerator::class => ['config.url_shortener.domain'],
 
-        Importer\ImportedLinksProcessor::class => ['em', Resolver\PersistenceDomainResolver::class],
+        Importer\ImportedLinksProcessor::class => [
+            'em',
+            Resolver\PersistenceDomainResolver::class,
+            Service\ShortUrl\ShortCodeHelper::class,
+        ],
     ],
 
 ];

module/Core/src/Entity/ShortUrl.php

@@ -133,7 +133,7 @@ class ShortUrl extends AbstractEntity
     /**
      * @throws ShortCodeCannotBeRegeneratedException
      */
-    public function regenerateShortCode(): self
+    public function regenerateShortCode(): void
     {
         // In ShortUrls where a custom slug was provided, throw error, unless it is an imported one
         if ($this->customSlugWasProvided && $this->importSource === null) {
@@ -146,7 +146,6 @@ class ShortUrl extends AbstractEntity
         }
 
         $this->shortCode = generateRandomShortCode($this->shortCodeLength);
-        return $this;
     }
 
     public function getValidSince(): ?Chronos

module/Core/src/Importer/ImportedLinksProcessor.php

@@ -7,8 +7,8 @@ namespace Shlinkio\Shlink\Core\Importer;
 use Doctrine\ORM\EntityManagerInterface;
 use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface;
 use Shlinkio\Shlink\Core\Entity\ShortUrl;
-use Shlinkio\Shlink\Core\Repository\ShortUrlRepository;
 use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface;
+use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface;
 use Shlinkio\Shlink\Core\Util\DoctrineBatchIterator;
 use Shlinkio\Shlink\Core\Util\TagManagerTrait;
 use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface;
@@ -23,11 +23,16 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface
 
     private EntityManagerInterface $em;
     private DomainResolverInterface $domainResolver;
+    private ShortCodeHelperInterface $shortCodeHelper;
 
-    public function __construct(EntityManagerInterface $em, DomainResolverInterface $domainResolver)
+    public function __construct(
-    {
+        EntityManagerInterface $em,
+        DomainResolverInterface $domainResolver,
+        ShortCodeHelperInterface $shortCodeHelper
+    ) {
         $this->em = $em;
         $this->domainResolver = $domainResolver;
+        $this->shortCodeHelper = $shortCodeHelper;
     }
 
     /**
@@ -68,7 +73,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface
         StyleInterface $io,
         bool $importShortCodes
     ): bool {
-        if ($this->ensureShortCodeUniqueness($shortUrl, $importShortCodes)) {
+        if ($this->shortCodeHelper->ensureShortCodeUniqueness($shortUrl, $importShortCodes)) {
             return true;
         }
 
@@ -87,26 +92,4 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface
 
         return $this->handleShortcodeUniqueness($url, $shortUrl, $io, false);
     }
-
-    private function ensureShortCodeUniqueness(ShortUrl $shortUrlToBeCreated, bool $hasCustomSlug): bool
-    {
-        $shortCode = $shortUrlToBeCreated->getShortCode();
-        $domain = $shortUrlToBeCreated->getDomain();
-        $domainAuthority = $domain !== null ? $domain->getAuthority() : null;
-
-        /** @var ShortUrlRepository $repo */
-        $repo = $this->em->getRepository(ShortUrl::class);
-        $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domainAuthority);
-
-        if (! $otherShortUrlsExist) {
-            return true;
-        }
-
-        if ($hasCustomSlug) {
-            return false;
-        }
-
-        $shortUrlToBeCreated->regenerateShortCode();
-        return $this->ensureShortCodeUniqueness($shortUrlToBeCreated, $hasCustomSlug);
-    }
 }

module/Core/src/Service/ShortUrl/ShortCodeHelper.php

@@ -0,0 +1,41 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Service\ShortUrl;
+
+use Doctrine\ORM\EntityManagerInterface;
+use Shlinkio\Shlink\Core\Entity\ShortUrl;
+use Shlinkio\Shlink\Core\Repository\ShortUrlRepository;
+
+class ShortCodeHelper implements ShortCodeHelperInterface
+{
+    private EntityManagerInterface $em;
+
+    public function __construct(EntityManagerInterface $em)
+    {
+        $this->em = $em;
+    }
+
+    public function ensureShortCodeUniqueness(ShortUrl $shortUrlToBeCreated, bool $hasCustomSlug): bool
+    {
+        $shortCode = $shortUrlToBeCreated->getShortCode();
+        $domain = $shortUrlToBeCreated->getDomain();
+        $domainAuthority = $domain !== null ? $domain->getAuthority() : null;
+
+        /** @var ShortUrlRepository $repo */
+        $repo = $this->em->getRepository(ShortUrl::class);
+        $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domainAuthority);
+
+        if (! $otherShortUrlsExist) {
+            return true;
+        }
+
+        if ($hasCustomSlug) {
+            return false;
+        }
+
+        $shortUrlToBeCreated->regenerateShortCode();
+        return $this->ensureShortCodeUniqueness($shortUrlToBeCreated, $hasCustomSlug);
+    }
+}

module/Core/src/Service/ShortUrl/ShortCodeHelperInterface.php

@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Service\ShortUrl;
+
+use Shlinkio\Shlink\Core\Entity\ShortUrl;
+
+interface ShortCodeHelperInterface
+{
+    public function ensureShortCodeUniqueness(ShortUrl $shortUrlToBeCreated, bool $hasCustomSlug): bool;
+}

module/Core/src/Service/UrlShortener.php

@@ -10,8 +10,8 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
 use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException;
 use Shlinkio\Shlink\Core\Model\ShortUrlMeta;
-use Shlinkio\Shlink\Core\Repository\ShortUrlRepository;
 use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface;
+use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface;
 use Shlinkio\Shlink\Core\Util\TagManagerTrait;
 use Shlinkio\Shlink\Core\Util\UrlValidatorInterface;
 use Throwable;
@@ -23,15 +23,18 @@ class UrlShortener implements UrlShortenerInterface
     private EntityManagerInterface $em;
     private UrlValidatorInterface $urlValidator;
     private DomainResolverInterface $domainResolver;
+    private ShortCodeHelperInterface $shortCodeHelper;
 
     public function __construct(
         UrlValidatorInterface $urlValidator,
         EntityManagerInterface $em,
-        DomainResolverInterface $domainResolver
+        DomainResolverInterface $domainResolver,
+        ShortCodeHelperInterface $shortCodeHelper
     ) {
         $this->urlValidator = $urlValidator;
         $this->em = $em;
         $this->domainResolver = $domainResolver;
+        $this->shortCodeHelper = $shortCodeHelper;
     }
 
     /**
@@ -83,20 +86,16 @@ class UrlShortener implements UrlShortenerInterface
 
     private function verifyShortCodeUniqueness(ShortUrlMeta $meta, ShortUrl $shortUrlToBeCreated): void
     {
-        $shortCode = $shortUrlToBeCreated->getShortCode();
+        $couldBeMadeUnique = $this->shortCodeHelper->ensureShortCodeUniqueness(
-        $domain = $meta->getDomain();
+            $shortUrlToBeCreated,
+            $meta->hasCustomSlug(),
+        );
 
-        /** @var ShortUrlRepository $repo */
+        if (! $couldBeMadeUnique) {
-        $repo = $this->em->getRepository(ShortUrl::class);
+            $domain = $shortUrlToBeCreated->getDomain();
-        $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domain);
+            $domainAuthority = $domain !== null ? $domain->getAuthority() : null;
 
-        if ($otherShortUrlsExist && $meta->hasCustomSlug()) {
+            throw NonUniqueSlugException::fromSlug($shortUrlToBeCreated->getShortCode(), $domainAuthority);
-            throw NonUniqueSlugException::fromSlug($shortCode, $domain);
-        }
-
-        if ($otherShortUrlsExist) {
-            $shortUrlToBeCreated->regenerateShortCode();
-            $this->verifyShortCodeUniqueness($meta, $shortUrlToBeCreated);
         }
     }
 }

module/Core/test/Entity/ShortUrlTest.php

@@ -10,8 +10,8 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException;
 use Shlinkio\Shlink\Core\Model\ShortUrlMeta;
 use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter;
-
 use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl;
+
 use function Functional\map;
 use function range;
 use function strlen;

module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php

@@ -0,0 +1,77 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\Service\ShortUrl;
+
+use Doctrine\ORM\EntityManagerInterface;
+use PHPUnit\Framework\TestCase;
+use Prophecy\Prophecy\ObjectProphecy;
+use Shlinkio\Shlink\Core\Entity\Domain;
+use Shlinkio\Shlink\Core\Entity\ShortUrl;
+use Shlinkio\Shlink\Core\Repository\ShortUrlRepository;
+use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelper;
+
+class ShortCodeHelperTest extends TestCase
+{
+    private ShortCodeHelper $helper;
+    private ObjectProphecy $em;
+    private ObjectProphecy $shortUrl;
+
+    protected function setUp(): void
+    {
+        $this->em = $this->prophesize(EntityManagerInterface::class);
+        $this->helper = new ShortCodeHelper($this->em->reveal());
+
+        $this->shortUrl = $this->prophesize(ShortUrl::class);
+        $this->shortUrl->getShortCode()->willReturn('abc123');
+    }
+
+    /**
+     * @test
+     * @dataProvider provideDomains
+     */
+    public function shortCodeIsRegeneratedIfAlreadyInUse(?Domain $domain, ?string $expectedAuthority): void
+    {
+        $callIndex = 0;
+        $expectedCalls = 3;
+        $repo = $this->prophesize(ShortUrlRepository::class);
+        $shortCodeIsInUse = $repo->shortCodeIsInUse('abc123', $expectedAuthority)->will(
+            function () use (&$callIndex, $expectedCalls) {
+                $callIndex++;
+                return $callIndex < $expectedCalls;
+            },
+        );
+        $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal());
+        $this->shortUrl->getDomain()->willReturn($domain);
+
+        $result = $this->helper->ensureShortCodeUniqueness($this->shortUrl->reveal(), false);
+
+        self::assertTrue($result);
+        $this->shortUrl->regenerateShortCode()->shouldHaveBeenCalledTimes($expectedCalls - 1);
+        $getRepo->shouldBeCalledTimes($expectedCalls);
+        $shortCodeIsInUse->shouldBeCalledTimes($expectedCalls);
+    }
+
+    public function provideDomains(): iterable
+    {
+        yield 'no domain' => [null, null];
+        yield 'domain' => [new Domain($authority = 'doma.in'), $authority];
+    }
+
+    /** @test */
+    public function inUseSlugReturnsError(): void
+    {
+        $repo = $this->prophesize(ShortUrlRepository::class);
+        $shortCodeIsInUse = $repo->shortCodeIsInUse('abc123', null)->willReturn(true);
+        $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal());
+        $this->shortUrl->getDomain()->willReturn(null);
+
+        $result = $this->helper->ensureShortCodeUniqueness($this->shortUrl->reveal(), true);
+
+        self::assertFalse($result);
+        $this->shortUrl->regenerateShortCode()->shouldNotHaveBeenCalled();
+        $getRepo->shouldBeCalledOnce();
+        $shortCodeIsInUse->shouldBeCalledOnce();
+    }
+}

module/Core/test/Service/UrlShortenerTest.php

@@ -18,6 +18,7 @@ use Shlinkio\Shlink\Core\Entity\Tag;
 use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException;
 use Shlinkio\Shlink\Core\Model\ShortUrlMeta;
 use Shlinkio\Shlink\Core\Repository\ShortUrlRepository;
+use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface;
 use Shlinkio\Shlink\Core\Service\UrlShortener;
 use Shlinkio\Shlink\Core\Util\UrlValidatorInterface;
 
@@ -26,6 +27,7 @@ class UrlShortenerTest extends TestCase
     private UrlShortener $urlShortener;
     private ObjectProphecy $em;
     private ObjectProphecy $urlValidator;
+    private ObjectProphecy $shortCodeHelper;
 
     public function setUp(): void
     {
@@ -51,10 +53,14 @@ class UrlShortenerTest extends TestCase
         $repo->shortCodeIsInUse(Argument::cetera())->willReturn(false);
         $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal());
 
+        $this->shortCodeHelper = $this->prophesize(ShortCodeHelperInterface::class);
+        $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(true);
+
         $this->urlShortener = new UrlShortener(
             $this->urlValidator->reveal(),
             $this->em->reveal(),
             new SimpleDomainResolver(),
+            $this->shortCodeHelper->reveal(),
         );
     }
 
@@ -71,29 +77,18 @@ class UrlShortenerTest extends TestCase
     }
 
     /** @test */
-    public function shortCodeIsRegeneratedIfAlreadyInUse(): void
+    public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void
     {
-        $callIndex = 0;
+        $ensureUniqueness = $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(false);
-        $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(
+        $ensureUniqueness->shouldBeCalledOnce();
+        $this->expectException(NonUniqueSlugException::class);
+
+        $this->urlShortener->urlToShortCode(
             'http://foobar.com/12345/hello?foo=bar',
             [],
-            ShortUrlMeta::createEmpty(),
+            ShortUrlMeta::fromRawData(['customSlug' => 'custom-slug']),
         );
-
-        self::assertEquals('http://foobar.com/12345/hello?foo=bar', $shortUrl->getLongUrl());
-        $getRepo->shouldBeCalledTimes($expectedCalls);
-        $shortCodeIsInUse->shouldBeCalledTimes($expectedCalls);
     }
 
     /** @test */
@@ -115,25 +110,6 @@ class UrlShortenerTest extends TestCase
         );
     }
 
-    /** @test */
-    public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void
-    {
-        $repo = $this->prophesize(ShortUrlRepository::class);
-        $shortCodeIsInUse = $repo->shortCodeIsInUse('custom-slug', null)->willReturn(true);
-        $repo->findBy(Argument::cetera())->willReturn([]);
-        $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal());
-
-        $shortCodeIsInUse->shouldBeCalledOnce();
-        $getRepo->shouldBeCalled();
-        $this->expectException(NonUniqueSlugException::class);
-
-        $this->urlShortener->urlToShortCode(
-            'http://foobar.com/12345/hello?foo=bar',
-            [],
-            ShortUrlMeta::fromRawData(['customSlug' => 'custom-slug']),
-        );
-    }
-
     /**
      * @test
      * @dataProvider provideExistingShortUrls