From 0135f205df6881f7952be964e75d9d0cd693ffb4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 17 Mar 2019 17:54:57 +0100 Subject: [PATCH 1/4] Updated changelog --- CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ca4c9b5a..7ecad85b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,12 +8,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Added -* [#153](https://github.com/shlinkio/shlink/issues/153) Updated to [doctrine/migrations](https://github.com/doctrine/migrations) version 2.0.0 -* [#376](https://github.com/shlinkio/shlink/issues/376) Allowed `visit:update-db` command to not return an error exit code even if download fails, by passing the `-i` flag. +* *Nothing* #### Changed -* *Nothing* +* [#153](https://github.com/shlinkio/shlink/issues/153) Updated to [doctrine/migrations](https://github.com/doctrine/migrations) version 2.0.0 +* [#376](https://github.com/shlinkio/shlink/issues/376) Allowed `visit:update-db` command to not return an error exit code even if download fails, by passing the `-i` flag. #### Deprecated @@ -25,7 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Fixed -* *Nothing* +* [#382](https://github.com/shlinkio/shlink/issues/382) Fixed existing short URLs not properly checked when providing the `findIfExists` flag. ## 1.16.2 - 2019-03-05 From 2906d42f978854a980acb3e95f0f9768dbe1eb31 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 30 Mar 2019 07:36:57 +0100 Subject: [PATCH 2/4] Updated how existing short URLs are checked, so that not only the first one matching the slug or url is checked --- module/Core/src/Service/UrlShortener.php | 49 +++++++++++-------- module/Core/test/Service/UrlShortenerTest.php | 4 +- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index 687a21fda..fe0419002 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -112,32 +112,39 @@ private function findExistingShortUrlIfExists(string $url, array $tags, ShortUrl if ($meta->hasCustomSlug()) { $criteria['shortCode'] = $meta->getCustomSlug(); } - /** @var ShortUrl|null $shortUrl */ - $shortUrl = $this->em->getRepository(ShortUrl::class)->findOneBy($criteria); - if ($shortUrl === null) { + /** @var ShortUrl[] $shortUrls */ + $shortUrls = $this->em->getRepository(ShortUrl::class)->findBy($criteria); + if (empty($shortUrls)) { return null; } - if ($meta->hasMaxVisits() && $meta->getMaxVisits() !== $shortUrl->getMaxVisits()) { - return null; - } - if ($meta->hasValidSince() && ! $meta->getValidSince()->eq($shortUrl->getValidSince())) { - return null; - } - if ($meta->hasValidUntil() && ! $meta->getValidUntil()->eq($shortUrl->getValidUntil())) { - return null; - } + // Iterate short URLs until one that matches is found, or return null otherwise + return array_reduce($shortUrls, function (?ShortUrl $found, ShortUrl $shortUrl) use ($tags, $meta) { + if ($found) { + return $found; + } - $shortUrlTags = invoke($shortUrl->getTags(), '__toString'); - $hasAllTags = count($shortUrlTags) === count($tags) && array_reduce( - $tags, - function (bool $hasAllTags, string $tag) use ($shortUrlTags) { - return $hasAllTags && contains($shortUrlTags, $tag); - }, - true - ); + if ($meta->hasMaxVisits() && $meta->getMaxVisits() !== $shortUrl->getMaxVisits()) { + return null; + } + if ($meta->hasValidSince() && ! $meta->getValidSince()->eq($shortUrl->getValidSince())) { + return null; + } + if ($meta->hasValidUntil() && ! $meta->getValidUntil()->eq($shortUrl->getValidUntil())) { + return null; + } - return $hasAllTags ? $shortUrl : null; + $shortUrlTags = invoke($shortUrl->getTags(), '__toString'); + $hasAllTags = count($shortUrlTags) === count($tags) && array_reduce( + $tags, + function (bool $hasAllTags, string $tag) use ($shortUrlTags) { + return $hasAllTags && contains($shortUrlTags, $tag); + }, + true + ); + + return $hasAllTags ? $shortUrl : null; + }); } private function checkUrlExists(string $url): void diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index bc91dcb8e..967e19b98 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -121,7 +121,7 @@ public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void { $repo = $this->prophesize(ShortUrlRepository::class); $countBySlug = $repo->count(['shortCode' => 'custom-slug'])->willReturn(1); - $repo->findOneBy(Argument::cetera())->willReturn(null); + $repo->findBy(Argument::cetera())->willReturn([]); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $countBySlug->shouldBeCalledOnce(); @@ -146,7 +146,7 @@ public function existingShortUrlIsReturnedWhenRequested( ?ShortUrl $expected ): void { $repo = $this->prophesize(ShortUrlRepository::class); - $findExisting = $repo->findOneBy(Argument::any())->willReturn($expected); + $findExisting = $repo->findBy(Argument::any())->willReturn([$expected]); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlShortener->urlToShortCode(new Uri($url), $tags, $meta); From 734fdf83c1a40f28facb311d87ed619c5d061d92 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 30 Mar 2019 07:45:57 +0100 Subject: [PATCH 3/4] Added test covering the case in which fetching existing short URLs, more than one result is found --- module/Core/test/Service/UrlShortenerTest.php | 40 ++++++++++++++++++- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 967e19b98..50ea91e14 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -27,6 +27,8 @@ use Shlinkio\Shlink\Core\Service\UrlShortener; use Zend\Diactoros\Uri; +use function array_map; + class UrlShortenerTest extends TestCase { /** @var UrlShortener */ @@ -146,20 +148,23 @@ public function existingShortUrlIsReturnedWhenRequested( ?ShortUrl $expected ): void { $repo = $this->prophesize(ShortUrlRepository::class); - $findExisting = $repo->findBy(Argument::any())->willReturn([$expected]); + $findExisting = $repo->findBy(Argument::any())->willReturn($expected !== null ? [$expected] : []); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlShortener->urlToShortCode(new Uri($url), $tags, $meta); - $this->assertSame($expected, $result); $findExisting->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); + if ($expected) { + $this->assertSame($expected, $result); + } } public function provideExistingShortUrls(): iterable { $url = 'http://foo.com'; + yield [$url, [], ShortUrlMeta::createFromRawData(['findIfExists' => true]), null]; yield [$url, [], ShortUrlMeta::createFromRawData(['findIfExists' => true]), new ShortUrl($url)]; yield [$url, [], ShortUrlMeta::createFromRawData( ['findIfExists' => true, 'customSlug' => 'foo'] @@ -203,6 +208,37 @@ public function provideExistingShortUrls(): iterable ]; } + /** @test */ + public function properExistingShortUrlIsReturnedWhenMultipleMatch(): void + { + $url = 'http://foo.com'; + $tags = ['baz', 'foo', 'bar']; + $meta = ShortUrlMeta::createFromRawData([ + 'findIfExists' => true, + 'validUntil' => Chronos::parse('2017-01-01'), + 'maxVisits' => 4, + ]); + $tagsCollection = new ArrayCollection(array_map(function (string $tag) { + return new Tag($tag); + }, $tags)); + $expected = (new ShortUrl($url, $meta))->setTags($tagsCollection); + + $repo = $this->prophesize(ShortUrlRepository::class); + $findExisting = $repo->findBy(Argument::any())->willReturn([ + new ShortUrl($url), + new ShortUrl($url, $meta), + $expected, + (new ShortUrl($url))->setTags($tagsCollection), + ]); + $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + + $result = $this->urlShortener->urlToShortCode(new Uri($url), $tags, $meta); + + $this->assertSame($expected, $result); + $findExisting->shouldHaveBeenCalledOnce(); + $getRepo->shouldHaveBeenCalledOnce(); + } + /** @test */ public function shortCodeIsProperlyParsed(): void { From e77e37076fa450211cce75f6fd1c5f6c8200eb30 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 30 Mar 2019 07:48:54 +0100 Subject: [PATCH 4/4] Updated changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ecad85b2..08bfd8b7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). -## [Unreleased] +## 1.16.3 - 2019-03-30 #### Added @@ -14,6 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#153](https://github.com/shlinkio/shlink/issues/153) Updated to [doctrine/migrations](https://github.com/doctrine/migrations) version 2.0.0 * [#376](https://github.com/shlinkio/shlink/issues/376) Allowed `visit:update-db` command to not return an error exit code even if download fails, by passing the `-i` flag. +* [#341](https://github.com/shlinkio/shlink/issues/341) Improved database tests so that they are executed against all supported database engines. #### Deprecated