Ensured domain is taken into account when checking if a slug is in use

module/Core/src/Domain/Resolver/PersistenceDomainResolver.php

@@ -22,6 +22,8 @@ class PersistenceDomainResolver implements DomainResolverInterface
             return null;
         }
 
-        return $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]) ?? new Domain($domain);
+        /** @var Domain|null $existingDomain */
+        $existingDomain = $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]);
+        return $existingDomain ?? new Domain($domain);
     }
 }

module/Core/src/Entity/ShortUrl.php

@@ -12,7 +12,10 @@ use Shlinkio\Shlink\Core\Domain\Resolver\SimpleDomainResolver;
 use Shlinkio\Shlink\Core\Model\ShortUrlMeta;
 use Zend\Diactoros\Uri;
 
+use function array_reduce;
 use function count;
+use function Functional\contains;
+use function Functional\invoke;
 
 class ShortUrl extends AbstractEntity
 {
@@ -161,4 +164,28 @@ class ShortUrl extends AbstractEntity
 
         return $this->domain->getAuthority();
     }
+
+    public function matchesCriteria(ShortUrlMeta $meta, array $tags): bool
+    {
+        if ($meta->hasMaxVisits() && $meta->getMaxVisits() !== $this->maxVisits) {
+            return false;
+        }
+        if ($meta->hasValidSince() && ! $meta->getValidSince()->eq($this->validSince)) {
+            return false;
+        }
+        if ($meta->hasValidUntil() && ! $meta->getValidUntil()->eq($this->validUntil)) {
+            return false;
+        }
+
+        $shortUrlTags = invoke($this->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;
+    }
 }

module/Core/src/Exception/NonUniqueSlugException.php

@@ -7,8 +7,13 @@ use function sprintf;
 
 class NonUniqueSlugException extends InvalidArgumentException
 {
-    public static function fromSlug(string $slug): self
+    public static function fromSlug(string $slug, ?string $domain): self
     {
-        return new self(sprintf('Provided slug "%s" is not unique.', $slug));
+        $suffix = '';
+        if ($domain !== null) {
+            $suffix = sprintf(' for domain "%s"', $domain);
+        }
+
+        return new self(sprintf('Provided slug "%s" is not unique%s.', $slug, $suffix));
     }
 }

module/Core/src/Repository/ShortUrlRepository.php

@@ -138,4 +138,25 @@ DQL;
         $result = $query->getOneOrNullResult();
         return $result === null || $result->maxVisitsReached() ? null : $result;
     }
+
+    public function slugIsInUse(string $slug, ?string $domain = null): bool
+    {
+        $qb = $this->getEntityManager()->createQueryBuilder();
+        $qb->select('COUNT(DISTINCT s.id)')
+           ->from(ShortUrl::class, 's')
+           ->where($qb->expr()->isNotNull('s.shortCode'))
+           ->andWhere($qb->expr()->eq('s.shortCode', ':slug'))
+           ->setParameter('slug', $slug);
+
+        if ($domain !== null) {
+            $qb->join('s.domain', 'd')
+               ->andWhere($qb->expr()->eq('d.authority', ':authority'))
+               ->setParameter('authority', $domain);
+        } else {
+            $qb->andWhere($qb->expr()->isNull('s.domain'));
+        }
+
+        $result = (int) $qb->getQuery()->getSingleScalarResult();
+        return $result > 0;
+    }
 }

module/Core/src/Repository/ShortUrlRepositoryInterface.php

@@ -27,4 +27,6 @@ interface ShortUrlRepositoryInterface extends ObjectRepository
     public function countList(?string $searchTerm = null, array $tags = []): int;
 
     public function findOneByShortCode(string $shortCode): ?ShortUrl;
+
+    public function slugIsInUse(string $slug, ?string $domain): bool;
 }

module/Core/src/Service/UrlShortener.php

@@ -23,11 +23,8 @@ use Shlinkio\Shlink\Core\Util\TagManagerTrait;
 use Throwable;
 
 use function array_reduce;
-use function count;
 use function floor;
 use function fmod;
-use function Functional\contains;
-use function Functional\invoke;
 use function preg_match;
 use function strlen;
 
@@ -121,30 +118,11 @@ class UrlShortener implements UrlShortenerInterface
 
         // 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) {
+            if ($found !== null) {
                 return $found;
             }
 
-            if ($meta->hasMaxVisits() && $meta->getMaxVisits() !== $shortUrl->getMaxVisits()) {
+            return $shortUrl->matchesCriteria($meta, $tags) ? $shortUrl : null;
-                return null;
-            }
-            if ($meta->hasValidSince() && ! $meta->getValidSince()->eq($shortUrl->getValidSince())) {
-                return null;
-            }
-            if ($meta->hasValidUntil() && ! $meta->getValidUntil()->eq($shortUrl->getValidUntil())) {
-                return 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;
         });
     }
 
@@ -166,12 +144,13 @@ class UrlShortener implements UrlShortenerInterface
         }
 
         $customSlug = $meta->getCustomSlug();
+        $domain = $meta->getDomain();
 
         /** @var ShortUrlRepository $repo */
         $repo = $this->em->getRepository(ShortUrl::class);
-        $shortUrlsCount = $repo->count(['shortCode' => $customSlug]);
+        $shortUrlsCount = $repo->slugIsInUse($customSlug, $domain);
         if ($shortUrlsCount > 0) {
-            throw NonUniqueSlugException::fromSlug($customSlug);
+            throw NonUniqueSlugException::fromSlug($customSlug, $domain);
         }
     }
 

module/Core/test-db/Repository/ShortUrlRepositoryTest.php

@@ -5,6 +5,7 @@ namespace ShlinkioTest\Shlink\Core\Repository;
 
 use Cake\Chronos\Chronos;
 use Doctrine\Common\Collections\ArrayCollection;
+use Shlinkio\Shlink\Core\Entity\Domain;
 use Shlinkio\Shlink\Core\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\Entity\Tag;
 use Shlinkio\Shlink\Core\Entity\Visit;
@@ -21,6 +22,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
         Tag::class,
         Visit::class,
         ShortUrl::class,
+        Domain::class,
     ];
 
     /** @var ShortUrlRepository */
@@ -32,7 +34,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
     }
 
     /** @test */
-    public function findOneByShortCodeReturnsProperData()
+    public function findOneByShortCodeReturnsProperData(): void
     {
         $foo = new ShortUrl('foo');
         $foo->setShortCode('foo');
@@ -62,7 +64,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
     }
 
     /** @test */
-    public function countListReturnsProperNumberOfResults()
+    public function countListReturnsProperNumberOfResults(): void
     {
         $count = 5;
         for ($i = 0; $i < $count; $i++) {
@@ -76,7 +78,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
     }
 
     /** @test */
-    public function findListProperlyFiltersByTagAndSearchTerm()
+    public function findListProperlyFiltersByTagAndSearchTerm(): void
     {
         $tag = new Tag('bar');
         $this->getEntityManager()->persist($tag);
@@ -121,7 +123,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
     }
 
     /** @test */
-    public function findListProperlyMapsFieldNamesToColumnNamesWhenOrdering()
+    public function findListProperlyMapsFieldNamesToColumnNamesWhenOrdering(): void
     {
         $urls = ['a', 'z', 'c', 'b'];
         foreach ($urls as $url) {
@@ -140,4 +142,26 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
         $this->assertEquals('c', $result[2]->getLongUrl());
         $this->assertEquals('z', $result[3]->getLongUrl());
     }
+
+    /** @test */
+    public function slugIsInUseLooksForShortUrlInProperSetOfTables(): void
+    {
+        $shortUrlWithoutDomain = (new ShortUrl('foo'))->setShortCode('my-cool-slug');
+        $this->getEntityManager()->persist($shortUrlWithoutDomain);
+
+        $shortUrlWithDomain = (new ShortUrl(
+            'foo',
+            ShortUrlMeta::createFromRawData(['domain' => 'doma.in'])
+        ))->setShortCode('another-slug');
+        $this->getEntityManager()->persist($shortUrlWithDomain);
+
+        $this->getEntityManager()->flush();
+
+        $this->assertTrue($this->repo->slugIsInUse('my-cool-slug'));
+        $this->assertFalse($this->repo->slugIsInUse('my-cool-slug', 'doma.in'));
+        $this->assertFalse($this->repo->slugIsInUse('slug-not-in-use'));
+        $this->assertFalse($this->repo->slugIsInUse('another-slug'));
+        $this->assertFalse($this->repo->slugIsInUse('another-slug', 'example.com'));
+        $this->assertTrue($this->repo->slugIsInUse('another-slug', 'doma.in'));
+    }
 }

module/Core/test/Exception/NonUniqueSlugExceptionTest.php

@@ -0,0 +1,34 @@
+<?php
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\Exception;
+
+use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException;
+
+class NonUniqueSlugExceptionTest extends TestCase
+{
+    /**
+     * @test
+     * @dataProvider provideMessages
+     */
+    public function properlyCreatesExceptionFromSlug(string $expectedMessage, string $slug, ?string $domain): void
+    {
+        $e = NonUniqueSlugException::fromSlug($slug, $domain);
+        $this->assertEquals($expectedMessage, $e->getMessage());
+    }
+
+    public function provideMessages(): iterable
+    {
+        yield 'without domain' => [
+            'Provided slug "foo" is not unique.',
+            'foo',
+            null,
+        ];
+        yield 'with domain' => [
+            'Provided slug "baz" is not unique for domain "bar".',
+            'baz',
+            'bar',
+        ];
+    }
+}

module/Core/test/Service/UrlShortenerTest.php

@@ -55,7 +55,7 @@ class UrlShortenerTest extends TestCase
             $shortUrl->setId('10');
         });
         $repo = $this->prophesize(ShortUrlRepository::class);
-        $repo->count(Argument::any())->willReturn(0);
+        $repo->slugIsInUse(Argument::cetera())->willReturn(false);
         $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal());
 
         $this->setUrlShortener(false);
@@ -122,11 +122,11 @@ class UrlShortenerTest extends TestCase
     public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void
     {
         $repo = $this->prophesize(ShortUrlRepository::class);
-        $countBySlug = $repo->count(['shortCode' => 'custom-slug'])->willReturn(1);
+        $slugIsInUse = $repo->slugIsInUse('custom-slug', null)->willReturn(true);
         $repo->findBy(Argument::cetera())->willReturn([]);
         $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal());
 
-        $countBySlug->shouldBeCalledOnce();
+        $slugIsInUse->shouldBeCalledOnce();
         $getRepo->shouldBeCalled();
         $this->expectException(NonUniqueSlugException::class);