From 5e722c830f231176de9484ae04ea65de0f06dfe5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 7 Dec 2021 21:13:47 +0100 Subject: [PATCH 1/7] Allowed to set redirects for default domain via command line or API --- docs/swagger/paths/v2_domains_redirects.json | 10 ------ module/Core/src/Domain/DomainService.php | 29 ++++++++++------ .../src/Domain/DomainServiceInterface.php | 2 -- .../src/Exception/InvalidDomainException.php | 33 ------------------- module/Core/test/Domain/DomainServiceTest.php | 14 +------- .../Exception/InvalidDomainExceptionTest.php | 24 -------------- 6 files changed, 20 insertions(+), 92 deletions(-) delete mode 100644 module/Core/src/Exception/InvalidDomainException.php delete mode 100644 module/Core/test/Exception/InvalidDomainExceptionTest.php diff --git a/docs/swagger/paths/v2_domains_redirects.json b/docs/swagger/paths/v2_domains_redirects.json index d9863dcd..031e1d43 100644 --- a/docs/swagger/paths/v2_domains_redirects.json +++ b/docs/swagger/paths/v2_domains_redirects.json @@ -99,16 +99,6 @@ } } }, - "403": { - "description": "Default domain was provided, and it cannot be edited this way.", - "content": { - "application/problem+json": { - "schema": { - "$ref": "../definitions/Error.json" - } - } - } - }, "500": { "description": "Unexpected error.", "content": { diff --git a/module/Core/src/Domain/DomainService.php b/module/Core/src/Domain/DomainService.php index b5141324..b1ba194f 100644 --- a/module/Core/src/Domain/DomainService.php +++ b/module/Core/src/Domain/DomainService.php @@ -10,11 +10,12 @@ 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\Exception\InvalidDomainException; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Rest\ApiKey\Role; use Shlinkio\Shlink\Rest\Entity\ApiKey; +use function Functional\first; +use function Functional\group; use function Functional\map; class DomainService implements DomainServiceInterface @@ -31,9 +32,7 @@ class DomainService implements DomainServiceInterface */ public function listDomains(?ApiKey $apiKey = null): array { - /** @var DomainRepositoryInterface $repo */ - $repo = $this->em->getRepository(Domain::class); - $domains = $repo->findDomainsWithout($this->defaultDomain, $apiKey); + [$default, $domains] = $this->defaultDomainAndRest($apiKey); $mappedDomains = map($domains, fn (Domain $domain) => DomainItem::forExistingDomain($domain)); if ($apiKey?->hasRole(Role::DOMAIN_SPECIFIC)) { @@ -41,11 +40,26 @@ class DomainService implements DomainServiceInterface } return [ - DomainItem::forDefaultDomain($this->defaultDomain, $this->redirectOptions), + DomainItem::forDefaultDomain($this->defaultDomain, $default ?? $this->redirectOptions), ...$mappedDomains, ]; } + /** + * @return array{Domain|null, Domain[]} + */ + private function defaultDomainAndRest(?ApiKey $apiKey): array + { + /** @var DomainRepositoryInterface $repo */ + $repo = $this->em->getRepository(Domain::class); + $groups = group( + $repo->findDomainsWithout(null, $apiKey), // FIXME Always called with null as first arg + fn (Domain $domain) => $domain->getAuthority() === $this->defaultDomain ? 'default' : 'domains', + ); + + return [first($groups['default'] ?? []), $groups['domains'] ?? []]; + } + /** * @throws DomainNotFoundException */ @@ -79,17 +93,12 @@ class DomainService implements DomainServiceInterface /** * @throws DomainNotFoundException - * @throws InvalidDomainException */ public function configureNotFoundRedirects( string $authority, NotFoundRedirects $notFoundRedirects, ?ApiKey $apiKey = null, ): Domain { - if ($authority === $this->defaultDomain) { - throw InvalidDomainException::forDefaultDomainRedirects(); - } - $domain = $this->getPersistedDomain($authority, $apiKey); $domain->configureNotFoundRedirects($notFoundRedirects); diff --git a/module/Core/src/Domain/DomainServiceInterface.php b/module/Core/src/Domain/DomainServiceInterface.php index 7748284d..9ac48e69 100644 --- a/module/Core/src/Domain/DomainServiceInterface.php +++ b/module/Core/src/Domain/DomainServiceInterface.php @@ -8,7 +8,6 @@ use Shlinkio\Shlink\Core\Config\NotFoundRedirects; use Shlinkio\Shlink\Core\Domain\Model\DomainItem; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Exception\DomainNotFoundException; -use Shlinkio\Shlink\Core\Exception\InvalidDomainException; use Shlinkio\Shlink\Rest\Entity\ApiKey; interface DomainServiceInterface @@ -32,7 +31,6 @@ interface DomainServiceInterface /** * @throws DomainNotFoundException If the API key is restricted to one domain and a different one is provided - * @throws InvalidDomainException If default domain is provided */ public function configureNotFoundRedirects( string $authority, diff --git a/module/Core/src/Exception/InvalidDomainException.php b/module/Core/src/Exception/InvalidDomainException.php deleted file mode 100644 index d41e71ac..00000000 --- a/module/Core/src/Exception/InvalidDomainException.php +++ /dev/null @@ -1,33 +0,0 @@ -detail = $e->getMessage(); - $e->title = self::TITLE; - $e->type = self::TYPE; - $e->status = StatusCodeInterface::STATUS_FORBIDDEN; - - return $e; - } -} diff --git a/module/Core/test/Domain/DomainServiceTest.php b/module/Core/test/Domain/DomainServiceTest.php index 159fb6ca..337438b5 100644 --- a/module/Core/test/Domain/DomainServiceTest.php +++ b/module/Core/test/Domain/DomainServiceTest.php @@ -15,7 +15,6 @@ 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\Exception\InvalidDomainException; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; @@ -42,7 +41,7 @@ class DomainServiceTest extends TestCase { $repo = $this->prophesize(DomainRepositoryInterface::class); $getRepo = $this->em->getRepository(Domain::class)->willReturn($repo->reveal()); - $findDomains = $repo->findDomainsWithout('default.com', $apiKey)->willReturn($domains); + $findDomains = $repo->findDomainsWithout(null, $apiKey)->willReturn($domains); $result = $this->domainService->listDomains($apiKey); @@ -214,15 +213,4 @@ class DomainServiceTest extends TestCase yield 'domain not found and author API key' => [null, $authorApiKey]; yield 'domain found and author API key' => [$domain, $authorApiKey]; } - - /** @test */ - public function anExceptionIsThrowsWhenTryingToEditRedirectsForDefaultDomain(): void - { - $this->expectException(InvalidDomainException::class); - $this->expectExceptionMessage( - 'You cannot configure default domain\'s redirects this way. Use the configuration or env vars.', - ); - - $this->domainService->configureNotFoundRedirects('default.com', NotFoundRedirects::withoutRedirects()); - } } diff --git a/module/Core/test/Exception/InvalidDomainExceptionTest.php b/module/Core/test/Exception/InvalidDomainExceptionTest.php deleted file mode 100644 index 06b78ff2..00000000 --- a/module/Core/test/Exception/InvalidDomainExceptionTest.php +++ /dev/null @@ -1,24 +0,0 @@ -getMessage()); - self::assertEquals($expected, $e->getDetail()); - self::assertEquals('Invalid domain', $e->getTitle()); - self::assertEquals('INVALID_DOMAIN', $e->getType()); - self::assertEquals(403, $e->getStatus()); - } -} From f8a48c16f0f5aeb88082d5b8d519fa8fbbf5823f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 9 Dec 2021 09:45:15 +0100 Subject: [PATCH 2/7] Renamed GenerateShortUrlCommand to CreateShortUrlCommand --- module/CLI/config/cli.config.php | 2 +- module/CLI/config/dependencies.config.php | 4 ++-- ...enerateShortUrlCommand.php => CreateShortUrlCommand.php} | 5 +++-- ...hortUrlCommandTest.php => CreateShortUrlCommandTest.php} | 6 +++--- 4 files changed, 9 insertions(+), 8 deletions(-) rename module/CLI/src/Command/ShortUrl/{GenerateShortUrlCommand.php => CreateShortUrlCommand.php} (98%) rename module/CLI/test/Command/ShortUrl/{GenerateShortUrlCommandTest.php => CreateShortUrlCommandTest.php} (95%) diff --git a/module/CLI/config/cli.config.php b/module/CLI/config/cli.config.php index 46bb90ef..e06ad727 100644 --- a/module/CLI/config/cli.config.php +++ b/module/CLI/config/cli.config.php @@ -8,7 +8,7 @@ return [ 'cli' => [ 'commands' => [ - Command\ShortUrl\GenerateShortUrlCommand::NAME => Command\ShortUrl\GenerateShortUrlCommand::class, + Command\ShortUrl\CreateShortUrlCommand::NAME => Command\ShortUrl\CreateShortUrlCommand::class, Command\ShortUrl\ResolveUrlCommand::NAME => Command\ShortUrl\ResolveUrlCommand::class, Command\ShortUrl\ListShortUrlsCommand::NAME => Command\ShortUrl\ListShortUrlsCommand::class, Command\ShortUrl\GetVisitsCommand::NAME => Command\ShortUrl\GetVisitsCommand::class, diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index d89a8af2..cde351ee 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -39,7 +39,7 @@ return [ ApiKey\RoleResolver::class => ConfigAbstractFactory::class, - Command\ShortUrl\GenerateShortUrlCommand::class => ConfigAbstractFactory::class, + Command\ShortUrl\CreateShortUrlCommand::class => ConfigAbstractFactory::class, Command\ShortUrl\ResolveUrlCommand::class => ConfigAbstractFactory::class, Command\ShortUrl\ListShortUrlsCommand::class => ConfigAbstractFactory::class, Command\ShortUrl\GetVisitsCommand::class => ConfigAbstractFactory::class, @@ -75,7 +75,7 @@ return [ Util\ProcessRunner::class => [SymfonyCli\Helper\ProcessHelper::class], ApiKey\RoleResolver::class => [DomainService::class], - Command\ShortUrl\GenerateShortUrlCommand::class => [ + Command\ShortUrl\CreateShortUrlCommand::class => [ Service\UrlShortener::class, ShortUrlStringifier::class, 'config.url_shortener.default_short_codes_length', diff --git a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php similarity index 98% rename from module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php rename to module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php index e43b4ec5..26673c6c 100644 --- a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php @@ -26,9 +26,9 @@ use function method_exists; use function sprintf; use function str_contains; -class GenerateShortUrlCommand extends BaseCommand +class CreateShortUrlCommand extends BaseCommand { - public const NAME = 'short-url:generate'; + public const NAME = 'short-url:create'; public function __construct( private UrlShortenerInterface $urlShortener, @@ -42,6 +42,7 @@ class GenerateShortUrlCommand extends BaseCommand { $this ->setName(self::NAME) + ->setAliases(['short-url:generate']) // Deprecated ->setDescription('Generates a short URL for provided long URL and returns it') ->addArgument('longUrl', InputArgument::REQUIRED, 'The long URL to parse') ->addOption( diff --git a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php similarity index 95% rename from module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php rename to module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php index 19767dc7..14d75f76 100644 --- a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php @@ -8,7 +8,7 @@ use PHPUnit\Framework\Assert; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; -use Shlinkio\Shlink\CLI\Command\ShortUrl\GenerateShortUrlCommand; +use Shlinkio\Shlink\CLI\Command\ShortUrl\CreateShortUrlCommand; use Shlinkio\Shlink\CLI\Util\ExitCodes; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; @@ -19,7 +19,7 @@ use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifierInterface; use ShlinkioTest\Shlink\CLI\CliTestUtilsTrait; use Symfony\Component\Console\Tester\CommandTester; -class GenerateShortUrlCommandTest extends TestCase +class CreateShortUrlCommandTest extends TestCase { use CliTestUtilsTrait; @@ -33,7 +33,7 @@ class GenerateShortUrlCommandTest extends TestCase $this->stringifier = $this->prophesize(ShortUrlStringifierInterface::class); $this->stringifier->stringify(Argument::type(ShortUrl::class))->willReturn(''); - $command = new GenerateShortUrlCommand($this->urlShortener->reveal(), $this->stringifier->reveal(), 5); + $command = new CreateShortUrlCommand($this->urlShortener->reveal(), $this->stringifier->reveal(), 5); $this->commandTester = $this->testerForCommand($command); } From cbd4b4849fb52473155cab0ca5f68be1e621323b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 9 Dec 2021 10:24:58 +0100 Subject: [PATCH 3/7] Ensured default domain is stripped when creating short URLs from CLI --- module/CLI/config/dependencies.config.php | 1 + .../ShortUrl/CreateShortUrlCommand.php | 24 +++++++++++- .../ShortUrl/CreateShortUrlCommandTest.php | 37 ++++++++++++++++++- 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index cde351ee..41d415dc 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -79,6 +79,7 @@ return [ Service\UrlShortener::class, ShortUrlStringifier::class, 'config.url_shortener.default_short_codes_length', + 'config.url_shortener.domain.hostname', ], Command\ShortUrl\ResolveUrlCommand::class => [Service\ShortUrl\ShortUrlResolver::class], Command\ShortUrl\ListShortUrlsCommand::class => [ diff --git a/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php index 26673c6c..62b50456 100644 --- a/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php @@ -30,10 +30,13 @@ class CreateShortUrlCommand extends BaseCommand { public const NAME = 'short-url:create'; + private ?SymfonyStyle $io; + public function __construct( private UrlShortenerInterface $urlShortener, private ShortUrlStringifierInterface $stringifier, private int $defaultShortCodeLength, + private string $defaultDomain, ) { parent::__construct(); } @@ -123,21 +126,33 @@ class CreateShortUrlCommand extends BaseCommand protected function interact(InputInterface $input, OutputInterface $output): void { - $io = new SymfonyStyle($input, $output); + $this->verifyLongUrlArgument($input, $output); + $this->verifyDomainArgument($input); + } + + private function verifyLongUrlArgument(InputInterface $input, OutputInterface $output): void + { $longUrl = $input->getArgument('longUrl'); if (! empty($longUrl)) { return; } + $io = $this->getIO($input, $output); $longUrl = $io->ask('Which URL do you want to shorten?'); if (! empty($longUrl)) { $input->setArgument('longUrl', $longUrl); } } + 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 = new SymfonyStyle($input, $output); + $io = $this->getIO($input, $output); $longUrl = $input->getArgument('longUrl'); if (empty($longUrl)) { $io->error('A URL was not provided!'); @@ -197,4 +212,9 @@ class CreateShortUrlCommand extends BaseCommand return null; } + + private function getIO(InputInterface $input, OutputInterface $output): SymfonyStyle + { + return $this->io ?? ($this->io = new SymfonyStyle($input, $output)); + } } diff --git a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php index 14d75f76..08389d61 100644 --- a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php @@ -23,6 +23,8 @@ class CreateShortUrlCommandTest extends TestCase { use CliTestUtilsTrait; + private const DEFAULT_DOMAIN = 'default.com'; + private CommandTester $commandTester; private ObjectProphecy $urlShortener; private ObjectProphecy $stringifier; @@ -33,7 +35,12 @@ class CreateShortUrlCommandTest extends TestCase $this->stringifier = $this->prophesize(ShortUrlStringifierInterface::class); $this->stringifier->stringify(Argument::type(ShortUrl::class))->willReturn(''); - $command = new CreateShortUrlCommand($this->urlShortener->reveal(), $this->stringifier->reveal(), 5); + $command = new CreateShortUrlCommand( + $this->urlShortener->reveal(), + $this->stringifier->reveal(), + 5, + self::DEFAULT_DOMAIN, + ); $this->commandTester = $this->testerForCommand($command); } @@ -110,6 +117,34 @@ class CreateShortUrlCommandTest extends TestCase $stringify->shouldHaveBeenCalledOnce(); } + /** + * @test + * @dataProvider provideDomains + */ + public function properlyProcessesProvidedDomain(array $input, ?string $expectedDomain): void + { + $shorten = $this->urlShortener->shorten( + Argument::that(function (ShortUrlMeta $meta) use ($expectedDomain) { + Assert::assertEquals($expectedDomain, $meta->getDomain()); + return true; + }), + )->willReturn(ShortUrl::createEmpty()); + + $input['longUrl'] = 'http://domain.com/foo/bar'; + $this->commandTester->execute($input); + + self::assertEquals(ExitCodes::EXIT_SUCCESS, $this->commandTester->getStatusCode()); + $shorten->shouldHaveBeenCalledOnce(); + } + + public 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]; + } + /** * @test * @dataProvider provideFlags From 0b22fb933ccedde30f7d7d7fee8bae56cd029463 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 9 Dec 2021 10:30:33 +0100 Subject: [PATCH 4/7] Defined new env vars for not-found redirects, deprecating old ones --- config/autoload/redirects.global.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/config/autoload/redirects.global.php b/config/autoload/redirects.global.php index 339ca27d..d2c73884 100644 --- a/config/autoload/redirects.global.php +++ b/config/autoload/redirects.global.php @@ -10,9 +10,10 @@ use const Shlinkio\Shlink\DEFAULT_REDIRECT_STATUS_CODE; return [ 'not_found_redirects' => [ - 'invalid_short_url' => env('INVALID_SHORT_URL_REDIRECT_TO'), - 'regular_404' => env('REGULAR_404_REDIRECT_TO'), - 'base_url' => env('BASE_URL_REDIRECT_TO'), + // Deprecated env vars + 'invalid_short_url' => env('DEFAULT_INVALID_SHORT_URL_REDIRECT', env('INVALID_SHORT_URL_REDIRECT_TO')), + 'regular_404' => env('DEFAULT_REGULAR_404_REDIRECT', env('REGULAR_404_REDIRECT_TO')), + 'base_url' => env('DEFAULT_BASE_URL_REDIRECT', env('BASE_URL_REDIRECT_TO')), ], 'url_shortener' => [ From 348ac78f5af1e14f2470453d4217cb3c7cec69fc Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 9 Dec 2021 12:11:09 +0100 Subject: [PATCH 5/7] Enhanced ListDomainsAction so that it returns default redirects in the response --- docs/swagger/paths/v2_domains.json | 10 ++++++- .../src/Options/NotFoundRedirectOptions.php | 12 ++++++++- module/Rest/config/dependencies.config.php | 6 ++--- .../src/Action/Domain/ListDomainsAction.php | 4 ++- .../test-api/Action/DomainRedirectsTest.php | 26 ++++++------------- .../Rest/test-api/Action/ListDomainsTest.php | 5 ++++ .../Action/Domain/ListDomainsActionTest.php | 5 +++- 7 files changed, 43 insertions(+), 25 deletions(-) diff --git a/docs/swagger/paths/v2_domains.json b/docs/swagger/paths/v2_domains.json index ef63ee4e..40448016 100644 --- a/docs/swagger/paths/v2_domains.json +++ b/docs/swagger/paths/v2_domains.json @@ -46,6 +46,9 @@ } } } + }, + "defaultRedirects": { + "$ref": "../definitions/NotFoundRedirects.json" } } } @@ -84,7 +87,12 @@ "invalidShortUrlRedirect": "https://example.com/invalid-url" } } - ] + ], + "defaultRedirects": { + "baseUrlRedirect": "https://somewhere.com", + "regular404Redirect": null, + "invalidShortUrlRedirect": null + } } } } diff --git a/module/Core/src/Options/NotFoundRedirectOptions.php b/module/Core/src/Options/NotFoundRedirectOptions.php index 2f2d813b..27f410a8 100644 --- a/module/Core/src/Options/NotFoundRedirectOptions.php +++ b/module/Core/src/Options/NotFoundRedirectOptions.php @@ -4,10 +4,11 @@ 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 +class NotFoundRedirectOptions extends AbstractOptions implements NotFoundRedirectConfigInterface, JsonSerializable { private ?string $invalidShortUrl = null; private ?string $regular404 = null; @@ -60,4 +61,13 @@ 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/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index 5e0267d6..98b385b0 100644 --- a/module/Rest/config/dependencies.config.php +++ b/module/Rest/config/dependencies.config.php @@ -9,7 +9,7 @@ use Laminas\ServiceManager\Factory\InvokableFactory; use Mezzio\Router\Middleware\ImplicitOptionsMiddleware; use Shlinkio\Shlink\Common\Mercure\LcobucciJwtProvider; use Shlinkio\Shlink\Core\Domain\DomainService; -use Shlinkio\Shlink\Core\Options\AppOptions; +use Shlinkio\Shlink\Core\Options; use Shlinkio\Shlink\Core\Service; use Shlinkio\Shlink\Core\ShortUrl\Transformer\ShortUrlDataTransformer; use Shlinkio\Shlink\Core\Tag\TagService; @@ -55,7 +55,7 @@ return [ ConfigAbstractFactory::class => [ ApiKeyService::class => ['em'], - Action\HealthAction::class => ['em', AppOptions::class], + Action\HealthAction::class => ['em', Options\AppOptions::class], Action\MercureInfoAction::class => [LcobucciJwtProvider::class, 'config.mercure'], Action\ShortUrl\CreateShortUrlAction::class => [Service\UrlShortener::class, ShortUrlDataTransformer::class], Action\ShortUrl\SingleStepCreateShortUrlAction::class => [ @@ -81,7 +81,7 @@ return [ Action\Tag\DeleteTagsAction::class => [TagService::class], Action\Tag\CreateTagsAction::class => [TagService::class], Action\Tag\UpdateTagAction::class => [TagService::class], - Action\Domain\ListDomainsAction::class => [DomainService::class], + Action\Domain\ListDomainsAction::class => [DomainService::class, Options\NotFoundRedirectOptions::class], Action\Domain\DomainRedirectsAction::class => [DomainService::class], Middleware\CrossDomainMiddleware::class => ['config.cors'], diff --git a/module/Rest/src/Action/Domain/ListDomainsAction.php b/module/Rest/src/Action/Domain/ListDomainsAction.php index c8f9a475..11b9e151 100644 --- a/module/Rest/src/Action/Domain/ListDomainsAction.php +++ b/module/Rest/src/Action/Domain/ListDomainsAction.php @@ -8,6 +8,7 @@ use Laminas\Diactoros\Response\JsonResponse; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Core\Domain\DomainServiceInterface; +use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; use Shlinkio\Shlink\Rest\Middleware\AuthenticationMiddleware; @@ -16,7 +17,7 @@ class ListDomainsAction extends AbstractRestAction protected const ROUTE_PATH = '/domains'; protected const ROUTE_ALLOWED_METHODS = [self::METHOD_GET]; - public function __construct(private DomainServiceInterface $domainService) + public function __construct(private DomainServiceInterface $domainService, private NotFoundRedirectOptions $options) { } @@ -28,6 +29,7 @@ class ListDomainsAction extends AbstractRestAction return new JsonResponse([ 'domains' => [ 'data' => $domainItems, + 'defaultRedirects' => $this->options, ], ]); } diff --git a/module/Rest/test-api/Action/DomainRedirectsTest.php b/module/Rest/test-api/Action/DomainRedirectsTest.php index 987c09d6..fdeec3b3 100644 --- a/module/Rest/test-api/Action/DomainRedirectsTest.php +++ b/module/Rest/test-api/Action/DomainRedirectsTest.php @@ -9,24 +9,6 @@ use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; class DomainRedirectsTest extends ApiTestCase { - /** @test */ - public function anErrorIsReturnedWhenTryingToEditDefaultDomain(): void - { - $resp = $this->callApiWithKey(self::METHOD_PATCH, '/domains/redirects', [ - RequestOptions::JSON => ['domain' => 'doma.in'], - ]); - $payload = $this->getJsonResponsePayload($resp); - - self::assertEquals(self::STATUS_FORBIDDEN, $resp->getStatusCode()); - self::assertEquals(self::STATUS_FORBIDDEN, $payload['status']); - self::assertEquals('INVALID_DOMAIN', $payload['type']); - self::assertEquals( - 'You cannot configure default domain\'s redirects this way. Use the configuration or env vars.', - $payload['detail'], - ); - self::assertEquals('Invalid domain', $payload['title']); - } - /** * @test * @dataProvider provideInvalidDomains @@ -78,6 +60,14 @@ class DomainRedirectsTest extends ApiTestCase 'regular404Redirect' => 'foo.com', 'invalidShortUrlRedirect' => null, ]]; + yield 'default domain' => [[ + 'domain' => 'doma.in', + 'regular404Redirect' => 'foo-for-default.com', + ], [ + 'baseUrlRedirect' => null, + 'regular404Redirect' => 'foo-for-default.com', + 'invalidShortUrlRedirect' => null, + ]]; yield 'existing domain with redirects' => [[ 'domain' => 'detached-with-redirects.com', 'baseUrlRedirect' => null, diff --git a/module/Rest/test-api/Action/ListDomainsTest.php b/module/Rest/test-api/Action/ListDomainsTest.php index 5f33c20b..54039c41 100644 --- a/module/Rest/test-api/Action/ListDomainsTest.php +++ b/module/Rest/test-api/Action/ListDomainsTest.php @@ -21,6 +21,11 @@ class ListDomainsTest extends ApiTestCase self::assertEquals([ 'domains' => [ 'data' => $expectedDomains, + 'defaultRedirects' => [ + 'baseUrlRedirect' => null, + 'regular404Redirect' => null, + 'invalidShortUrlRedirect' => null, + ], ], ], $respPayload); } diff --git a/module/Rest/test/Action/Domain/ListDomainsActionTest.php b/module/Rest/test/Action/Domain/ListDomainsActionTest.php index 45575cc6..9bbe9723 100644 --- a/module/Rest/test/Action/Domain/ListDomainsActionTest.php +++ b/module/Rest/test/Action/Domain/ListDomainsActionTest.php @@ -22,11 +22,13 @@ class ListDomainsActionTest extends TestCase private ListDomainsAction $action; private ObjectProphecy $domainService; + private NotFoundRedirectOptions $options; public function setUp(): void { $this->domainService = $this->prophesize(DomainServiceInterface::class); - $this->action = new ListDomainsAction($this->domainService->reveal()); + $this->options = new NotFoundRedirectOptions(); + $this->action = new ListDomainsAction($this->domainService->reveal(), $this->options); } /** @test */ @@ -46,6 +48,7 @@ class ListDomainsActionTest extends TestCase self::assertEquals([ 'domains' => [ 'data' => $domains, + 'defaultRedirects' => $this->options, ], ], $payload); $listDomains->shouldHaveBeenCalledOnce(); From ee43e68a57115b4e8395b45cc7496419dcf8961d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 9 Dec 2021 12:32:02 +0100 Subject: [PATCH 6/7] 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(); From 9752abff1905c3e527a6162e85dbff8fdbfaba3e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 9 Dec 2021 12:43:49 +0100 Subject: [PATCH 7/7] Refactored method in DomainRepo, as one fo their arguments was no longer used --- docs/swagger/paths/v2_domains.json | 4 +-- module/Core/src/Domain/DomainService.php | 5 ++- .../Domain/Repository/DomainRepository.php | 13 +++---- .../Repository/DomainRepositoryInterface.php | 2 +- .../Core/src/Domain/Spec/IsNotAuthority.php | 22 ------------ .../Repository/DomainRepositoryTest.php | 35 ++++--------------- module/Core/test/Domain/DomainServiceTest.php | 2 +- 7 files changed, 16 insertions(+), 67 deletions(-) delete mode 100644 module/Core/src/Domain/Spec/IsNotAuthority.php diff --git a/docs/swagger/paths/v2_domains.json b/docs/swagger/paths/v2_domains.json index 40448016..d92677c1 100644 --- a/docs/swagger/paths/v2_domains.json +++ b/docs/swagger/paths/v2_domains.json @@ -4,8 +4,8 @@ "tags": [ "Domains" ], - "summary": "List existing domains", - "description": "Returns the list of all domains ever used, with a flag that tells if they are the default domain", + "summary": "List configured domains", + "description": "Returns the list of all domains that have been either used for some short URL, or have explicitly configured redirects.
It also includes the domain redirects, plus the default redirects that will be used for any non-explicitly-configured one.", "security": [ { "ApiKey": [] diff --git a/module/Core/src/Domain/DomainService.php b/module/Core/src/Domain/DomainService.php index 743312b6..d5e5d88c 100644 --- a/module/Core/src/Domain/DomainService.php +++ b/module/Core/src/Domain/DomainService.php @@ -50,7 +50,7 @@ class DomainService implements DomainServiceInterface /** @var DomainRepositoryInterface $repo */ $repo = $this->em->getRepository(Domain::class); $groups = group( - $repo->findDomainsWithout(null, $apiKey), // FIXME Always called with null as first arg + $repo->findDomains($apiKey), fn (Domain $domain) => $domain->getAuthority() === $this->defaultDomain ? 'default' : 'domains', ); @@ -73,8 +73,7 @@ class DomainService implements DomainServiceInterface public function findByAuthority(string $authority, ?ApiKey $apiKey = null): ?Domain { - $repo = $this->em->getRepository(Domain::class); - return $repo->findOneByAuthority($authority, $apiKey); + return $this->em->getRepository(Domain::class)->findOneByAuthority($authority, $apiKey); } /** diff --git a/module/Core/src/Domain/Repository/DomainRepository.php b/module/Core/src/Domain/Repository/DomainRepository.php index 1741cea7..4de3ea36 100644 --- a/module/Core/src/Domain/Repository/DomainRepository.php +++ b/module/Core/src/Domain/Repository/DomainRepository.php @@ -8,7 +8,6 @@ use Doctrine\ORM\Query\Expr\Join; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Happyr\DoctrineSpecification\Spec; use Shlinkio\Shlink\Core\Domain\Spec\IsDomain; -use Shlinkio\Shlink\Core\Domain\Spec\IsNotAuthority; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Spec\BelongsToApiKey; @@ -20,7 +19,7 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe /** * @return Domain[] */ - public function findDomainsWithout(?string $excludedAuthority, ?ApiKey $apiKey = null): array + public function findDomains(?ApiKey $apiKey = null): array { $qb = $this->createQueryBuilder('d'); $qb->leftJoin(ShortUrl::class, 's', Join::WITH, 's.domain = d') @@ -31,7 +30,7 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe ->orHaving($qb->expr()->isNotNull('d.regular404Redirect')) ->orHaving($qb->expr()->isNotNull('d.invalidShortUrlRedirect')); - $specs = $this->determineExtraSpecs($excludedAuthority, $apiKey); + $specs = $this->determineExtraSpecs($apiKey); foreach ($specs as [$alias, $spec]) { $this->applySpecification($qb, $spec, $alias); } @@ -47,7 +46,7 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe ->setParameter('authority', $authority) ->setMaxResults(1); - $specs = $this->determineExtraSpecs(null, $apiKey); + $specs = $this->determineExtraSpecs($apiKey); foreach ($specs as [$alias, $spec]) { $this->applySpecification($qb, $spec, $alias); } @@ -55,12 +54,8 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe return $qb->getQuery()->getOneOrNullResult(); } - private function determineExtraSpecs(?string $excludedAuthority, ?ApiKey $apiKey): iterable + private function determineExtraSpecs(?ApiKey $apiKey): iterable { - if ($excludedAuthority !== null) { - yield ['d', new IsNotAuthority($excludedAuthority)]; - } - // FIXME The $apiKey->spec() method cannot be used here, as it returns a single spec which assumes the // ShortUrl is the root entity. Here, the Domain is the root entity. // Think on a way to centralize the conditional behavior and make $apiKey->spec() more flexible. diff --git a/module/Core/src/Domain/Repository/DomainRepositoryInterface.php b/module/Core/src/Domain/Repository/DomainRepositoryInterface.php index 123e349d..69e74e5b 100644 --- a/module/Core/src/Domain/Repository/DomainRepositoryInterface.php +++ b/module/Core/src/Domain/Repository/DomainRepositoryInterface.php @@ -14,7 +14,7 @@ interface DomainRepositoryInterface extends ObjectRepository, EntitySpecificatio /** * @return Domain[] */ - public function findDomainsWithout(?string $excludedAuthority, ?ApiKey $apiKey = null): array; + public function findDomains(?ApiKey $apiKey = null): array; public function findOneByAuthority(string $authority, ?ApiKey $apiKey = null): ?Domain; } diff --git a/module/Core/src/Domain/Spec/IsNotAuthority.php b/module/Core/src/Domain/Spec/IsNotAuthority.php deleted file mode 100644 index 0f0f0653..00000000 --- a/module/Core/src/Domain/Spec/IsNotAuthority.php +++ /dev/null @@ -1,22 +0,0 @@ -authority)); - } -} diff --git a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php index 1eaf6ea9..382e58dd 100644 --- a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php +++ b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php @@ -50,27 +50,7 @@ class DomainRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - self::assertEquals( - [$barDomain, $bazDomain, $detachedWithRedirects, $fooDomain], - $this->repo->findDomainsWithout(null), - ); - self::assertEquals( - [$barDomain, $bazDomain, $detachedWithRedirects], - $this->repo->findDomainsWithout('foo.com'), - ); - self::assertEquals( - [$bazDomain, $detachedWithRedirects, $fooDomain], - $this->repo->findDomainsWithout('bar.com'), - ); - self::assertEquals( - [$barDomain, $detachedWithRedirects, $fooDomain], - $this->repo->findDomainsWithout('baz.com'), - ); - self::assertEquals( - [$barDomain, $bazDomain, $fooDomain], - $this->repo->findDomainsWithout('detached-with-redirects.com'), - ); - + self::assertEquals([$barDomain, $bazDomain, $detachedWithRedirects, $fooDomain], $this->repo->findDomains()); self::assertEquals($barDomain, $this->repo->findOneByAuthority('bar.com')); self::assertEquals($detachedWithRedirects, $this->repo->findOneByAuthority('detached-with-redirects.com')); self::assertNull($this->repo->findOneByAuthority('does-not-exist.com')); @@ -121,14 +101,11 @@ class DomainRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - self::assertEquals([$fooDomain], $this->repo->findDomainsWithout(null, $fooDomainApiKey)); - self::assertEquals([$barDomain], $this->repo->findDomainsWithout(null, $barDomainApiKey)); - self::assertEquals( - [$detachedWithRedirects], - $this->repo->findDomainsWithout(null, $detachedWithRedirectsApiKey), - ); - self::assertEquals([$bazDomain, $fooDomain], $this->repo->findDomainsWithout(null, $authorApiKey)); - self::assertEquals([], $this->repo->findDomainsWithout(null, $authorAndDomainApiKey)); + self::assertEquals([$fooDomain], $this->repo->findDomains($fooDomainApiKey)); + self::assertEquals([$barDomain], $this->repo->findDomains($barDomainApiKey)); + self::assertEquals([$detachedWithRedirects], $this->repo->findDomains($detachedWithRedirectsApiKey)); + self::assertEquals([$bazDomain, $fooDomain], $this->repo->findDomains($authorApiKey)); + self::assertEquals([], $this->repo->findDomains($authorAndDomainApiKey)); self::assertEquals($fooDomain, $this->repo->findOneByAuthority('foo.com', $authorApiKey)); self::assertNull($this->repo->findOneByAuthority('bar.com', $authorApiKey)); diff --git a/module/Core/test/Domain/DomainServiceTest.php b/module/Core/test/Domain/DomainServiceTest.php index 71922fe3..ea3cfe02 100644 --- a/module/Core/test/Domain/DomainServiceTest.php +++ b/module/Core/test/Domain/DomainServiceTest.php @@ -41,7 +41,7 @@ class DomainServiceTest extends TestCase { $repo = $this->prophesize(DomainRepositoryInterface::class); $getRepo = $this->em->getRepository(Domain::class)->willReturn($repo->reveal()); - $findDomains = $repo->findDomainsWithout(null, $apiKey)->willReturn($domains); + $findDomains = $repo->findDomains($apiKey)->willReturn($domains); $result = $this->domainService->listDomains($apiKey);