diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c88f036..bb0b6bb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Fixed * [#1041](https://github.com/shlinkio/shlink/issues/1041) Ensured the default value for the version while building the docker image is `latest`. +* [#1067](https://github.com/shlinkio/shlink/issues/1067) Fixed exception when persisting multiple short URLs in one batch which include the same new tags/domains. This can potentially happen when importing URLs. ## [2.6.2] - 2021-03-12 diff --git a/composer.json b/composer.json index 41885c62..7b72c92a 100644 --- a/composer.json +++ b/composer.json @@ -46,7 +46,7 @@ "predis/predis": "^1.1", "pugx/shortid-php": "^0.7", "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-event-dispatcher": "^2.1", "shlinkio/shlink-importer": "^2.2", diff --git a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php index fd0428bf..eb7fddad 100644 --- a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php @@ -7,18 +7,26 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Resolver; use Doctrine\Common\Collections; use Doctrine\Common\Collections\Collection; use Doctrine\ORM\EntityManagerInterface; +use Doctrine\ORM\Events; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\Tag; use function Functional\map; +use function Functional\unique; class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInterface { private EntityManagerInterface $em; + /** @var array */ + private array $memoizedNewDomains = []; + /** @var array */ + private array $memoizedNewTags = []; + public function __construct(EntityManagerInterface $em) { $this->em = $em; + $this->em->getEventManager()->addEventListener(Events::postFlush, $this); } public function resolveDomain(?string $domain): ?Domain @@ -29,7 +37,14 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt /** @var Domain|null $existingDomain */ $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); } /** @@ -42,12 +57,26 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt return new Collections\ArrayCollection(); } + $tags = unique($tags); $repo = $this->em->getRepository(Tag::class); + 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); 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 = []; + } } diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php index 463ee1ef..8660099c 100644 --- a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\ShortUrl\Resolver; +use Doctrine\Common\EventManager; use Doctrine\ORM\EntityManagerInterface; use Doctrine\Persistence\ObjectRepository; use PHPUnit\Framework\TestCase; @@ -15,6 +16,8 @@ use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Repository\TagRepositoryInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver; +use function count; + class PersistenceShortUrlRelationResolverTest extends TestCase { use ProphecyTrait; @@ -25,6 +28,8 @@ class PersistenceShortUrlRelationResolverTest extends TestCase public function setUp(): void { $this->em = $this->prophesize(EntityManagerInterface::class); + $this->em->getEventManager()->willReturn(new EventManager()); + $this->resolver = new PersistenceShortUrlRelationResolver($this->em->reveal()); } @@ -66,10 +71,13 @@ class PersistenceShortUrlRelationResolverTest extends TestCase yield 'found domain' => [new Domain($authority), $authority]; } - /** @test */ - public function findsAndPersistsTagsWrappedIntoCollection(): void + /** + * @test + * @dataProvider provideTags + */ + public function findsAndPersistsTagsWrappedIntoCollection(array $tags, array $expectedTags): void { - $tags = ['foo', 'bar', 'baz']; + $expectedPersistedTags = count($expectedTags); $tagRepo = $this->prophesize(TagRepositoryInterface::class); $findTag = $tagRepo->findOneBy(Argument::type('array'))->will(function (array $args): ?Tag { @@ -81,11 +89,17 @@ class PersistenceShortUrlRelationResolverTest extends TestCase $result = $this->resolver->resolveTags($tags); - self::assertCount(3, $result); - self::assertEquals([new Tag('foo'), new Tag('bar'), new Tag('baz')], $result->toArray()); - $findTag->shouldHaveBeenCalledTimes(3); + self::assertCount($expectedPersistedTags, $result); + self::assertEquals($expectedTags, $result->toArray()); + $findTag->shouldHaveBeenCalledTimes($expectedPersistedTags); $getRepo->shouldHaveBeenCalledOnce(); - $persist->shouldHaveBeenCalledTimes(3); + $persist->shouldHaveBeenCalledTimes($expectedPersistedTags); + } + + public function provideTags(): iterable + { + yield 'no duplicated tags' => [['foo', 'bar', 'baz'], [new Tag('foo'), new Tag('bar'), new Tag('baz')]]; + yield 'duplicated tags' => [['foo', 'bar', 'bar'], [new Tag('foo'), new Tag('bar')]]; } /** @test */ @@ -103,4 +117,45 @@ class PersistenceShortUrlRelationResolverTest extends TestCase $getRepo->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); + } }