Merge pull request #1737 from shlinkio/develop

Release 3.5.3

CHANGELOG.md

@@ -4,6 +4,25 @@ 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).
 
+## [3.5.3] - 2023-03-31
+### Added
+* *Nothing*
+
+### Changed
+* *Nothing*
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* [#1715](https://github.com/shlinkio/shlink/issues/1715) Fix short URL creation/edition allowing long URLs without schema. Now a validation error is thrown.
+* [#1537](https://github.com/shlinkio/shlink/issues/1537) Fix incorrect list of tags being returned for some author-only API keys.
+* [#1738](https://github.com/shlinkio/shlink/issues/1738) Fix memory leak when importing short URLs with many visits.
+
+
 ## [3.5.2] - 2023-02-16
 ### Added
 * *Nothing*

composer.json

@@ -32,7 +32,6 @@
         "laminas/laminas-inputfilter": "^2.24",
         "laminas/laminas-servicemanager": "^3.20",
         "laminas/laminas-stdlib": "^3.16",
-        "lcobucci/jwt": "^4.3",
         "league/uri": "^6.8",
         "lstrojny/functional-php": "^1.17",
         "mezzio/mezzio": "^3.15",
@@ -46,7 +45,7 @@
         "php-middleware/request-id": "^4.1",
         "pugx/shortid-php": "^1.1",
         "ramsey/uuid": "^4.7",
-        "shlinkio/shlink-common": "^5.3.1",
+        "shlinkio/shlink-common": "^5.4",
         "shlinkio/shlink-config": "^2.4",
         "shlinkio/shlink-event-dispatcher": "^2.6",
         "shlinkio/shlink-importer": "^5.0",

config/autoload/entity-manager.local.php.dist

@@ -15,6 +15,14 @@ return [
 //            'dbname' => 'shlink_foo',
             'charset' => 'utf8mb4',
 
+            // MariaDB
+//            'user' => 'root',
+//            'password' => 'root',
+//            'driver' => 'pdo_mysql',
+//            'host' => 'shlink_db_maria',
+//            'dbname' => 'shlink_foo',
+//            'charset' => 'utf8mb4',
+
             // Postgres
 //            'user' => 'postgres',
 //            'password' => 'root',

config/constants.php

@@ -13,6 +13,7 @@
 const DEFAULT_REDIRECT_CACHE_LIFETIME = 30;
 const LOCAL_LOCK_FACTORY = 'Shlinkio\Shlink\LocalLockFactory';
 const TITLE_TAG_VALUE = '/<title[^>]*>(.*?)<\/title>/i'; // Matches the value inside a html title tag
+const LOOSE_URI_MATCHER = '/(.+)\:\/\/(.+)/i'; // Matches anything starting with a schema.
 const DEFAULT_QR_CODE_SIZE = 300;
 const DEFAULT_QR_CODE_MARGIN = 0;
 const DEFAULT_QR_CODE_FORMAT = 'png';

data/migrations/Version20230303164233.php

@@ -0,0 +1,28 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkMigrations;
+
+use Doctrine\DBAL\Platforms\MySQLPlatform;
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+final class Version20230303164233 extends AbstractMigration
+{
+    private const INDEX_NAME = 'visits_potential_bot_IDX';
+
+    public function up(Schema $schema): void
+    {
+        $visits = $schema->getTable('visits');
+        $this->skipIf($visits->hasIndex(self::INDEX_NAME));
+
+        $visits->dropIndex('IDX_visits_potential_bot'); // Old index
+        $visits->addIndex(['potential_bot'], self::INDEX_NAME);
+    }
+
+    public function isTransactional(): bool
+    {
+        return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform);
+    }
+}

docker/config/php.ini

@@ -1,4 +1,4 @@
 log_errors_max_len=0
 zend.assertions=1
 assert.exception=1
-memory_limit=256M
+memory_limit=512M

module/CLI/src/Command/Api/GenerateKeyCommand.php

@@ -9,6 +9,7 @@
 use Shlinkio\Shlink\CLI\Util\ExitCodes;
 use Shlinkio\Shlink\CLI\Util\ShlinkTable;
 use Shlinkio\Shlink\Rest\ApiKey\Role;
+use Shlinkio\Shlink\Rest\Entity\ApiKey;
 use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface;
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputInterface;
@@ -99,7 +100,7 @@ protected function execute(InputInterface $input, OutputInterface $output): ?int
         $io = new SymfonyStyle($input, $output);
         $io->success(sprintf('Generated API key: "%s"', $apiKey->toString()));
 
-        if (! $apiKey->isAdmin()) {
+        if (! ApiKey::isAdmin($apiKey)) {
             ShlinkTable::default($io)->render(
                 ['Role name', 'Role metadata'],
                 $apiKey->mapRoles(fn (Role $role, array $meta) => [$role->value, arrayToString($meta, 0)]),

module/CLI/src/Command/Api/ListKeysCommand.php

@@ -59,7 +59,7 @@ protected function execute(InputInterface $input, OutputInterface $output): ?int
                 $rowData[] = sprintf($messagePattern, $this->getEnabledSymbol($apiKey));
             }
             $rowData[] = $expiration?->toAtomString() ?? '-';
-            $rowData[] = $apiKey->isAdmin() ? 'Admin' : implode("\n", $apiKey->mapRoles(
+            $rowData[] = ApiKey::isAdmin($apiKey) ? 'Admin' : implode("\n", $apiKey->mapRoles(
                 fn (Role $role, array $meta) =>
                     empty($meta)
                         ? $role->toFriendlyName()

module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php

@@ -49,7 +49,7 @@ public function loadingMorePagesCallsListMoreTimes(): void
         // The paginator will return more than one page
         $data = [];
         for ($i = 0; $i < 50; $i++) {
-            $data[] = ShortUrl::withLongUrl('url_' . $i);
+            $data[] = ShortUrl::withLongUrl('https://url_' . $i);
         }
 
         $this->shortUrlService->expects($this->exactly(3))->method('listShortUrls')->withAnyParameters()
@@ -71,7 +71,7 @@ public function havingMorePagesButAnsweringNoCallsListJustOnce(): void
         // The paginator will return more than one page
         $data = [];
         for ($i = 0; $i < 30; $i++) {
-            $data[] = ShortUrl::withLongUrl('url_' . $i);
+            $data[] = ShortUrl::withLongUrl('https://url_' . $i);
         }
 
         $this->shortUrlService->expects($this->once())->method('listShortUrls')->with(
@@ -114,7 +114,7 @@ public function provideOptionalFlagsMakesNewColumnsToBeIncluded(
             ShortUrlsParams::emptyInstance(),
         )->willReturn(new Paginator(new ArrayAdapter([
             ShortUrl::create(ShortUrlCreation::fromRawData([
-                'longUrl' => 'foo.com',
+                'longUrl' => 'https://foo.com',
                 'tags' => ['foo', 'bar', 'baz'],
                 'apiKey' => $apiKey,
             ])),

module/Core/src/Importer/ImportedLinksProcessor.php

@@ -18,7 +18,6 @@
 use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl;
 use Shlinkio\Shlink\Importer\Model\ImportResult;
 use Shlinkio\Shlink\Importer\Params\ImportParams;
-use Shlinkio\Shlink\Importer\Sources\ImportSource;
 use Symfony\Component\Console\Style\OutputStyle;
 use Symfony\Component\Console\Style\StyleInterface;
 use Throwable;
@@ -55,8 +54,7 @@ public function process(StyleInterface $io, ImportResult $result, ImportParams $
     private function importShortUrls(StyleInterface $io, iterable $shlinkUrls, ImportParams $params): void
     {
         $importShortCodes = $params->importShortCodes;
-        $source = $params->source;
-        $iterable = $this->batchHelper->wrapIterable($shlinkUrls, $source === ImportSource::SHLINK ? 10 : 100);
+        $iterable = $this->batchHelper->wrapIterable($shlinkUrls, $params->importVisits ? 10 : 100);
 
         foreach ($iterable as $importedUrl) {
             $skipOnShortCodeConflict = static fn (): bool => $io->choice(sprintf(
@@ -82,7 +80,10 @@ private function importShortUrls(StyleInterface $io, iterable $shlinkUrls, Impor
                 continue;
             }
 
-            $resultMessage = $shortUrlImporting->importVisits($importedUrl->visits, $this->em);
+            $resultMessage = $shortUrlImporting->importVisits(
+                $this->batchHelper->wrapIterable($importedUrl->visits, 100),
+                $this->em,
+            );
             $io->text(sprintf('%s: %s', $longUrl, $resultMessage));
         }
     }

module/Core/src/Importer/ShortUrlImporting.php

@@ -33,7 +33,7 @@ public static function fromNewShortUrl(ShortUrl $shortUrl): self
      */
     public function importVisits(iterable $visits, EntityManagerInterface $em): string
     {
-        $mostRecentImportedDate = $this->shortUrl->mostRecentImportedVisitDate();
+        $mostRecentImportedDate = $this->resolveShortUrl($em)->mostRecentImportedVisitDate();
 
         $importedVisits = 0;
         foreach ($visits as $importedVisit) {
@@ -42,7 +42,7 @@ public function importVisits(iterable $visits, EntityManagerInterface $em): stri
                 continue;
             }
 
-            $em->persist(Visit::fromImport($this->shortUrl, $importedVisit));
+            $em->persist(Visit::fromImport($this->resolveShortUrl($em), $importedVisit));
             $importedVisits++;
         }
 
@@ -54,4 +54,14 @@ public function importVisits(iterable $visits, EntityManagerInterface $em): stri
             ? sprintf('<info>Imported</info> with <info>%s</info> visits', $importedVisits)
             : sprintf('<comment>Skipped</comment>. Imported <info>%s</info> visits', $importedVisits);
     }
+
+    private function resolveShortUrl(EntityManagerInterface $em): ShortUrl
+    {
+        // Instead of directly accessing wrapped ShortUrl entity, try to get it from the EM.
+        // With this, we will get the same entity from memory if it is known by the EM, but if it was cleared, the EM
+        // will fetch it again from the database, preventing errors at runtime.
+        // However, if the EM was not flushed yet, the entity will not be found by ID, but it is known by the EM.
+        // In that case, we fall back to wrapped ShortUrl entity directly.
+        return $em->find(ShortUrl::class, $this->shortUrl->getId()) ?? $this->shortUrl;
+    }
 }

module/Core/src/ShortUrl/Entity/ShortUrl.php

@@ -68,7 +68,7 @@ private function __construct()
      */
     public static function createFake(): self
     {
-        return self::withLongUrl('foo');
+        return self::withLongUrl('https://foo');
     }
 
     /**

module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php

@@ -12,8 +12,11 @@
 use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
 
+use function is_string;
+use function preg_match;
 use function substr;
 
+use const Shlinkio\Shlink\LOOSE_URI_MATCHER;
 use const Shlinkio\Shlink\MIN_SHORT_CODES_LENGTH;
 
 /**
@@ -59,27 +62,13 @@ public static function withNonRequiredLongUrl(array $data): self
 
     private function initialize(bool $requireLongUrl, UrlShortenerOptions $options): void
     {
-        $longUrlNotEmptyCommonOptions = [
-            Validator\NotEmpty::OBJECT,
-            Validator\NotEmpty::SPACE,
-            Validator\NotEmpty::EMPTY_ARRAY,
-            Validator\NotEmpty::BOOLEAN,
-            Validator\NotEmpty::STRING,
-        ];
-
         $longUrlInput = $this->createInput(self::LONG_URL, $requireLongUrl);
-        $longUrlInput->getValidatorChain()->attach(new Validator\NotEmpty([
-            ...$longUrlNotEmptyCommonOptions,
-            Validator\NotEmpty::NULL,
-        ]));
+        $longUrlInput->getValidatorChain()->merge($this->longUrlValidators());
         $this->add($longUrlInput);
 
         $deviceLongUrlsInput = $this->createInput(self::DEVICE_LONG_URLS, false);
         $deviceLongUrlsInput->getValidatorChain()->attach(
-            new DeviceLongUrlsValidator(new Validator\NotEmpty([
-                ...$longUrlNotEmptyCommonOptions,
-                ...($requireLongUrl ? [Validator\NotEmpty::NULL] : []),
-            ])),
+            new DeviceLongUrlsValidator($this->longUrlValidators(allowNull: ! $requireLongUrl)),
         );
         $this->add($deviceLongUrlsInput);
 
@@ -129,4 +118,25 @@ private function initialize(bool $requireLongUrl, UrlShortenerOptions $options):
 
         $this->add($this->createBooleanInput(self::CRAWLABLE, false));
     }
+
+    private function longUrlValidators(bool $allowNull = false): Validator\ValidatorChain
+    {
+        $emptyModifiers = [
+            Validator\NotEmpty::OBJECT,
+            Validator\NotEmpty::SPACE,
+            Validator\NotEmpty::EMPTY_ARRAY,
+            Validator\NotEmpty::BOOLEAN,
+            Validator\NotEmpty::STRING,
+        ];
+        if (! $allowNull) {
+            $emptyModifiers[] = Validator\NotEmpty::NULL;
+        }
+
+        return (new Validator\ValidatorChain())
+            ->attach(new Validator\NotEmpty($emptyModifiers))
+            ->attach(new Validator\Callback(
+                // Non-strings is always allowed. Other validators will take care of those
+                static fn (mixed $value) => ! is_string($value) || preg_match(LOOSE_URI_MATCHER, $value) === 1,
+            ));
+    }
 }

module/Core/src/ShortUrl/Repository/CrawlableShortCodesQuery.php

@@ -20,7 +20,8 @@ public function __invoke(): iterable
            ->from(ShortUrl::class, 's')
            ->where($qb->expr()->eq('s.crawlable', ':crawlable'))
            ->setParameter('crawlable', true)
-           ->setMaxResults($blockSize);
+           ->setMaxResults($blockSize)
+           ->orderBy('s.shortCode');
 
         $page = 0;
         do {

module/Core/src/Tag/Model/OrderableField.php

@@ -4,8 +4,6 @@
 
 namespace Shlinkio\Shlink\Core\Tag\Model;
 
-use function Shlinkio\Shlink\Core\camelCaseToSnakeCase;
-
 enum OrderableField: string
 {
     case TAG = 'tag';
@@ -15,20 +13,12 @@ enum OrderableField: string
     /** @deprecated Use VISITS instead */
     case VISITS_COUNT = 'visitsCount';
 
-    public static function isAggregateField(string $field): bool
+    public static function toSnakeCaseValidField(?string $field): self
     {
-        $parsed = self::tryFrom($field);
-        return $parsed !== null && $parsed !== self::TAG;
-    }
-
-    public static function toSnakeCaseValidField(?string $field): string
-    {
-        $parsed = $field !== null ? self::tryFrom($field) : self::VISITS;
-        $normalized = match ($parsed) {
+        $parsed = $field !== null ? self::tryFrom($field) : self::TAG;
+        return match ($parsed) {
             self::VISITS_COUNT, null => self::VISITS,
             default => $parsed,
         };
-
-        return camelCaseToSnakeCase($normalized->value);
     }
 }