From 823573cea7565d799c6e9ee879f5c47cb58a3e50 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 10 Apr 2021 10:16:09 +0200 Subject: [PATCH 1/3] Updated PersistenceShortUrlRelationResolver to prevent duplicated tags --- .../PersistenceShortUrlRelationResolver.php | 3 +++ ...ersistenceShortUrlRelationResolverTest.php | 24 +++++++++++++------ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php index fd0428bf..1b004a95 100644 --- a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\Tag; use function Functional\map; +use function Functional\unique; class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInterface { @@ -42,7 +43,9 @@ 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); $this->em->persist($tag); diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php index 463ee1ef..187ccbe6 100644 --- a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php @@ -14,6 +14,7 @@ use Shlinkio\Shlink\Core\Entity\Domain; 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 { @@ -66,10 +67,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 +85,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 */ From 28c06de685d274a3809cc1f0450cd1d77a490185 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 10 Apr 2021 11:59:43 +0200 Subject: [PATCH 2/3] Fixed issue when trying to persist several short URLs which include the same new tag/domain at once --- composer.json | 2 +- .../PersistenceShortUrlRelationResolver.php | 30 ++++++++++++- ...ersistenceShortUrlRelationResolverTest.php | 45 +++++++++++++++++++ 3 files changed, 74 insertions(+), 3 deletions(-) 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 1b004a95..eb7fddad 100644 --- a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php @@ -7,6 +7,7 @@ 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; @@ -17,9 +18,15 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt { 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 @@ -30,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); } /** @@ -47,10 +61,22 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt $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 187ccbe6..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; @@ -14,6 +15,7 @@ use Shlinkio\Shlink\Core\Entity\Domain; 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 @@ -26,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()); } @@ -113,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); + } } From 6387e50276a4cb233e961ea88979fbe8ac8058e0 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 10 Apr 2021 12:03:40 +0200 Subject: [PATCH 3/3] Updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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