Drastically improved performance when creating new short URLs with findIfExists by moving logic to DB query

2024-11-28 09:03:07 +03:00 · 2020-09-23 00:22:29 +02:00 · 2020-09-23 00:22:29 +02:00 · 460ca032d2
commit 460ca032d2
parent 8d438aa6aa
7 changed files with 98 additions and 110 deletions
--- 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,
-        );
-    }
 }
--- 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;
+    }
 }
--- 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;
 }
--- 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
--- 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,
+//        ];
+//    }
 }
--- 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,
-        ];
-    }
 }
--- 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();
-    }
 }