From 495643f4f160c5aca531023e97b7f8235ca072f2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 1 Oct 2019 21:42:35 +0200 Subject: [PATCH] Ensured domain is taken into account when checking if a slug is in use --- .../Resolver/PersistenceDomainResolver.php | 4 ++- module/Core/src/Entity/ShortUrl.php | 27 +++++++++++++++ .../src/Exception/NonUniqueSlugException.php | 9 +++-- .../src/Repository/ShortUrlRepository.php | 21 ++++++++++++ .../ShortUrlRepositoryInterface.php | 2 ++ module/Core/src/Service/UrlShortener.php | 31 +++-------------- .../Repository/ShortUrlRepositoryTest.php | 32 ++++++++++++++--- .../Exception/NonUniqueSlugExceptionTest.php | 34 +++++++++++++++++++ module/Core/test/Service/UrlShortenerTest.php | 6 ++-- 9 files changed, 130 insertions(+), 36 deletions(-) create mode 100644 module/Core/test/Exception/NonUniqueSlugExceptionTest.php diff --git a/module/Core/src/Domain/Resolver/PersistenceDomainResolver.php b/module/Core/src/Domain/Resolver/PersistenceDomainResolver.php index 10ae41ae..065e870c 100644 --- a/module/Core/src/Domain/Resolver/PersistenceDomainResolver.php +++ b/module/Core/src/Domain/Resolver/PersistenceDomainResolver.php @@ -22,6 +22,8 @@ class PersistenceDomainResolver implements DomainResolverInterface return null; } - return $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]) ?? new Domain($domain); + /** @var Domain|null $existingDomain */ + $existingDomain = $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]); + return $existingDomain ?? new Domain($domain); } } diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index f3f85cb3..2e30ba2d 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -12,7 +12,10 @@ use Shlinkio\Shlink\Core\Domain\Resolver\SimpleDomainResolver; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Zend\Diactoros\Uri; +use function array_reduce; use function count; +use function Functional\contains; +use function Functional\invoke; class ShortUrl extends AbstractEntity { @@ -161,4 +164,28 @@ class ShortUrl extends AbstractEntity return $this->domain->getAuthority(); } + + public function matchesCriteria(ShortUrlMeta $meta, array $tags): bool + { + if ($meta->hasMaxVisits() && $meta->getMaxVisits() !== $this->maxVisits) { + return false; + } + if ($meta->hasValidSince() && ! $meta->getValidSince()->eq($this->validSince)) { + return false; + } + if ($meta->hasValidUntil() && ! $meta->getValidUntil()->eq($this->validUntil)) { + return false; + } + + $shortUrlTags = invoke($this->getTags(), '__toString'); + $hasAllTags = count($shortUrlTags) === count($tags) && array_reduce( + $tags, + function (bool $hasAllTags, string $tag) use ($shortUrlTags) { + return $hasAllTags && contains($shortUrlTags, $tag); + }, + true + ); + + return $hasAllTags; + } } diff --git a/module/Core/src/Exception/NonUniqueSlugException.php b/module/Core/src/Exception/NonUniqueSlugException.php index 09e22b9b..2b975808 100644 --- a/module/Core/src/Exception/NonUniqueSlugException.php +++ b/module/Core/src/Exception/NonUniqueSlugException.php @@ -7,8 +7,13 @@ use function sprintf; class NonUniqueSlugException extends InvalidArgumentException { - public static function fromSlug(string $slug): self + public static function fromSlug(string $slug, ?string $domain): self { - return new self(sprintf('Provided slug "%s" is not unique.', $slug)); + $suffix = ''; + if ($domain !== null) { + $suffix = sprintf(' for domain "%s"', $domain); + } + + return new self(sprintf('Provided slug "%s" is not unique%s.', $slug, $suffix)); } } diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index b1d22b08..b6fdb7f1 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -138,4 +138,25 @@ DQL; $result = $query->getOneOrNullResult(); return $result === null || $result->maxVisitsReached() ? null : $result; } + + public function slugIsInUse(string $slug, ?string $domain = null): bool + { + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->select('COUNT(DISTINCT s.id)') + ->from(ShortUrl::class, 's') + ->where($qb->expr()->isNotNull('s.shortCode')) + ->andWhere($qb->expr()->eq('s.shortCode', ':slug')) + ->setParameter('slug', $slug); + + if ($domain !== null) { + $qb->join('s.domain', 'd') + ->andWhere($qb->expr()->eq('d.authority', ':authority')) + ->setParameter('authority', $domain); + } else { + $qb->andWhere($qb->expr()->isNull('s.domain')); + } + + $result = (int) $qb->getQuery()->getSingleScalarResult(); + return $result > 0; + } } diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index fcec73e4..b257f20a 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -27,4 +27,6 @@ interface ShortUrlRepositoryInterface extends ObjectRepository public function countList(?string $searchTerm = null, array $tags = []): int; public function findOneByShortCode(string $shortCode): ?ShortUrl; + + public function slugIsInUse(string $slug, ?string $domain): bool; } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index 8b456010..ec257e26 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -23,11 +23,8 @@ use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Throwable; use function array_reduce; -use function count; use function floor; use function fmod; -use function Functional\contains; -use function Functional\invoke; use function preg_match; use function strlen; @@ -121,30 +118,11 @@ class UrlShortener implements UrlShortenerInterface // Iterate short URLs until one that matches is found, or return null otherwise return array_reduce($shortUrls, function (?ShortUrl $found, ShortUrl $shortUrl) use ($tags, $meta) { - if ($found) { + if ($found !== null) { return $found; } - if ($meta->hasMaxVisits() && $meta->getMaxVisits() !== $shortUrl->getMaxVisits()) { - return null; - } - if ($meta->hasValidSince() && ! $meta->getValidSince()->eq($shortUrl->getValidSince())) { - return null; - } - if ($meta->hasValidUntil() && ! $meta->getValidUntil()->eq($shortUrl->getValidUntil())) { - return null; - } - - $shortUrlTags = invoke($shortUrl->getTags(), '__toString'); - $hasAllTags = count($shortUrlTags) === count($tags) && array_reduce( - $tags, - function (bool $hasAllTags, string $tag) use ($shortUrlTags) { - return $hasAllTags && contains($shortUrlTags, $tag); - }, - true - ); - - return $hasAllTags ? $shortUrl : null; + return $shortUrl->matchesCriteria($meta, $tags) ? $shortUrl : null; }); } @@ -166,12 +144,13 @@ class UrlShortener implements UrlShortenerInterface } $customSlug = $meta->getCustomSlug(); + $domain = $meta->getDomain(); /** @var ShortUrlRepository $repo */ $repo = $this->em->getRepository(ShortUrl::class); - $shortUrlsCount = $repo->count(['shortCode' => $customSlug]); + $shortUrlsCount = $repo->slugIsInUse($customSlug, $domain); if ($shortUrlsCount > 0) { - throw NonUniqueSlugException::fromSlug($customSlug); + throw NonUniqueSlugException::fromSlug($customSlug, $domain); } } diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index 2a243ce3..35d647e7 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -5,6 +5,7 @@ namespace ShlinkioTest\Shlink\Core\Repository; use Cake\Chronos\Chronos; use Doctrine\Common\Collections\ArrayCollection; +use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Entity\Visit; @@ -21,6 +22,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase Tag::class, Visit::class, ShortUrl::class, + Domain::class, ]; /** @var ShortUrlRepository */ @@ -32,7 +34,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase } /** @test */ - public function findOneByShortCodeReturnsProperData() + public function findOneByShortCodeReturnsProperData(): void { $foo = new ShortUrl('foo'); $foo->setShortCode('foo'); @@ -62,7 +64,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase } /** @test */ - public function countListReturnsProperNumberOfResults() + public function countListReturnsProperNumberOfResults(): void { $count = 5; for ($i = 0; $i < $count; $i++) { @@ -76,7 +78,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase } /** @test */ - public function findListProperlyFiltersByTagAndSearchTerm() + public function findListProperlyFiltersByTagAndSearchTerm(): void { $tag = new Tag('bar'); $this->getEntityManager()->persist($tag); @@ -121,7 +123,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase } /** @test */ - public function findListProperlyMapsFieldNamesToColumnNamesWhenOrdering() + public function findListProperlyMapsFieldNamesToColumnNamesWhenOrdering(): void { $urls = ['a', 'z', 'c', 'b']; foreach ($urls as $url) { @@ -140,4 +142,26 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->assertEquals('c', $result[2]->getLongUrl()); $this->assertEquals('z', $result[3]->getLongUrl()); } + + /** @test */ + public function slugIsInUseLooksForShortUrlInProperSetOfTables(): void + { + $shortUrlWithoutDomain = (new ShortUrl('foo'))->setShortCode('my-cool-slug'); + $this->getEntityManager()->persist($shortUrlWithoutDomain); + + $shortUrlWithDomain = (new ShortUrl( + 'foo', + ShortUrlMeta::createFromRawData(['domain' => 'doma.in']) + ))->setShortCode('another-slug'); + $this->getEntityManager()->persist($shortUrlWithDomain); + + $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')); + } } diff --git a/module/Core/test/Exception/NonUniqueSlugExceptionTest.php b/module/Core/test/Exception/NonUniqueSlugExceptionTest.php new file mode 100644 index 00000000..aa491b5d --- /dev/null +++ b/module/Core/test/Exception/NonUniqueSlugExceptionTest.php @@ -0,0 +1,34 @@ +assertEquals($expectedMessage, $e->getMessage()); + } + + public function provideMessages(): iterable + { + yield 'without domain' => [ + 'Provided slug "foo" is not unique.', + 'foo', + null, + ]; + yield 'with domain' => [ + 'Provided slug "baz" is not unique for domain "bar".', + 'baz', + 'bar', + ]; + } +} diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 50ea91e1..891211af 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -55,7 +55,7 @@ class UrlShortenerTest extends TestCase $shortUrl->setId('10'); }); $repo = $this->prophesize(ShortUrlRepository::class); - $repo->count(Argument::any())->willReturn(0); + $repo->slugIsInUse(Argument::cetera())->willReturn(false); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->setUrlShortener(false); @@ -122,11 +122,11 @@ class UrlShortenerTest extends TestCase public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void { $repo = $this->prophesize(ShortUrlRepository::class); - $countBySlug = $repo->count(['shortCode' => 'custom-slug'])->willReturn(1); + $slugIsInUse = $repo->slugIsInUse('custom-slug', null)->willReturn(true); $repo->findBy(Argument::cetera())->willReturn([]); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - $countBySlug->shouldBeCalledOnce(); + $slugIsInUse->shouldBeCalledOnce(); $getRepo->shouldBeCalled(); $this->expectException(NonUniqueSlugException::class);