From c582eba753196723595e3feb3474d49ea5178a67 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 22 Apr 2023 19:44:04 +0200 Subject: [PATCH 1/7] Make sure short URL domain is resolved as null when default one is provided --- .../Command/ShortUrl/CreateShortUrlCommand.php | 9 --------- .../ShortUrl/CreateShortUrlCommandTest.php | 9 +++------ module/Core/config/dependencies.config.php | 2 +- module/Core/src/Options/UrlShortenerOptions.php | 5 +++++ .../PersistenceShortUrlRelationResolver.php | 15 ++++++++------- .../Core/src/ShortUrl/ShortUrlListService.php | 2 +- .../PersistenceShortUrlRelationResolverTest.php | 17 +++++++++++++---- 7 files changed, 31 insertions(+), 28 deletions(-) diff --git a/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php index bb332d82..a998a677 100644 --- a/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php +++ b/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); diff --git a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php index fd474007..60482138 100644 --- a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php +++ b/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')] diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 567d57ce..008db777 100644 --- a/module/Core/config/dependencies.config.php +++ b/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], diff --git a/module/Core/src/Options/UrlShortenerOptions.php b/module/Core/src/Options/UrlShortenerOptions.php index 6e6ac087..32b40033 100644 --- a/module/Core/src/Options/UrlShortenerOptions.php +++ b/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'] ?? ''; + } } diff --git a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php index db6721d5..48ec634e 100644 --- a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php +++ b/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 */ 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 diff --git a/module/Core/src/ShortUrl/ShortUrlListService.php b/module/Core/src/ShortUrl/ShortUrlListService.php index d83647f0..60f56554 100644 --- a/module/Core/src/ShortUrl/ShortUrlListService.php +++ b/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); diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php index 43f99462..d31f3d9b 100644 --- a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php +++ b/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')] From f2ecbceae9f44541fefaba25b063124e31c851ba Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 22 Apr 2023 19:46:28 +0200 Subject: [PATCH 2/7] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9ca10a0..f9c1dc57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ 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. ## [3.5.4] - 2023-04-12 From cf49393ef2d183ad9b74d0d63176a0d34e5ca911 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 23 Apr 2023 11:19:05 +0200 Subject: [PATCH 3/7] Add --show-domain flag to list short URLs command --- .../Command/ShortUrl/ListShortUrlsCommand.php | 10 +++++++ .../ShortUrl/ListShortUrlsCommandTest.php | 29 ++++++++++++++++--- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php index 7a9c77af..9202957b 100644 --- a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php +++ b/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() ?? ''; diff --git a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php index 52d1eeb3..d81172ed 100644 --- a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php +++ b/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, ]; From 1b8334499598d20383abb0f523b6ea081ad7bdaa Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 23 Apr 2023 11:20:54 +0200 Subject: [PATCH 4/7] Create CLI test checking default domain is ignored even if explicitly provided --- .../test-cli/Command/CreateShortUrlTest.php | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 module/CLI/test-cli/Command/CreateShortUrlTest.php diff --git a/module/CLI/test-cli/Command/CreateShortUrlTest.php b/module/CLI/test-cli/Command/CreateShortUrlTest.php new file mode 100644 index 00000000..d4d8a583 --- /dev/null +++ b/module/CLI/test-cli/Command/CreateShortUrlTest.php @@ -0,0 +1,31 @@ +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); + } +} From d06e92ffc269617c9ccca2c19865cbf54de27a71 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 23 Apr 2023 13:22:04 +0200 Subject: [PATCH 5/7] Created CLI test for short URL importing --- .../test-cli/Command/ImportShortUrlsTest.php | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 module/CLI/test-cli/Command/ImportShortUrlsTest.php diff --git a/module/CLI/test-cli/Command/ImportShortUrlsTest.php b/module/CLI/test-cli/Command/ImportShortUrlsTest.php new file mode 100644 index 00000000..3a710af0 --- /dev/null +++ b/module/CLI/test-cli/Command/ImportShortUrlsTest.php @@ -0,0 +1,79 @@ +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, + <<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); + } +} From 9fa291a32f7cdbfabe682524c3c82bc2a2063f36 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 23 Apr 2023 15:20:33 +0200 Subject: [PATCH 6/7] Update shlink-common --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 826f4896..2dd10f8a 100644 --- a/composer.json +++ b/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", From f129544f8391ea7c80ab65ae23eaa102153e4b57 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 23 Apr 2023 15:22:40 +0200 Subject: [PATCH 7/7] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9c1dc57..40be8aad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Fixed * [#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