Merge pull request #842 from acelaya-forks/feature/find-if-exists-performance

Feature/find if exists performance

.travis.yml

@@ -56,7 +56,7 @@ before_script:
   - export DOCKERFILE_CHANGED=$(git diff ${TRAVIS_COMMIT_RANGE:-origin/main} --name-only | grep Dockerfile)
 
 script:
-  - bin/test/run-api-tests.sh --coverage-php build/coverage-api.cov && composer ci
+  - composer ci
   - if [[ ! -z "${DOCKERFILE_CHANGED}" && "${TRAVIS_PHP_VERSION}" == "7.4" ]]; then docker build -t shlink-docker-image:temp . ; fi
 
 after_success:

CHANGELOG.md

@@ -24,7 +24,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 
 #### Fixed
 
-* *Nothing*
+* [#837](https://github.com/shlinkio/shlink/issues/837) Drastically improved performance when creating a new shortUrl and providing `findIfExists = true`.
 
 ## 2.3.0 - 2020-08-09
 

composer.json

@@ -112,7 +112,8 @@
         ],
         "test:ci": [
             "@test:unit:ci",
-            "@test:db"
+            "@test:db",
+            "@test:api:ci"
         ],
         "test:unit": "phpdbg -qrr vendor/bin/phpunit --order-by=random --colors=always --coverage-php build/coverage-unit.cov --testdox",
         "test:unit:ci": "@test:unit --coverage-clover=build/clover.xml --coverage-xml=build/coverage-unit/coverage-xml --log-junit=build/coverage-unit/junit.xml",
@@ -130,6 +131,7 @@
         "test:db:postgres": "DB_DRIVER=postgres composer test:db:sqlite",
         "test:db:ms": "DB_DRIVER=mssql composer test:db:sqlite",
         "test:api": "bin/test/run-api-tests.sh",
+        "test:api:ci": "bin/test/run-api-tests.sh --coverage-php build/coverage-api.cov",
         "test:unit:pretty": "phpdbg -qrr vendor/bin/phpunit --order-by=random --colors=always --coverage-html build/coverage",
         "infect": "infection --threads=4 --min-msi=80 --log-verbosity=default --only-covered",
         "infect:ci:base": "@infect --skip-initial-tests",

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,
-        );
-    }
 }

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,59 @@ 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)
+           ->orderBy('s.id');
+
+        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, $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->join('s.tags', 't')
+           ->groupBy('s')
+           ->having($qb->expr()->eq('COUNT(t.id)', ':tagsAmount'))
+           ->setParameter('tagsAmount', $tagsAmount);
+
+        return $qb->getQuery()->getOneOrNullResult();
+    }
 }

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;
 }

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];
+        /** @var ShortUrlRepositoryInterface $repo */
-        if ($meta->hasCustomSlug()) {
+        $repo = $this->em->getRepository(ShortUrl::class);
-            $criteria['shortCode'] = $meta->getCustomSlug();
+        return $repo->findOneMatching($url, $tags, $meta);
-        }
-        /** @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;
-        });
     }
 
     private function verifyShortCodeUniqueness(ShortUrlMeta $meta, ShortUrl $shortUrlToBeCreated): void

module/Core/test-db/Repository/ShortUrlRepositoryTest.php

@@ -16,12 +16,15 @@ use Shlinkio\Shlink\Core\Model\ShortUrlMeta;
 use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering;
 use Shlinkio\Shlink\Core\Model\Visitor;
 use Shlinkio\Shlink\Core\Repository\ShortUrlRepository;
+use Shlinkio\Shlink\Core\Util\TagManagerTrait;
 use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase;
 
 use function count;
 
 class ShortUrlRepositoryTest extends DatabaseTestCase
 {
+    use TagManagerTrait;
+
     protected const ENTITIES_TO_EMPTY = [
         Tag::class,
         Visit::class,
@@ -210,4 +213,111 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
         $this->assertNull($this->repo->findOne('another-slug', 'example.com'));
         $this->assertNotNull($this->repo->findOne('another-slug', 'doma.in'));
     }
+
+    /** @test */
+    public function findOneMatchingReturnsNullForNonExistingShortUrls(): void
+    {
+        $this->assertNull($this->repo->findOneMatching('', [], ShortUrlMeta::createEmpty()));
+        $this->assertNull($this->repo->findOneMatching('foobar', [], ShortUrlMeta::createEmpty()));
+        $this->assertNull($this->repo->findOneMatching('foobar', ['foo', 'bar'], ShortUrlMeta::createEmpty()));
+        $this->assertNull($this->repo->findOneMatching('foobar', ['foo', 'bar'], ShortUrlMeta::fromRawData([
+            'validSince' => Chronos::parse('2020-03-05 20:18:30'),
+            'customSlug' => 'this_slug_does_not_exist',
+        ])));
+    }
+
+    /** @test */
+    public function findOneMatchingAppliesProperConditions(): void
+    {
+        $start = Chronos::parse('2020-03-05 20:18:30');
+        $end = Chronos::parse('2021-03-05 20:18:30');
+
+        $shortUrl = new ShortUrl('foo', ShortUrlMeta::fromRawData(['validSince' => $start]));
+        $shortUrl->setTags($this->tagNamesToEntities($this->getEntityManager(), ['foo', 'bar']));
+        $this->getEntityManager()->persist($shortUrl);
+
+        $shortUrl2 = new ShortUrl('bar', ShortUrlMeta::fromRawData(['validUntil' => $end]));
+        $this->getEntityManager()->persist($shortUrl2);
+
+        $shortUrl3 = new ShortUrl('baz', ShortUrlMeta::fromRawData(['validSince' => $start, 'validUntil' => $end]));
+        $this->getEntityManager()->persist($shortUrl3);
+
+        $shortUrl4 = new ShortUrl('foo', ShortUrlMeta::fromRawData(['customSlug' => 'custom', 'validUntil' => $end]));
+        $this->getEntityManager()->persist($shortUrl4);
+
+        $shortUrl5 = new ShortUrl('foo', ShortUrlMeta::fromRawData(['maxVisits' => 3]));
+        $this->getEntityManager()->persist($shortUrl5);
+
+        $shortUrl6 = new ShortUrl('foo', ShortUrlMeta::fromRawData(['domain' => 'doma.in']));
+        $this->getEntityManager()->persist($shortUrl6);
+
+        $this->getEntityManager()->flush();
+
+        $this->assertSame(
+            $shortUrl,
+            $this->repo->findOneMatching('foo', ['foo', 'bar'], ShortUrlMeta::fromRawData(['validSince' => $start])),
+        );
+        $this->assertSame(
+            $shortUrl2,
+            $this->repo->findOneMatching('bar', [], ShortUrlMeta::fromRawData(['validUntil' => $end])),
+        );
+        $this->assertSame(
+            $shortUrl3,
+            $this->repo->findOneMatching('baz', [], ShortUrlMeta::fromRawData([
+                'validSince' => $start,
+                'validUntil' => $end,
+            ])),
+        );
+        $this->assertSame(
+            $shortUrl4,
+            $this->repo->findOneMatching('foo', [], ShortUrlMeta::fromRawData([
+                'customSlug' => 'custom',
+                'validUntil' => $end,
+            ])),
+        );
+        $this->assertSame(
+            $shortUrl5,
+            $this->repo->findOneMatching('foo', [], ShortUrlMeta::fromRawData(['maxVisits' => 3])),
+        );
+        $this->assertSame(
+            $shortUrl6,
+            $this->repo->findOneMatching('foo', [], ShortUrlMeta::fromRawData(['domain' => 'doma.in'])),
+        );
+    }
+
+    /** @test */
+    public function findOneMatchingReturnsOldestOneWhenThereAreMultipleMatches(): void
+    {
+        $start = Chronos::parse('2020-03-05 20:18:30');
+        $meta = ['validSince' => $start, 'maxVisits' => 50];
+        $tags = ['foo', 'bar'];
+        $tagEntities = $this->tagNamesToEntities($this->getEntityManager(), $tags);
+
+        $shortUrl1 = new ShortUrl('foo', ShortUrlMeta::fromRawData($meta));
+        $shortUrl1->setTags($tagEntities);
+        $this->getEntityManager()->persist($shortUrl1);
+
+        $shortUrl2 = new ShortUrl('foo', ShortUrlMeta::fromRawData($meta));
+        $shortUrl2->setTags($tagEntities);
+        $this->getEntityManager()->persist($shortUrl2);
+
+        $shortUrl3 = new ShortUrl('foo', ShortUrlMeta::fromRawData($meta));
+        $shortUrl3->setTags($tagEntities);
+        $this->getEntityManager()->persist($shortUrl3);
+
+        $this->getEntityManager()->flush();
+
+        $this->assertSame(
+            $shortUrl1,
+            $this->repo->findOneMatching('foo', $tags, ShortUrlMeta::fromRawData($meta)),
+        );
+        $this->assertNotSame(
+            $shortUrl2,
+            $this->repo->findOneMatching('foo', $tags, ShortUrlMeta::fromRawData($meta)),
+        );
+        $this->assertNotSame(
+            $shortUrl3,
+            $this->repo->findOneMatching('foo', $tags, ShortUrlMeta::fromRawData($meta)),
+        );
+    }
 }

module/Core/test/Entity/ShortUrlTest.php

@@ -4,7 +4,6 @@ declare(strict_types=1);
 
 namespace ShlinkioTest\Shlink\Core\Entity;
 
-use Cake\Chronos\Chronos;
 use PHPUnit\Framework\TestCase;
 use Shlinkio\Shlink\Core\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException;
@@ -75,38 +74,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,
-        ];
-    }
 }

module/Core/test/Service/UrlShortenerTest.php

@@ -21,8 +21,6 @@ use Shlinkio\Shlink\Core\Repository\ShortUrlRepository;
 use Shlinkio\Shlink\Core\Service\UrlShortener;
 use Shlinkio\Shlink\Core\Util\UrlValidatorInterface;
 
-use function array_map;
-
 class UrlShortenerTest extends TestCase
 {
     private UrlShortener $urlShortener;
@@ -147,7 +145,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 +209,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();
-    }
 }