diff --git a/module/Core/src/Model/ShortUrlIdentifier.php b/module/Core/src/Model/ShortUrlIdentifier.php index 4a74ba07..a277782c 100644 --- a/module/Core/src/Model/ShortUrlIdentifier.php +++ b/module/Core/src/Model/ShortUrlIdentifier.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Model; use Psr\Http\Message\ServerRequestInterface; +use Shlinkio\Shlink\Core\Entity\ShortUrl; use Symfony\Component\Console\Input\InputInterface; final class ShortUrlIdentifier @@ -42,6 +43,19 @@ final class ShortUrlIdentifier return new self($shortCode, $domain); } + public static function fromShortUrl(ShortUrl $shortUrl): self + { + $domain = $shortUrl->getDomain(); + $domainAuthority = $domain !== null ? $domain->getAuthority() : null; + + return new self($shortUrl->getShortCode(), $domainAuthority); + } + + public static function fromShortCodeAndDomain(string $shortCode, ?string $domain = null): self + { + return new self($shortCode, $domain); + } + public function shortCode(): string { return $this->shortCode; diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index eacf293b..a1b93f4d 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Repository; +use Doctrine\DBAL\LockMode; use Doctrine\ORM\Query\Expr\Join; use Doctrine\ORM\QueryBuilder; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; @@ -11,6 +12,7 @@ use Happyr\DoctrineSpecification\Specification\Specification; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; @@ -180,12 +182,24 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU return $qb->getQuery()->getOneOrNullResult(); } - public function shortCodeIsInUse(string $slug, ?string $domain = null, ?Specification $spec = null): bool + public function shortCodeIsInUse(ShortUrlIdentifier $identifier, ?Specification $spec = null): bool { - $qb = $this->createFindOneQueryBuilder($slug, $domain, $spec); - $qb->select('COUNT(DISTINCT s.id)'); + return $this->doShortCodeIsInUse($identifier, $spec, null); + } - return ((int) $qb->getQuery()->getSingleScalarResult()) > 0; + public function shortCodeIsInUseWithLock(ShortUrlIdentifier $identifier, ?Specification $spec = null): bool + { + return $this->doShortCodeIsInUse($identifier, $spec, LockMode::PESSIMISTIC_WRITE); + } + + private function doShortCodeIsInUse(ShortUrlIdentifier $identifier, ?Specification $spec, ?int $lockMode): bool + { + $qb = $this->createFindOneQueryBuilder($identifier->shortCode(), $identifier->domain(), $spec); + $qb->select('s.id'); + + $query = $qb->getQuery()->setLockMode($lockMode); + + return $query->getOneOrNullResult() !== null; } private function createFindOneQueryBuilder(string $slug, ?string $domain, ?Specification $spec): QueryBuilder diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index 5d8fa924..9b6dffc8 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -9,6 +9,7 @@ use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepositoryInterfa use Happyr\DoctrineSpecification\Specification\Specification; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; @@ -36,7 +37,9 @@ interface ShortUrlRepositoryInterface extends ObjectRepository, EntitySpecificat public function findOne(string $shortCode, ?string $domain = null, ?Specification $spec = null): ?ShortUrl; - public function shortCodeIsInUse(string $slug, ?string $domain, ?Specification $spec = null): bool; + public function shortCodeIsInUse(ShortUrlIdentifier $identifier, ?Specification $spec = null): bool; + + public function shortCodeIsInUseWithLock(ShortUrlIdentifier $identifier, ?Specification $spec = null): bool; public function findOneMatching(ShortUrlMeta $meta): ?ShortUrl; diff --git a/module/Core/src/Service/ShortUrl/ShortCodeHelper.php b/module/Core/src/Service/ShortUrl/ShortCodeHelper.php index 3df4c016..83c3397e 100644 --- a/module/Core/src/Service/ShortUrl/ShortCodeHelper.php +++ b/module/Core/src/Service/ShortUrl/ShortCodeHelper.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Service\ShortUrl; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; class ShortCodeHelper implements ShortCodeHelperInterface // TODO Rename to ShortCodeUniquenessHelper @@ -19,13 +20,9 @@ class ShortCodeHelper implements ShortCodeHelperInterface // TODO Rename to Shor 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); + $otherShortUrlsExist = $repo->shortCodeIsInUseWithLock(ShortUrlIdentifier::fromShortUrl($shortUrlToBeCreated)); if (! $otherShortUrlsExist) { return true; diff --git a/module/Core/src/Visit/VisitsStatsHelper.php b/module/Core/src/Visit/VisitsStatsHelper.php index 1aa03c46..dfa00a4c 100644 --- a/module/Core/src/Visit/VisitsStatsHelper.php +++ b/module/Core/src/Visit/VisitsStatsHelper.php @@ -58,7 +58,7 @@ class VisitsStatsHelper implements VisitsStatsHelperInterface /** @var ShortUrlRepositoryInterface $repo */ $repo = $this->em->getRepository(ShortUrl::class); - if (! $repo->shortCodeIsInUse($identifier->shortCode(), $identifier->domain(), $spec)) { + if (! $repo->shortCodeIsInUse($identifier, $spec)) { throw ShortUrlNotFoundException::fromNotFound($identifier); } diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index bd5b22d4..345e0911 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Core\Model\Visitor; @@ -180,12 +181,18 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - self::assertTrue($this->repo->shortCodeIsInUse('my-cool-slug')); - self::assertFalse($this->repo->shortCodeIsInUse('my-cool-slug', 'doma.in')); - self::assertFalse($this->repo->shortCodeIsInUse('slug-not-in-use')); - self::assertFalse($this->repo->shortCodeIsInUse('another-slug')); - self::assertFalse($this->repo->shortCodeIsInUse('another-slug', 'example.com')); - self::assertTrue($this->repo->shortCodeIsInUse('another-slug', 'doma.in')); + self::assertTrue($this->repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain('my-cool-slug'))); + self::assertFalse($this->repo->shortCodeIsInUse( + ShortUrlIdentifier::fromShortCodeAndDomain('my-cool-slug', 'doma.in'), + )); + self::assertFalse($this->repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain('slug-not-in-use'))); + self::assertFalse($this->repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain('another-slug'))); + self::assertFalse($this->repo->shortCodeIsInUse( + ShortUrlIdentifier::fromShortCodeAndDomain('another-slug', 'example.com'), + )); + self::assertTrue($this->repo->shortCodeIsInUse( + ShortUrlIdentifier::fromShortCodeAndDomain('another-slug', 'doma.in'), + )); } /** @test */ diff --git a/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php b/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php index 047dbc96..ca3b463f 100644 --- a/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php +++ b/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php @@ -10,6 +10,7 @@ use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelper; @@ -39,12 +40,12 @@ class ShortCodeHelperTest extends TestCase $callIndex = 0; $expectedCalls = 3; $repo = $this->prophesize(ShortUrlRepository::class); - $shortCodeIsInUse = $repo->shortCodeIsInUse('abc123', $expectedAuthority)->will( - function () use (&$callIndex, $expectedCalls) { - $callIndex++; - return $callIndex < $expectedCalls; - }, - ); + $shortCodeIsInUse = $repo->shortCodeIsInUseWithLock( + ShortUrlIdentifier::fromShortCodeAndDomain('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); @@ -66,7 +67,9 @@ class ShortCodeHelperTest extends TestCase public function inUseSlugReturnsError(): void { $repo = $this->prophesize(ShortUrlRepository::class); - $shortCodeIsInUse = $repo->shortCodeIsInUse('abc123', null)->willReturn(true); + $shortCodeIsInUse = $repo->shortCodeIsInUseWithLock( + ShortUrlIdentifier::fromShortCodeAndDomain('abc123'), + )->willReturn(true); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->shortUrl->getDomain()->willReturn(null); diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 7e319314..6bac432e 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -46,7 +46,6 @@ class UrlShortenerTest extends TestCase return $callback(); }); $repo = $this->prophesize(ShortUrlRepository::class); - $repo->shortCodeIsInUse(Argument::cetera())->willReturn(false); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->shortCodeHelper = $this->prophesize(ShortCodeHelperInterface::class); diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index e6f067da..372bda9a 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -81,7 +81,9 @@ class VisitsStatsHelperTest extends TestCase $shortCode = '123ABC'; $spec = $apiKey === null ? null : $apiKey->spec(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $count = $repo->shortCodeIsInUse($shortCode, null, $spec)->willReturn(true); + $count = $repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), $spec)->willReturn( + true, + ); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); @@ -101,7 +103,9 @@ class VisitsStatsHelperTest extends TestCase { $shortCode = '123ABC'; $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $count = $repo->shortCodeIsInUse($shortCode, null, null)->willReturn(false); + $count = $repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), null)->willReturn( + false, + ); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); $this->expectException(ShortUrlNotFoundException::class);