Fixed issue when trying to persist several short URLs which include the same new tag/domain at once

This commit is contained in:
Alejandro Celaya 2021-04-10 11:59:43 +02:00
parent 823573cea7
commit 28c06de685
3 changed files with 74 additions and 3 deletions

View file

@ -46,7 +46,7 @@
"predis/predis": "^1.1", "predis/predis": "^1.1",
"pugx/shortid-php": "^0.7", "pugx/shortid-php": "^0.7",
"ramsey/uuid": "^3.9", "ramsey/uuid": "^3.9",
"shlinkio/shlink-common": "dev-main#3777189 as 3.7", "shlinkio/shlink-common": "dev-main#554e370 as 3.7",
"shlinkio/shlink-config": "^1.0", "shlinkio/shlink-config": "^1.0",
"shlinkio/shlink-event-dispatcher": "^2.1", "shlinkio/shlink-event-dispatcher": "^2.1",
"shlinkio/shlink-importer": "^2.2", "shlinkio/shlink-importer": "^2.2",

View file

@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Resolver;
use Doctrine\Common\Collections; 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 Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\Domain;
use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Entity\Tag;
@ -17,9 +18,15 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt
{ {
private EntityManagerInterface $em; private EntityManagerInterface $em;
/** @var array<string, Domain> */
private array $memoizedNewDomains = [];
/** @var array<string, Tag> */
private array $memoizedNewTags = [];
public function __construct(EntityManagerInterface $em) public function __construct(EntityManagerInterface $em)
{ {
$this->em = $em; $this->em = $em;
$this->em->getEventManager()->addEventListener(Events::postFlush, $this);
} }
public function resolveDomain(?string $domain): ?Domain public function resolveDomain(?string $domain): ?Domain
@ -30,7 +37,14 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt
/** @var Domain|null $existingDomain */ /** @var Domain|null $existingDomain */
$existingDomain = $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]); $existingDomain = $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]);
return $existingDomain ?? new Domain($domain);
// Memoize only new domains, and let doctrine handle objects hydrated from persistence
return $existingDomain ?? $this->memoizeNewDomain($domain);
}
private function memoizeNewDomain(string $domain): Domain
{
return $this->memoizedNewDomains[$domain] = $this->memoizedNewDomains[$domain] ?? new Domain($domain);
} }
/** /**
@ -47,10 +61,22 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt
$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 {
$tag = $repo->findOneBy(['name' => $tagName]) ?? new Tag($tagName); // Memoize only new tags, and let doctrine handle objects hydrated from persistence
$tag = $repo->findOneBy(['name' => $tagName]) ?? $this->memoizeNewTag($tagName);
$this->em->persist($tag); $this->em->persist($tag);
return $tag; return $tag;
})); }));
} }
private function memoizeNewTag(string $tagName): Tag
{
return $this->memoizedNewTags[$tagName] = $this->memoizedNewTags[$tagName] ?? new Tag($tagName);
}
public function postFlush(): void
{
$this->memoizedNewDomains = [];
$this->memoizedNewTags = [];
}
} }

View file

@ -4,6 +4,7 @@ declare(strict_types=1);
namespace ShlinkioTest\Shlink\Core\ShortUrl\Resolver; namespace ShlinkioTest\Shlink\Core\ShortUrl\Resolver;
use Doctrine\Common\EventManager;
use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\EntityManagerInterface;
use Doctrine\Persistence\ObjectRepository; use Doctrine\Persistence\ObjectRepository;
use PHPUnit\Framework\TestCase; use PHPUnit\Framework\TestCase;
@ -14,6 +15,7 @@ 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 Shlinkio\Shlink\Core\Repository\TagRepositoryInterface;
use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver; use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver;
use function count; use function count;
class PersistenceShortUrlRelationResolverTest extends TestCase class PersistenceShortUrlRelationResolverTest extends TestCase
@ -26,6 +28,8 @@ class PersistenceShortUrlRelationResolverTest extends TestCase
public function setUp(): void public function setUp(): void
{ {
$this->em = $this->prophesize(EntityManagerInterface::class); $this->em = $this->prophesize(EntityManagerInterface::class);
$this->em->getEventManager()->willReturn(new EventManager());
$this->resolver = new PersistenceShortUrlRelationResolver($this->em->reveal()); $this->resolver = new PersistenceShortUrlRelationResolver($this->em->reveal());
} }
@ -113,4 +117,45 @@ class PersistenceShortUrlRelationResolverTest extends TestCase
$getRepo->shouldNotHaveBeenCalled(); $getRepo->shouldNotHaveBeenCalled();
$persist->shouldNotHaveBeenCalled(); $persist->shouldNotHaveBeenCalled();
} }
/** @test */
public function newDomainsAreMemoizedUntilStateIsCleared(): void
{
$repo = $this->prophesize(ObjectRepository::class);
$repo->findOneBy(Argument::type('array'))->willReturn(null);
$this->em->getRepository(Domain::class)->willReturn($repo->reveal());
$authority = 'foo.com';
$domain1 = $this->resolver->resolveDomain($authority);
$domain2 = $this->resolver->resolveDomain($authority);
self::assertSame($domain1, $domain2);
$this->resolver->postFlush();
$domain3 = $this->resolver->resolveDomain($authority);
self::assertNotSame($domain1, $domain3);
}
/** @test */
public function newTagsAreMemoizedUntilStateIsCleared(): void
{
$tagRepo = $this->prophesize(TagRepositoryInterface::class);
$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 {
});
$tags = ['foo', 'bar'];
[$foo1, $bar1] = $this->resolver->resolveTags($tags);
[$foo2, $bar2] = $this->resolver->resolveTags($tags);
self::assertSame($foo1, $foo2);
self::assertSame($bar1, $bar2);
$this->resolver->postFlush();
[$foo3, $bar3] = $this->resolver->resolveTags($tags);
self::assertNotSame($foo1, $foo3);
self::assertNotSame($bar1, $bar3);
}
} }