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 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; } } } diff --git a/module/Core/src/Exception/TagConflictException.php b/module/Core/src/Exception/TagConflictException.php new file mode 100644 index 00000000..db26682e --- /dev/null +++ b/module/Core/src/Exception/TagConflictException.php @@ -0,0 +1,15 @@ +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($oldName, $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'); } } 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()); + } +}