diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index cebe1785..6cf6378a 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -10,6 +10,7 @@ use Doctrine\Common\Collections\Collection; use Shlinkio\Shlink\Common\Entity\AbstractEntity; use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface; use Shlinkio\Shlink\Core\Domain\Resolver\SimpleDomainResolver; +use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Zend\Diactoros\Uri; @@ -39,6 +40,8 @@ class ShortUrl extends AbstractEntity private $maxVisits; /** @var Domain|null */ private $domain; + /** @var bool */ + private $customSlugWasProvided; public function __construct( string $longUrl, @@ -54,6 +57,7 @@ class ShortUrl extends AbstractEntity $this->validSince = $meta->getValidSince(); $this->validUntil = $meta->getValidUntil(); $this->maxVisits = $meta->getMaxVisits(); + $this->customSlugWasProvided = $meta->hasCustomSlug(); $this->shortCode = $meta->getCustomSlug() ?? generateRandomShortCode(); $this->domain = ($domainResolver ?? new SimpleDomainResolver())->resolveDomain($meta->getDomain()); } @@ -103,6 +107,25 @@ class ShortUrl extends AbstractEntity } } + /** + * @throws ShortCodeCannotBeRegeneratedException + */ + public function regenerateShortCode(): self + { + // In ShortUrls where a custom slug was provided, do nothing + if ($this->customSlugWasProvided) { + throw ShortCodeCannotBeRegeneratedException::forShortUrlWithCustomSlug(); + } + + // The short code can be regenerated only on ShortUrl which have not been persisted yet + if ($this->id !== null) { + throw ShortCodeCannotBeRegeneratedException::forShortUrlAlreadyPersisted(); + } + + $this->shortCode = generateRandomShortCode(); + return $this; + } + public function getValidSince(): ?Chronos { return $this->validSince; diff --git a/module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php b/module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php new file mode 100644 index 00000000..e1df3068 --- /dev/null +++ b/module/Core/src/Exception/ShortCodeCannotBeRegeneratedException.php @@ -0,0 +1,29 @@ +reasonIsSlug = true; + + return $e; + } + + public static function forShortUrlAlreadyPersisted(): self + { + return new self('The short code can be regenerated only on new ShortUrls which have not been persisted yet.'); + } + + public function reasonIsSlug(): bool + { + return $this->reasonIsSlug; + } +} diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index 1638fb33..47f8f985 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -155,7 +155,7 @@ DQL; return $shortUrl !== null && ! $shortUrl->maxVisitsReached() ? $shortUrl : null; } - public function slugIsInUse(string $slug, ?string $domain = null): bool + public function shortCodeIsInUse(string $slug, ?string $domain = null): bool { $qb = $this->getEntityManager()->createQueryBuilder(); $qb->select('COUNT(DISTINCT s.id)') diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index 0535d979..da5cef61 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -29,5 +29,5 @@ interface ShortUrlRepositoryInterface extends ObjectRepository public function findOneByShortCode(string $shortCode, ?string $domain = null): ?ShortUrl; - public function slugIsInUse(string $slug, ?string $domain): bool; + public function shortCodeIsInUse(string $slug, ?string $domain): bool; } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index 510f6d41..f282ba54 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -16,7 +16,6 @@ use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; -use Shlinkio\Shlink\Core\Exception\RuntimeException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; @@ -47,7 +46,7 @@ class UrlShortener implements UrlShortenerInterface * @param string[] $tags * @throws NonUniqueSlugException * @throws InvalidUrlException - * @throws RuntimeException + * @throws Throwable */ public function urlToShortCode(UriInterface $url, array $tags, ShortUrlMeta $meta): ShortUrl { @@ -63,28 +62,26 @@ class UrlShortener implements UrlShortenerInterface if ($this->options->isUrlValidationEnabled()) { $this->checkUrlExists($url); } - $this->verifyCustomSlug($meta); - // Transactionally insert the short url, then generate the short code and finally update the short code + $this->em->beginTransaction(); + $shortUrl = new ShortUrl($url, $meta, new PersistenceDomainResolver($this->em)); + $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); + try { - $this->em->beginTransaction(); - - // First, create the short URL with an empty short code - $shortUrl = new ShortUrl($url, $meta, new PersistenceDomainResolver($this->em)); - $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); + $this->verifyShortCodeUniqueness($meta, $shortUrl); $this->em->persist($shortUrl); $this->em->flush(); - $this->em->commit(); - return $shortUrl; } catch (Throwable $e) { if ($this->em->getConnection()->isTransactionActive()) { $this->em->rollback(); $this->em->close(); } - throw new RuntimeException('An error occurred while persisting the short URL', -1, $e); + throw $e; } + + return $shortUrl; } private function findExistingShortUrlIfExists(string $url, array $tags, ShortUrlMeta $meta): ?ShortUrl @@ -124,20 +121,22 @@ class UrlShortener implements UrlShortenerInterface } } - private function verifyCustomSlug(ShortUrlMeta $meta): void + private function verifyShortCodeUniqueness(ShortUrlMeta $meta, ShortUrl $shortUrlToBeCreated): void { - if (! $meta->hasCustomSlug()) { - return; - } - - $customSlug = $meta->getCustomSlug(); + $shortCode = $shortUrlToBeCreated->getShortCode(); $domain = $meta->getDomain(); /** @var ShortUrlRepository $repo */ $repo = $this->em->getRepository(ShortUrl::class); - $shortUrlsCount = $repo->slugIsInUse($customSlug, $domain); - if ($shortUrlsCount > 0) { - throw NonUniqueSlugException::fromSlug($customSlug, $domain); + $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domain); + + if ($otherShortUrlsExist && $meta->hasCustomSlug()) { + throw NonUniqueSlugException::fromSlug($shortCode, $domain); + } + + if ($otherShortUrlsExist) { + $shortUrlToBeCreated->regenerateShortCode(); + $this->verifyShortCodeUniqueness($meta, $shortUrlToBeCreated); } } diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index 29c9860b..f3913ea2 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -169,7 +169,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase } /** @test */ - public function slugIsInUseLooksForShortUrlInProperSetOfTables(): void + public function shortCodeIsInUseLooksForShortUrlInProperSetOfTables(): void { $shortUrlWithoutDomain = new ShortUrl('foo', ShortUrlMeta::createFromRawData(['customSlug' => 'my-cool-slug'])); $this->getEntityManager()->persist($shortUrlWithoutDomain); @@ -182,11 +182,11 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - $this->assertTrue($this->repo->slugIsInUse('my-cool-slug')); - $this->assertFalse($this->repo->slugIsInUse('my-cool-slug', 'doma.in')); - $this->assertFalse($this->repo->slugIsInUse('slug-not-in-use')); - $this->assertFalse($this->repo->slugIsInUse('another-slug')); - $this->assertFalse($this->repo->slugIsInUse('another-slug', 'example.com')); - $this->assertTrue($this->repo->slugIsInUse('another-slug', 'doma.in')); + $this->assertTrue($this->repo->shortCodeIsInUse('my-cool-slug')); + $this->assertFalse($this->repo->shortCodeIsInUse('my-cool-slug', 'doma.in')); + $this->assertFalse($this->repo->shortCodeIsInUse('slug-not-in-use')); + $this->assertFalse($this->repo->shortCodeIsInUse('another-slug')); + $this->assertFalse($this->repo->shortCodeIsInUse('another-slug', 'example.com')); + $this->assertTrue($this->repo->shortCodeIsInUse('another-slug', 'doma.in')); } } diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 55f2452c..0185ef1a 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -19,7 +19,6 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; -use Shlinkio\Shlink\Core\Exception\RuntimeException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; @@ -55,7 +54,7 @@ class UrlShortenerTest extends TestCase $shortUrl->setId('10'); }); $repo = $this->prophesize(ShortUrlRepository::class); - $repo->slugIsInUse(Argument::cetera())->willReturn(false); + $repo->shortCodeIsInUse(Argument::cetera())->willReturn(false); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->setUrlShortener(false); @@ -83,7 +82,7 @@ class UrlShortenerTest extends TestCase } /** @test */ - public function exceptionIsThrownWhenOrmThrowsException(): void + public function transactionIsRolledBackAndExceptionRethrownWhenExceptionIsThrown(): void { $conn = $this->prophesize(Connection::class); $conn->isTransactionActive()->willReturn(true); @@ -93,7 +92,7 @@ class UrlShortenerTest extends TestCase $this->em->flush()->willThrow(new ORMException()); - $this->expectException(RuntimeException::class); + $this->expectException(ORMException::class); $this->urlShortener->urlToShortCode( new Uri('http://foobar.com/12345/hello?foo=bar'), [], @@ -122,11 +121,11 @@ class UrlShortenerTest extends TestCase public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void { $repo = $this->prophesize(ShortUrlRepository::class); - $slugIsInUse = $repo->slugIsInUse('custom-slug', null)->willReturn(true); + $shortCodeIsInUse = $repo->shortCodeIsInUse('custom-slug', null)->willReturn(true); $repo->findBy(Argument::cetera())->willReturn([]); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - $slugIsInUse->shouldBeCalledOnce(); + $shortCodeIsInUse->shouldBeCalledOnce(); $getRepo->shouldBeCalled(); $this->expectException(NonUniqueSlugException::class); @@ -145,26 +144,24 @@ class UrlShortenerTest extends TestCase string $url, array $tags, ShortUrlMeta $meta, - ?ShortUrl $expected + ShortUrl $expected ): void { $repo = $this->prophesize(ShortUrlRepository::class); - $findExisting = $repo->findBy(Argument::any())->willReturn($expected !== null ? [$expected] : []); + $findExisting = $repo->findBy(Argument::any())->willReturn([$expected]); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlShortener->urlToShortCode(new Uri($url), $tags, $meta); $findExisting->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); - if ($expected) { - $this->assertSame($expected, $result); - } + $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->assertSame($expected, $result); } public function provideExistingShortUrls(): iterable { $url = 'http://foo.com'; - yield [$url, [], ShortUrlMeta::createFromRawData(['findIfExists' => true]), null]; yield [$url, [], ShortUrlMeta::createFromRawData(['findIfExists' => true]), new ShortUrl($url)]; yield [$url, [], ShortUrlMeta::createFromRawData( ['findIfExists' => true, 'customSlug' => 'foo']