From ee43e68a57115b4e8395b45cc7496419dcf8961d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 9 Dec 2021 12:32:02 +0100 Subject: [PATCH] Changed behavior of domains list so that it does not return configured redirects as redirects for default domain --- CHANGELOG.md | 8 ++++ .../Domain/DomainRedirectsCommandTest.php | 8 ++-- .../Command/Domain/ListDomainsCommandTest.php | 4 +- module/Core/config/dependencies.config.php | 6 +-- .../Config/EmptyNotFoundRedirectConfig.php | 38 +++++++++++++++++++ module/Core/src/Domain/DomainService.php | 13 +++---- module/Core/src/Domain/Model/DomainItem.php | 2 +- .../src/Options/NotFoundRedirectOptions.php | 12 +----- .../EmptyNotFoundRedirectConfigTest.php | 29 ++++++++++++++ module/Core/test/Domain/DomainServiceTest.php | 24 ++++++------ .../src/Action/Domain/ListDomainsAction.php | 3 +- .../Action/Domain/ListDomainsActionTest.php | 5 ++- 12 files changed, 106 insertions(+), 46 deletions(-) create mode 100644 module/Core/src/Config/EmptyNotFoundRedirectConfig.php create mode 100644 module/Core/test/Config/EmptyNotFoundRedirectConfigTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index dc6cdef3..9aeca402 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ## [Unreleased] ### Added +* [#1163](https://github.com/shlinkio/shlink/issues/1163) Allowed setting not-found redirects for default domain in the same way it's done for any other domain. + + This implies a few non-breaking changes: + + * The domains list no longer has the values of `INVALID_SHORT_URL_REDIRECT_TO`, `REGULAR_404_REDIRECT_TO` and `BASE_URL_REDIRECT_TO` on the default domain redirects. + * The `GET /domains` endpoint includes a new `defaultRedirects` property in the response, with the default redirects set via config or env vars. + * The `INVALID_SHORT_URL_REDIRECT_TO`, `REGULAR_404_REDIRECT_TO` and `BASE_URL_REDIRECT_TO` env vars are now deprecated, and should be replaced by `DEFAULT_INVALID_SHORT_URL_REDIRECT`, `DEFAULT_REGULAR_404_REDIRECT` and `DEFAULT_BASE_URL_REDIRECT` respectively. Deprecated ones will continue to work until v3.0.0, where they will be removed. + * [#1204](https://github.com/shlinkio/shlink/issues/1204) Added support for `openswoole` and migrated official docker image to `openswoole`. * [#1242](https://github.com/shlinkio/shlink/issues/1242) Added support to import urls and visits from YOURLS. diff --git a/module/CLI/test/Command/Domain/DomainRedirectsCommandTest.php b/module/CLI/test/Command/Domain/DomainRedirectsCommandTest.php index 9801930e..6b6e1036 100644 --- a/module/CLI/test/Command/Domain/DomainRedirectsCommandTest.php +++ b/module/CLI/test/Command/Domain/DomainRedirectsCommandTest.php @@ -126,8 +126,8 @@ class DomainRedirectsCommandTest extends TestCase $listDomains = $this->domainService->listDomains()->willReturn([ DomainItem::forDefaultDomain('default-domain.com', new NotFoundRedirectOptions()), - DomainItem::forExistingDomain(Domain::withAuthority('existing-one.com')), - DomainItem::forExistingDomain(Domain::withAuthority($domainAuthority)), + DomainItem::forNonDefaultDomain(Domain::withAuthority('existing-one.com')), + DomainItem::forNonDefaultDomain(Domain::withAuthority($domainAuthority)), ]); $findDomain = $this->domainService->findByAuthority($domainAuthority)->willReturn($domain); $configureRedirects = $this->domainService->configureNotFoundRedirects( @@ -156,8 +156,8 @@ class DomainRedirectsCommandTest extends TestCase $listDomains = $this->domainService->listDomains()->willReturn([ DomainItem::forDefaultDomain('default-domain.com', new NotFoundRedirectOptions()), - DomainItem::forExistingDomain(Domain::withAuthority('existing-one.com')), - DomainItem::forExistingDomain(Domain::withAuthority('existing-two.com')), + DomainItem::forNonDefaultDomain(Domain::withAuthority('existing-one.com')), + DomainItem::forNonDefaultDomain(Domain::withAuthority('existing-two.com')), ]); $findDomain = $this->domainService->findByAuthority($domainAuthority)->willReturn($domain); $configureRedirects = $this->domainService->configureNotFoundRedirects( diff --git a/module/CLI/test/Command/Domain/ListDomainsCommandTest.php b/module/CLI/test/Command/Domain/ListDomainsCommandTest.php index 13e6d062..6d56ea69 100644 --- a/module/CLI/test/Command/Domain/ListDomainsCommandTest.php +++ b/module/CLI/test/Command/Domain/ListDomainsCommandTest.php @@ -47,8 +47,8 @@ class ListDomainsCommandTest extends TestCase 'base_url' => 'https://foo.com/default/base', 'invalid_short_url' => 'https://foo.com/default/invalid', ])), - DomainItem::forExistingDomain(Domain::withAuthority('bar.com')), - DomainItem::forExistingDomain($bazDomain), + DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com')), + DomainItem::forNonDefaultDomain($bazDomain), ]); $this->commandTester->execute($input); diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 16b84819..fdfecef9 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -119,11 +119,7 @@ return [ ], Service\ShortUrl\ShortUrlResolver::class => ['em'], Service\ShortUrl\ShortCodeHelper::class => ['em'], - Domain\DomainService::class => [ - 'em', - 'config.url_shortener.domain.hostname', - Options\NotFoundRedirectOptions::class, - ], + Domain\DomainService::class => ['em', 'config.url_shortener.domain.hostname'], Util\UrlValidator::class => ['httpClient', Options\UrlShortenerOptions::class], Util\DoctrineBatchHelper::class => ['em'], diff --git a/module/Core/src/Config/EmptyNotFoundRedirectConfig.php b/module/Core/src/Config/EmptyNotFoundRedirectConfig.php new file mode 100644 index 00000000..6ccb3848 --- /dev/null +++ b/module/Core/src/Config/EmptyNotFoundRedirectConfig.php @@ -0,0 +1,38 @@ +defaultDomainAndRest($apiKey); - $mappedDomains = map($domains, fn (Domain $domain) => DomainItem::forExistingDomain($domain)); + $mappedDomains = map($domains, fn (Domain $domain) => DomainItem::forNonDefaultDomain($domain)); if ($apiKey?->hasRole(Role::DOMAIN_SPECIFIC)) { return $mappedDomains; } return [ - DomainItem::forDefaultDomain($this->defaultDomain, $default ?? $this->redirectOptions), + DomainItem::forDefaultDomain($this->defaultDomain, $default ?? new EmptyNotFoundRedirectConfig()), ...$mappedDomains, ]; } diff --git a/module/Core/src/Domain/Model/DomainItem.php b/module/Core/src/Domain/Model/DomainItem.php index 909cca7d..5547fe8d 100644 --- a/module/Core/src/Domain/Model/DomainItem.php +++ b/module/Core/src/Domain/Model/DomainItem.php @@ -18,7 +18,7 @@ final class DomainItem implements JsonSerializable ) { } - public static function forExistingDomain(Domain $domain): self + public static function forNonDefaultDomain(Domain $domain): self { return new self($domain->getAuthority(), $domain, false); } diff --git a/module/Core/src/Options/NotFoundRedirectOptions.php b/module/Core/src/Options/NotFoundRedirectOptions.php index 27f410a8..2f2d813b 100644 --- a/module/Core/src/Options/NotFoundRedirectOptions.php +++ b/module/Core/src/Options/NotFoundRedirectOptions.php @@ -4,11 +4,10 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Options; -use JsonSerializable; use Laminas\Stdlib\AbstractOptions; use Shlinkio\Shlink\Core\Config\NotFoundRedirectConfigInterface; -class NotFoundRedirectOptions extends AbstractOptions implements NotFoundRedirectConfigInterface, JsonSerializable +class NotFoundRedirectOptions extends AbstractOptions implements NotFoundRedirectConfigInterface { private ?string $invalidShortUrl = null; private ?string $regular404 = null; @@ -61,13 +60,4 @@ class NotFoundRedirectOptions extends AbstractOptions implements NotFoundRedirec $this->baseUrl = $baseUrl; return $this; } - - public function jsonSerialize(): array - { - return [ - 'baseUrlRedirect' => $this->baseUrl, - 'regular404Redirect' => $this->regular404, - 'invalidShortUrlRedirect' => $this->invalidShortUrl, - ]; - } } diff --git a/module/Core/test/Config/EmptyNotFoundRedirectConfigTest.php b/module/Core/test/Config/EmptyNotFoundRedirectConfigTest.php new file mode 100644 index 00000000..d1c47e10 --- /dev/null +++ b/module/Core/test/Config/EmptyNotFoundRedirectConfigTest.php @@ -0,0 +1,29 @@ +redirectsConfig = new EmptyNotFoundRedirectConfig(); + } + + /** @test */ + public function allMethodsReturnHardcodedValues(): void + { + self::assertNull($this->redirectsConfig->invalidShortUrlRedirect()); + self::assertFalse($this->redirectsConfig->hasInvalidShortUrlRedirect()); + self::assertNull($this->redirectsConfig->regular404Redirect()); + self::assertFalse($this->redirectsConfig->hasRegular404Redirect()); + self::assertNull($this->redirectsConfig->baseUrlRedirect()); + self::assertFalse($this->redirectsConfig->hasBaseUrlRedirect()); + } +} diff --git a/module/Core/test/Domain/DomainServiceTest.php b/module/Core/test/Domain/DomainServiceTest.php index 337438b5..71922fe3 100644 --- a/module/Core/test/Domain/DomainServiceTest.php +++ b/module/Core/test/Domain/DomainServiceTest.php @@ -9,13 +9,13 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; +use Shlinkio\Shlink\Core\Config\EmptyNotFoundRedirectConfig; use Shlinkio\Shlink\Core\Config\NotFoundRedirects; use Shlinkio\Shlink\Core\Domain\DomainService; use Shlinkio\Shlink\Core\Domain\Model\DomainItem; use Shlinkio\Shlink\Core\Domain\Repository\DomainRepositoryInterface; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Exception\DomainNotFoundException; -use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -30,7 +30,7 @@ class DomainServiceTest extends TestCase public function setUp(): void { $this->em = $this->prophesize(EntityManagerInterface::class); - $this->domainService = new DomainService($this->em->reveal(), 'default.com', new NotFoundRedirectOptions()); + $this->domainService = new DomainService($this->em->reveal(), 'default.com'); } /** @@ -52,7 +52,7 @@ class DomainServiceTest extends TestCase public function provideExcludedDomains(): iterable { - $default = DomainItem::forDefaultDomain('default.com', new NotFoundRedirectOptions()); + $default = DomainItem::forDefaultDomain('default.com', new EmptyNotFoundRedirectConfig()); $adminApiKey = ApiKey::create(); $domainSpecificApiKey = ApiKey::fromMeta( ApiKeyMeta::withRoles(RoleDefinition::forDomain(Domain::withAuthority('')->setId('123'))), @@ -61,15 +61,15 @@ class DomainServiceTest extends TestCase yield 'empty list without API key' => [[], [$default], null]; yield 'one item without API key' => [ [Domain::withAuthority('bar.com')], - [$default, DomainItem::forExistingDomain(Domain::withAuthority('bar.com'))], + [$default, DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com'))], null, ]; yield 'multiple items without API key' => [ [Domain::withAuthority('foo.com'), Domain::withAuthority('bar.com')], [ $default, - DomainItem::forExistingDomain(Domain::withAuthority('foo.com')), - DomainItem::forExistingDomain(Domain::withAuthority('bar.com')), + DomainItem::forNonDefaultDomain(Domain::withAuthority('foo.com')), + DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com')), ], null, ]; @@ -77,15 +77,15 @@ class DomainServiceTest extends TestCase yield 'empty list with admin API key' => [[], [$default], $adminApiKey]; yield 'one item with admin API key' => [ [Domain::withAuthority('bar.com')], - [$default, DomainItem::forExistingDomain(Domain::withAuthority('bar.com'))], + [$default, DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com'))], $adminApiKey, ]; yield 'multiple items with admin API key' => [ [Domain::withAuthority('foo.com'), Domain::withAuthority('bar.com')], [ $default, - DomainItem::forExistingDomain(Domain::withAuthority('foo.com')), - DomainItem::forExistingDomain(Domain::withAuthority('bar.com')), + DomainItem::forNonDefaultDomain(Domain::withAuthority('foo.com')), + DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com')), ], $adminApiKey, ]; @@ -93,14 +93,14 @@ class DomainServiceTest extends TestCase yield 'empty list with domain-specific API key' => [[], [], $domainSpecificApiKey]; yield 'one item with domain-specific API key' => [ [Domain::withAuthority('bar.com')], - [DomainItem::forExistingDomain(Domain::withAuthority('bar.com'))], + [DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com'))], $domainSpecificApiKey, ]; yield 'multiple items with domain-specific API key' => [ [Domain::withAuthority('foo.com'), Domain::withAuthority('bar.com')], [ - DomainItem::forExistingDomain(Domain::withAuthority('foo.com')), - DomainItem::forExistingDomain(Domain::withAuthority('bar.com')), + DomainItem::forNonDefaultDomain(Domain::withAuthority('foo.com')), + DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com')), ], $domainSpecificApiKey, ]; diff --git a/module/Rest/src/Action/Domain/ListDomainsAction.php b/module/Rest/src/Action/Domain/ListDomainsAction.php index 11b9e151..e50ada16 100644 --- a/module/Rest/src/Action/Domain/ListDomainsAction.php +++ b/module/Rest/src/Action/Domain/ListDomainsAction.php @@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Rest\Action\Domain; use Laminas\Diactoros\Response\JsonResponse; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; +use Shlinkio\Shlink\Core\Config\NotFoundRedirects; use Shlinkio\Shlink\Core\Domain\DomainServiceInterface; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; @@ -29,7 +30,7 @@ class ListDomainsAction extends AbstractRestAction return new JsonResponse([ 'domains' => [ 'data' => $domainItems, - 'defaultRedirects' => $this->options, + 'defaultRedirects' => NotFoundRedirects::fromConfig($this->options), ], ]); } diff --git a/module/Rest/test/Action/Domain/ListDomainsActionTest.php b/module/Rest/test/Action/Domain/ListDomainsActionTest.php index 9bbe9723..bc852b34 100644 --- a/module/Rest/test/Action/Domain/ListDomainsActionTest.php +++ b/module/Rest/test/Action/Domain/ListDomainsActionTest.php @@ -9,6 +9,7 @@ use Laminas\Diactoros\ServerRequestFactory; use PHPUnit\Framework\TestCase; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; +use Shlinkio\Shlink\Core\Config\NotFoundRedirects; use Shlinkio\Shlink\Core\Domain\DomainServiceInterface; use Shlinkio\Shlink\Core\Domain\Model\DomainItem; use Shlinkio\Shlink\Core\Entity\Domain; @@ -37,7 +38,7 @@ class ListDomainsActionTest extends TestCase $apiKey = ApiKey::create(); $domains = [ DomainItem::forDefaultDomain('bar.com', new NotFoundRedirectOptions()), - DomainItem::forExistingDomain(Domain::withAuthority('baz.com')), + DomainItem::forNonDefaultDomain(Domain::withAuthority('baz.com')), ]; $listDomains = $this->domainService->listDomains($apiKey)->willReturn($domains); @@ -48,7 +49,7 @@ class ListDomainsActionTest extends TestCase self::assertEquals([ 'domains' => [ 'data' => $domains, - 'defaultRedirects' => $this->options, + 'defaultRedirects' => NotFoundRedirects::fromConfig($this->options), ], ], $payload); $listDomains->shouldHaveBeenCalledOnce();