Merge pull request #565 from acelaya-forks/bugfix/control-rename-tag

Bugfix/control rename tag

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

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;
         }
     }
 }

module/Core/src/Exception/TagConflictException.php

@@ -0,0 +1,15 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Exception;
+
+use function sprintf;
+
+class TagConflictException extends RuntimeException
+{
+    public static function fromExistingTag(string $oldName, string $newName): self
+    {
+        return new self(sprintf('You cannot rename tag %s to %s, because it already exists', $oldName, $newName));
+    }
+}

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($oldName, $newName);
+        }
+
         $tag->rename($newName);
 
         /** @var ORM\EntityManager $em */

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;
 }

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');
     }
 }

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);
         }
     }
 }

module/Rest/test-api/Action/UpdateTagActionTest.php

@@ -0,0 +1,35 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioApiTest\Shlink\Rest\Action;
+
+use GuzzleHttp\RequestOptions;
+use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase;
+
+class UpdateTagActionTest extends ApiTestCase
+{
+    /** @test */
+    public function errorIsThrownWhenTryingToRenameTagToAnotherTagName(): void
+    {
+        $resp = $this->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());
+    }
+}