Merge pull request #1068 from acelaya-forks/feature/dependency-persistence

Feature/dependency persistence

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

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",

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<string, Domain> */
+    private array $memoizedNewDomains = [];
+    /** @var array<string, Tag> */
+    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 = [];
+    }
 }

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);
+    }
 }