diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 09c4d96e..2f0a6aa8 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -32,6 +32,7 @@ return [ Tag\TagService::class => ConfigAbstractFactory::class, Service\ShortUrl\DeleteShortUrlService::class => ConfigAbstractFactory::class, Service\ShortUrl\ShortUrlResolver::class => ConfigAbstractFactory::class, + Service\ShortUrl\ShortCodeHelper::class => ConfigAbstractFactory::class, Domain\DomainService::class => ConfigAbstractFactory::class, Util\UrlValidator::class => ConfigAbstractFactory::class, @@ -61,7 +62,12 @@ return [ Options\NotFoundRedirectOptions::class => ['config.not_found_redirects'], Options\UrlShortenerOptions::class => ['config.url_shortener'], - Service\UrlShortener::class => [Util\UrlValidator::class, 'em', Resolver\PersistenceDomainResolver::class], + Service\UrlShortener::class => [ + Util\UrlValidator::class, + 'em', + Resolver\PersistenceDomainResolver::class, + Service\ShortUrl\ShortCodeHelper::class, + ], Service\VisitsTracker::class => [ 'em', EventDispatcherInterface::class, @@ -77,6 +83,7 @@ return [ Service\ShortUrl\ShortUrlResolver::class, ], Service\ShortUrl\ShortUrlResolver::class => ['em'], + Service\ShortUrl\ShortCodeHelper::class => ['em'], Domain\DomainService::class => ['em'], Util\UrlValidator::class => ['httpClient', Options\UrlShortenerOptions::class], @@ -104,7 +111,11 @@ return [ Mercure\MercureUpdatesGenerator::class => ['config.url_shortener.domain'], - Importer\ImportedLinksProcessor::class => ['em', Resolver\PersistenceDomainResolver::class], + Importer\ImportedLinksProcessor::class => [ + 'em', + Resolver\PersistenceDomainResolver::class, + Service\ShortUrl\ShortCodeHelper::class, + ], ], ]; diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index ea56fc82..8afa8206 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -133,7 +133,7 @@ class ShortUrl extends AbstractEntity /** * @throws ShortCodeCannotBeRegeneratedException */ - public function regenerateShortCode(): self + public function regenerateShortCode(): void { // In ShortUrls where a custom slug was provided, throw error, unless it is an imported one if ($this->customSlugWasProvided && $this->importSource === null) { @@ -146,7 +146,6 @@ class ShortUrl extends AbstractEntity } $this->shortCode = generateRandomShortCode($this->shortCodeLength); - return $this; } public function getValidSince(): ?Chronos diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index 4dc8d1a4..90866057 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -7,8 +7,8 @@ namespace Shlinkio\Shlink\Core\Importer; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; -use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; +use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; use Shlinkio\Shlink\Core\Util\DoctrineBatchIterator; use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; @@ -23,11 +23,16 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface private EntityManagerInterface $em; private DomainResolverInterface $domainResolver; + private ShortCodeHelperInterface $shortCodeHelper; - public function __construct(EntityManagerInterface $em, DomainResolverInterface $domainResolver) - { + public function __construct( + EntityManagerInterface $em, + DomainResolverInterface $domainResolver, + ShortCodeHelperInterface $shortCodeHelper + ) { $this->em = $em; $this->domainResolver = $domainResolver; + $this->shortCodeHelper = $shortCodeHelper; } /** @@ -68,7 +73,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface StyleInterface $io, bool $importShortCodes ): bool { - if ($this->ensureShortCodeUniqueness($shortUrl, $importShortCodes)) { + if ($this->shortCodeHelper->ensureShortCodeUniqueness($shortUrl, $importShortCodes)) { return true; } @@ -87,26 +92,4 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface return $this->handleShortcodeUniqueness($url, $shortUrl, $io, false); } - - private function ensureShortCodeUniqueness(ShortUrl $shortUrlToBeCreated, bool $hasCustomSlug): bool - { - $shortCode = $shortUrlToBeCreated->getShortCode(); - $domain = $shortUrlToBeCreated->getDomain(); - $domainAuthority = $domain !== null ? $domain->getAuthority() : null; - - /** @var ShortUrlRepository $repo */ - $repo = $this->em->getRepository(ShortUrl::class); - $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domainAuthority); - - if (! $otherShortUrlsExist) { - return true; - } - - if ($hasCustomSlug) { - return false; - } - - $shortUrlToBeCreated->regenerateShortCode(); - return $this->ensureShortCodeUniqueness($shortUrlToBeCreated, $hasCustomSlug); - } } diff --git a/module/Core/src/Service/ShortUrl/ShortCodeHelper.php b/module/Core/src/Service/ShortUrl/ShortCodeHelper.php new file mode 100644 index 00000000..6e4e57ac --- /dev/null +++ b/module/Core/src/Service/ShortUrl/ShortCodeHelper.php @@ -0,0 +1,41 @@ +em = $em; + } + + public function ensureShortCodeUniqueness(ShortUrl $shortUrlToBeCreated, bool $hasCustomSlug): bool + { + $shortCode = $shortUrlToBeCreated->getShortCode(); + $domain = $shortUrlToBeCreated->getDomain(); + $domainAuthority = $domain !== null ? $domain->getAuthority() : null; + + /** @var ShortUrlRepository $repo */ + $repo = $this->em->getRepository(ShortUrl::class); + $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domainAuthority); + + if (! $otherShortUrlsExist) { + return true; + } + + if ($hasCustomSlug) { + return false; + } + + $shortUrlToBeCreated->regenerateShortCode(); + return $this->ensureShortCodeUniqueness($shortUrlToBeCreated, $hasCustomSlug); + } +} diff --git a/module/Core/src/Service/ShortUrl/ShortCodeHelperInterface.php b/module/Core/src/Service/ShortUrl/ShortCodeHelperInterface.php new file mode 100644 index 00000000..af3f2aa5 --- /dev/null +++ b/module/Core/src/Service/ShortUrl/ShortCodeHelperInterface.php @@ -0,0 +1,12 @@ +urlValidator = $urlValidator; $this->em = $em; $this->domainResolver = $domainResolver; + $this->shortCodeHelper = $shortCodeHelper; } /** @@ -83,20 +86,16 @@ class UrlShortener implements UrlShortenerInterface private function verifyShortCodeUniqueness(ShortUrlMeta $meta, ShortUrl $shortUrlToBeCreated): void { - $shortCode = $shortUrlToBeCreated->getShortCode(); - $domain = $meta->getDomain(); + $couldBeMadeUnique = $this->shortCodeHelper->ensureShortCodeUniqueness( + $shortUrlToBeCreated, + $meta->hasCustomSlug(), + ); - /** @var ShortUrlRepository $repo */ - $repo = $this->em->getRepository(ShortUrl::class); - $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domain); + if (! $couldBeMadeUnique) { + $domain = $shortUrlToBeCreated->getDomain(); + $domainAuthority = $domain !== null ? $domain->getAuthority() : null; - if ($otherShortUrlsExist && $meta->hasCustomSlug()) { - throw NonUniqueSlugException::fromSlug($shortCode, $domain); - } - - if ($otherShortUrlsExist) { - $shortUrlToBeCreated->regenerateShortCode(); - $this->verifyShortCodeUniqueness($meta, $shortUrlToBeCreated); + throw NonUniqueSlugException::fromSlug($shortUrlToBeCreated->getShortCode(), $domainAuthority); } } } diff --git a/module/Core/test/Entity/ShortUrlTest.php b/module/Core/test/Entity/ShortUrlTest.php index c143ae76..9f28c41b 100644 --- a/module/Core/test/Entity/ShortUrlTest.php +++ b/module/Core/test/Entity/ShortUrlTest.php @@ -10,8 +10,8 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; - use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; + use function Functional\map; use function range; use function strlen; diff --git a/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php b/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php new file mode 100644 index 00000000..d77aecd5 --- /dev/null +++ b/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php @@ -0,0 +1,77 @@ +em = $this->prophesize(EntityManagerInterface::class); + $this->helper = new ShortCodeHelper($this->em->reveal()); + + $this->shortUrl = $this->prophesize(ShortUrl::class); + $this->shortUrl->getShortCode()->willReturn('abc123'); + } + + /** + * @test + * @dataProvider provideDomains + */ + public function shortCodeIsRegeneratedIfAlreadyInUse(?Domain $domain, ?string $expectedAuthority): void + { + $callIndex = 0; + $expectedCalls = 3; + $repo = $this->prophesize(ShortUrlRepository::class); + $shortCodeIsInUse = $repo->shortCodeIsInUse('abc123', $expectedAuthority)->will( + function () use (&$callIndex, $expectedCalls) { + $callIndex++; + return $callIndex < $expectedCalls; + }, + ); + $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + $this->shortUrl->getDomain()->willReturn($domain); + + $result = $this->helper->ensureShortCodeUniqueness($this->shortUrl->reveal(), false); + + self::assertTrue($result); + $this->shortUrl->regenerateShortCode()->shouldHaveBeenCalledTimes($expectedCalls - 1); + $getRepo->shouldBeCalledTimes($expectedCalls); + $shortCodeIsInUse->shouldBeCalledTimes($expectedCalls); + } + + public function provideDomains(): iterable + { + yield 'no domain' => [null, null]; + yield 'domain' => [new Domain($authority = 'doma.in'), $authority]; + } + + /** @test */ + public function inUseSlugReturnsError(): void + { + $repo = $this->prophesize(ShortUrlRepository::class); + $shortCodeIsInUse = $repo->shortCodeIsInUse('abc123', null)->willReturn(true); + $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + $this->shortUrl->getDomain()->willReturn(null); + + $result = $this->helper->ensureShortCodeUniqueness($this->shortUrl->reveal(), true); + + self::assertFalse($result); + $this->shortUrl->regenerateShortCode()->shouldNotHaveBeenCalled(); + $getRepo->shouldBeCalledOnce(); + $shortCodeIsInUse->shouldBeCalledOnce(); + } +} diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index ba7185bf..7945c32b 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -18,6 +18,7 @@ use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; +use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; use Shlinkio\Shlink\Core\Service\UrlShortener; use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; @@ -26,6 +27,7 @@ class UrlShortenerTest extends TestCase private UrlShortener $urlShortener; private ObjectProphecy $em; private ObjectProphecy $urlValidator; + private ObjectProphecy $shortCodeHelper; public function setUp(): void { @@ -51,10 +53,14 @@ class UrlShortenerTest extends TestCase $repo->shortCodeIsInUse(Argument::cetera())->willReturn(false); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + $this->shortCodeHelper = $this->prophesize(ShortCodeHelperInterface::class); + $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(true); + $this->urlShortener = new UrlShortener( $this->urlValidator->reveal(), $this->em->reveal(), new SimpleDomainResolver(), + $this->shortCodeHelper->reveal(), ); } @@ -71,29 +77,18 @@ class UrlShortenerTest extends TestCase } /** @test */ - public function shortCodeIsRegeneratedIfAlreadyInUse(): void + public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void { - $callIndex = 0; - $expectedCalls = 3; - $repo = $this->prophesize(ShortUrlRepository::class); - $shortCodeIsInUse = $repo->shortCodeIsInUse(Argument::cetera())->will( - function () use (&$callIndex, $expectedCalls) { - $callIndex++; - return $callIndex < $expectedCalls; - }, - ); - $repo->findBy(Argument::cetera())->willReturn([]); - $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + $ensureUniqueness = $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(false); - $shortUrl = $this->urlShortener->urlToShortCode( + $ensureUniqueness->shouldBeCalledOnce(); + $this->expectException(NonUniqueSlugException::class); + + $this->urlShortener->urlToShortCode( 'http://foobar.com/12345/hello?foo=bar', [], - ShortUrlMeta::createEmpty(), + ShortUrlMeta::fromRawData(['customSlug' => 'custom-slug']), ); - - self::assertEquals('http://foobar.com/12345/hello?foo=bar', $shortUrl->getLongUrl()); - $getRepo->shouldBeCalledTimes($expectedCalls); - $shortCodeIsInUse->shouldBeCalledTimes($expectedCalls); } /** @test */ @@ -115,25 +110,6 @@ class UrlShortenerTest extends TestCase ); } - /** @test */ - public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void - { - $repo = $this->prophesize(ShortUrlRepository::class); - $shortCodeIsInUse = $repo->shortCodeIsInUse('custom-slug', null)->willReturn(true); - $repo->findBy(Argument::cetera())->willReturn([]); - $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - - $shortCodeIsInUse->shouldBeCalledOnce(); - $getRepo->shouldBeCalled(); - $this->expectException(NonUniqueSlugException::class); - - $this->urlShortener->urlToShortCode( - 'http://foobar.com/12345/hello?foo=bar', - [], - ShortUrlMeta::fromRawData(['customSlug' => 'custom-slug']), - ); - } - /** * @test * @dataProvider provideExistingShortUrls