From 608742c2e2f12d7317c2229de96a5ccf64a782cf Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 5 Feb 2021 17:59:34 +0100 Subject: [PATCH] Added helper service to avoid code duplication when resolving short URLs titles --- module/Core/config/dependencies.config.php | 6 ++- module/Core/src/Model/ShortUrlEdit.php | 13 ++++- module/Core/src/Model/ShortUrlMeta.php | 3 +- module/Core/src/Service/ShortUrlService.php | 22 +++------ module/Core/src/Service/UrlShortener.php | 22 +++------ .../Helper/ShortUrlTitleResolutionHelper.php | 28 +++++++++++ ...ShortUrlTitleResolutionHelperInterface.php | 15 ++++++ .../Helper/TitleResolutionModelInterface.php | 16 ++++++ .../Core/test/Service/ShortUrlServiceTest.php | 17 ++++--- module/Core/test/Service/UrlShortenerTest.php | 49 +++++-------------- .../ShortUrlTitleResolutionHelperTest.php | 49 +++++++++++++++++++ 11 files changed, 159 insertions(+), 81 deletions(-) create mode 100644 module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php create mode 100644 module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php create mode 100644 module/Core/src/ShortUrl/Helper/TitleResolutionModelInterface.php create mode 100644 module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 48166aeb..e742ad43 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -44,6 +44,7 @@ return [ ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ConfigAbstractFactory::class, ShortUrl\Helper\ShortUrlStringifier::class => ConfigAbstractFactory::class, + ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => ConfigAbstractFactory::class, ShortUrl\Transformer\ShortUrlDataTransformer::class => ConfigAbstractFactory::class, Mercure\MercureUpdatesGenerator::class => ConfigAbstractFactory::class, @@ -69,7 +70,7 @@ return [ Options\UrlShortenerOptions::class => ['config.url_shortener'], Service\UrlShortener::class => [ - Util\UrlValidator::class, + ShortUrl\Helper\ShortUrlTitleResolutionHelper::class, 'em', ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class, Service\ShortUrl\ShortCodeHelper::class, @@ -82,7 +83,7 @@ return [ Service\ShortUrlService::class => [ 'em', Service\ShortUrl\ShortUrlResolver::class, - Util\UrlValidator::class, + ShortUrl\Helper\ShortUrlTitleResolutionHelper::class, ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class, ], Visit\VisitLocator::class => ['em'], @@ -122,6 +123,7 @@ return [ ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ['em'], ShortUrl\Helper\ShortUrlStringifier::class => ['config.url_shortener.domain', 'config.router.base_path'], + ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => [Util\UrlValidator::class], ShortUrl\Transformer\ShortUrlDataTransformer::class => [ShortUrl\Helper\ShortUrlStringifier::class], Mercure\MercureUpdatesGenerator::class => [ShortUrl\Transformer\ShortUrlDataTransformer::class], diff --git a/module/Core/src/Model/ShortUrlEdit.php b/module/Core/src/Model/ShortUrlEdit.php index c2f97eed..3327aad4 100644 --- a/module/Core/src/Model/ShortUrlEdit.php +++ b/module/Core/src/Model/ShortUrlEdit.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Model; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Core\Exception\ValidationException; +use Shlinkio\Shlink\Core\ShortUrl\Helper\TitleResolutionModelInterface; use Shlinkio\Shlink\Core\Validation\ShortUrlInputFilter; use function array_key_exists; @@ -13,7 +14,7 @@ use function Shlinkio\Shlink\Core\getOptionalBoolFromInputFilter; use function Shlinkio\Shlink\Core\getOptionalIntFromInputFilter; use function Shlinkio\Shlink\Core\parseDateField; -final class ShortUrlEdit +final class ShortUrlEdit implements TitleResolutionModelInterface { private bool $longUrlPropWasProvided = false; private ?string $longUrl = null; @@ -75,6 +76,11 @@ final class ShortUrlEdit return $this->longUrl; } + public function getLongUrl(): string + { + return $this->longUrl() ?? ''; + } + public function longUrlWasProvided(): bool { return $this->longUrlPropWasProvided && $this->longUrl !== null; @@ -133,6 +139,11 @@ final class ShortUrlEdit return $this->titlePropWasProvided; } + public function hasTitle(): bool + { + return $this->titleWasProvided(); + } + public function titleWasAutoResolved(): bool { return $this->titleWasAutoResolved; diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index a741f78c..df25735c 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Model; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Core\Exception\ValidationException; +use Shlinkio\Shlink\Core\ShortUrl\Helper\TitleResolutionModelInterface; use Shlinkio\Shlink\Core\Validation\ShortUrlInputFilter; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -15,7 +16,7 @@ use function Shlinkio\Shlink\Core\parseDateField; use const Shlinkio\Shlink\Core\DEFAULT_SHORT_CODES_LENGTH; -final class ShortUrlMeta +final class ShortUrlMeta implements TitleResolutionModelInterface { private string $longUrl; private ?Chronos $validSince = null; diff --git a/module/Core/src/Service/ShortUrlService.php b/module/Core/src/Service/ShortUrlService.php index e412b63b..dcb1d8cc 100644 --- a/module/Core/src/Service/ShortUrlService.php +++ b/module/Core/src/Service/ShortUrlService.php @@ -15,26 +15,26 @@ use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Paginator\Adapter\ShortUrlRepositoryAdapter; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; +use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlTitleResolutionHelperInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\ShortUrlRelationResolverInterface; -use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; class ShortUrlService implements ShortUrlServiceInterface { private ORM\EntityManagerInterface $em; private ShortUrlResolverInterface $urlResolver; - private UrlValidatorInterface $urlValidator; + private ShortUrlTitleResolutionHelperInterface $titleResolutionHelper; private ShortUrlRelationResolverInterface $relationResolver; public function __construct( ORM\EntityManagerInterface $em, ShortUrlResolverInterface $urlResolver, - UrlValidatorInterface $urlValidator, + ShortUrlTitleResolutionHelperInterface $titleResolutionHelper, ShortUrlRelationResolverInterface $relationResolver ) { $this->em = $em; $this->urlResolver = $urlResolver; - $this->urlValidator = $urlValidator; + $this->titleResolutionHelper = $titleResolutionHelper; $this->relationResolver = $relationResolver; } @@ -62,7 +62,8 @@ class ShortUrlService implements ShortUrlServiceInterface ?ApiKey $apiKey = null ): ShortUrl { if ($shortUrlEdit->longUrlWasProvided()) { - $shortUrlEdit = $this->processTitleAndValidateUrl($shortUrlEdit); + /** @var ShortUrlEdit $shortUrlEdit */ + $shortUrlEdit = $this->titleResolutionHelper->processTitleAndValidateUrl($shortUrlEdit); } $shortUrl = $this->urlResolver->resolveShortUrl($identifier, $apiKey); @@ -72,15 +73,4 @@ class ShortUrlService implements ShortUrlServiceInterface return $shortUrl; } - - private function processTitleAndValidateUrl(ShortUrlEdit $shortUrlEdit): ShortUrlEdit - { - if ($shortUrlEdit->titleWasProvided()) { - $this->urlValidator->validateUrl($shortUrlEdit->longUrl(), $shortUrlEdit->doValidateUrl()); - return $shortUrlEdit; - } - - $title = $this->urlValidator->validateUrlWithTitle($shortUrlEdit->longUrl(), $shortUrlEdit->doValidateUrl()); - return $title === null ? $shortUrlEdit : $shortUrlEdit->withResolvedTitle($title); - } } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index 6083e1f4..78064259 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -11,23 +11,23 @@ use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; +use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlTitleResolutionHelperInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\ShortUrlRelationResolverInterface; -use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; class UrlShortener implements UrlShortenerInterface { private EntityManagerInterface $em; - private UrlValidatorInterface $urlValidator; + private ShortUrlTitleResolutionHelperInterface $titleResolutionHelper; private ShortUrlRelationResolverInterface $relationResolver; private ShortCodeHelperInterface $shortCodeHelper; public function __construct( - UrlValidatorInterface $urlValidator, + ShortUrlTitleResolutionHelperInterface $titleResolutionHelper, EntityManagerInterface $em, ShortUrlRelationResolverInterface $relationResolver, ShortCodeHelperInterface $shortCodeHelper ) { - $this->urlValidator = $urlValidator; + $this->titleResolutionHelper = $titleResolutionHelper; $this->em = $em; $this->relationResolver = $relationResolver; $this->shortCodeHelper = $shortCodeHelper; @@ -45,7 +45,8 @@ class UrlShortener implements UrlShortenerInterface return $existingShortUrl; } - $meta = $this->processTitleAndValidateUrl($meta); + /** @var ShortUrlMeta $meta */ + $meta = $this->titleResolutionHelper->processTitleAndValidateUrl($meta); return $this->em->transactional(function () use ($meta) { $shortUrl = ShortUrl::fromMeta($meta, $this->relationResolver); @@ -82,15 +83,4 @@ class UrlShortener implements UrlShortenerInterface throw NonUniqueSlugException::fromSlug($shortUrlToBeCreated->getShortCode(), $domainAuthority); } } - - private function processTitleAndValidateUrl(ShortUrlMeta $meta): ShortUrlMeta - { - if ($meta->hasTitle()) { - $this->urlValidator->validateUrl($meta->getLongUrl(), $meta->doValidateUrl()); - return $meta; - } - - $title = $this->urlValidator->validateUrlWithTitle($meta->getLongUrl(), $meta->doValidateUrl()); - return $title === null ? $meta : $meta->withResolvedTitle($title); - } } diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php new file mode 100644 index 00000000..4615e45f --- /dev/null +++ b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php @@ -0,0 +1,28 @@ +urlValidator = $urlValidator; + } + + public function processTitleAndValidateUrl(TitleResolutionModelInterface $data): TitleResolutionModelInterface + { + if ($data->hasTitle()) { + $this->urlValidator->validateUrl($data->getLongUrl(), $data->doValidateUrl()); + return $data; + } + + $title = $this->urlValidator->validateUrlWithTitle($data->getLongUrl(), $data->doValidateUrl()); + return $title === null ? $data : $data->withResolvedTitle($title); + } +} diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php new file mode 100644 index 00000000..50022746 --- /dev/null +++ b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php @@ -0,0 +1,15 @@ +em->flush()->willReturn(null); $this->urlResolver = $this->prophesize(ShortUrlResolverInterface::class); - $this->urlValidator = $this->prophesize(UrlValidatorInterface::class); + $this->titleResolutionHelper = $this->prophesize(ShortUrlTitleResolutionHelperInterface::class); $this->service = new ShortUrlService( $this->em->reveal(), $this->urlResolver->reveal(), - $this->urlValidator->reveal(), + $this->titleResolutionHelper->reveal(), new SimpleShortUrlRelationResolver(), ); } @@ -93,6 +93,10 @@ class ShortUrlServiceTest extends TestCase )->willReturn($shortUrl); $flush = $this->em->flush()->willReturn(null); + $processTitle = $this->titleResolutionHelper->processTitleAndValidateUrl($shortUrlEdit)->willReturn( + $shortUrlEdit, + ); + $result = $this->service->updateShortUrl(new ShortUrlIdentifier('abc123'), $shortUrlEdit, $apiKey); self::assertSame($shortUrl, $result); @@ -102,10 +106,7 @@ class ShortUrlServiceTest extends TestCase self::assertEquals($shortUrlEdit->longUrl() ?? $originalLongUrl, $shortUrl->getLongUrl()); $findShortUrl->shouldHaveBeenCalled(); $flush->shouldHaveBeenCalled(); - $this->urlValidator->validateUrlWithTitle( - $shortUrlEdit->longUrl(), - $shortUrlEdit->doValidateUrl(), - )->shouldHaveBeenCalledTimes($expectedValidateCalls); + $processTitle->shouldHaveBeenCalledTimes($expectedValidateCalls); } public function provideShortUrlEdits(): iterable diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 9fbf82ed..7e319314 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -16,8 +16,8 @@ use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; use Shlinkio\Shlink\Core\Service\UrlShortener; +use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlTitleResolutionHelperInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\SimpleShortUrlRelationResolver; -use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; class UrlShortenerTest extends TestCase { @@ -25,12 +25,13 @@ class UrlShortenerTest extends TestCase private UrlShortener $urlShortener; private ObjectProphecy $em; - private ObjectProphecy $urlValidator; + private ObjectProphecy $titleResolutionHelper; private ObjectProphecy $shortCodeHelper; public function setUp(): void { - $this->urlValidator = $this->prophesize(UrlValidatorInterface::class); + $this->titleResolutionHelper = $this->prophesize(ShortUrlTitleResolutionHelperInterface::class); + $this->titleResolutionHelper->processTitleAndValidateUrl(Argument::cetera())->willReturnArgument(); $this->em = $this->prophesize(EntityManagerInterface::class); $this->em->persist(Argument::any())->will(function ($arguments): void { @@ -52,47 +53,22 @@ class UrlShortenerTest extends TestCase $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(true); $this->urlShortener = new UrlShortener( - $this->urlValidator->reveal(), + $this->titleResolutionHelper->reveal(), $this->em->reveal(), new SimpleShortUrlRelationResolver(), $this->shortCodeHelper->reveal(), ); } - /** - * @test - * @dataProvider provideTitles - */ - public function urlIsProperlyShortened(?string $title, int $validateWithTitleCallsNum, int $validateCallsNum): void + /** @test */ + public function urlIsProperlyShortened(): void { $longUrl = 'http://foobar.com/12345/hello?foo=bar'; - $shortUrl = $this->urlShortener->shorten(ShortUrlMeta::fromRawData(['longUrl' => $longUrl, 'title' => $title])); + $meta = ShortUrlMeta::fromRawData(['longUrl' => $longUrl]); + $shortUrl = $this->urlShortener->shorten($meta); - self::assertEquals('http://foobar.com/12345/hello?foo=bar', $shortUrl->getLongUrl()); - $this->urlValidator->validateUrlWithTitle($longUrl, null)->shouldHaveBeenCalledTimes( - $validateWithTitleCallsNum, - ); - $this->urlValidator->validateUrl($longUrl, null)->shouldHaveBeenCalledTimes($validateCallsNum); - } - - /** - * @test - * @dataProvider provideTitles - */ - public function urlIsProperlyShortenedWithExpectedResolvedTitle(?string $title): void - { - $validateWithTitle = $this->urlValidator->validateUrlWithTitle(Argument::cetera())->willReturn($title); - - $shortUrl = $this->urlShortener->shorten(ShortUrlMeta::fromRawData(['longUrl' => 'foo'])); - - self::assertEquals($title, $shortUrl->getTitle()); - $validateWithTitle->shouldHaveBeenCalledOnce(); - } - - public function provideTitles(): iterable - { - yield 'no title' => [null, 1, 0]; - yield 'title' => ['link title', 0, 1]; + self::assertEquals($longUrl, $shortUrl->getLongUrl()); + $this->titleResolutionHelper->processTitleAndValidateUrl($meta)->shouldHaveBeenCalledOnce(); } /** @test */ @@ -123,8 +99,7 @@ class UrlShortenerTest extends TestCase $findExisting->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); - $this->urlValidator->validateUrl(Argument::cetera())->shouldNotHaveBeenCalled(); - $this->urlValidator->validateUrlWithTitle(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->titleResolutionHelper->processTitleAndValidateUrl(Argument::cetera())->shouldNotHaveBeenCalled(); self::assertSame($expected, $result); } diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php new file mode 100644 index 00000000..6783303c --- /dev/null +++ b/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php @@ -0,0 +1,49 @@ +urlValidator = $this->prophesize(UrlValidatorInterface::class); + $this->helper = new ShortUrlTitleResolutionHelper($this->urlValidator->reveal()); + } + + /** + * @test + * @dataProvider provideTitles + */ + public function urlIsProperlyShortened(?string $title, int $validateWithTitleCallsNum, int $validateCallsNum): void + { + $longUrl = 'http://foobar.com/12345/hello?foo=bar'; + $this->helper->processTitleAndValidateUrl( + ShortUrlMeta::fromRawData(['longUrl' => $longUrl, 'title' => $title]), + ); + + $this->urlValidator->validateUrlWithTitle($longUrl, null)->shouldHaveBeenCalledTimes( + $validateWithTitleCallsNum, + ); + $this->urlValidator->validateUrl($longUrl, null)->shouldHaveBeenCalledTimes($validateCallsNum); + } + + public function provideTitles(): iterable + { + yield 'no title' => [null, 1, 0]; + yield 'title' => ['link title', 0, 1]; + } +}