Merge pull request #1424 from acelaya-forks/feature/skip-invalid-imports

Added errorhandling for individual imported URLs, so that one failing…

CHANGELOG.md

@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 * [#1294](https://github.com/shlinkio/shlink/issues/1294) Allowed to specify a specific domain when importing URLs from YOURLS.
 * [#1416](https://github.com/shlinkio/shlink/issues/1416) Added support to import URLs from Kutt.it.
 * [#1418](https://github.com/shlinkio/shlink/issues/1418) Added support to customize the timezone used by Shlink, falling back to the default one set in PHP config.
+* [#1309](https://github.com/shlinkio/shlink/issues/1309) Improved URL importing, ensuring individual errors do not make the whole process fail, and instead, failing URLs are skipped.
 
 ### Changed
 * [#1359](https://github.com/shlinkio/shlink/issues/1359) Hidden database commands.

module/Core/src/Importer/ImportedLinksProcessor.php

@@ -15,7 +15,9 @@ use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface;
 use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl;
 use Shlinkio\Shlink\Importer\Params\ImportParams;
 use Shlinkio\Shlink\Importer\Sources\ImportSources;
+use Symfony\Component\Console\Style\OutputStyle;
 use Symfony\Component\Console\Style\StyleInterface;
+use Throwable;
 
 use function sprintf;
 
@@ -33,7 +35,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface
     }
 
     /**
-     * @param iterable|ImportedShlinkUrl[] $shlinkUrls
+     * @param iterable<ImportedShlinkUrl> $shlinkUrls
      */
     public function process(StyleInterface $io, iterable $shlinkUrls, ImportParams $params): void
     {
@@ -43,22 +45,26 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface
 
         /** @var ImportedShlinkUrl $importedUrl */
         foreach ($iterable as $importedUrl) {
-            $skipOnShortCodeConflict = static function () use ($io, $importedUrl): bool {
+            $skipOnShortCodeConflict = static fn (): bool => $io->choice(sprintf(
-                $action = $io->choice(sprintf(
+                'Failed to import URL "%s" because its short-code "%s" is already in use. Do you want to generate '
-                    '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?',
-                    . 'a new one or skip it?',
+                $importedUrl->longUrl(),
-                    $importedUrl->longUrl(),
+                $importedUrl->shortCode(),
-                    $importedUrl->shortCode(),
+            ), ['Generate new short-code', 'Skip'], 1) === 'Skip';
-                ), ['Generate new short-code', 'Skip'], 1);
-
-                return $action === 'Skip';
-            };
             $longUrl = $importedUrl->longUrl();
 
             try {
                 $shortUrlImporting = $this->resolveShortUrl($importedUrl, $importShortCodes, $skipOnShortCodeConflict);
             } catch (NonUniqueSlugException) {
                 $io->text(sprintf('%s: <fg=red>Error</>', $longUrl));
+                continue;
+            } catch (Throwable $e) {
+                $io->text(sprintf('%s: <comment>Skipped</comment>. Reason: %s.', $longUrl, $e->getMessage()));
+
+                if ($io instanceof OutputStyle && $io->isVerbose()) {
+                    $io->text($e->__toString());
+                }
+
                 continue;
             }
 

module/Core/src/Importer/ShortUrlImporting.php

@@ -29,7 +29,7 @@ final class ShortUrlImporting
     }
 
     /**
-     * @param iterable|ImportedShlinkVisit[] $visits
+     * @param iterable<ImportedShlinkVisit> $visits
      */
     public function importVisits(iterable $visits, EntityManagerInterface $em): string
     {

module/Core/test/Importer/ImportedLinksProcessorTest.php

@@ -11,6 +11,7 @@ use PHPUnit\Framework\TestCase;
 use Prophecy\Argument;
 use Prophecy\PhpUnit\ProphecyTrait;
 use Prophecy\Prophecy\ObjectProphecy;
+use RuntimeException;
 use Shlinkio\Shlink\Core\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Importer\ImportedLinksProcessor;
@@ -81,6 +82,37 @@ class ImportedLinksProcessorTest extends TestCase
         $this->io->text(Argument::type('string'))->shouldHaveBeenCalledTimes($expectedCalls);
     }
 
+    /** @test */
+    public function newUrlsWithErrorsAreSkipped(): void
+    {
+        $urls = [
+            new ImportedShlinkUrl('', 'foo', [], Chronos::now(), null, 'foo', null),
+            new ImportedShlinkUrl('', 'bar', [], Chronos::now(), null, 'bar', 'foo'),
+            new ImportedShlinkUrl('', 'baz', [], Chronos::now(), null, 'baz', null),
+        ];
+
+        $importedUrlExists = $this->repo->findOneByImportedUrl(Argument::cetera())->willReturn(null);
+        $ensureUniqueness = $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(true);
+        $persist = $this->em->persist(Argument::type(ShortUrl::class))->will(function (array $args): void {
+            /** @var ShortUrl $shortUrl */
+            [$shortUrl] = $args;
+
+            if ($shortUrl->getShortCode() === 'baz') {
+                throw new RuntimeException('Whatever error');
+            }
+        });
+
+        $this->processor->process($this->io->reveal(), $urls, $this->buildParams());
+
+        $importedUrlExists->shouldHaveBeenCalledTimes(3);
+        $ensureUniqueness->shouldHaveBeenCalledTimes(3);
+        $persist->shouldHaveBeenCalledTimes(3);
+        $this->io->text(Argument::containingString('<info>Imported</info>'))->shouldHaveBeenCalledTimes(2);
+        $this->io->text(
+            Argument::containingString('<comment>Skipped</comment>. Reason: Whatever error'),
+        )->shouldHaveBeenCalledOnce();
+    }
+
     /** @test */
     public function alreadyImportedUrlsAreSkipped(): void
     {