Merge pull request #1761 from acelaya-forks/feature/null-default-domain

Feature/null default domain

CHANGELOG.md

@@ -18,7 +18,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 * *Nothing*
 
 ### Fixed
-* *Nothing*
+* [#1760](https://github.com/shlinkio/shlink/issues/1760) Fix domain not being set to null when importing short URLs with default domain.
+* Fix Shlink trying to connect to RabbitMQ even if configuration set to not connect.
 
 
 ## [3.5.4] - 2023-04-12

composer.json

@@ -45,7 +45,7 @@
         "php-middleware/request-id": "^4.1",
         "pugx/shortid-php": "^1.1",
         "ramsey/uuid": "^4.7",
-        "shlinkio/shlink-common": "dev-main#9eecf8c as 5.5",
+        "shlinkio/shlink-common": "dev-main#29dd933 as 5.5",
         "shlinkio/shlink-config": "^2.4",
         "shlinkio/shlink-event-dispatcher": "dev-main#8c677ae as 3.0",
         "shlinkio/shlink-importer": "dev-main#6b63b12 as 5.1",

module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php

@@ -31,7 +31,6 @@ class CreateShortUrlCommand extends Command
     public const NAME = 'short-url:create';
 
     private ?SymfonyStyle $io;
-    private string $defaultDomain;
 
     public function __construct(
         private readonly UrlShortenerInterface $urlShortener,
@@ -39,7 +38,6 @@ class CreateShortUrlCommand extends Command
         private readonly UrlShortenerOptions $options,
     ) {
         parent::__construct();
-        $this->defaultDomain = $this->options->domain['hostname'] ?? '';
     }
 
     protected function configure(): void
@@ -121,7 +119,6 @@ class CreateShortUrlCommand extends Command
     protected function interact(InputInterface $input, OutputInterface $output): void
     {
         $this->verifyLongUrlArgument($input, $output);
-        $this->verifyDomainArgument($input);
     }
 
     private function verifyLongUrlArgument(InputInterface $input, OutputInterface $output): void
@@ -138,12 +135,6 @@ class CreateShortUrlCommand extends Command
         }
     }
 
-    private function verifyDomainArgument(InputInterface $input): void
-    {
-        $domain = $input->getOption('domain');
-        $input->setOption('domain', $domain === $this->defaultDomain ? null : $domain);
-    }
-
     protected function execute(InputInterface $input, OutputInterface $output): ?int
     {
         $io = $this->getIO($input, $output);

module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php

@@ -102,6 +102,12 @@ class ListShortUrlsCommand extends Command
                 InputOption::VALUE_NONE,
                 'Whether to display the tags or not.',
             )
+            ->addOption(
+                'show-domain',
+                null,
+                InputOption::VALUE_NONE,
+                'Whether to display the domain or not. Those belonging to default domain will have value "DEFAULT".',
+            )
             ->addOption(
                 'show-api-key',
                 'k',
@@ -217,6 +223,10 @@ class ListShortUrlsCommand extends Command
         if ($input->getOption('show-tags')) {
             $columnsMap['Tags'] = static fn (array $shortUrl): string => implode(', ', $shortUrl['tags']);
         }
+        if ($input->getOption('show-domain')) {
+            $columnsMap['Domain'] = static fn (array $_, ShortUrl $shortUrl): string =>
+                $shortUrl->getDomain()?->authority ?? 'DEFAULT';
+        }
         if ($input->getOption('show-api-key')) {
             $columnsMap['API Key'] = static fn (array $_, ShortUrl $shortUrl): string =>
                 $shortUrl->authorApiKey()?->__toString() ?? '';

module/CLI/test-cli/Command/CreateShortUrlTest.php

@@ -0,0 +1,31 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioCliTest\Shlink\CLI\Command;
+
+use PHPUnit\Framework\Attributes\Test;
+use Shlinkio\Shlink\CLI\Command\ShortUrl\CreateShortUrlCommand;
+use Shlinkio\Shlink\CLI\Command\ShortUrl\ListShortUrlsCommand;
+use Shlinkio\Shlink\CLI\Util\ExitCodes;
+use Shlinkio\Shlink\TestUtils\CliTest\CliTestCase;
+
+class CreateShortUrlTest extends CliTestCase
+{
+    #[Test]
+    public function defaultDomainIsIgnoredWhenExplicitlyProvided(): void
+    {
+        $slug = 'testing-default-domain';
+        $defaultDomain = 's.test';
+
+        [$output, $exitCode] = $this->exec(
+            [CreateShortUrlCommand::NAME, 'https://example.com', '--domain', $defaultDomain, '--custom-slug', $slug],
+        );
+
+        self::assertEquals(ExitCodes::EXIT_SUCCESS, $exitCode);
+        self::assertStringContainsString('Generated short URL: http://' . $defaultDomain . '/' . $slug, $output);
+
+        [$listOutput] = $this->exec([ListShortUrlsCommand::NAME, '--show-domain', '--search-term', $slug]);
+        self::assertStringContainsString('DEFAULT', $listOutput);
+    }
+}

module/CLI/test-cli/Command/ImportShortUrlsTest.php

@@ -0,0 +1,79 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioCliTest\Shlink\CLI\Command;
+
+use PHPUnit\Framework\Attributes\Test;
+use Shlinkio\Shlink\CLI\Command\ShortUrl\ListShortUrlsCommand;
+use Shlinkio\Shlink\Importer\Command\ImportCommand;
+use Shlinkio\Shlink\TestUtils\CliTest\CliTestCase;
+
+use function fclose;
+use function fopen;
+use function fwrite;
+use function is_string;
+use function sys_get_temp_dir;
+use function tempnam;
+use function unlink;
+
+class ImportShortUrlsTest extends CliTestCase
+{
+    /**
+     * @var false|string|null
+     * @todo Use native type once PHP 8.1 support is dropped
+     */
+    private mixed $tempCsvFile = null;
+
+    protected function setUp(): void
+    {
+        $this->tempCsvFile = tempnam(sys_get_temp_dir(), 'shlink_csv');
+        if (! $this->tempCsvFile) {
+            return;
+        }
+
+        $handle = fopen($this->tempCsvFile, 'w+');
+        if (! $handle) {
+            $this->fail('It was not possible to open the temporary file to write CSV on it');
+        }
+
+        fwrite(
+            $handle,
+            <<<CSV
+            longURL;tags;domain;short code;Title
+            https://shlink.io;foo,baz;s.test;testing-default-domain-import-1;
+            https://example.com;foo;s.test;testing-default-domain-import-2;
+            CSV,
+        );
+        fclose($handle);
+    }
+
+    protected function tearDown(): void
+    {
+        if (is_string($this->tempCsvFile)) {
+            unlink($this->tempCsvFile);
+        }
+    }
+
+    #[Test]
+    public function defaultDomainIsIgnoredWhenExplicitlyProvided(): void
+    {
+        if (! $this->tempCsvFile) {
+            $this->fail('It was not possible to create a temporary CSV file');
+        }
+
+        [$output] = $this->exec([ImportCommand::NAME, 'csv'], [$this->tempCsvFile, ';']);
+
+        self::assertStringContainsString('https://shlink.io: Imported', $output);
+        self::assertStringContainsString('https://example.com: Imported', $output);
+
+        [$listOutput1] = $this->exec(
+            [ListShortUrlsCommand::NAME, '--show-domain', '--search-term', 'testing-default-domain-import-1'],
+        );
+        self::assertStringContainsString('DEFAULT', $listOutput1);
+        [$listOutput1] = $this->exec(
+            [ListShortUrlsCommand::NAME, '--show-domain', '--search-term', 'testing-default-domain-import-2'],
+        );
+        self::assertStringContainsString('DEFAULT', $listOutput1);
+    }
+}

module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php

@@ -28,8 +28,6 @@ class CreateShortUrlCommandTest extends TestCase
 {
     use CliTestUtilsTrait;
 
-    private const DEFAULT_DOMAIN = 'default.com';
-
     private CommandTester $commandTester;
     private MockObject & UrlShortenerInterface $urlShortener;
     private MockObject & ShortUrlStringifierInterface $stringifier;
@@ -43,7 +41,7 @@ class CreateShortUrlCommandTest extends TestCase
             $this->urlShortener,
             $this->stringifier,
             new UrlShortenerOptions(
-                domain: ['hostname' => self::DEFAULT_DOMAIN, 'schema' => ''],
+                domain: ['hostname' => 'example.com', 'schema' => ''],
                 defaultShortCodesLength: 5,
             ),
         );
@@ -147,9 +145,8 @@ class CreateShortUrlCommandTest extends TestCase
     public static function provideDomains(): iterable
     {
         yield 'no domain' => [[], null];
-        yield 'non-default domain foo' => [['--domain' => 'foo.com'], 'foo.com'];
-        yield 'non-default domain bar' => [['-d' => 'bar.com'], 'bar.com'];
-        yield 'default domain' => [['--domain' => self::DEFAULT_DOMAIN], null];
+        yield 'domain foo' => [['--domain' => 'foo.com'], 'foo.com'];
+        yield 'domain bar' => [['-d' => 'bar.com'], 'bar.com'];
     }
 
     #[Test, DataProvider('provideFlags')]

module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php

@@ -144,13 +144,19 @@ class ListShortUrlsCommandTest extends TestCase
         yield 'tags only' => [
             ['--show-tags' => true],
             ['| Tags    ', '| foo, bar, baz'],
-            ['| API Key    ', '| API Key Name |', $key, '| my api key'],
+            ['| API Key    ', '| API Key Name |', $key, '| my api key', '| Domain', '| DEFAULT'],
+            $apiKey,
+        ];
+        yield 'domain only' => [
+            ['--show-domain' => true],
+            ['| Domain', '| DEFAULT'],
+            ['| Tags    ', '| foo, bar, baz', '| API Key    ', '| API Key Name |', $key, '| my api key'],
             $apiKey,
         ];
         yield 'api key only' => [
             ['--show-api-key' => true],
             ['| API Key    ', $key],
-            ['| Tags    ', '| foo, bar, baz', '| API Key Name |', '| my api key'],
+            ['| Tags    ', '| foo, bar, baz', '| API Key Name |', '| my api key', '| Domain', '| DEFAULT'],
             $apiKey,
         ];
         yield 'api key name only' => [
@@ -165,9 +171,24 @@ class ListShortUrlsCommandTest extends TestCase
             ['| API Key Name |', '| my api key'],
             $apiKey,
         ];
+        yield 'tags and domain' => [
+            ['--show-tags' => true, '--show-domain' => true],
+            ['| Tags    ', '| foo, bar, baz', '| Domain', '| DEFAULT'],
+            ['| API Key Name |', '| my api key'],
+            $apiKey,
+        ];
         yield 'all' => [
-            ['--show-tags' => true, '--show-api-key' => true, '--show-api-key-name' => true],
-            ['| API Key    ', '| Tags    ', '| API Key Name |', '| foo, bar, baz', $key, '| my api key'],
+            ['--show-tags' => true, '--show-domain' => true, '--show-api-key' => true, '--show-api-key-name' => true],
+            [
+                '| API Key    ',
+                '| Tags    ',
+                '| API Key Name |',
+                '| foo, bar, baz',
+                $key,
+                '| my api key',
+                '| Domain',
+                '| DEFAULT',
+            ],
             [],
             $apiKey,
         ];

module/Core/config/dependencies.config.php

@@ -161,7 +161,7 @@ return [
         ],
         Action\RobotsAction::class => [Crawling\CrawlingHelper::class],
 
-        ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ['em'],
+        ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ['em', Options\UrlShortenerOptions::class],
         ShortUrl\Helper\ShortUrlStringifier::class => ['config.url_shortener.domain', 'config.router.base_path'],
         ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => [Util\UrlValidator::class],
         ShortUrl\Helper\ShortUrlRedirectionBuilder::class => [Options\TrackingOptions::class],

module/Core/src/Options/UrlShortenerOptions.php

@@ -26,4 +26,9 @@ final class UrlShortenerOptions
     {
         return $this->mode === ShortUrlMode::LOOSE;
     }
+
+    public function defaultDomain(): string
+    {
+        return $this->domain['hostname'] ?? '';
+    }
 }

module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php

@@ -9,6 +9,7 @@ use Doctrine\Common\Collections\Collection;
 use Doctrine\ORM\EntityManagerInterface;
 use Doctrine\ORM\Events;
 use Shlinkio\Shlink\Core\Domain\Entity\Domain;
+use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
 use Shlinkio\Shlink\Core\Tag\Entity\Tag;
 
 use function Functional\map;
@@ -21,15 +22,17 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt
     /** @var array<string, Tag> */
     private array $memoizedNewTags = [];
 
-    public function __construct(private readonly EntityManagerInterface $em)
-    {
+    public function __construct(
+        private readonly EntityManagerInterface $em,
+        private readonly UrlShortenerOptions $options = new UrlShortenerOptions(),
+    ) {
         // Registering this as an event listener will make the postFlush method to be called automatically
         $this->em->getEventManager()->addEventListener(Events::postFlush, $this);
     }
 
     public function resolveDomain(?string $domain): ?Domain
     {
-        if ($domain === null) {
+        if ($domain === null || $domain === $this->options->defaultDomain()) {
             return null;
         }
 
@@ -42,9 +45,7 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt
 
     private function memoizeNewDomain(string $domain): Domain
     {
-        return $this->memoizedNewDomains[$domain] = $this->memoizedNewDomains[$domain] ?? Domain::withAuthority(
-            $domain,
-        );
+        return $this->memoizedNewDomains[$domain] ??= Domain::withAuthority($domain);
     }
 
     /**
@@ -71,7 +72,7 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt
 
     private function memoizeNewTag(string $tagName): Tag
     {
-        return $this->memoizedNewTags[$tagName] = $this->memoizedNewTags[$tagName] ?? new Tag($tagName);
+        return $this->memoizedNewTags[$tagName] ??= new Tag($tagName);
     }
 
     public function postFlush(): void

module/Core/src/ShortUrl/ShortUrlListService.php

@@ -25,7 +25,7 @@ class ShortUrlListService implements ShortUrlListServiceInterface
      */
     public function listShortUrls(ShortUrlsParams $params, ?ApiKey $apiKey = null): Paginator
     {
-        $defaultDomain = $this->urlShortenerOptions->domain['hostname'] ?? '';
+        $defaultDomain = $this->urlShortenerOptions->defaultDomain();
         $paginator = new Paginator(new ShortUrlRepositoryAdapter($this->repo, $params, $apiKey, $defaultDomain));
         $paginator->setMaxPerPage($params->itemsPerPage)
                   ->setCurrentPage($params->page);

module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php

@@ -12,6 +12,7 @@ use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Shlinkio\Shlink\Core\Domain\Entity\Domain;
 use Shlinkio\Shlink\Core\Domain\Repository\DomainRepositoryInterface;
+use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
 use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver;
 use Shlinkio\Shlink\Core\Tag\Entity\Tag;
 use Shlinkio\Shlink\Core\Tag\Repository\TagRepositoryInterface;
@@ -28,14 +29,22 @@ class PersistenceShortUrlRelationResolverTest extends TestCase
         $this->em = $this->createMock(EntityManagerInterface::class);
         $this->em->method('getEventManager')->willReturn(new EventManager());
 
-        $this->resolver = new PersistenceShortUrlRelationResolver($this->em);
+        $this->resolver = new PersistenceShortUrlRelationResolver($this->em, new UrlShortenerOptions(
+            domain: ['schema' => 'https', 'hostname' => 'default.com'],
+        ));
     }
 
-    #[Test]
-    public function returnsEmptyWhenNoDomainIsProvided(): void
+    #[Test, DataProvider('provideDomainsThatEmpty')]
+    public function returnsEmptyInSomeCases(?string $domain): void
     {
         $this->em->expects($this->never())->method('getRepository')->with(Domain::class);
-        self::assertNull($this->resolver->resolveDomain(null));
+        self::assertNull($this->resolver->resolveDomain($domain));
+    }
+
+    public static function provideDomainsThatEmpty(): iterable
+    {
+        yield 'null' => [null];
+        yield 'default domain' => ['default.com'];
     }
 
     #[Test, DataProvider('provideFoundDomains')]