From 97f89bcede81fac7915c2f406c24f40b88f301b5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Nov 2020 20:05:57 +0100 Subject: [PATCH] Simplified transactional URL shortening --- config/autoload/redirects.global.php | 2 +- .../ShortUrl/GenerateShortUrlCommand.php | 2 +- .../ShortUrl/GenerateShortUrlCommandTest.php | 10 +++--- .../Shlinkio.Shlink.Core.Entity.ShortUrl.php | 5 +++ module/Core/src/Entity/ShortUrl.php | 2 ++ module/Core/src/Service/UrlShortener.php | 23 ++++-------- .../src/Service/UrlShortenerInterface.php | 2 +- module/Core/test/Service/UrlShortenerTest.php | 36 +++++-------------- .../ShortUrl/AbstractCreateShortUrlAction.php | 2 +- .../ShortUrl/CreateShortUrlActionTest.php | 4 +-- .../SingleStepCreateShortUrlActionTest.php | 2 +- 11 files changed, 35 insertions(+), 55 deletions(-) diff --git a/config/autoload/redirects.global.php b/config/autoload/redirects.global.php index 90137aa8..173c435c 100644 --- a/config/autoload/redirects.global.php +++ b/config/autoload/redirects.global.php @@ -5,7 +5,7 @@ declare(strict_types=1); return [ 'not_found_redirects' => [ - 'invalid_short_url' => null, // Formerly url_shortener.not_found_short_url.redirect_to + 'invalid_short_url' => null, 'regular_404' => null, 'base_url' => null, ], diff --git a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php index 36293377..12bbb3fb 100644 --- a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php @@ -142,7 +142,7 @@ class GenerateShortUrlCommand extends Command $doValidateUrl = $this->doValidateUrl($input); try { - $shortUrl = $this->urlShortener->urlToShortCode($longUrl, $tags, ShortUrlMeta::fromRawData([ + $shortUrl = $this->urlShortener->shorten($longUrl, $tags, ShortUrlMeta::fromRawData([ ShortUrlMetaInputFilter::VALID_SINCE => $input->getOption('validSince'), ShortUrlMetaInputFilter::VALID_UNTIL => $input->getOption('validUntil'), ShortUrlMetaInputFilter::CUSTOM_SLUG => $customSlug, diff --git a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php index b11b455b..82f38713 100644 --- a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php @@ -44,7 +44,7 @@ class GenerateShortUrlCommandTest extends TestCase public function properShortCodeIsCreatedIfLongUrlIsCorrect(): void { $shortUrl = new ShortUrl(''); - $urlToShortCode = $this->urlShortener->urlToShortCode(Argument::cetera())->willReturn($shortUrl); + $urlToShortCode = $this->urlShortener->shorten(Argument::cetera())->willReturn($shortUrl); $this->commandTester->execute([ 'longUrl' => 'http://domain.com/foo/bar', @@ -61,7 +61,7 @@ class GenerateShortUrlCommandTest extends TestCase public function exceptionWhileParsingLongUrlOutputsError(): void { $url = 'http://domain.com/invalid'; - $this->urlShortener->urlToShortCode(Argument::cetera())->willThrow(InvalidUrlException::fromUrl($url)) + $this->urlShortener->shorten(Argument::cetera())->willThrow(InvalidUrlException::fromUrl($url)) ->shouldBeCalledOnce(); $this->commandTester->execute(['longUrl' => $url]); @@ -74,7 +74,7 @@ class GenerateShortUrlCommandTest extends TestCase /** @test */ public function providingNonUniqueSlugOutputsError(): void { - $urlToShortCode = $this->urlShortener->urlToShortCode(Argument::cetera())->willThrow( + $urlToShortCode = $this->urlShortener->shorten(Argument::cetera())->willThrow( NonUniqueSlugException::fromSlug('my-slug'), ); @@ -90,7 +90,7 @@ class GenerateShortUrlCommandTest extends TestCase public function properlyProcessesProvidedTags(): void { $shortUrl = new ShortUrl(''); - $urlToShortCode = $this->urlShortener->urlToShortCode( + $urlToShortCode = $this->urlShortener->shorten( Argument::type('string'), Argument::that(function (array $tags) { Assert::assertEquals(['foo', 'bar', 'baz', 'boo', 'zar'], $tags); @@ -117,7 +117,7 @@ class GenerateShortUrlCommandTest extends TestCase public function urlValidationHasExpectedValueBasedOnProvidedTags(array $options, ?bool $expectedValidateUrl): void { $shortUrl = new ShortUrl(''); - $urlToShortCode = $this->urlShortener->urlToShortCode( + $urlToShortCode = $this->urlShortener->shorten( Argument::type('string'), Argument::type('array'), Argument::that(function (ShortUrlMeta $meta) use ($expectedValidateUrl) { diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php index 4f9b3747..da4506af 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php @@ -8,6 +8,7 @@ use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; use Doctrine\ORM\Mapping\ClassMetadata; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; +use Shlinkio\Shlink\Rest\Entity\ApiKey; return static function (ClassMetadata $metadata, array $emConfig): void { $builder = new ClassMetadataBuilder($metadata); @@ -78,5 +79,9 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->cascadePersist() ->build(); + $builder->createManyToOne('authorApiKey', ApiKey::class) + ->addJoinColumn('author_api_key_id', 'id', true, false, 'SET NULL') + ->build(); + $builder->addUniqueConstraint(['short_code', 'domain_id'], 'unique_short_code_plus_domain'); }; diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index 2d5cb6de..3d7ea0dd 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -16,6 +16,7 @@ use Shlinkio\Shlink\Core\Model\ShortUrlEdit; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; +use Shlinkio\Shlink\Rest\Entity\ApiKey; use function count; use function Shlinkio\Shlink\Core\generateRandomShortCode; @@ -37,6 +38,7 @@ class ShortUrl extends AbstractEntity private int $shortCodeLength; private ?string $importSource = null; private ?string $importOriginalShortCode = null; + private ?ApiKey $authorApiKey = null; public function __construct( string $longUrl, diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index d399b534..fb745114 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -43,7 +43,7 @@ class UrlShortener implements UrlShortenerInterface * @throws InvalidUrlException * @throws Throwable */ - public function urlToShortCode(string $url, array $tags, ShortUrlMeta $meta): ShortUrl + public function shorten(string $url, array $tags, ShortUrlMeta $meta): ShortUrl { // First, check if a short URL exists for all provided params $existingShortUrl = $this->findExistingShortUrlIfExists($url, $tags, $meta); @@ -52,25 +52,16 @@ class UrlShortener implements UrlShortenerInterface } $this->urlValidator->validateUrl($url, $meta->doValidateUrl()); - $this->em->beginTransaction(); - $shortUrl = new ShortUrl($url, $meta, $this->domainResolver); - $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); - try { + return $this->em->transactional(function () use ($url, $tags, $meta) { + $shortUrl = new ShortUrl($url, $meta, $this->domainResolver); + $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); + $this->verifyShortCodeUniqueness($meta, $shortUrl); $this->em->persist($shortUrl); - $this->em->flush(); - $this->em->commit(); - } catch (Throwable $e) { - if ($this->em->getConnection()->isTransactionActive()) { - $this->em->rollback(); - $this->em->close(); - } - throw $e; - } - - return $shortUrl; + return $shortUrl; + }); } private function findExistingShortUrlIfExists(string $url, array $tags, ShortUrlMeta $meta): ?ShortUrl diff --git a/module/Core/src/Service/UrlShortenerInterface.php b/module/Core/src/Service/UrlShortenerInterface.php index e26530ca..45b1eb8a 100644 --- a/module/Core/src/Service/UrlShortenerInterface.php +++ b/module/Core/src/Service/UrlShortenerInterface.php @@ -16,5 +16,5 @@ interface UrlShortenerInterface * @throws NonUniqueSlugException * @throws InvalidUrlException */ - public function urlToShortCode(string $url, array $tags, ShortUrlMeta $meta): ShortUrl; + public function shorten(string $url, array $tags, ShortUrlMeta $meta): ShortUrl; } diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 467aeb7b..436b42e3 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -8,7 +8,6 @@ use Cake\Chronos\Chronos; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\DBAL\Connection; use Doctrine\ORM\EntityManagerInterface; -use Doctrine\ORM\ORMException; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; @@ -42,16 +41,18 @@ class UrlShortenerTest extends TestCase $this->em = $this->prophesize(EntityManagerInterface::class); $conn = $this->prophesize(Connection::class); - $conn->isTransactionActive()->willReturn(false); $this->em->getConnection()->willReturn($conn->reveal()); - $this->em->flush()->willReturn(null); - $this->em->commit()->willReturn(null); - $this->em->beginTransaction()->willReturn(null); $this->em->persist(Argument::any())->will(function ($arguments): void { /** @var ShortUrl $shortUrl */ [$shortUrl] = $arguments; $shortUrl->setId('10'); }); + $this->em->transactional(Argument::type('callable'))->will(function (array $args) { + /** @var callable $callback */ + [$callback] = $args; + + return $callback(); + }); $repo = $this->prophesize(ShortUrlRepository::class); $repo->shortCodeIsInUse(Argument::cetera())->willReturn(false); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); @@ -70,7 +71,7 @@ class UrlShortenerTest extends TestCase /** @test */ public function urlIsProperlyShortened(): void { - $shortUrl = $this->urlShortener->urlToShortCode( + $shortUrl = $this->urlShortener->shorten( 'http://foobar.com/12345/hello?foo=bar', [], ShortUrlMeta::createEmpty(), @@ -87,32 +88,13 @@ class UrlShortenerTest extends TestCase $ensureUniqueness->shouldBeCalledOnce(); $this->expectException(NonUniqueSlugException::class); - $this->urlShortener->urlToShortCode( + $this->urlShortener->shorten( 'http://foobar.com/12345/hello?foo=bar', [], ShortUrlMeta::fromRawData(['customSlug' => 'custom-slug']), ); } - /** @test */ - public function transactionIsRolledBackAndExceptionRethrownWhenExceptionIsThrown(): void - { - $conn = $this->prophesize(Connection::class); - $conn->isTransactionActive()->willReturn(true); - $this->em->getConnection()->willReturn($conn->reveal()); - $this->em->rollback()->shouldBeCalledOnce(); - $this->em->close()->shouldBeCalledOnce(); - - $this->em->flush()->willThrow(new ORMException()); - - $this->expectException(ORMException::class); - $this->urlShortener->urlToShortCode( - 'http://foobar.com/12345/hello?foo=bar', - [], - ShortUrlMeta::createEmpty(), - ); - } - /** * @test * @dataProvider provideExistingShortUrls @@ -127,7 +109,7 @@ class UrlShortenerTest extends TestCase $findExisting = $repo->findOneMatching(Argument::cetera())->willReturn($expected); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - $result = $this->urlShortener->urlToShortCode($url, $tags, $meta); + $result = $this->urlShortener->shorten($url, $tags, $meta); $findExisting->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); diff --git a/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php index feed626d..8d4ea777 100644 --- a/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php @@ -31,7 +31,7 @@ abstract class AbstractCreateShortUrlAction extends AbstractRestAction $tags = $shortUrlData->getTags(); $shortUrlMeta = $shortUrlData->getMeta(); - $shortUrl = $this->urlShortener->urlToShortCode($longUrl, $tags, $shortUrlMeta); + $shortUrl = $this->urlShortener->shorten($longUrl, $tags, $shortUrlMeta); $transformer = new ShortUrlDataTransformer($this->domainConfig); return new JsonResponse($transformer->transform($shortUrl)); diff --git a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php index 1adc01a2..fd26da99 100644 --- a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php @@ -51,7 +51,7 @@ class CreateShortUrlActionTest extends TestCase public function properShortcodeConversionReturnsData(array $body, ShortUrlMeta $expectedMeta): void { $shortUrl = new ShortUrl(''); - $shorten = $this->urlShortener->urlToShortCode( + $shorten = $this->urlShortener->shorten( Argument::type('string'), Argument::type('array'), $expectedMeta, @@ -88,7 +88,7 @@ class CreateShortUrlActionTest extends TestCase public function anInvalidDomainReturnsError(string $domain): void { $shortUrl = new ShortUrl(''); - $urlToShortCode = $this->urlShortener->urlToShortCode(Argument::cetera())->willReturn($shortUrl); + $urlToShortCode = $this->urlShortener->shorten(Argument::cetera())->willReturn($shortUrl); $request = (new ServerRequest())->withParsedBody([ 'longUrl' => 'http://www.domain.com/foo/bar', diff --git a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php index 53bb6d8b..d7b432c2 100644 --- a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php @@ -72,7 +72,7 @@ class SingleStepCreateShortUrlActionTest extends TestCase 'longUrl' => 'http://foobar.com', ]); $findApiKey = $this->apiKeyService->check('abc123')->willReturn(true); - $generateShortCode = $this->urlShortener->urlToShortCode( + $generateShortCode = $this->urlShortener->shorten( Argument::that(function (string $argument): string { Assert::assertEquals('http://foobar.com', $argument); return $argument;