From 3ff4ac84c499c820342e0eb118532cdf87282b86 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 May 2021 10:33:27 +0200 Subject: [PATCH 1/5] Added locking to short URL creation when checking if URL exists --- module/Core/src/Model/ShortUrlIdentifier.php | 14 ++++++++++++ .../src/Repository/ShortUrlRepository.php | 22 +++++++++++++++---- .../ShortUrlRepositoryInterface.php | 5 ++++- .../src/Service/ShortUrl/ShortCodeHelper.php | 7 ++---- module/Core/src/Visit/VisitsStatsHelper.php | 2 +- .../Repository/ShortUrlRepositoryTest.php | 19 +++++++++++----- .../Service/ShortUrl/ShortCodeHelperTest.php | 17 ++++++++------ module/Core/test/Service/UrlShortenerTest.php | 1 - .../Core/test/Visit/VisitsStatsHelperTest.php | 8 +++++-- 9 files changed, 68 insertions(+), 27 deletions(-) diff --git a/module/Core/src/Model/ShortUrlIdentifier.php b/module/Core/src/Model/ShortUrlIdentifier.php index 4a74ba07..a277782c 100644 --- a/module/Core/src/Model/ShortUrlIdentifier.php +++ b/module/Core/src/Model/ShortUrlIdentifier.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Model; use Psr\Http\Message\ServerRequestInterface; +use Shlinkio\Shlink\Core\Entity\ShortUrl; use Symfony\Component\Console\Input\InputInterface; final class ShortUrlIdentifier @@ -42,6 +43,19 @@ final class ShortUrlIdentifier return new self($shortCode, $domain); } + public static function fromShortUrl(ShortUrl $shortUrl): self + { + $domain = $shortUrl->getDomain(); + $domainAuthority = $domain !== null ? $domain->getAuthority() : null; + + return new self($shortUrl->getShortCode(), $domainAuthority); + } + + public static function fromShortCodeAndDomain(string $shortCode, ?string $domain = null): self + { + return new self($shortCode, $domain); + } + public function shortCode(): string { return $this->shortCode; diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index eacf293b..a1b93f4d 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Repository; +use Doctrine\DBAL\LockMode; use Doctrine\ORM\Query\Expr\Join; use Doctrine\ORM\QueryBuilder; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; @@ -11,6 +12,7 @@ use Happyr\DoctrineSpecification\Specification\Specification; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; @@ -180,12 +182,24 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU return $qb->getQuery()->getOneOrNullResult(); } - public function shortCodeIsInUse(string $slug, ?string $domain = null, ?Specification $spec = null): bool + public function shortCodeIsInUse(ShortUrlIdentifier $identifier, ?Specification $spec = null): bool { - $qb = $this->createFindOneQueryBuilder($slug, $domain, $spec); - $qb->select('COUNT(DISTINCT s.id)'); + return $this->doShortCodeIsInUse($identifier, $spec, null); + } - return ((int) $qb->getQuery()->getSingleScalarResult()) > 0; + public function shortCodeIsInUseWithLock(ShortUrlIdentifier $identifier, ?Specification $spec = null): bool + { + return $this->doShortCodeIsInUse($identifier, $spec, LockMode::PESSIMISTIC_WRITE); + } + + private function doShortCodeIsInUse(ShortUrlIdentifier $identifier, ?Specification $spec, ?int $lockMode): bool + { + $qb = $this->createFindOneQueryBuilder($identifier->shortCode(), $identifier->domain(), $spec); + $qb->select('s.id'); + + $query = $qb->getQuery()->setLockMode($lockMode); + + return $query->getOneOrNullResult() !== null; } private function createFindOneQueryBuilder(string $slug, ?string $domain, ?Specification $spec): QueryBuilder diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index 5d8fa924..9b6dffc8 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -9,6 +9,7 @@ use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepositoryInterfa use Happyr\DoctrineSpecification\Specification\Specification; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; @@ -36,7 +37,9 @@ interface ShortUrlRepositoryInterface extends ObjectRepository, EntitySpecificat public function findOne(string $shortCode, ?string $domain = null, ?Specification $spec = null): ?ShortUrl; - public function shortCodeIsInUse(string $slug, ?string $domain, ?Specification $spec = null): bool; + public function shortCodeIsInUse(ShortUrlIdentifier $identifier, ?Specification $spec = null): bool; + + public function shortCodeIsInUseWithLock(ShortUrlIdentifier $identifier, ?Specification $spec = null): bool; public function findOneMatching(ShortUrlMeta $meta): ?ShortUrl; diff --git a/module/Core/src/Service/ShortUrl/ShortCodeHelper.php b/module/Core/src/Service/ShortUrl/ShortCodeHelper.php index 3df4c016..83c3397e 100644 --- a/module/Core/src/Service/ShortUrl/ShortCodeHelper.php +++ b/module/Core/src/Service/ShortUrl/ShortCodeHelper.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Service\ShortUrl; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; class ShortCodeHelper implements ShortCodeHelperInterface // TODO Rename to ShortCodeUniquenessHelper @@ -19,13 +20,9 @@ class ShortCodeHelper implements ShortCodeHelperInterface // TODO Rename to Shor public function ensureShortCodeUniqueness(ShortUrl $shortUrlToBeCreated, bool $hasCustomSlug): bool { - $shortCode = $shortUrlToBeCreated->getShortCode(); - $domain = $shortUrlToBeCreated->getDomain(); - $domainAuthority = $domain !== null ? $domain->getAuthority() : null; - /** @var ShortUrlRepository $repo */ $repo = $this->em->getRepository(ShortUrl::class); - $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domainAuthority); + $otherShortUrlsExist = $repo->shortCodeIsInUseWithLock(ShortUrlIdentifier::fromShortUrl($shortUrlToBeCreated)); if (! $otherShortUrlsExist) { return true; diff --git a/module/Core/src/Visit/VisitsStatsHelper.php b/module/Core/src/Visit/VisitsStatsHelper.php index 1aa03c46..dfa00a4c 100644 --- a/module/Core/src/Visit/VisitsStatsHelper.php +++ b/module/Core/src/Visit/VisitsStatsHelper.php @@ -58,7 +58,7 @@ class VisitsStatsHelper implements VisitsStatsHelperInterface /** @var ShortUrlRepositoryInterface $repo */ $repo = $this->em->getRepository(ShortUrl::class); - if (! $repo->shortCodeIsInUse($identifier->shortCode(), $identifier->domain(), $spec)) { + if (! $repo->shortCodeIsInUse($identifier, $spec)) { throw ShortUrlNotFoundException::fromNotFound($identifier); } diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index bd5b22d4..345e0911 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Core\Model\Visitor; @@ -180,12 +181,18 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - self::assertTrue($this->repo->shortCodeIsInUse('my-cool-slug')); - self::assertFalse($this->repo->shortCodeIsInUse('my-cool-slug', 'doma.in')); - self::assertFalse($this->repo->shortCodeIsInUse('slug-not-in-use')); - self::assertFalse($this->repo->shortCodeIsInUse('another-slug')); - self::assertFalse($this->repo->shortCodeIsInUse('another-slug', 'example.com')); - self::assertTrue($this->repo->shortCodeIsInUse('another-slug', 'doma.in')); + self::assertTrue($this->repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain('my-cool-slug'))); + self::assertFalse($this->repo->shortCodeIsInUse( + ShortUrlIdentifier::fromShortCodeAndDomain('my-cool-slug', 'doma.in'), + )); + self::assertFalse($this->repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain('slug-not-in-use'))); + self::assertFalse($this->repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain('another-slug'))); + self::assertFalse($this->repo->shortCodeIsInUse( + ShortUrlIdentifier::fromShortCodeAndDomain('another-slug', 'example.com'), + )); + self::assertTrue($this->repo->shortCodeIsInUse( + ShortUrlIdentifier::fromShortCodeAndDomain('another-slug', 'doma.in'), + )); } /** @test */ diff --git a/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php b/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php index 047dbc96..ca3b463f 100644 --- a/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php +++ b/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php @@ -10,6 +10,7 @@ use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelper; @@ -39,12 +40,12 @@ class ShortCodeHelperTest extends TestCase $callIndex = 0; $expectedCalls = 3; $repo = $this->prophesize(ShortUrlRepository::class); - $shortCodeIsInUse = $repo->shortCodeIsInUse('abc123', $expectedAuthority)->will( - function () use (&$callIndex, $expectedCalls) { - $callIndex++; - return $callIndex < $expectedCalls; - }, - ); + $shortCodeIsInUse = $repo->shortCodeIsInUseWithLock( + ShortUrlIdentifier::fromShortCodeAndDomain('abc123', $expectedAuthority), + )->will(function () use (&$callIndex, $expectedCalls) { + $callIndex++; + return $callIndex < $expectedCalls; + }); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->shortUrl->getDomain()->willReturn($domain); @@ -66,7 +67,9 @@ class ShortCodeHelperTest extends TestCase public function inUseSlugReturnsError(): void { $repo = $this->prophesize(ShortUrlRepository::class); - $shortCodeIsInUse = $repo->shortCodeIsInUse('abc123', null)->willReturn(true); + $shortCodeIsInUse = $repo->shortCodeIsInUseWithLock( + ShortUrlIdentifier::fromShortCodeAndDomain('abc123'), + )->willReturn(true); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->shortUrl->getDomain()->willReturn(null); diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 7e319314..6bac432e 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -46,7 +46,6 @@ class UrlShortenerTest extends TestCase return $callback(); }); $repo = $this->prophesize(ShortUrlRepository::class); - $repo->shortCodeIsInUse(Argument::cetera())->willReturn(false); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->shortCodeHelper = $this->prophesize(ShortCodeHelperInterface::class); diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index e6f067da..372bda9a 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -81,7 +81,9 @@ class VisitsStatsHelperTest extends TestCase $shortCode = '123ABC'; $spec = $apiKey === null ? null : $apiKey->spec(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $count = $repo->shortCodeIsInUse($shortCode, null, $spec)->willReturn(true); + $count = $repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), $spec)->willReturn( + true, + ); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); @@ -101,7 +103,9 @@ class VisitsStatsHelperTest extends TestCase { $shortCode = '123ABC'; $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $count = $repo->shortCodeIsInUse($shortCode, null, null)->willReturn(false); + $count = $repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), null)->willReturn( + false, + ); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); $this->expectException(ShortUrlNotFoundException::class); From f82e103bc559d1b50c0006982fd1980ccb49af41 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 15 May 2021 12:11:45 +0200 Subject: [PATCH 2/5] Added locks to tag and domain creation during short URL creation --- .../src/Domain/Repository/DomainRepository.php | 13 +++++++++++++ .../Repository/DomainRepositoryInterface.php | 2 ++ module/Core/src/Repository/TagRepository.php | 13 +++++++++++++ .../src/Repository/TagRepositoryInterface.php | 3 +++ .../PersistenceShortUrlRelationResolver.php | 10 +++++++--- .../PersistenceShortUrlRelationResolverTest.php | 16 ++++++++-------- 6 files changed, 46 insertions(+), 11 deletions(-) diff --git a/module/Core/src/Domain/Repository/DomainRepository.php b/module/Core/src/Domain/Repository/DomainRepository.php index 2e4f3bb2..99a7a927 100644 --- a/module/Core/src/Domain/Repository/DomainRepository.php +++ b/module/Core/src/Domain/Repository/DomainRepository.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Domain\Repository; +use Doctrine\DBAL\LockMode; use Doctrine\ORM\Query\Expr\Join; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Shlinkio\Shlink\Core\Entity\Domain; @@ -32,4 +33,16 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe return $qb->getQuery()->getResult(); } + + public function findOneByAuthorityWithLock(string $authority): ?Domain + { + $qb = $this->createQueryBuilder('d'); + $qb->where($qb->expr()->eq('d.authority', ':authority')) + ->setParameter('authority', $authority) + ->setMaxResults(1); + + $query = $qb->getQuery()->setLockMode(LockMode::PESSIMISTIC_WRITE); + + return $query->getOneOrNullResult(); + } } diff --git a/module/Core/src/Domain/Repository/DomainRepositoryInterface.php b/module/Core/src/Domain/Repository/DomainRepositoryInterface.php index 1d201520..5dc2f873 100644 --- a/module/Core/src/Domain/Repository/DomainRepositoryInterface.php +++ b/module/Core/src/Domain/Repository/DomainRepositoryInterface.php @@ -15,4 +15,6 @@ interface DomainRepositoryInterface extends ObjectRepository, EntitySpecificatio * @return Domain[] */ public function findDomainsWithout(?string $excludedAuthority, ?ApiKey $apiKey = null): array; + + public function findOneByAuthorityWithLock(string $authority): ?Domain; } diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index d21122d0..38aae1dc 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Repository; +use Doctrine\DBAL\LockMode; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Happyr\DoctrineSpecification\Spec; use Shlinkio\Shlink\Core\Entity\Tag; @@ -62,4 +63,16 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito return $result > 0; } + + public function findOneByNameWithLock(string $name): ?Tag + { + $qb = $this->createQueryBuilder('t'); + $qb->where($qb->expr()->eq('t.name', ':name')) + ->setParameter('name', $name) + ->setMaxResults(1); + + $query = $qb->getQuery()->setLockMode(LockMode::PESSIMISTIC_WRITE); + + return $query->getOneOrNullResult(); + } } diff --git a/module/Core/src/Repository/TagRepositoryInterface.php b/module/Core/src/Repository/TagRepositoryInterface.php index 924706ff..b340c1fc 100644 --- a/module/Core/src/Repository/TagRepositoryInterface.php +++ b/module/Core/src/Repository/TagRepositoryInterface.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Repository; use Doctrine\Persistence\ObjectRepository; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepositoryInterface; +use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Tag\Model\TagInfo; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -19,4 +20,6 @@ interface TagRepositoryInterface extends ObjectRepository, EntitySpecificationRe public function findTagsWithInfo(?ApiKey $apiKey = null): array; public function tagExists(string $tag, ?ApiKey $apiKey = null): bool; + + public function findOneByNameWithLock(string $name): ?Tag; } diff --git a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php index eb7fddad..b7e27a82 100644 --- a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php @@ -8,8 +8,10 @@ use Doctrine\Common\Collections; use Doctrine\Common\Collections\Collection; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\Events; +use Shlinkio\Shlink\Core\Domain\Repository\DomainRepositoryInterface; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\Tag; +use Shlinkio\Shlink\Core\Repository\TagRepositoryInterface; use function Functional\map; use function Functional\unique; @@ -35,8 +37,9 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt return null; } - /** @var Domain|null $existingDomain */ - $existingDomain = $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]); + /** @var DomainRepositoryInterface $repo */ + $repo = $this->em->getRepository(Domain::class); + $existingDomain = $repo->findOneByAuthorityWithLock($domain); // Memoize only new domains, and let doctrine handle objects hydrated from persistence return $existingDomain ?? $this->memoizeNewDomain($domain); @@ -58,11 +61,12 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt } $tags = unique($tags); + /** @var TagRepositoryInterface $repo */ $repo = $this->em->getRepository(Tag::class); return new Collections\ArrayCollection(map($tags, function (string $tagName) use ($repo): Tag { // Memoize only new tags, and let doctrine handle objects hydrated from persistence - $tag = $repo->findOneBy(['name' => $tagName]) ?? $this->memoizeNewTag($tagName); + $tag = $repo->findOneByNameWithLock($tagName) ?? $this->memoizeNewTag($tagName); $this->em->persist($tag); return $tag; diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php index 8660099c..8698bec0 100644 --- a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php @@ -6,11 +6,11 @@ namespace ShlinkioTest\Shlink\Core\ShortUrl\Resolver; use Doctrine\Common\EventManager; use Doctrine\ORM\EntityManagerInterface; -use Doctrine\Persistence\ObjectRepository; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; +use Shlinkio\Shlink\Core\Domain\Repository\DomainRepositoryInterface; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Repository\TagRepositoryInterface; @@ -48,8 +48,8 @@ class PersistenceShortUrlRelationResolverTest extends TestCase */ public function findsOrCreatesDomainWhenValueIsProvided(?Domain $foundDomain, string $authority): void { - $repo = $this->prophesize(ObjectRepository::class); - $findDomain = $repo->findOneBy(['authority' => $authority])->willReturn($foundDomain); + $repo = $this->prophesize(DomainRepositoryInterface::class); + $findDomain = $repo->findOneByAuthorityWithLock($authority)->willReturn($foundDomain); $getRepository = $this->em->getRepository(Domain::class)->willReturn($repo->reveal()); $result = $this->resolver->resolveDomain($authority); @@ -80,7 +80,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase $expectedPersistedTags = count($expectedTags); $tagRepo = $this->prophesize(TagRepositoryInterface::class); - $findTag = $tagRepo->findOneBy(Argument::type('array'))->will(function (array $args): ?Tag { + $findTag = $tagRepo->findOneByNameWithLock(Argument::type('string'))->will(function (array $args): ?Tag { ['name' => $name] = $args[0]; return $name === 'foo' ? new Tag($name) : null; }); @@ -106,7 +106,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase public function returnsEmptyCollectionWhenProvidingEmptyListOfTags(): void { $tagRepo = $this->prophesize(TagRepositoryInterface::class); - $findTag = $tagRepo->findOneBy(Argument::type('array'))->willReturn(null); + $findTag = $tagRepo->findOneByNameWithLock(Argument::type('string'))->willReturn(null); $getRepo = $this->em->getRepository(Tag::class)->willReturn($tagRepo->reveal()); $persist = $this->em->persist(Argument::type(Tag::class)); @@ -121,8 +121,8 @@ class PersistenceShortUrlRelationResolverTest extends TestCase /** @test */ public function newDomainsAreMemoizedUntilStateIsCleared(): void { - $repo = $this->prophesize(ObjectRepository::class); - $repo->findOneBy(Argument::type('array'))->willReturn(null); + $repo = $this->prophesize(DomainRepositoryInterface::class); + $repo->findOneByAuthorityWithLock(Argument::type('string'))->willReturn(null); $this->em->getRepository(Domain::class)->willReturn($repo->reveal()); $authority = 'foo.com'; @@ -141,7 +141,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase public function newTagsAreMemoizedUntilStateIsCleared(): void { $tagRepo = $this->prophesize(TagRepositoryInterface::class); - $tagRepo->findOneBy(Argument::type('array'))->willReturn(null); + $tagRepo->findOneByNameWithLock(Argument::type('string'))->willReturn(null); $this->em->getRepository(Tag::class)->willReturn($tagRepo->reveal()); $this->em->persist(Argument::type(Tag::class))->will(function (): void { }); From cd198764198d8c40ea8187d3058b241e3685cd03 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 23 May 2021 08:21:40 +0200 Subject: [PATCH 3/5] Removed methods to create tags and domains with lock, as they do not really lock as expected --- .../Core/src/Domain/Repository/DomainRepository.php | 13 ------------- .../Domain/Repository/DomainRepositoryInterface.php | 2 -- module/Core/src/Repository/TagRepository.php | 13 ------------- .../Core/src/Repository/TagRepositoryInterface.php | 3 --- .../PersistenceShortUrlRelationResolver.php | 10 +++------- .../PersistenceShortUrlRelationResolverTest.php | 10 +++++----- 6 files changed, 8 insertions(+), 43 deletions(-) diff --git a/module/Core/src/Domain/Repository/DomainRepository.php b/module/Core/src/Domain/Repository/DomainRepository.php index 99a7a927..2e4f3bb2 100644 --- a/module/Core/src/Domain/Repository/DomainRepository.php +++ b/module/Core/src/Domain/Repository/DomainRepository.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Domain\Repository; -use Doctrine\DBAL\LockMode; use Doctrine\ORM\Query\Expr\Join; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Shlinkio\Shlink\Core\Entity\Domain; @@ -33,16 +32,4 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe return $qb->getQuery()->getResult(); } - - public function findOneByAuthorityWithLock(string $authority): ?Domain - { - $qb = $this->createQueryBuilder('d'); - $qb->where($qb->expr()->eq('d.authority', ':authority')) - ->setParameter('authority', $authority) - ->setMaxResults(1); - - $query = $qb->getQuery()->setLockMode(LockMode::PESSIMISTIC_WRITE); - - return $query->getOneOrNullResult(); - } } diff --git a/module/Core/src/Domain/Repository/DomainRepositoryInterface.php b/module/Core/src/Domain/Repository/DomainRepositoryInterface.php index 5dc2f873..1d201520 100644 --- a/module/Core/src/Domain/Repository/DomainRepositoryInterface.php +++ b/module/Core/src/Domain/Repository/DomainRepositoryInterface.php @@ -15,6 +15,4 @@ interface DomainRepositoryInterface extends ObjectRepository, EntitySpecificatio * @return Domain[] */ public function findDomainsWithout(?string $excludedAuthority, ?ApiKey $apiKey = null): array; - - public function findOneByAuthorityWithLock(string $authority): ?Domain; } diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index 38aae1dc..d21122d0 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Repository; -use Doctrine\DBAL\LockMode; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Happyr\DoctrineSpecification\Spec; use Shlinkio\Shlink\Core\Entity\Tag; @@ -63,16 +62,4 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito return $result > 0; } - - public function findOneByNameWithLock(string $name): ?Tag - { - $qb = $this->createQueryBuilder('t'); - $qb->where($qb->expr()->eq('t.name', ':name')) - ->setParameter('name', $name) - ->setMaxResults(1); - - $query = $qb->getQuery()->setLockMode(LockMode::PESSIMISTIC_WRITE); - - return $query->getOneOrNullResult(); - } } diff --git a/module/Core/src/Repository/TagRepositoryInterface.php b/module/Core/src/Repository/TagRepositoryInterface.php index b340c1fc..924706ff 100644 --- a/module/Core/src/Repository/TagRepositoryInterface.php +++ b/module/Core/src/Repository/TagRepositoryInterface.php @@ -6,7 +6,6 @@ namespace Shlinkio\Shlink\Core\Repository; use Doctrine\Persistence\ObjectRepository; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepositoryInterface; -use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Tag\Model\TagInfo; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -20,6 +19,4 @@ interface TagRepositoryInterface extends ObjectRepository, EntitySpecificationRe public function findTagsWithInfo(?ApiKey $apiKey = null): array; public function tagExists(string $tag, ?ApiKey $apiKey = null): bool; - - public function findOneByNameWithLock(string $name): ?Tag; } diff --git a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php index b7e27a82..8601a045 100644 --- a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php @@ -8,10 +8,8 @@ use Doctrine\Common\Collections; use Doctrine\Common\Collections\Collection; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\Events; -use Shlinkio\Shlink\Core\Domain\Repository\DomainRepositoryInterface; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\Tag; -use Shlinkio\Shlink\Core\Repository\TagRepositoryInterface; use function Functional\map; use function Functional\unique; @@ -37,9 +35,8 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt return null; } - /** @var DomainRepositoryInterface $repo */ - $repo = $this->em->getRepository(Domain::class); - $existingDomain = $repo->findOneByAuthorityWithLock($domain); + /** @var Domain|null $existingDomain */ + $existingDomain = $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]); // Memoize only new domains, and let doctrine handle objects hydrated from persistence return $existingDomain ?? $this->memoizeNewDomain($domain); @@ -61,12 +58,11 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt } $tags = unique($tags); - /** @var TagRepositoryInterface $repo */ $repo = $this->em->getRepository(Tag::class); return new Collections\ArrayCollection(map($tags, function (string $tagName) use ($repo): Tag { // Memoize only new tags, and let doctrine handle objects hydrated from persistence - $tag = $repo->findOneByNameWithLock($tagName) ?? $this->memoizeNewTag($tagName); + $tag = $repo->findOneBy(['name' => $tagName]) ?? $this->memoizeNewTag($tagName); $this->em->persist($tag); return $tag; diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php index 8698bec0..aeef3f47 100644 --- a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php @@ -49,7 +49,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase public function findsOrCreatesDomainWhenValueIsProvided(?Domain $foundDomain, string $authority): void { $repo = $this->prophesize(DomainRepositoryInterface::class); - $findDomain = $repo->findOneByAuthorityWithLock($authority)->willReturn($foundDomain); + $findDomain = $repo->findOneBy(['authority' => $authority])->willReturn($foundDomain); $getRepository = $this->em->getRepository(Domain::class)->willReturn($repo->reveal()); $result = $this->resolver->resolveDomain($authority); @@ -80,7 +80,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase $expectedPersistedTags = count($expectedTags); $tagRepo = $this->prophesize(TagRepositoryInterface::class); - $findTag = $tagRepo->findOneByNameWithLock(Argument::type('string'))->will(function (array $args): ?Tag { + $findTag = $tagRepo->findOneBy(Argument::type('array'))->will(function (array $args): ?Tag { ['name' => $name] = $args[0]; return $name === 'foo' ? new Tag($name) : null; }); @@ -106,7 +106,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase public function returnsEmptyCollectionWhenProvidingEmptyListOfTags(): void { $tagRepo = $this->prophesize(TagRepositoryInterface::class); - $findTag = $tagRepo->findOneByNameWithLock(Argument::type('string'))->willReturn(null); + $findTag = $tagRepo->findOneBy(Argument::type('array'))->willReturn(null); $getRepo = $this->em->getRepository(Tag::class)->willReturn($tagRepo->reveal()); $persist = $this->em->persist(Argument::type(Tag::class)); @@ -122,7 +122,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase public function newDomainsAreMemoizedUntilStateIsCleared(): void { $repo = $this->prophesize(DomainRepositoryInterface::class); - $repo->findOneByAuthorityWithLock(Argument::type('string'))->willReturn(null); + $repo->findOneBy(Argument::type('array'))->willReturn(null); $this->em->getRepository(Domain::class)->willReturn($repo->reveal()); $authority = 'foo.com'; @@ -141,7 +141,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase public function newTagsAreMemoizedUntilStateIsCleared(): void { $tagRepo = $this->prophesize(TagRepositoryInterface::class); - $tagRepo->findOneByNameWithLock(Argument::type('string'))->willReturn(null); + $tagRepo->findOneBy(Argument::type('array'))->willReturn(null); $this->em->getRepository(Tag::class)->willReturn($tagRepo->reveal()); $this->em->persist(Argument::type(Tag::class))->will(function (): void { }); From 5e6d2881bcaaf006ed9d1403d1ea29abe10086b9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 23 May 2021 08:41:42 +0200 Subject: [PATCH 4/5] Used ShorturlIdentifier model whenever possible --- .../Adapter/VisitsPaginatorAdapter.php | 6 +- .../src/Repository/ShortUrlRepository.php | 12 +-- .../ShortUrlRepositoryInterface.php | 2 +- .../Core/src/Repository/VisitRepository.php | 14 +-- .../Repository/VisitRepositoryInterface.php | 5 +- .../src/Service/ShortUrl/ShortUrlResolver.php | 6 +- .../Repository/ShortUrlRepositoryTest.php | 16 +-- .../Repository/VisitRepositoryTest.php | 99 ++++++++++++------- .../Adapter/VisitsPaginatorAdapterTest.php | 6 +- .../Service/ShortUrl/ShortUrlResolverTest.php | 10 +- .../Core/test/Visit/VisitsStatsHelperTest.php | 16 +-- 11 files changed, 112 insertions(+), 80 deletions(-) diff --git a/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php b/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php index 5369bd05..d651b1b5 100644 --- a/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php +++ b/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php @@ -33,8 +33,7 @@ class VisitsPaginatorAdapter extends AbstractCacheableCountPaginatorAdapter public function getSlice($offset, $length): array // phpcs:ignore { return $this->visitRepository->findVisitsByShortCode( - $this->identifier->shortCode(), - $this->identifier->domain(), + $this->identifier, new VisitsListFiltering( $this->params->getDateRange(), $this->params->excludeBots(), @@ -48,8 +47,7 @@ class VisitsPaginatorAdapter extends AbstractCacheableCountPaginatorAdapter protected function doCount(): int { return $this->visitRepository->countVisitsByShortCode( - $this->identifier->shortCode(), - $this->identifier->domain(), + $this->identifier, new VisitsCountFiltering( $this->params->getDateRange(), $this->params->excludeBots(), diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index a1b93f4d..c658d478 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -174,9 +174,9 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU return $query->getOneOrNullResult(); } - public function findOne(string $shortCode, ?string $domain = null, ?Specification $spec = null): ?ShortUrl + public function findOne(ShortUrlIdentifier $identifier, ?Specification $spec = null): ?ShortUrl { - $qb = $this->createFindOneQueryBuilder($shortCode, $domain, $spec); + $qb = $this->createFindOneQueryBuilder($identifier, $spec); $qb->select('s'); return $qb->getQuery()->getOneOrNullResult(); @@ -194,7 +194,7 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU private function doShortCodeIsInUse(ShortUrlIdentifier $identifier, ?Specification $spec, ?int $lockMode): bool { - $qb = $this->createFindOneQueryBuilder($identifier->shortCode(), $identifier->domain(), $spec); + $qb = $this->createFindOneQueryBuilder($identifier, $spec); $qb->select('s.id'); $query = $qb->getQuery()->setLockMode($lockMode); @@ -202,16 +202,16 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU return $query->getOneOrNullResult() !== null; } - private function createFindOneQueryBuilder(string $slug, ?string $domain, ?Specification $spec): QueryBuilder + private function createFindOneQueryBuilder(ShortUrlIdentifier $identifier, ?Specification $spec): QueryBuilder { $qb = $this->getEntityManager()->createQueryBuilder(); $qb->from(ShortUrl::class, 's') ->where($qb->expr()->isNotNull('s.shortCode')) ->andWhere($qb->expr()->eq('s.shortCode', ':slug')) - ->setParameter('slug', $slug) + ->setParameter('slug', $identifier->shortCode()) ->setMaxResults(1); - $this->whereDomainIs($qb, $domain); + $this->whereDomainIs($qb, $identifier->domain()); $this->applySpecification($qb, $spec, 's'); diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index 9b6dffc8..7489f2a0 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -35,7 +35,7 @@ interface ShortUrlRepositoryInterface extends ObjectRepository, EntitySpecificat public function findOneWithDomainFallback(string $shortCode, ?string $domain = null): ?ShortUrl; - public function findOne(string $shortCode, ?string $domain = null, ?Specification $spec = null): ?ShortUrl; + public function findOne(ShortUrlIdentifier $identifier, ?Specification $spec = null): ?ShortUrl; public function shortCodeIsInUse(ShortUrlIdentifier $identifier, ?Specification $spec = null): bool; diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index bc3f958b..61cd108e 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Spec\CountOfOrphanVisits; @@ -84,28 +85,27 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo /** * @return Visit[] */ - public function findVisitsByShortCode(string $shortCode, ?string $domain, VisitsListFiltering $filtering): array + public function findVisitsByShortCode(ShortUrlIdentifier $identifier, VisitsListFiltering $filtering): array { - $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $domain, $filtering); + $qb = $this->createVisitsByShortCodeQueryBuilder($identifier, $filtering); return $this->resolveVisitsWithNativeQuery($qb, $filtering->limit(), $filtering->offset()); } - public function countVisitsByShortCode(string $shortCode, ?string $domain, VisitsCountFiltering $filtering): int + public function countVisitsByShortCode(ShortUrlIdentifier $identifier, VisitsCountFiltering $filtering): int { - $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $domain, $filtering); + $qb = $this->createVisitsByShortCodeQueryBuilder($identifier, $filtering); $qb->select('COUNT(v.id)'); return (int) $qb->getQuery()->getSingleScalarResult(); } private function createVisitsByShortCodeQueryBuilder( - string $shortCode, - ?string $domain, + ShortUrlIdentifier $identifier, VisitsCountFiltering $filtering ): QueryBuilder { /** @var ShortUrlRepositoryInterface $shortUrlRepo */ $shortUrlRepo = $this->getEntityManager()->getRepository(ShortUrl::class); - $shortUrl = $shortUrlRepo->findOne($shortCode, $domain, $filtering->spec()); + $shortUrl = $shortUrlRepo->findOne($identifier, $filtering->spec()); $shortUrlId = $shortUrl !== null ? $shortUrl->getId() : -1; // Parameters in this query need to be part of the query itself, as we need to use it a sub-query later diff --git a/module/Core/src/Repository/VisitRepositoryInterface.php b/module/Core/src/Repository/VisitRepositoryInterface.php index 05a59720..28f1e9a8 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\Repository; use Doctrine\Persistence\ObjectRepository; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepositoryInterface; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -33,9 +34,9 @@ interface VisitRepositoryInterface extends ObjectRepository, EntitySpecification /** * @return Visit[] */ - public function findVisitsByShortCode(string $shortCode, ?string $domain, VisitsListFiltering $filtering): array; + public function findVisitsByShortCode(ShortUrlIdentifier $identifier, VisitsListFiltering $filtering): array; - public function countVisitsByShortCode(string $shortCode, ?string $domain, VisitsCountFiltering $filtering): int; + public function countVisitsByShortCode(ShortUrlIdentifier $identifier, VisitsCountFiltering $filtering): int; /** * @return Visit[] diff --git a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php index 6e03114c..1394e1ab 100644 --- a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php +++ b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php @@ -27,11 +27,7 @@ class ShortUrlResolver implements ShortUrlResolverInterface { /** @var ShortUrlRepository $shortUrlRepo */ $shortUrlRepo = $this->em->getRepository(ShortUrl::class); - $shortUrl = $shortUrlRepo->findOne( - $identifier->shortCode(), - $identifier->domain(), - $apiKey !== null ? $apiKey->spec() : null, - ); + $shortUrl = $shortUrlRepo->findOne($identifier, $apiKey !== null ? $apiKey->spec() : null); 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 345e0911..867ff3f2 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -210,12 +210,16 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - self::assertNotNull($this->repo->findOne('my-cool-slug')); - self::assertNull($this->repo->findOne('my-cool-slug', 'doma.in')); - self::assertNull($this->repo->findOne('slug-not-in-use')); - self::assertNull($this->repo->findOne('another-slug')); - self::assertNull($this->repo->findOne('another-slug', 'example.com')); - self::assertNotNull($this->repo->findOne('another-slug', 'doma.in')); + self::assertNotNull($this->repo->findOne(ShortUrlIdentifier::fromShortCodeAndDomain('my-cool-slug'))); + self::assertNull($this->repo->findOne(ShortUrlIdentifier::fromShortCodeAndDomain('my-cool-slug', 'doma.in'))); + self::assertNull($this->repo->findOne(ShortUrlIdentifier::fromShortCodeAndDomain('slug-not-in-use'))); + self::assertNull($this->repo->findOne(ShortUrlIdentifier::fromShortCodeAndDomain('another-slug'))); + self::assertNull($this->repo->findOne( + ShortUrlIdentifier::fromShortCodeAndDomain('another-slug', 'example.com'), + )); + self::assertNotNull($this->repo->findOne( + ShortUrlIdentifier::fromShortCodeAndDomain('another-slug', 'doma.in'), + )); } /** @test */ diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php index f77d7fd0..15fe34f4 100644 --- a/module/Core/test-db/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Repository/VisitRepositoryTest.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Repository\VisitRepository; @@ -89,32 +90,46 @@ class VisitRepositoryTest extends DatabaseTestCase { [$shortCode, $domain] = $this->createShortUrlsAndVisits(); - self::assertCount(0, $this->repo->findVisitsByShortCode('invalid', null, new VisitsListFiltering())); - self::assertCount(6, $this->repo->findVisitsByShortCode($shortCode, null, new VisitsListFiltering())); - self::assertCount(4, $this->repo->findVisitsByShortCode($shortCode, null, new VisitsListFiltering(null, true))); - self::assertCount(3, $this->repo->findVisitsByShortCode($shortCode, $domain, new VisitsListFiltering())); - self::assertCount(2, $this->repo->findVisitsByShortCode($shortCode, null, new VisitsListFiltering( - DateRange::withStartAndEndDate(Chronos::parse('2016-01-02'), Chronos::parse('2016-01-03')), - ))); - self::assertCount(4, $this->repo->findVisitsByShortCode($shortCode, null, new VisitsListFiltering( - DateRange::withStartDate(Chronos::parse('2016-01-03')), - ))); - self::assertCount(1, $this->repo->findVisitsByShortCode($shortCode, $domain, new VisitsListFiltering( - DateRange::withStartDate(Chronos::parse('2016-01-03')), - ))); + self::assertCount(0, $this->repo->findVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain('invalid'), + new VisitsListFiltering(), + )); + self::assertCount(6, $this->repo->findVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), + new VisitsListFiltering(), + )); + self::assertCount(4, $this->repo->findVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), + new VisitsListFiltering(null, true), + )); self::assertCount(3, $this->repo->findVisitsByShortCode( - $shortCode, - null, + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode, $domain), + new VisitsListFiltering(), + )); + self::assertCount(2, $this->repo->findVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), + new VisitsListFiltering( + DateRange::withStartAndEndDate(Chronos::parse('2016-01-02'), Chronos::parse('2016-01-03')), + ), + )); + self::assertCount(4, $this->repo->findVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), + new VisitsListFiltering(DateRange::withStartDate(Chronos::parse('2016-01-03'))), + )); + self::assertCount(1, $this->repo->findVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode, $domain), + new VisitsListFiltering(DateRange::withStartDate(Chronos::parse('2016-01-03'))), + )); + self::assertCount(3, $this->repo->findVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), new VisitsListFiltering(null, false, null, 3, 2), )); self::assertCount(2, $this->repo->findVisitsByShortCode( - $shortCode, - null, + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), new VisitsListFiltering(null, false, null, 5, 4), )); self::assertCount(1, $this->repo->findVisitsByShortCode( - $shortCode, - $domain, + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode, $domain), new VisitsListFiltering(null, false, null, 3, 2), )); } @@ -124,22 +139,36 @@ class VisitRepositoryTest extends DatabaseTestCase { [$shortCode, $domain] = $this->createShortUrlsAndVisits(); - self::assertEquals(0, $this->repo->countVisitsByShortCode('invalid', null, new VisitsCountFiltering())); - self::assertEquals(6, $this->repo->countVisitsByShortCode($shortCode, null, new VisitsCountFiltering())); - self::assertEquals(4, $this->repo->countVisitsByShortCode($shortCode, null, new VisitsCountFiltering( - null, - true, - ))); - self::assertEquals(3, $this->repo->countVisitsByShortCode($shortCode, $domain, new VisitsCountFiltering())); - self::assertEquals(2, $this->repo->countVisitsByShortCode($shortCode, null, new VisitsCountFiltering( - DateRange::withStartAndEndDate(Chronos::parse('2016-01-02'), Chronos::parse('2016-01-03')), - ))); - self::assertEquals(4, $this->repo->countVisitsByShortCode($shortCode, null, new VisitsCountFiltering( - DateRange::withStartDate(Chronos::parse('2016-01-03')), - ))); - self::assertEquals(1, $this->repo->countVisitsByShortCode($shortCode, $domain, new VisitsCountFiltering( - DateRange::withStartDate(Chronos::parse('2016-01-03')), - ))); + self::assertEquals(0, $this->repo->countVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain('invalid'), + new VisitsCountFiltering(), + )); + self::assertEquals(6, $this->repo->countVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), + new VisitsCountFiltering(), + )); + self::assertEquals(4, $this->repo->countVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), + new VisitsCountFiltering(null, true), + )); + self::assertEquals(3, $this->repo->countVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode, $domain), + new VisitsCountFiltering(), + )); + self::assertEquals(2, $this->repo->countVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), + new VisitsCountFiltering( + DateRange::withStartAndEndDate(Chronos::parse('2016-01-02'), Chronos::parse('2016-01-03')), + ), + )); + self::assertEquals(4, $this->repo->countVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), + new VisitsCountFiltering(DateRange::withStartDate(Chronos::parse('2016-01-03'))), + )); + self::assertEquals(1, $this->repo->countVisitsByShortCode( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode, $domain), + new VisitsCountFiltering(DateRange::withStartDate(Chronos::parse('2016-01-03'))), + )); } /** @test */ diff --git a/module/Core/test/Paginator/Adapter/VisitsPaginatorAdapterTest.php b/module/Core/test/Paginator/Adapter/VisitsPaginatorAdapterTest.php index d7b454b0..2a9e5fc4 100644 --- a/module/Core/test/Paginator/Adapter/VisitsPaginatorAdapterTest.php +++ b/module/Core/test/Paginator/Adapter/VisitsPaginatorAdapterTest.php @@ -35,8 +35,7 @@ class VisitsPaginatorAdapterTest extends TestCase $offset = 5; $adapter = $this->createAdapter(null); $findVisits = $this->repo->findVisitsByShortCode( - '', - null, + ShortUrlIdentifier::fromShortCodeAndDomain(''), new VisitsListFiltering(new DateRange(), false, null, $limit, $offset), )->willReturn([]); @@ -54,8 +53,7 @@ class VisitsPaginatorAdapterTest extends TestCase $apiKey = ApiKey::create(); $adapter = $this->createAdapter($apiKey); $countVisits = $this->repo->countVisitsByShortCode( - '', - null, + ShortUrlIdentifier::fromShortCodeAndDomain(''), new VisitsCountFiltering(new DateRange(), false, $apiKey->spec()), )->willReturn(3); diff --git a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php index cf2330b3..73823729 100644 --- a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php +++ b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php @@ -46,12 +46,13 @@ class ShortUrlResolverTest extends TestCase { $shortUrl = ShortUrl::withLongUrl('expected_url'); $shortCode = $shortUrl->getShortCode(); + $identifier = ShortUrlIdentifier::fromShortCodeAndDomain($shortCode); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOne = $repo->findOne($shortCode, null, $apiKey !== null ? $apiKey->spec() : null)->willReturn($shortUrl); + $findOne = $repo->findOne($identifier, $apiKey !== null ? $apiKey->spec() : null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - $result = $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode), $apiKey); + $result = $this->urlResolver->resolveShortUrl($identifier, $apiKey); self::assertSame($shortUrl, $result); $findOne->shouldHaveBeenCalledOnce(); @@ -65,16 +66,17 @@ class ShortUrlResolverTest extends TestCase public function exceptionIsThrownIfShortcodeIsNotFound(?ApiKey $apiKey): void { $shortCode = 'abc123'; + $identifier = ShortUrlIdentifier::fromShortCodeAndDomain($shortCode); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOne = $repo->findOne($shortCode, null, $apiKey !== null ? $apiKey->spec() : null)->willReturn(null); + $findOne = $repo->findOne($identifier, $apiKey !== null ? $apiKey->spec() : null)->willReturn(null); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal(), $apiKey); $this->expectException(ShortUrlNotFoundException::class); $findOne->shouldBeCalledOnce(); $getRepo->shouldBeCalledOnce(); - $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode), $apiKey); + $this->urlResolver->resolveShortUrl($identifier, $apiKey); } /** @test */ diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index 372bda9a..cae3fbb1 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -79,20 +79,22 @@ class VisitsStatsHelperTest extends TestCase public function infoReturnsVisitsForCertainShortCode(?ApiKey $apiKey): void { $shortCode = '123ABC'; + $identifier = ShortUrlIdentifier::fromShortCodeAndDomain($shortCode); $spec = $apiKey === null ? null : $apiKey->spec(); + $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $count = $repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), $spec)->willReturn( + $count = $repo->shortCodeIsInUse($identifier, $spec)->willReturn( true, ); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); $repo2 = $this->prophesize(VisitRepository::class); - $repo2->findVisitsByShortCode($shortCode, null, Argument::type(VisitsListFiltering::class))->willReturn($list); - $repo2->countVisitsByShortCode($shortCode, null, Argument::type(VisitsCountFiltering::class))->willReturn(1); + $repo2->findVisitsByShortCode($identifier, Argument::type(VisitsListFiltering::class))->willReturn($list); + $repo2->countVisitsByShortCode($identifier, Argument::type(VisitsCountFiltering::class))->willReturn(1); $this->em->getRepository(Visit::class)->willReturn($repo2->reveal())->shouldBeCalledOnce(); - $paginator = $this->helper->visitsForShortUrl(new ShortUrlIdentifier($shortCode), new VisitsParams(), $apiKey); + $paginator = $this->helper->visitsForShortUrl($identifier, new VisitsParams(), $apiKey); self::assertEquals($list, ArrayUtils::iteratorToArray($paginator->getCurrentPageResults())); $count->shouldHaveBeenCalledOnce(); @@ -102,8 +104,10 @@ class VisitsStatsHelperTest extends TestCase public function throwsExceptionWhenRequestingVisitsForInvalidShortCode(): void { $shortCode = '123ABC'; + $identifier = ShortUrlIdentifier::fromShortCodeAndDomain($shortCode); + $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $count = $repo->shortCodeIsInUse(ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), null)->willReturn( + $count = $repo->shortCodeIsInUse($identifier, null)->willReturn( false, ); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); @@ -111,7 +115,7 @@ class VisitsStatsHelperTest extends TestCase $this->expectException(ShortUrlNotFoundException::class); $count->shouldBeCalledOnce(); - $this->helper->visitsForShortUrl(new ShortUrlIdentifier($shortCode), new VisitsParams()); + $this->helper->visitsForShortUrl($identifier, new VisitsParams()); } /** @test */ From 46bea241e632fbd5c6d51c9ab49c0503d39fbf9a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 23 May 2021 08:42:26 +0200 Subject: [PATCH 5/5] Updated changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22176f7e..b7df04c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). -## [Unreleased] +## [2.7.0] - 2021-05-23 ### Added * [#1044](https://github.com/shlinkio/shlink/issues/1044) Added ability to set names on API keys, which helps to identify them when the list grows. * [#819](https://github.com/shlinkio/shlink/issues/819) Visits are now always located in real time, even when not using swoole.