Merge pull request #387 from acelaya/feature/fix-check-exists

Feature/fix check exists
shlinkio · Mar 30, 2019 · 20c3bde · 20c3bde
2 parents 781c6e9 + e77e370
commit 20c3bde
Show file tree

Hide file tree

Showing 3 changed files with 73 additions and 29 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,16 +4,17 @@ 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
 
-* [#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.
+* [#341](https://github.com/shlinkio/shlink/issues/341) Improved database tests so that they are executed against all supported database engines.
 
 #### Deprecated
 
@@ -25,7 +26,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

diff --git 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
@@ -27,6 +27,8 @@
 use Shlinkio\Shlink\Core\Service\UrlShortener;
 use Zend\Diactoros\Uri;
 
+use function array_map;
+
 class UrlShortenerTest extends TestCase
 {
     /** @var UrlShortener */
@@ -121,7 +123,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,20 +148,23 @@ 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 !== 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
     {