From 1b14bb07b108771c681f81ddab854dde4e649703 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Sat, 16 Dec 2023 20:21:59 +0100
Subject: [PATCH] Fix error when importing short URLs while using Postgres

---
 CHANGELOG.md                                  |  2 +-
 .../Core/src/Importer/ShortUrlImporting.php   | 13 ++++++++---
 .../Importer/ImportedLinksProcessorTest.php   | 23 +++++++++++++------
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3b483eced..02c2396f2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,7 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 * *Nothing*
 
 ### Fixed
-* *Nothing*
+* [#1947](https://github.com/shlinkio/shlink/issues/1947) Fix error when importing short URLs while using Postgres.
 
 
 ## [3.7.0] - 2023-11-25
diff --git a/module/Core/src/Importer/ShortUrlImporting.php b/module/Core/src/Importer/ShortUrlImporting.php
index f806f856a..ad812e8c8 100644
--- a/module/Core/src/Importer/ShortUrlImporting.php
+++ b/module/Core/src/Importer/ShortUrlImporting.php
@@ -12,9 +12,9 @@
 use function Shlinkio\Shlink\Core\normalizeDate;
 use function sprintf;
 
-final class ShortUrlImporting
+final readonly class ShortUrlImporting
 {
-    private function __construct(private readonly ShortUrl $shortUrl, private readonly bool $isNew)
+    private function __construct(private ShortUrl $shortUrl, private bool $isNew)
     {
     }
 
@@ -57,11 +57,18 @@ public function importVisits(iterable $visits, EntityManagerInterface $em): stri
 
     private function resolveShortUrl(EntityManagerInterface $em): ShortUrl
     {
+        // If wrapped ShortUrl has no ID, avoid trying to query the EM, as it would fail in Postgres.
+        // See https://github.com/shlinkio/shlink/issues/1947
+        $id = $this->shortUrl->getId();
+        if (!$id) {
+            return $this->shortUrl;
+        }
+
         // Instead of directly accessing wrapped ShortUrl entity, try to get it from the EM.
         // With this, we will get the same entity from memory if it is known by the EM, but if it was cleared, the EM
         // will fetch it again from the database, preventing errors at runtime.
         // However, if the EM was not flushed yet, the entity will not be found by ID, but it is known by the EM.
         // In that case, we fall back to wrapped ShortUrl entity directly.
-        return $em->find(ShortUrl::class, $this->shortUrl->getId()) ?? $this->shortUrl;
+        return $em->find(ShortUrl::class, $id) ?? $this->shortUrl;
     }
 }
diff --git a/module/Core/test/Importer/ImportedLinksProcessorTest.php b/module/Core/test/Importer/ImportedLinksProcessorTest.php
index 921273c1e..e267744d1 100644
--- a/module/Core/test/Importer/ImportedLinksProcessorTest.php
+++ b/module/Core/test/Importer/ImportedLinksProcessorTest.php
@@ -232,15 +232,20 @@ public static function provideUrlsWithVisits(): iterable
     }
 
     #[Test, DataProvider('provideFoundShortUrls')]
-    public function visitsArePersistedWithProperShortUrl(?ShortUrl $foundShortUrl): void
+    public function visitsArePersistedWithProperShortUrl(ShortUrl $originalShortUrl, ?ShortUrl $foundShortUrl): void
     {
-        $originalShortUrl = ShortUrl::withLongUrl('https://foo');
-
         $this->em->method('getRepository')->with(ShortUrl::class)->willReturn($this->repo);
         $this->repo->expects($this->once())->method('findOneByImportedUrl')->willReturn($originalShortUrl);
-        $this->em->expects($this->exactly(2))->method('find')->willReturn($foundShortUrl);
+        if (!$originalShortUrl->getId()) {
+            $this->em->expects($this->never())->method('find');
+        } else {
+            $this->em->expects($this->exactly(2))->method('find')->willReturn($foundShortUrl);
+        }
         $this->em->expects($this->once())->method('persist')->willReturnCallback(
-            static fn (Visit $visit)  => Assert::assertSame($foundShortUrl ?? $originalShortUrl, $visit->getShortUrl()),
+            static fn (Visit $visit)  => Assert::assertSame(
+                $foundShortUrl ?? $originalShortUrl,
+                $visit->getShortUrl(),
+            ),
         );
 
         $now = Chronos::now();
@@ -253,8 +258,12 @@ public function visitsArePersistedWithProperShortUrl(?ShortUrl $foundShortUrl):
 
     public static function provideFoundShortUrls(): iterable
     {
-        yield [null];
-        yield [ShortUrl::withLongUrl('https://bar')];
+        yield 'not found new URL' => [ShortUrl::withLongUrl('https://foo')->setId('123'), null];
+        yield 'found new URL' => [
+            ShortUrl::withLongUrl('https://foo')->setId('123'),
+            ShortUrl::withLongUrl('https://bar'),
+        ];
+        yield 'old URL without ID' => [$originalShortUrl = ShortUrl::withLongUrl('https://foo'), $originalShortUrl];
     }
 
     /**