From 82091c7951ab1762a38bc2ccf0aec4c9ed5a9327 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 31 Jan 2021 10:53:18 +0100 Subject: [PATCH] Added logic to resolve tags during short URL creation through ShortUrlRelationResolver --- module/Core/src/Entity/ShortUrl.php | 4 +++- .../src/Importer/ImportedLinksProcessor.php | 5 ----- module/Core/src/Model/ShortUrlEdit.php | 17 +++++++++++++++++ module/Core/src/Service/ShortUrlService.php | 3 ++- .../src/Service/ShortUrlServiceInterface.php | 3 ++- module/Core/src/Service/UrlShortener.php | 4 ---- .../PersistenceShortUrlRelationResolver.php | 19 +++++++++++++++++++ .../ShortUrlRelationResolverInterface.php | 8 ++++++++ .../SimpleShortUrlRelationResolver.php | 14 ++++++++++++++ module/Core/src/Util/TagManagerTrait.php | 2 ++ .../Repository/DomainRepositoryTest.php | 7 +++++++ .../Core/test/Service/ShortUrlServiceTest.php | 4 ++-- .../Action/ShortUrl/EditShortUrlAction.php | 2 +- .../test-api/Action/CreateShortUrlTest.php | 18 ++++++++++++++---- .../ShortUrl/EditShortUrlActionTest.php | 2 +- 15 files changed, 92 insertions(+), 20 deletions(-) diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index 2c86f9ec..3f55c3f4 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -64,7 +64,7 @@ class ShortUrl extends AbstractEntity $instance->longUrl = $meta->getLongUrl(); $instance->dateCreated = Chronos::now(); $instance->visits = new ArrayCollection(); - $instance->tags = new ArrayCollection(); + $instance->tags = $relationResolver->resolveTags($meta->getTags()); $instance->validSince = $meta->getValidSince(); $instance->validUntil = $meta->getValidUntil(); $instance->maxVisits = $meta->getMaxVisits(); @@ -85,6 +85,7 @@ class ShortUrl extends AbstractEntity $meta = [ ShortUrlInputFilter::LONG_URL => $url->longUrl(), ShortUrlInputFilter::DOMAIN => $url->domain(), + ShortUrlInputFilter::TAGS => $url->tags(), ShortUrlInputFilter::VALIDATE_URL => false, ]; if ($importShortCode) { @@ -129,6 +130,7 @@ class ShortUrl extends AbstractEntity /** * @param Collection|Tag[] $tags + * @deprecated */ public function setTags(Collection $tags): self { diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index 8bac7395..2b5cde17 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -10,7 +10,6 @@ use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\ShortUrlRelationResolverInterface; use Shlinkio\Shlink\Core\Util\DoctrineBatchHelperInterface; -use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use Symfony\Component\Console\Style\StyleInterface; @@ -19,8 +18,6 @@ use function sprintf; class ImportedLinksProcessor implements ImportedLinksProcessorInterface { - use TagManagerTrait; - private EntityManagerInterface $em; private ShortUrlRelationResolverInterface $relationResolver; private ShortCodeHelperInterface $shortCodeHelper; @@ -59,8 +56,6 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface } $shortUrl = ShortUrl::fromImport($url, $importShortCodes, $this->relationResolver); - $shortUrl->setTags($this->tagNamesToEntities($this->em, $url->tags())); - if (! $this->handleShortCodeUniqueness($url, $shortUrl, $io, $importShortCodes)) { continue; } diff --git a/module/Core/src/Model/ShortUrlEdit.php b/module/Core/src/Model/ShortUrlEdit.php index 824a5f60..b8cb0e0c 100644 --- a/module/Core/src/Model/ShortUrlEdit.php +++ b/module/Core/src/Model/ShortUrlEdit.php @@ -23,6 +23,8 @@ final class ShortUrlEdit private ?Chronos $validUntil = null; private bool $maxVisitsPropWasProvided = false; private ?int $maxVisits = null; + private bool $tagsPropWasProvided = false; + private array $tags = []; private ?bool $validateUrl = null; private function __construct() @@ -53,12 +55,14 @@ final class ShortUrlEdit $this->validSincePropWasProvided = array_key_exists(ShortUrlInputFilter::VALID_SINCE, $data); $this->validUntilPropWasProvided = array_key_exists(ShortUrlInputFilter::VALID_UNTIL, $data); $this->maxVisitsPropWasProvided = array_key_exists(ShortUrlInputFilter::MAX_VISITS, $data); + $this->tagsPropWasProvided = array_key_exists(ShortUrlInputFilter::TAGS, $data); $this->longUrl = $inputFilter->getValue(ShortUrlInputFilter::LONG_URL); $this->validSince = parseDateField($inputFilter->getValue(ShortUrlInputFilter::VALID_SINCE)); $this->validUntil = parseDateField($inputFilter->getValue(ShortUrlInputFilter::VALID_UNTIL)); $this->maxVisits = getOptionalIntFromInputFilter($inputFilter, ShortUrlInputFilter::MAX_VISITS); $this->validateUrl = getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::VALIDATE_URL); + $this->tags = $inputFilter->getValue(ShortUrlInputFilter::TAGS); } public function longUrl(): ?string @@ -101,6 +105,19 @@ final class ShortUrlEdit return $this->maxVisitsPropWasProvided; } + /** + * @return string[] + */ + public function tags(): array + { + return $this->tags; + } + + public function hasTags(): bool + { + return $this->tagsPropWasProvided; + } + public function doValidateUrl(): ?bool { return $this->validateUrl; diff --git a/module/Core/src/Service/ShortUrlService.php b/module/Core/src/Service/ShortUrlService.php index aeb5233b..8b5a6362 100644 --- a/module/Core/src/Service/ShortUrlService.php +++ b/module/Core/src/Service/ShortUrlService.php @@ -53,6 +53,7 @@ class ShortUrlService implements ShortUrlServiceInterface /** * @param string[] $tags + * @deprecated Use updateShortUrl instead * @throws ShortUrlNotFoundException */ public function setTagsByShortCode(ShortUrlIdentifier $identifier, array $tags, ?ApiKey $apiKey = null): ShortUrl @@ -69,7 +70,7 @@ class ShortUrlService implements ShortUrlServiceInterface * @throws ShortUrlNotFoundException * @throws InvalidUrlException */ - public function updateMetadataByShortCode( + public function updateShortUrl( ShortUrlIdentifier $identifier, ShortUrlEdit $shortUrlEdit, ?ApiKey $apiKey = null diff --git a/module/Core/src/Service/ShortUrlServiceInterface.php b/module/Core/src/Service/ShortUrlServiceInterface.php index b1cfbc2d..ac9f2095 100644 --- a/module/Core/src/Service/ShortUrlServiceInterface.php +++ b/module/Core/src/Service/ShortUrlServiceInterface.php @@ -22,6 +22,7 @@ interface ShortUrlServiceInterface /** * @param string[] $tags + * @deprecated Use updateShortUrl instead * @throws ShortUrlNotFoundException */ public function setTagsByShortCode(ShortUrlIdentifier $identifier, array $tags, ?ApiKey $apiKey = null): ShortUrl; @@ -30,7 +31,7 @@ interface ShortUrlServiceInterface * @throws ShortUrlNotFoundException * @throws InvalidUrlException */ - public function updateMetadataByShortCode( + public function updateShortUrl( ShortUrlIdentifier $identifier, ShortUrlEdit $shortUrlEdit, ?ApiKey $apiKey = null diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index 08e64e60..f8125524 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -12,14 +12,11 @@ use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\ShortUrlRelationResolverInterface; -use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; use Throwable; class UrlShortener implements UrlShortenerInterface { - use TagManagerTrait; - private EntityManagerInterface $em; private UrlValidatorInterface $urlValidator; private ShortUrlRelationResolverInterface $relationResolver; @@ -54,7 +51,6 @@ class UrlShortener implements UrlShortenerInterface return $this->em->transactional(function () use ($meta) { $shortUrl = ShortUrl::fromMeta($meta, $this->relationResolver); - $shortUrl->setTags($this->tagNamesToEntities($this->em, $meta->getTags())); $this->verifyShortCodeUniqueness($meta, $shortUrl); $this->em->persist($shortUrl); diff --git a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php index 0e3afa23..940a5a9f 100644 --- a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php @@ -4,8 +4,13 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl\Resolver; +use Doctrine\Common\Collections; +use Doctrine\Common\Collections\Collection; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Entity\Domain; +use Shlinkio\Shlink\Core\Entity\Tag; + +use function Functional\map; class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInterface { @@ -26,4 +31,18 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt $existingDomain = $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]); return $existingDomain ?? new Domain($domain); } + + /** + * @param string[] $tags + * @return Collection|Tag[] + */ + public function resolveTags(array $tags): Collections\Collection + { + return new Collections\ArrayCollection(map($tags, function (string $tagName): Tag { + $tag = $this->em->getRepository(Tag::class)->findOneBy(['name' => $tagName]) ?? new Tag($tagName); + $this->em->persist($tag); + + return $tag; + })); + } } diff --git a/module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php b/module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php index bc576dbd..2d46a17b 100644 --- a/module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php +++ b/module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php @@ -4,9 +4,17 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl\Resolver; +use Doctrine\Common\Collections\Collection; use Shlinkio\Shlink\Core\Entity\Domain; +use Shlinkio\Shlink\Core\Entity\Tag; interface ShortUrlRelationResolverInterface { public function resolveDomain(?string $domain): ?Domain; + + /** + * @param string[] $tags + * @return Collection|Tag[] + */ + public function resolveTags(array $tags): Collection; } diff --git a/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php index 4e4620f5..2cda44df 100644 --- a/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php @@ -4,7 +4,12 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl\Resolver; +use Doctrine\Common\Collections; +use Doctrine\Common\Collections\Collection; use Shlinkio\Shlink\Core\Entity\Domain; +use Shlinkio\Shlink\Core\Entity\Tag; + +use function Functional\map; class SimpleShortUrlRelationResolver implements ShortUrlRelationResolverInterface { @@ -12,4 +17,13 @@ class SimpleShortUrlRelationResolver implements ShortUrlRelationResolverInterfac { return $domain !== null ? new Domain($domain) : null; } + + /** + * @param string[] $tags + * @return Collection|Tag[] + */ + public function resolveTags(array $tags): Collections\Collection + { + return new Collections\ArrayCollection(map($tags, fn (string $tag) => new Tag($tag))); + } } diff --git a/module/Core/src/Util/TagManagerTrait.php b/module/Core/src/Util/TagManagerTrait.php index 27fb22b5..c6258f91 100644 --- a/module/Core/src/Util/TagManagerTrait.php +++ b/module/Core/src/Util/TagManagerTrait.php @@ -13,10 +13,12 @@ use function str_replace; use function strtolower; use function trim; +/** @deprecated */ trait TagManagerTrait { /** * @param string[] $tags + * @deprecated * @return Collections\Collection|Tag[] */ private function tagNamesToEntities(EntityManagerInterface $em, array $tags): Collections\Collection diff --git a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php index 3231eec4..49265eb0 100644 --- a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php +++ b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php @@ -4,6 +4,8 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Domain\Repository; +use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\Common\Collections\Collection; use Shlinkio\Shlink\Core\Domain\Repository\DomainRepository; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; @@ -102,6 +104,11 @@ class DomainRepositoryTest extends DatabaseTestCase { return $this->domain; } + + public function resolveTags(array $tags): Collection + { + return new ArrayCollection(); + } }, ); } diff --git a/module/Core/test/Service/ShortUrlServiceTest.php b/module/Core/test/Service/ShortUrlServiceTest.php index d056b91b..d83d5e4f 100644 --- a/module/Core/test/Service/ShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrlServiceTest.php @@ -100,7 +100,7 @@ class ShortUrlServiceTest extends TestCase * @test * @dataProvider provideShortUrlEdits */ - public function updateMetadataByShortCodeUpdatesProvidedData( + public function updateShortUrlUpdatesProvidedData( int $expectedValidateCalls, ShortUrlEdit $shortUrlEdit, ?ApiKey $apiKey @@ -114,7 +114,7 @@ class ShortUrlServiceTest extends TestCase )->willReturn($shortUrl); $flush = $this->em->flush()->willReturn(null); - $result = $this->service->updateMetadataByShortCode(new ShortUrlIdentifier('abc123'), $shortUrlEdit, $apiKey); + $result = $this->service->updateShortUrl(new ShortUrlIdentifier('abc123'), $shortUrlEdit, $apiKey); self::assertSame($shortUrl, $result); self::assertEquals($shortUrlEdit->validSince(), $shortUrl->getValidSince()); diff --git a/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php b/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php index 32d95b2d..d7d283c7 100644 --- a/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php @@ -31,7 +31,7 @@ class EditShortUrlAction extends AbstractRestAction $identifier = ShortUrlIdentifier::fromApiRequest($request); $apiKey = AuthenticationMiddleware::apiKeyFromRequest($request); - $this->shortUrlService->updateMetadataByShortCode($identifier, $shortUrlEdit, $apiKey); + $this->shortUrlService->updateShortUrl($identifier, $shortUrlEdit, $apiKey); return new EmptyResponse(); } } diff --git a/module/Rest/test-api/Action/CreateShortUrlTest.php b/module/Rest/test-api/Action/CreateShortUrlTest.php index 54ea0218..868ad142 100644 --- a/module/Rest/test-api/Action/CreateShortUrlTest.php +++ b/module/Rest/test-api/Action/CreateShortUrlTest.php @@ -60,13 +60,23 @@ class CreateShortUrlTest extends ApiTestCase } } - /** @test */ - public function createsNewShortUrlWithTags(): void + /** + * @test + * @dataProvider provideTags + */ + public function createsNewShortUrlWithTags(array $providedTags, array $expectedTags): void { - [$statusCode, ['tags' => $tags]] = $this->createShortUrl(['tags' => ['foo', 'bar', 'baz']]); + [$statusCode, ['tags' => $tags]] = $this->createShortUrl(['tags' => $providedTags]); self::assertEquals(self::STATUS_OK, $statusCode); - self::assertEquals(['foo', 'bar', 'baz'], $tags); + self::assertEquals($expectedTags, $tags); + } + + public function provideTags(): iterable + { + yield 'simple tags' => [$simpleTags = ['foo', 'bar', 'baz'], $simpleTags]; + yield 'tags with spaces' => [['fo o', ' bar', 'b az'], ['fo-o', 'bar', 'b-az']]; + yield 'tags with special chars' => [['UUU', 'Aäa'], ['uuu', 'aäa']]; } /** diff --git a/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php index ad482098..7a7d19ee 100644 --- a/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php @@ -48,7 +48,7 @@ class EditShortUrlActionTest extends TestCase ->withParsedBody([ 'maxVisits' => 5, ]); - $updateMeta = $this->shortUrlService->updateMetadataByShortCode(Argument::cetera())->willReturn( + $updateMeta = $this->shortUrlService->updateShortUrl(Argument::cetera())->willReturn( ShortUrl::createEmpty(), );