Removed methods to create tags and domains with lock, as they do not really lock as expected

This commit is contained in:
Alejandro Celaya 2021-05-23 08:21:40 +02:00
parent f82e103bc5
commit cd19876419
6 changed files with 8 additions and 43 deletions

View File

@ -4,7 +4,6 @@ declare(strict_types=1);
namespace Shlinkio\Shlink\Core\Domain\Repository; namespace Shlinkio\Shlink\Core\Domain\Repository;
use Doctrine\DBAL\LockMode;
use Doctrine\ORM\Query\Expr\Join; use Doctrine\ORM\Query\Expr\Join;
use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository;
use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\Domain;
@ -33,16 +32,4 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe
return $qb->getQuery()->getResult(); 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();
}
} }

View File

@ -15,6 +15,4 @@ interface DomainRepositoryInterface extends ObjectRepository, EntitySpecificatio
* @return Domain[] * @return Domain[]
*/ */
public function findDomainsWithout(?string $excludedAuthority, ?ApiKey $apiKey = null): array; public function findDomainsWithout(?string $excludedAuthority, ?ApiKey $apiKey = null): array;
public function findOneByAuthorityWithLock(string $authority): ?Domain;
} }

View File

@ -4,7 +4,6 @@ declare(strict_types=1);
namespace Shlinkio\Shlink\Core\Repository; namespace Shlinkio\Shlink\Core\Repository;
use Doctrine\DBAL\LockMode;
use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository;
use Happyr\DoctrineSpecification\Spec; use Happyr\DoctrineSpecification\Spec;
use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Entity\Tag;
@ -63,16 +62,4 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
return $result > 0; 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();
}
} }

View File

@ -6,7 +6,6 @@ namespace Shlinkio\Shlink\Core\Repository;
use Doctrine\Persistence\ObjectRepository; use Doctrine\Persistence\ObjectRepository;
use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepositoryInterface; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepositoryInterface;
use Shlinkio\Shlink\Core\Entity\Tag;
use Shlinkio\Shlink\Core\Tag\Model\TagInfo; use Shlinkio\Shlink\Core\Tag\Model\TagInfo;
use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Entity\ApiKey;
@ -20,6 +19,4 @@ interface TagRepositoryInterface extends ObjectRepository, EntitySpecificationRe
public function findTagsWithInfo(?ApiKey $apiKey = null): array; public function findTagsWithInfo(?ApiKey $apiKey = null): array;
public function tagExists(string $tag, ?ApiKey $apiKey = null): bool; public function tagExists(string $tag, ?ApiKey $apiKey = null): bool;
public function findOneByNameWithLock(string $name): ?Tag;
} }

View File

@ -8,10 +8,8 @@ use Doctrine\Common\Collections;
use Doctrine\Common\Collections\Collection; use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Events; use Doctrine\ORM\Events;
use Shlinkio\Shlink\Core\Domain\Repository\DomainRepositoryInterface;
use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\Domain;
use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Entity\Tag;
use Shlinkio\Shlink\Core\Repository\TagRepositoryInterface;
use function Functional\map; use function Functional\map;
use function Functional\unique; use function Functional\unique;
@ -37,9 +35,8 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt
return null; return null;
} }
/** @var DomainRepositoryInterface $repo */ /** @var Domain|null $existingDomain */
$repo = $this->em->getRepository(Domain::class); $existingDomain = $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]);
$existingDomain = $repo->findOneByAuthorityWithLock($domain);
// Memoize only new domains, and let doctrine handle objects hydrated from persistence // Memoize only new domains, and let doctrine handle objects hydrated from persistence
return $existingDomain ?? $this->memoizeNewDomain($domain); return $existingDomain ?? $this->memoizeNewDomain($domain);
@ -61,12 +58,11 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt
} }
$tags = unique($tags); $tags = unique($tags);
/** @var TagRepositoryInterface $repo */
$repo = $this->em->getRepository(Tag::class); $repo = $this->em->getRepository(Tag::class);
return new Collections\ArrayCollection(map($tags, function (string $tagName) use ($repo): Tag { return new Collections\ArrayCollection(map($tags, function (string $tagName) use ($repo): Tag {
// Memoize only new tags, and let doctrine handle objects hydrated from persistence // 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); $this->em->persist($tag);
return $tag; return $tag;

View File

@ -49,7 +49,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase
public function findsOrCreatesDomainWhenValueIsProvided(?Domain $foundDomain, string $authority): void public function findsOrCreatesDomainWhenValueIsProvided(?Domain $foundDomain, string $authority): void
{ {
$repo = $this->prophesize(DomainRepositoryInterface::class); $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()); $getRepository = $this->em->getRepository(Domain::class)->willReturn($repo->reveal());
$result = $this->resolver->resolveDomain($authority); $result = $this->resolver->resolveDomain($authority);
@ -80,7 +80,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase
$expectedPersistedTags = count($expectedTags); $expectedPersistedTags = count($expectedTags);
$tagRepo = $this->prophesize(TagRepositoryInterface::class); $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]; ['name' => $name] = $args[0];
return $name === 'foo' ? new Tag($name) : null; return $name === 'foo' ? new Tag($name) : null;
}); });
@ -106,7 +106,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase
public function returnsEmptyCollectionWhenProvidingEmptyListOfTags(): void public function returnsEmptyCollectionWhenProvidingEmptyListOfTags(): void
{ {
$tagRepo = $this->prophesize(TagRepositoryInterface::class); $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()); $getRepo = $this->em->getRepository(Tag::class)->willReturn($tagRepo->reveal());
$persist = $this->em->persist(Argument::type(Tag::class)); $persist = $this->em->persist(Argument::type(Tag::class));
@ -122,7 +122,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase
public function newDomainsAreMemoizedUntilStateIsCleared(): void public function newDomainsAreMemoizedUntilStateIsCleared(): void
{ {
$repo = $this->prophesize(DomainRepositoryInterface::class); $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()); $this->em->getRepository(Domain::class)->willReturn($repo->reveal());
$authority = 'foo.com'; $authority = 'foo.com';
@ -141,7 +141,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase
public function newTagsAreMemoizedUntilStateIsCleared(): void public function newTagsAreMemoizedUntilStateIsCleared(): void
{ {
$tagRepo = $this->prophesize(TagRepositoryInterface::class); $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->getRepository(Tag::class)->willReturn($tagRepo->reveal());
$this->em->persist(Argument::type(Tag::class))->will(function (): void { $this->em->persist(Argument::type(Tag::class))->will(function (): void {
}); });