diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index cd96acfc..a9d21952 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -90,8 +90,8 @@ class ShortUrlRepository extends EntityRepository implements ShortUrlRepositoryI ?DateRange $dateRange = null ): QueryBuilder { $qb = $this->getEntityManager()->createQueryBuilder(); - $qb->from(ShortUrl::class, 's'); - $qb->where('1=1'); + $qb->from(ShortUrl::class, 's') + ->where('1=1'); if ($dateRange !== null && $dateRange->getStartDate() !== null) { $qb->andWhere($qb->expr()->gte('s.dateCreated', ':startDate')); @@ -127,7 +127,7 @@ class ShortUrlRepository extends EntityRepository implements ShortUrlRepositoryI return $qb; } - public function findOneByShortCode(string $shortCode, ?string $domain = null): ?ShortUrl + public function findOneWithDomainFallback(string $shortCode, ?string $domain = null): ?ShortUrl { // When ordering DESC, Postgres puts nulls at the beginning while the rest of supported DB engines put them at // the bottom @@ -159,14 +159,30 @@ DQL; return $query->getOneOrNullResult(); } + public function findOne(string $shortCode, ?string $domain = null): ?ShortUrl + { + $qb = $this->createFindOneQueryBuilder($shortCode, $domain); + $qb->select('s'); + + return $qb->getQuery()->getOneOrNullResult(); + } + public function shortCodeIsInUse(string $slug, ?string $domain = null): bool + { + $qb = $this->createFindOneQueryBuilder($slug, $domain); + $qb->select('COUNT(DISTINCT s.id)'); + + return ((int) $qb->getQuery()->getSingleScalarResult()) > 0; + } + + private function createFindOneQueryBuilder(string $slug, ?string $domain = null): QueryBuilder { $qb = $this->getEntityManager()->createQueryBuilder(); - $qb->select('COUNT(DISTINCT s.id)') - ->from(ShortUrl::class, 's') + $qb->from(ShortUrl::class, 's') ->where($qb->expr()->isNotNull('s.shortCode')) ->andWhere($qb->expr()->eq('s.shortCode', ':slug')) - ->setParameter('slug', $slug); + ->setParameter('slug', $slug) + ->setMaxResults(1); if ($domain !== null) { $qb->join('s.domain', 'd') @@ -176,7 +192,6 @@ DQL; $qb->andWhere($qb->expr()->isNull('s.domain')); } - $result = (int) $qb->getQuery()->getSingleScalarResult(); - return $result > 0; + return $qb; } } diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index d0acdf1a..065198b4 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -22,7 +22,9 @@ interface ShortUrlRepositoryInterface extends ObjectRepository public function countList(?string $searchTerm = null, array $tags = [], ?DateRange $dateRange = null): int; - public function findOneByShortCode(string $shortCode, ?string $domain = null): ?ShortUrl; + public function findOneWithDomainFallback(string $shortCode, ?string $domain = null): ?ShortUrl; + + public function findOne(string $shortCode, ?string $domain = null): ?ShortUrl; public function shortCodeIsInUse(string $slug, ?string $domain): bool; } diff --git a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php index d7a71b1c..4f44f671 100644 --- a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php +++ b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php @@ -26,7 +26,7 @@ class ShortUrlResolver implements ShortUrlResolverInterface { /** @var ShortUrlRepository $shortUrlRepo */ $shortUrlRepo = $this->em->getRepository(ShortUrl::class); - $shortUrl = $shortUrlRepo->findOneByShortCode($identifier->shortCode(), $identifier->domain()); + $shortUrl = $shortUrlRepo->findOneWithDomainFallback($identifier->shortCode(), $identifier->domain()); if ($shortUrl === null) { throw ShortUrlNotFoundException::fromNotFound($identifier); } diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index 4af2fd61..4829fada 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -37,7 +37,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase } /** @test */ - public function findOneByShortCodeReturnsProperData(): void + public function findOneWithDomainFallbackReturnsProperData(): void { $regularOne = new ShortUrl('foo', ShortUrlMeta::fromRawData(['customSlug' => 'foo'])); $this->getEntityManager()->persist($regularOne); @@ -54,20 +54,25 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - $this->assertSame($regularOne, $this->repo->findOneByShortCode($regularOne->getShortCode())); - $this->assertSame($regularOne, $this->repo->findOneByShortCode($withDomainDuplicatingRegular->getShortCode())); - $this->assertSame($withDomain, $this->repo->findOneByShortCode($withDomain->getShortCode(), 'example.com')); + $this->assertSame($regularOne, $this->repo->findOneWithDomainFallback($regularOne->getShortCode())); + $this->assertSame($regularOne, $this->repo->findOneWithDomainFallback( + $withDomainDuplicatingRegular->getShortCode(), + )); + $this->assertSame($withDomain, $this->repo->findOneWithDomainFallback( + $withDomain->getShortCode(), + 'example.com', + )); $this->assertSame( $withDomainDuplicatingRegular, - $this->repo->findOneByShortCode($withDomainDuplicatingRegular->getShortCode(), 'doma.in'), + $this->repo->findOneWithDomainFallback($withDomainDuplicatingRegular->getShortCode(), 'doma.in'), ); $this->assertSame( $regularOne, - $this->repo->findOneByShortCode($withDomainDuplicatingRegular->getShortCode(), 'other-domain.com'), + $this->repo->findOneWithDomainFallback($withDomainDuplicatingRegular->getShortCode(), 'other-domain.com'), ); - $this->assertNull($this->repo->findOneByShortCode('invalid')); - $this->assertNull($this->repo->findOneByShortCode($withDomain->getShortCode())); - $this->assertNull($this->repo->findOneByShortCode($withDomain->getShortCode(), 'other-domain.com')); + $this->assertNull($this->repo->findOneWithDomainFallback('invalid')); + $this->assertNull($this->repo->findOneWithDomainFallback($withDomain->getShortCode())); + $this->assertNull($this->repo->findOneWithDomainFallback($withDomain->getShortCode(), 'other-domain.com')); } /** @test */ @@ -183,4 +188,26 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->assertFalse($this->repo->shortCodeIsInUse('another-slug', 'example.com')); $this->assertTrue($this->repo->shortCodeIsInUse('another-slug', 'doma.in')); } + + /** @test */ + public function findOneLooksForShortUrlInProperSetOfTables(): void + { + $shortUrlWithoutDomain = new ShortUrl('foo', ShortUrlMeta::fromRawData(['customSlug' => 'my-cool-slug'])); + $this->getEntityManager()->persist($shortUrlWithoutDomain); + + $shortUrlWithDomain = new ShortUrl( + 'foo', + ShortUrlMeta::fromRawData(['domain' => 'doma.in', 'customSlug' => 'another-slug']), + ); + $this->getEntityManager()->persist($shortUrlWithDomain); + + $this->getEntityManager()->flush(); + + $this->assertNotNull($this->repo->findOne('my-cool-slug')); + $this->assertNull($this->repo->findOne('my-cool-slug', 'doma.in')); + $this->assertNull($this->repo->findOne('slug-not-in-use')); + $this->assertNull($this->repo->findOne('another-slug')); + $this->assertNull($this->repo->findOne('another-slug', 'example.com')); + $this->assertNotNull($this->repo->findOne('another-slug', 'doma.in')); + } } diff --git a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php index cde37599..fb3b4e68 100644 --- a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php +++ b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php @@ -39,7 +39,7 @@ class ShortUrlResolverTest extends TestCase $shortCode = $shortUrl->getShortCode(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneByShortCode($shortCode, null)->willReturn($shortUrl); + $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode)); @@ -55,7 +55,7 @@ class ShortUrlResolverTest extends TestCase $shortCode = 'abc123'; $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneByShortCode($shortCode, null)->willReturn(null); + $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn(null); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->expectException(ShortUrlNotFoundException::class); @@ -72,7 +72,7 @@ class ShortUrlResolverTest extends TestCase $shortCode = $shortUrl->getShortCode(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneByShortCode($shortCode, null)->willReturn($shortUrl); + $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode)); @@ -91,7 +91,7 @@ class ShortUrlResolverTest extends TestCase $shortCode = $shortUrl->getShortCode(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneByShortCode($shortCode, null)->willReturn($shortUrl); + $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->expectException(ShortUrlNotFoundException::class);