Merge pull request #586 from acelaya-forks/hotfix/1.20.3

Hotfix/1.20.3

CHANGELOG.md

@@ -4,6 +4,54 @@ 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).
 
+## 1.20.3 - 2019-12-23
+
+#### Added
+
+* *Nothing*
+
+#### Changed
+
+* *Nothing*
+
+#### Deprecated
+
+* *Nothing*
+
+#### Removed
+
+* *Nothing*
+
+#### Fixed
+
+* [#585](https://github.com/shlinkio/shlink/issues/585) Fixed `PHP Fatal error: Uncaught Error: Class 'Shlinkio\Shlink\LocalLockFactory' not found` happening when running some CLI commands.
+
+
+## 1.20.2 - 2019-12-06
+
+#### 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.
+* [#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
 
 #### Added

config/autoload/locks.global.php

@@ -8,9 +8,7 @@ use Shlinkio\Shlink\Common\Logger\LoggerAwareDelegatorFactory;
 use Symfony\Component\Lock;
 use Zend\ServiceManager\AbstractFactory\ConfigAbstractFactory;
 
-// This class alias tricks the ConfigAbstractFactory to return Lock\Factory instances even with a different service name
 $localLockFactory = 'Shlinkio\Shlink\LocalLockFactory';
-class_alias(Lock\Factory::class, $localLockFactory);
 
 return [
 

config/container.php

@@ -3,6 +3,7 @@
 declare(strict_types=1);
 
 use Symfony\Component\Dotenv\Dotenv;
+use Symfony\Component\Lock;
 use Zend\ServiceManager\ServiceManager;
 
 chdir(dirname(__DIR__));
@@ -17,6 +18,9 @@ if (class_exists(Dotenv::class)) {
     $dotenv->load(__DIR__ . '/../.env');
 }
 
+// This class alias tricks the ConfigAbstractFactory to return Lock\Factory instances even with a different service name
+class_alias(Lock\Factory::class, 'Shlinkio\Shlink\LocalLockFactory');
+
 // Build container
 $config = require __DIR__ . '/config.php';
 $container = new ServiceManager($config['dependencies']);

migrations.php

@@ -0,0 +1,11 @@
+<?php
+
+declare(strict_types=1);
+
+return [
+    'name' => 'ShlinkMigrations',
+    'migrations_namespace' => 'ShlinkMigrations',
+    'table_name' => 'migrations',
+    'migrations_directory' => 'data/migrations',
+    'custom_template' => 'data/migrations_template.txt',
+];

migrations.yml

@@ -1,5 +0,0 @@
-name: ShlinkMigrations
-migrations_namespace: ShlinkMigrations
-table_name: migrations
-migrations_directory: data/migrations
-custom_template: data/migrations_template.txt

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

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

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/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,14 @@ 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,
+            ShortUrlMetaInputFilter::DOMAIN => 4,
+        ]];
     }
 
     /** @test */

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

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