From 76a603104d5b66c2851f2bfb5cd62596a2b0eec2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Dec 2019 09:50:37 +0100 Subject: [PATCH 01/10] Migrated migrations config file from yaml to plain PHP --- migrations.php | 11 +++++++++++ migrations.yml | 5 ----- 2 files changed, 11 insertions(+), 5 deletions(-) create mode 100644 migrations.php delete mode 100644 migrations.yml diff --git a/migrations.php b/migrations.php new file mode 100644 index 00000000..364f52ad --- /dev/null +++ b/migrations.php @@ -0,0 +1,11 @@ + 'ShlinkMigrations', + 'migrations_namespace' => 'ShlinkMigrations', + 'table_name' => 'migrations', + 'migrations_directory' => 'data/migrations', + 'custom_template' => 'data/migrations_template.txt', +]; diff --git a/migrations.yml b/migrations.yml deleted file mode 100644 index db3a57b3..00000000 --- a/migrations.yml +++ /dev/null @@ -1,5 +0,0 @@ -name: ShlinkMigrations -migrations_namespace: ShlinkMigrations -table_name: migrations -migrations_directory: data/migrations -custom_template: data/migrations_template.txt From 4af27650cdf0158ec58192e7fdc217a47aa18059 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Dec 2019 09:52:23 +0100 Subject: [PATCH 02/10] Updated changelog --- CHANGELOG.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 283fdbe5..25b8e82f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,29 @@ 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] + +#### Added + +* *Nothing* + +#### Changed + +* *Nothing* + +#### Deprecated + +* *Nothing* + +#### Removed + +* *Nothing* + +#### Fixed + +* [#561](https://github.com/shlinkio/shlink/issues/561) Fixed `db:migrate` command failing because yaml extension is not installed, which makes config file not to be readable. + + ## 1.20.1 - 2019-11-17 #### Added From f62ed66e2685690929aeb23c5524e321f0807d13 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Dec 2019 10:20:56 +0100 Subject: [PATCH 03/10] Created TagConflictException --- .../Core/src/Exception/TagConflictException.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 module/Core/src/Exception/TagConflictException.php diff --git a/module/Core/src/Exception/TagConflictException.php b/module/Core/src/Exception/TagConflictException.php new file mode 100644 index 00000000..235cccc5 --- /dev/null +++ b/module/Core/src/Exception/TagConflictException.php @@ -0,0 +1,15 @@ + Date: Fri, 6 Dec 2019 20:44:41 +0100 Subject: [PATCH 04/10] Ensured a specific exception is thrown from TagService when trying to rename a tag to the name of another tag which already exists --- module/Core/src/Service/Tag/TagService.php | 12 +++++- .../src/Service/Tag/TagServiceInterface.php | 2 + .../Core/test/Service/Tag/TagServiceTest.php | 38 +++++++++++++++++-- 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/module/Core/src/Service/Tag/TagService.php b/module/Core/src/Service/Tag/TagService.php index fd32efe5..24856188 100644 --- a/module/Core/src/Service/Tag/TagService.php +++ b/module/Core/src/Service/Tag/TagService.php @@ -8,6 +8,7 @@ use Doctrine\Common\Collections\Collection; use Doctrine\ORM; use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Repository\TagRepository; use Shlinkio\Shlink\Core\Util\TagManagerTrait; @@ -64,17 +65,26 @@ class TagService implements TagServiceInterface * @param string $newName * @return Tag * @throws EntityDoesNotExistException + * @throws TagConflictException * @throws ORM\OptimisticLockException */ public function renameTag($oldName, $newName): Tag { + /** @var TagRepository $repo */ + $repo = $this->em->getRepository(Tag::class); $criteria = ['name' => $oldName]; + /** @var Tag|null $tag */ - $tag = $this->em->getRepository(Tag::class)->findOneBy($criteria); + $tag = $repo->findOneBy($criteria); if ($tag === null) { throw EntityDoesNotExistException::createFromEntityAndConditions(Tag::class, $criteria); } + $newNameExists = $newName !== $oldName && $repo->count(['name' => $newName]) > 0; + if ($newNameExists) { + throw TagConflictException::fromExistingTag($newName); + } + $tag->rename($newName); /** @var ORM\EntityManager $em */ diff --git a/module/Core/src/Service/Tag/TagServiceInterface.php b/module/Core/src/Service/Tag/TagServiceInterface.php index 1eb11112..942da3a6 100644 --- a/module/Core/src/Service/Tag/TagServiceInterface.php +++ b/module/Core/src/Service/Tag/TagServiceInterface.php @@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\Service\Tag; use Doctrine\Common\Collections\Collection; use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\TagConflictException; interface TagServiceInterface { @@ -34,6 +35,7 @@ interface TagServiceInterface * @param string $newName * @return Tag * @throws EntityDoesNotExistException + * @throws TagConflictException */ public function renameTag($oldName, $newName): Tag; } diff --git a/module/Core/test/Service/Tag/TagServiceTest.php b/module/Core/test/Service/Tag/TagServiceTest.php index 7670eafd..6b904fe5 100644 --- a/module/Core/test/Service/Tag/TagServiceTest.php +++ b/module/Core/test/Service/Tag/TagServiceTest.php @@ -11,6 +11,7 @@ use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Repository\TagRepository; use Shlinkio\Shlink\Core\Service\Tag\TagService; @@ -88,22 +89,51 @@ class TagServiceTest extends TestCase $this->service->renameTag('foo', 'bar'); } - /** @test */ - public function renameValidTagChangesItsName() + /** + * @test + * @dataProvider provideValidRenames + */ + public function renameValidTagChangesItsName(string $oldName, string $newName, int $count): void { $expected = new Tag('foo'); $repo = $this->prophesize(TagRepository::class); $find = $repo->findOneBy(Argument::cetera())->willReturn($expected); + $countTags = $repo->count(Argument::cetera())->willReturn($count); $getRepo = $this->em->getRepository(Tag::class)->willReturn($repo->reveal()); $flush = $this->em->flush($expected)->willReturn(null); - $tag = $this->service->renameTag('foo', 'bar'); + $tag = $this->service->renameTag($oldName, $newName); $this->assertSame($expected, $tag); - $this->assertEquals('bar', (string) $tag); + $this->assertEquals($newName, (string) $tag); $find->shouldHaveBeenCalled(); $getRepo->shouldHaveBeenCalled(); $flush->shouldHaveBeenCalled(); + $countTags->shouldHaveBeenCalledTimes($count > 0 ? 0 : 1); + } + + public function provideValidRenames(): iterable + { + yield 'same names' => ['foo', 'foo', 1]; + yield 'different names names' => ['foo', 'bar', 0]; + } + + /** @test */ + public function renameTagToAnExistingNameThrowsException(): void + { + $repo = $this->prophesize(TagRepository::class); + $find = $repo->findOneBy(Argument::cetera())->willReturn(new Tag('foo')); + $countTags = $repo->count(Argument::cetera())->willReturn(1); + $getRepo = $this->em->getRepository(Tag::class)->willReturn($repo->reveal()); + $flush = $this->em->flush(Argument::any())->willReturn(null); + + $find->shouldBeCalled(); + $getRepo->shouldBeCalled(); + $countTags->shouldBeCalled(); + $flush->shouldNotBeCalled(); + $this->expectException(TagConflictException::class); + + $this->service->renameTag('foo', 'bar'); } } From 05451e3d1a003db13e7d23e62a3de2beed6ebdfd Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Dec 2019 21:03:22 +0100 Subject: [PATCH 05/10] Handled tag conflict from rename tag action --- .../src/Exception/TagConflictException.php | 4 +-- module/Core/src/Service/Tag/TagService.php | 2 +- .../Rest/src/Action/Tag/UpdateTagAction.php | 10 ++++++ .../test-api/Action/UpdateTagActionTest.php | 35 +++++++++++++++++++ 4 files changed, 48 insertions(+), 3 deletions(-) create mode 100644 module/Rest/test-api/Action/UpdateTagActionTest.php diff --git a/module/Core/src/Exception/TagConflictException.php b/module/Core/src/Exception/TagConflictException.php index 235cccc5..db26682e 100644 --- a/module/Core/src/Exception/TagConflictException.php +++ b/module/Core/src/Exception/TagConflictException.php @@ -8,8 +8,8 @@ use function sprintf; class TagConflictException extends RuntimeException { - public static function fromExistingTag(string $tag): self + public static function fromExistingTag(string $oldName, string $newName): self { - return new self(sprintf('Tag with name %s already exists', $tag)); + return new self(sprintf('You cannot rename tag %s to %s, because it already exists', $oldName, $newName)); } } diff --git a/module/Core/src/Service/Tag/TagService.php b/module/Core/src/Service/Tag/TagService.php index 24856188..4a75a061 100644 --- a/module/Core/src/Service/Tag/TagService.php +++ b/module/Core/src/Service/Tag/TagService.php @@ -82,7 +82,7 @@ class TagService implements TagServiceInterface $newNameExists = $newName !== $oldName && $repo->count(['name' => $newName]) > 0; if ($newNameExists) { - throw TagConflictException::fromExistingTag($newName); + throw TagConflictException::fromExistingTag($oldName, $newName); } $tag->rename($newName); diff --git a/module/Rest/src/Action/Tag/UpdateTagAction.php b/module/Rest/src/Action/Tag/UpdateTagAction.php index 64ec7a21..d494673c 100644 --- a/module/Rest/src/Action/Tag/UpdateTagAction.php +++ b/module/Rest/src/Action/Tag/UpdateTagAction.php @@ -8,6 +8,7 @@ use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Service\Tag\TagServiceInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; use Shlinkio\Shlink\Rest\Util\RestUtils; @@ -58,6 +59,15 @@ class UpdateTagAction extends AbstractRestAction 'error' => RestUtils::NOT_FOUND_ERROR, 'message' => sprintf('It was not possible to find a tag with name %s', $body['oldName']), ], self::STATUS_NOT_FOUND); + } catch (TagConflictException $e) { + return new JsonResponse([ + 'error' => 'TAG_CONFLICT', + 'message' => sprintf( + 'You cannot rename tag %s to %s, because it already exists', + $body['oldName'], + $body['newName'] + ), + ], self::STATUS_CONFLICT); } } } diff --git a/module/Rest/test-api/Action/UpdateTagActionTest.php b/module/Rest/test-api/Action/UpdateTagActionTest.php new file mode 100644 index 00000000..206428af --- /dev/null +++ b/module/Rest/test-api/Action/UpdateTagActionTest.php @@ -0,0 +1,35 @@ +callApiWithKey(self::METHOD_PUT, '/tags', [RequestOptions::JSON => [ + 'oldName' => 'foo', + 'newName' => 'bar', + ]]); + $payload = $this->getJsonResponsePayload($resp); + + $this->assertEquals(self::STATUS_CONFLICT, $resp->getStatusCode()); + $this->assertEquals('TAG_CONFLICT', $payload['error']); + } + + /** @test */ + public function tagIsProperlyRenamedWhenRenamingToItself(): void + { + $resp = $this->callApiWithKey(self::METHOD_PUT, '/tags', [RequestOptions::JSON => [ + 'oldName' => 'foo', + 'newName' => 'foo', + ]]); + + $this->assertEquals(self::STATUS_NO_CONTENT, $resp->getStatusCode()); + } +} From 27aa8f9875f2882370a56e3a01b72d0803f0f42e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Dec 2019 21:04:52 +0100 Subject: [PATCH 06/10] Handled rename tag error from command --- module/CLI/src/Command/Tag/RenameTagCommand.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/module/CLI/src/Command/Tag/RenameTagCommand.php b/module/CLI/src/Command/Tag/RenameTagCommand.php index 3e9f021b..1638108a 100644 --- a/module/CLI/src/Command/Tag/RenameTagCommand.php +++ b/module/CLI/src/Command/Tag/RenameTagCommand.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\CLI\Command\Tag; use Shlinkio\Shlink\CLI\Util\ExitCodes; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; +use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Service\Tag\TagServiceInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -50,6 +51,11 @@ class RenameTagCommand extends Command } catch (EntityDoesNotExistException $e) { $io->error(sprintf('A tag with name "%s" was not found', $oldName)); return ExitCodes::EXIT_FAILURE; + } catch (TagConflictException $e) { + $io->error( + sprintf('A tag with name "%s" cannot be renamed to "%s" because it already exists', $oldName, $newName) + ); + return ExitCodes::EXIT_FAILURE; } } } From 3455df9214e0ae7eceaf7021e6bcf00505f84959 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Dec 2019 21:06:47 +0100 Subject: [PATCH 07/10] Updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25b8e82f..32621aa9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Fixed * [#561](https://github.com/shlinkio/shlink/issues/561) Fixed `db:migrate` command failing because yaml extension is not installed, which makes config file not to be readable. +* [#562](https://github.com/shlinkio/shlink/issues/562) Fixed internal server error being returned when renaming a tag to another tag's name. Now a meaningful API error with status 409 is returned. ## 1.20.1 - 2019-11-17 From 05a64b8d9e57f1ca54d8a0dd7b89b4f5792f35f7 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Dec 2019 22:38:22 +0100 Subject: [PATCH 08/10] Ensured dates parsing does not mask actual validation errors --- .../src/Command/ShortUrl/GenerateShortUrlCommand.php | 11 ++--------- module/Core/src/Model/ShortUrlMeta.php | 7 ++++++- module/Core/test/Model/ShortUrlMetaTest.php | 12 ++++++++++++ .../src/Action/ShortUrl/CreateShortUrlAction.php | 10 ++-------- 4 files changed, 22 insertions(+), 18 deletions(-) diff --git a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php index 9d9c464b..d7323a0d 100644 --- a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\ShortUrl; -use Cake\Chronos\Chronos; use Shlinkio\Shlink\CLI\Util\ExitCodes; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; @@ -127,8 +126,8 @@ class GenerateShortUrlCommand extends Command new Uri($longUrl), $tags, ShortUrlMeta::createFromParams( - $this->getOptionalDate($input, 'validSince'), - $this->getOptionalDate($input, 'validUntil'), + $input->getOption('validSince'), + $input->getOption('validUntil'), $customSlug, $maxVisits !== null ? (int) $maxVisits : null, $input->getOption('findIfExists'), @@ -151,10 +150,4 @@ class GenerateShortUrlCommand extends Command return ExitCodes::EXIT_FAILURE; } } - - private function getOptionalDate(InputInterface $input, string $fieldName): ?Chronos - { - $since = $input->getOption($fieldName); - return $since !== null ? Chronos::parse($since) : null; - } } diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index bf31a01b..24e770c6 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Model; use Cake\Chronos\Chronos; +use DateTimeInterface; use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; @@ -96,7 +97,7 @@ final class ShortUrlMeta } /** - * @param string|Chronos|null $date + * @param string|DateTimeInterface|Chronos|null $date */ private function parseDateField($date): ?Chronos { @@ -104,6 +105,10 @@ final class ShortUrlMeta return $date; } + if ($date instanceof DateTimeInterface) { + return Chronos::instance($date); + } + return Chronos::parse($date); } diff --git a/module/Core/test/Model/ShortUrlMetaTest.php b/module/Core/test/Model/ShortUrlMetaTest.php index d606e07f..7c8e5aae 100644 --- a/module/Core/test/Model/ShortUrlMetaTest.php +++ b/module/Core/test/Model/ShortUrlMetaTest.php @@ -9,6 +9,7 @@ use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; +use stdClass; class ShortUrlMetaTest extends TestCase { @@ -35,6 +36,17 @@ class ShortUrlMetaTest extends TestCase ShortUrlMetaInputFilter::VALID_SINCE => '2017', ShortUrlMetaInputFilter::MAX_VISITS => 5, ]]; + yield [[ + ShortUrlMetaInputFilter::VALID_SINCE => new stdClass(), + ShortUrlMetaInputFilter::VALID_UNTIL => 'foo', + ]]; + yield [[ + ShortUrlMetaInputFilter::VALID_UNTIL => 500, + ]]; + yield [[ + ShortUrlMetaInputFilter::MAX_VISITS => new stdClass(), + ShortUrlMetaInputFilter::DOMAIN => 4, + ]]; } /** @test */ diff --git a/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php index 3b0a0b61..96145ce0 100644 --- a/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Action\ShortUrl; -use Cake\Chronos\Chronos; use Psr\Http\Message\ServerRequestInterface as Request; use Shlinkio\Shlink\Core\Exception\InvalidArgumentException; use Shlinkio\Shlink\Core\Exception\ValidationException; @@ -32,8 +31,8 @@ class CreateShortUrlAction extends AbstractCreateShortUrlAction try { $meta = ShortUrlMeta::createFromParams( - $this->getOptionalDate($postData, 'validSince'), - $this->getOptionalDate($postData, 'validUntil'), + $postData['validSince'] ?? null, + $postData['validUntil'] ?? null, $postData['customSlug'] ?? null, $postData['maxVisits'] ?? null, $postData['findIfExists'] ?? null, @@ -45,9 +44,4 @@ class CreateShortUrlAction extends AbstractCreateShortUrlAction throw new InvalidArgumentException('Provided meta data is not valid', -1, $e); } } - - private function getOptionalDate(array $postData, string $fieldName): ?Chronos - { - return isset($postData[$fieldName]) ? Chronos::parse($postData[$fieldName]) : null; - } } From 57c91aca3cd65049bba58bbd835f0686e2f041e2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Dec 2019 22:40:08 +0100 Subject: [PATCH 09/10] Updated changelog with v1.20.2 --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 32621aa9..c28aa0af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ 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.20.2 - 2019-12-06 #### Added @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#561](https://github.com/shlinkio/shlink/issues/561) Fixed `db:migrate` command failing because yaml extension is not installed, which makes config file not to be readable. * [#562](https://github.com/shlinkio/shlink/issues/562) Fixed internal server error being returned when renaming a tag to another tag's name. Now a meaningful API error with status 409 is returned. +* [#555](https://github.com/shlinkio/shlink/issues/555) Fixed internal server error being returned when invalid dates are provided for new short URLs. Now a 400 is returned, as intended. ## 1.20.1 - 2019-11-17 From 3e9b7751143c4d6654398ce7a086706fd1fc8252 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Dec 2019 22:45:07 +0100 Subject: [PATCH 10/10] Fixed failing test --- module/Core/test/Model/ShortUrlMetaTest.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/module/Core/test/Model/ShortUrlMetaTest.php b/module/Core/test/Model/ShortUrlMetaTest.php index 7c8e5aae..fb02fcbc 100644 --- a/module/Core/test/Model/ShortUrlMetaTest.php +++ b/module/Core/test/Model/ShortUrlMetaTest.php @@ -42,9 +42,6 @@ class ShortUrlMetaTest extends TestCase ]]; yield [[ ShortUrlMetaInputFilter::VALID_UNTIL => 500, - ]]; - yield [[ - ShortUrlMetaInputFilter::MAX_VISITS => new stdClass(), ShortUrlMetaInputFilter::DOMAIN => 4, ]]; }