From 460ca032d28302a13eaf810bcf0d3ab92a95e596 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 23 Sep 2020 00:22:29 +0200 Subject: [PATCH] Drastically improved performance when creating new short URLs with findIfExists by moving logic to DB query --- module/Core/src/Entity/ShortUrl.php | 26 -------- .../src/Repository/ShortUrlRepository.php | 60 +++++++++++++++++++ .../ShortUrlRepositoryInterface.php | 3 + module/Core/src/Service/UrlShortener.php | 24 ++------ .../Repository/ShortUrlRepositoryTest.php | 30 ++++++++++ module/Core/test/Entity/ShortUrlTest.php | 34 ----------- module/Core/test/Service/UrlShortenerTest.php | 31 +--------- 7 files changed, 98 insertions(+), 110 deletions(-) diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index 0f2811fb..ba10a44a 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -15,10 +15,7 @@ use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; use Shlinkio\Shlink\Core\Model\ShortUrlEdit; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; -use function array_reduce; use function count; -use function Functional\contains; -use function Functional\invoke; use function Shlinkio\Shlink\Core\generateRandomShortCode; class ShortUrl extends AbstractEntity @@ -195,27 +192,4 @@ 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->hasDomain() && $meta->getDomain() !== $this->resolveDomain()) { - return false; - } - if ($meta->hasValidSince() && ($this->validSince === null || ! $meta->getValidSince()->eq($this->validSince))) { - return false; - } - if ($meta->hasValidUntil() && ($this->validUntil === null || ! $meta->getValidUntil()->eq($this->validUntil))) { - return false; - } - - $shortUrlTags = invoke($this->getTags(), '__toString'); - return count($shortUrlTags) === count($tags) && array_reduce( - $tags, - fn (bool $hasAllTags, string $tag) => $hasAllTags && contains($shortUrlTags, $tag), - true, - ); - } } diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index 31fe1385..4bcbe059 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -5,13 +5,16 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Repository; use Doctrine\ORM\EntityRepository; +use Doctrine\ORM\Query\Expr\Join; use Doctrine\ORM\QueryBuilder; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use function array_column; use function array_key_exists; +use function count; use function Functional\contains; class ShortUrlRepository extends EntityRepository implements ShortUrlRepositoryInterface @@ -196,4 +199,61 @@ DQL; return $qb; } + + public function findOneMatching(string $url, array $tags, ShortUrlMeta $meta): ?ShortUrl + { + $qb = $this->getEntityManager()->createQueryBuilder(); + + $qb->select('s') + ->from(ShortUrl::class, 's') + ->where($qb->expr()->eq('s.longUrl', ':longUrl')) + ->setParameter('longUrl', $url) + ->setMaxResults(1); + + if ($meta->hasCustomSlug()) { + $qb->andWhere($qb->expr()->eq('s.shortCode', ':slug')) + ->setParameter('slug', $meta->getCustomSlug()); + } + if ($meta->hasMaxVisits()) { + $qb->andWhere($qb->expr()->eq('s.maxVisits', ':maxVisits')) + ->setParameter('maxVisits', $meta->getMaxVisits()); + } + if ($meta->hasValidSince()) { + $qb->andWhere($qb->expr()->eq('s.validSince', ':validSince')) + ->setParameter('validSince', $meta->getValidSince()); + } + if ($meta->hasValidUntil()) { + $qb->andWhere($qb->expr()->eq('s.validUntil', ':validUntil')) + ->setParameter('validUntil', $meta->getValidUntil()); + } + + if ($meta->hasDomain()) { + $qb->join('s.domain', 'd') + ->andWhere($qb->expr()->eq('d.authority', ':domain')) + ->setParameter('domain', $meta->getDomain()); + } + + $tagsAmount = count($tags); + if ($tagsAmount === 0) { + return $qb->getQuery()->getOneOrNullResult(); + } + + foreach ($tags as $index => $tag) { + $alias = 't_' . $index; + $qb->join('s.tags', $alias, Join::WITH, $qb->expr()->eq($alias . '.name', ':tag' . $index)) + ->setParameter('tag' . $index, $tag); + } + + // If tags where provided, we need an extra join to see the amount of tags that every short URL has, so that we + // can discard those that also have more tags, making sure only those fully matching are included. + $qb->addSelect('COUNT(t.id) as tagsAmount') + ->join('s.tags', 't') + ->groupBy('s') + ->having($qb->expr()->eq('tagsAmount', ':tagsAmount')) + ->setParameter('tagsAmount', $tagsAmount); + + $result = $qb->getQuery()->getOneOrNullResult() ?? []; + + return $result[0] ?? null; + } } diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index 065198b4..65278a85 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\Repository; use Doctrine\Persistence\ObjectRepository; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; interface ShortUrlRepositoryInterface extends ObjectRepository @@ -27,4 +28,6 @@ interface ShortUrlRepositoryInterface extends ObjectRepository public function findOne(string $shortCode, ?string $domain = null): ?ShortUrl; public function shortCodeIsInUse(string $slug, ?string $domain): bool; + + public function findOneMatching(string $url, array $tags, ShortUrlMeta $meta): ?ShortUrl; } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index 7892f959..cf17d0bd 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -11,12 +11,11 @@ use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; +use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; use Throwable; -use function array_reduce; - class UrlShortener implements UrlShortenerInterface { use TagManagerTrait; @@ -77,24 +76,9 @@ class UrlShortener implements UrlShortenerInterface return null; } - $criteria = ['longUrl' => $url]; - if ($meta->hasCustomSlug()) { - $criteria['shortCode'] = $meta->getCustomSlug(); - } - /** @var ShortUrl[] $shortUrls */ - $shortUrls = $this->em->getRepository(ShortUrl::class)->findBy($criteria); - if (empty($shortUrls)) { - return null; - } - - // 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 !== null) { - return $found; - } - - return $shortUrl->matchesCriteria($meta, $tags) ? $shortUrl : null; - }); + /** @var ShortUrlRepositoryInterface $repo */ + $repo = $this->em->getRepository(ShortUrl::class); + return $repo->findOneMatching($url, $tags, $meta); } private function verifyShortCodeUniqueness(ShortUrlMeta $meta, ShortUrl $shortUrlToBeCreated): void diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index 4829fada..85df47de 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -210,4 +210,34 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->assertNull($this->repo->findOne('another-slug', 'example.com')); $this->assertNotNull($this->repo->findOne('another-slug', 'doma.in')); } + +// public function findOneMatchingAppliesProperConditions(): void +// { +// $matches = $this->provideCriteriaToMatch(); +// } +// +// private function provideCriteriaToMatch(): iterable +// { +// $start = Chronos::parse('2020-03-05 20:18:30'); +// $end = Chronos::parse('2021-03-05 20:18:30'); +// +// yield [new ShortUrl('foo'), ShortUrlMeta::fromRawData(['validSince' => $start]), false]; +// yield [new ShortUrl('foo'), ShortUrlMeta::fromRawData(['validUntil' => $end]), false]; +// yield [new ShortUrl('foo'), ShortUrlMeta::fromRawData(['validSince' => $start, 'validUntil' => $end]), false]; +// yield [ +// new ShortUrl('foo', ShortUrlMeta::fromRawData(['validSince' => $start])), +// ShortUrlMeta::fromRawData(['validSince' => $start]), +// true, +// ]; +// yield [ +// new ShortUrl('foo', ShortUrlMeta::fromRawData(['validUntil' => $end])), +// ShortUrlMeta::fromRawData(['validUntil' => $end]), +// true, +// ]; +// yield [ +// new ShortUrl('foo', ShortUrlMeta::fromRawData(['validUntil' => $end, 'validSince' => $start])), +// ShortUrlMeta::fromRawData(['validUntil' => $end, 'validSince' => $start]), +// true, +// ]; +// } } diff --git a/module/Core/test/Entity/ShortUrlTest.php b/module/Core/test/Entity/ShortUrlTest.php index 07308580..c5933e39 100644 --- a/module/Core/test/Entity/ShortUrlTest.php +++ b/module/Core/test/Entity/ShortUrlTest.php @@ -75,38 +75,4 @@ class ShortUrlTest extends TestCase yield [null, DEFAULT_SHORT_CODES_LENGTH]; yield from map(range(4, 10), fn (int $value) => [$value, $value]); } - - /** - * @test - * @dataProvider provideCriteriaToMatch - */ - public function criteriaIsMatchedWhenDatesMatch(ShortUrl $shortUrl, ShortUrlMeta $meta, bool $expected): void - { - $this->assertEquals($expected, $shortUrl->matchesCriteria($meta, [])); - } - - public function provideCriteriaToMatch(): iterable - { - $start = Chronos::parse('2020-03-05 20:18:30'); - $end = Chronos::parse('2021-03-05 20:18:30'); - - yield [new ShortUrl('foo'), ShortUrlMeta::fromRawData(['validSince' => $start]), false]; - yield [new ShortUrl('foo'), ShortUrlMeta::fromRawData(['validUntil' => $end]), false]; - yield [new ShortUrl('foo'), ShortUrlMeta::fromRawData(['validSince' => $start, 'validUntil' => $end]), false]; - yield [ - new ShortUrl('foo', ShortUrlMeta::fromRawData(['validSince' => $start])), - ShortUrlMeta::fromRawData(['validSince' => $start]), - true, - ]; - yield [ - new ShortUrl('foo', ShortUrlMeta::fromRawData(['validUntil' => $end])), - ShortUrlMeta::fromRawData(['validUntil' => $end]), - true, - ]; - yield [ - new ShortUrl('foo', ShortUrlMeta::fromRawData(['validUntil' => $end, 'validSince' => $start])), - ShortUrlMeta::fromRawData(['validUntil' => $end, 'validSince' => $start]), - true, - ]; - } } diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index f1ef88af..9e33af6b 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -147,7 +147,7 @@ class UrlShortenerTest extends TestCase ShortUrl $expected ): void { $repo = $this->prophesize(ShortUrlRepository::class); - $findExisting = $repo->findBy(Argument::any())->willReturn([$expected]); + $findExisting = $repo->findOneMatching(Argument::cetera())->willReturn($expected); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlShortener->urlToShortCode($url, $tags, $meta); @@ -211,33 +211,4 @@ class UrlShortenerTest extends TestCase ])))->setTags(new ArrayCollection([new Tag('foo'), new Tag('bar'), new Tag('baz')])), ]; } - - /** @test */ - public function properExistingShortUrlIsReturnedWhenMultipleMatch(): void - { - $url = 'http://foo.com'; - $tags = ['baz', 'foo', 'bar']; - $meta = ShortUrlMeta::fromRawData([ - 'findIfExists' => true, - 'validUntil' => Chronos::parse('2017-01-01'), - 'maxVisits' => 4, - ]); - $tagsCollection = new ArrayCollection(array_map(fn (string $tag) => new Tag($tag), $tags)); - $expected = (new ShortUrl($url, $meta))->setTags($tagsCollection); - - $repo = $this->prophesize(ShortUrlRepository::class); - $findExisting = $repo->findBy(Argument::any())->willReturn([ - new ShortUrl($url), - new ShortUrl($url, $meta), - $expected, - (new ShortUrl($url))->setTags($tagsCollection), - ]); - $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - - $result = $this->urlShortener->urlToShortCode($url, $tags, $meta); - - $this->assertSame($expected, $result); - $findExisting->shouldHaveBeenCalledOnce(); - $getRepo->shouldHaveBeenCalledOnce(); - } }