From b1a073b1abd7c6232b9aecaba44968caf48e348d Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Sun, 25 Oct 2020 10:26:11 +0100
Subject: [PATCH] Ensured uniqueness on imported short URLs short code

---
 module/Core/src/Entity/ShortUrl.php           |  4 +-
 .../src/Importer/ImportedLinksProcessor.php   | 62 +++++++++++++++++--
 module/Core/test/Entity/ShortUrlTest.php      | 18 +++++-
 3 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php
index aba0235f..ea56fc82 100644
--- a/module/Core/src/Entity/ShortUrl.php
+++ b/module/Core/src/Entity/ShortUrl.php
@@ -135,8 +135,8 @@ class ShortUrl extends AbstractEntity
      */
     public function regenerateShortCode(): self
     {
-        // In ShortUrls where a custom slug was provided, do nothing
-        if ($this->customSlugWasProvided) {
+        // In ShortUrls where a custom slug was provided, throw error, unless it is an imported one
+        if ($this->customSlugWasProvided && $this->importSource === null) {
             throw ShortCodeCannotBeRegeneratedException::forShortUrlWithCustomSlug();
         }
 
diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php
index 81b8e923..4dc8d1a4 100644
--- a/module/Core/src/Importer/ImportedLinksProcessor.php
+++ b/module/Core/src/Importer/ImportedLinksProcessor.php
@@ -7,12 +7,14 @@ 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\Util\DoctrineBatchIterator;
 use Shlinkio\Shlink\Core\Util\TagManagerTrait;
 use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface;
 use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl;
 use Symfony\Component\Console\Style\StyleInterface;
+
 use function sprintf;
 
 class ImportedLinksProcessor implements ImportedLinksProcessorInterface
@@ -40,21 +42,71 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface
 
         /** @var ImportedShlinkUrl $url */
         foreach ($iterable as $url) {
+            $longUrl = $url->longUrl();
+
             // Skip already imported URLs
             if ($shortUrlRepo->importedUrlExists($url, $importShortCodes)) {
-                $io->text(sprintf('%s: <comment>Skipped</comment>', $url->longUrl()));
+                $io->text(sprintf('%s: <comment>Skipped</comment>', $longUrl));
                 continue;
             }
 
             $shortUrl = ShortUrl::fromImport($url, $importShortCodes, $this->domainResolver);
             $shortUrl->setTags($this->tagNamesToEntities($this->em, $url->tags()));
 
+            if (! $this->handleShortcodeUniqueness($url, $shortUrl, $io, $importShortCodes)) {
+                continue;
+            }
 
-            // TODO Handle errors while creating short URLs, to avoid making the whole process fail
-            //        * Duplicated short code
             $this->em->persist($shortUrl);
-
-            $io->text(sprintf('%s: <info>Imported</info>', $url->longUrl()));
+            $io->text(sprintf('%s: <info>Imported</info>', $longUrl));
         }
     }
+
+    private function handleShortcodeUniqueness(
+        ImportedShlinkUrl $url,
+        ShortUrl $shortUrl,
+        StyleInterface $io,
+        bool $importShortCodes
+    ): bool {
+        if ($this->ensureShortCodeUniqueness($shortUrl, $importShortCodes)) {
+            return true;
+        }
+
+        $longUrl = $url->longUrl();
+        $action = $io->choice(sprintf(
+            'Failed to import URL "%s" because its short-code "%s" is already in use. Do you want to generate a new '
+            . 'one or skip it?',
+            $longUrl,
+            $url->shortCode(),
+        ), ['Generate new short-code', 'Skip'], 1);
+
+        if ($action === 'Skip') {
+            $io->text(sprintf('%s: <comment>Skipped</comment>', $longUrl));
+            return false;
+        }
+
+        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);
+    }
 }
diff --git a/module/Core/test/Entity/ShortUrlTest.php b/module/Core/test/Entity/ShortUrlTest.php
index 054182ff..c143ae76 100644
--- a/module/Core/test/Entity/ShortUrlTest.php
+++ b/module/Core/test/Entity/ShortUrlTest.php
@@ -4,12 +4,14 @@ declare(strict_types=1);
 
 namespace ShlinkioTest\Shlink\Core\Entity;
 
+use Cake\Chronos\Chronos;
 use PHPUnit\Framework\TestCase;
 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;
@@ -44,10 +46,12 @@ class ShortUrlTest extends TestCase
         ];
     }
 
-    /** @test */
-    public function regenerateShortCodeProperlyChangesTheValueOnValidShortUrls(): void
+    /**
+     * @test
+     * @dataProvider provideValidShortUrls
+     */
+    public function regenerateShortCodeProperlyChangesTheValueOnValidShortUrls(ShortUrl $shortUrl): void
     {
-        $shortUrl = new ShortUrl('');
         $firstShortCode = $shortUrl->getShortCode();
 
         $shortUrl->regenerateShortCode();
@@ -56,6 +60,14 @@ class ShortUrlTest extends TestCase
         self::assertNotEquals($firstShortCode, $secondShortCode);
     }
 
+    public function provideValidShortUrls(): iterable
+    {
+        yield 'no custom slug' => [new ShortUrl('')];
+        yield 'imported with custom slug' => [
+            ShortUrl::fromImport(new ImportedShlinkUrl('', '', [], Chronos::now(), null, 'custom-slug'), true),
+        ];
+    }
+
     /**
      * @test
      * @dataProvider provideLengths