Skip to content

Commit

Permalink
Merge pull request #1948 from acelaya-forks/feature/fix-postgres-import
Browse files Browse the repository at this point in the history
Fix error when importing short URLs while using Postgres
  • Loading branch information
acelaya authored Dec 16, 2023
2 parents 3a43aa4 + 1b14bb0 commit 4d28adf
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 11 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 10 additions & 3 deletions module/Core/src/Importer/ShortUrlImporting.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
}

Expand Down Expand Up @@ -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;
}
}
23 changes: 16 additions & 7 deletions module/Core/test/Importer/ImportedLinksProcessorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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];
}

/**
Expand Down

0 comments on commit 4d28adf

Please sign in to comment.