Merge pull request #1691 from shlinkio/develop

Release 3.5.1

.github/actions/ci-setup/action.yml

@@ -28,7 +28,7 @@ runs:
         extensions: ${{ inputs.php-extensions }}
         key: ${{ inputs.extensions-cache-key }}
     - name: Cache extensions
-      uses: actions/cache@v3
+      uses: actions/cache@v2
       with:
         path: ${{ steps.extcache.outputs.dir }}
         key: ${{ steps.extcache.outputs.key }}

.github/workflows/ci-mutation-tests.yml

@@ -27,14 +27,14 @@ jobs:
           path: build
       - name: Resolve infection args
         id: infection_args
-        run: echo "args=--logger-github=false" >> $GITHUB_OUTPUT
+        run: echo "::set-output name=args::--logger-github=false"
 #        TODO Try to filter mutation tests to improve execution times. Investigate why --git-diff-lines --git-diff-base=develop does not work
 #        run: |
 #          BRANCH="${GITHUB_REF#refs/heads/}" |
 #          if [[ $BRANCH == 'main' || $BRANCH == 'develop' ]]; then
-#            echo "args=--logger-github=false" >> $GITHUB_OUTPUT
+#            echo "::set-output name=args::--logger-github=false"
 #          else
-#            echo "args=--logger-github=false --git-diff-lines --git-diff-base=develop" >> $GITHUB_OUTPUT
+#            echo "::set-output name=args::--logger-github=false --git-diff-lines --git-diff-base=develop"
 #          fi;
         shell: bash
       - if: ${{ inputs.test-group == 'unit' }}

.github/workflows/publish-swagger-spec.yml

@@ -15,7 +15,7 @@ jobs:
       - uses: actions/checkout@v3
       - name: Determine version
         id: determine_version
-        run: echo "version=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT
+        run: echo "::set-output name=version::${GITHUB_REF#refs/tags/}"
         shell: bash
       - uses: './.github/actions/ci-setup'
         with:

CHANGELOG.md

@@ -4,24 +4,6 @@ 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
-* [#1698](https://github.com/shlinkio/shlink/issues/1698) Fixed error 500 in `robots.txt`.
-* [#1688](https://github.com/shlinkio/shlink/issues/1688) Fixed huge performance degradation on `/tags/stats` endpoint.
-
-
 ## [3.5.1] - 2023-02-04
 ### Added
 * *Nothing*

config/autoload/logger.local.php.dist

@@ -5,16 +5,14 @@ declare(strict_types=1);
 use Monolog\Level;
 use Shlinkio\Shlink\Common\Logger\LoggerType;
 
-use function Shlinkio\Shlink\Config\runningInOpenswoole;
-
-$logToStream = runningInOpenswoole();
+$isSwoole = extension_loaded('openswoole');
 
 return [
 
     'logger' => [
         'Shlink' => [
-            // For openswoole, send logs as stream
-            'type' => $logToStream ? LoggerType::STREAM->value : LoggerType::FILE->value,
+            // For swoole, send logs as stream
+            'type' => $isSwoole ? LoggerType::STREAM->value : LoggerType::FILE->value,
             'level' => Level::Debug->value,
         ],
     ],

data/infra/ci/install-ms-odbc.sh

@@ -3,7 +3,7 @@
 set -ex
 
 curl https://packages.microsoft.com/keys/microsoft.asc | apt-key add -
-curl https://packages.microsoft.com/config/ubuntu/22.04/prod.list > /etc/apt/sources.list.d/mssql-release.list
+curl https://packages.microsoft.com/config/ubuntu/20.04/prod.list > /etc/apt/sources.list.d/mssql-release.list
 apt-get update
-ACCEPT_EULA=Y apt-get install msodbcsql18
+ACCEPT_EULA=Y apt-get install msodbcsql17
 apt-get install unixodbc-dev

data/migrations/Version20230211171904.php

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

module/Core/config/dependencies.config.php

@@ -187,7 +187,7 @@ return [
             Util\DoctrineBatchHelper::class,
         ],
 
-        Crawling\CrawlingHelper::class => [ShortUrl\Repository\CrawlableShortCodesQuery::class],
+        Crawling\CrawlingHelper::class => ['em'],
     ],
 
 ];

module/Core/src/Tag/Repository/TagRepository.php

@@ -4,7 +4,6 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Core\Tag\Repository;
 
-use Doctrine\DBAL\Query\QueryBuilder as NativeQueryBuilder;
 use Doctrine\ORM\Query\ResultSetMappingBuilder;
 use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository;
 use Happyr\DoctrineSpecification\Spec;
@@ -46,6 +45,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
         $orderDir = $filtering?->orderBy?->direction;
         $orderMainQuery = $orderField !== null && OrderableField::isAggregateField($orderField);
 
+        $conn = $this->getEntityManager()->getConnection();
         $subQb = $this->createQueryBuilder('t');
         $subQb->select('t.id', 't.name');
 
@@ -53,51 +53,15 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
             $subQb->orderBy('t.name', $orderDir ?? 'ASC')
                   ->setMaxResults($filtering?->limit ?? PHP_INT_MAX)
                   ->setFirstResult($filtering?->offset ?? 0);
-            // TODO Check if applying limit/offset ot visits sub-queries is needed with large amounts of tags
         }
 
-        $conn = $this->getEntityManager()->getConnection();
-        $buildVisitsSubQuery = static function (bool $excludeBots, string $aggregateAlias) use ($conn) {
-            $visitsSubQuery = $conn->createQueryBuilder();
-            $commonJoinCondition = $visitsSubQuery->expr()->eq('v.short_url_id', 's.id');
-            $visitsJoin = ! $excludeBots
-                ? $commonJoinCondition
-                : $visitsSubQuery->expr()->and(
-                    $commonJoinCondition,
-                    $visitsSubQuery->expr()->eq('v.potential_bot', $conn->quote('0')),
-                );
-
-            return $visitsSubQuery
-                ->select('st.tag_id AS tag_id', 'COUNT(DISTINCT v.id) AS ' . $aggregateAlias)
-                ->from('visits', 'v')
-                ->join('v', 'short_urls', 's', $visitsJoin) // @phpstan-ignore-line
-                ->join('s', 'short_urls_in_tags', 'st', $visitsSubQuery->expr()->eq('st.short_url_id', 's.id'))
-                ->groupBy('st.tag_id');
-        };
-        $allVisitsSubQuery = $buildVisitsSubQuery(false, 'visits');
-        $nonBotVisitsSubQuery = $buildVisitsSubQuery(true, 'non_bot_visits');
-
         $searchTerm = $filtering?->searchTerm;
         if ($searchTerm !== null) {
             $subQb->andWhere($subQb->expr()->like('t.name', $conn->quote('%' . $searchTerm . '%')));
-            // TODO Check if applying this to all sub-queries makes it faster or slower
         }
 
         $apiKey = $filtering?->apiKey;
-        $applyApiKeyToNativeQuery = static fn (?ApiKey $apiKey, NativeQueryBuilder $nativeQueryBuilder) =>
-            $apiKey?->mapRoles(static fn (Role $role, array $meta) => match ($role) {
-                Role::DOMAIN_SPECIFIC => $nativeQueryBuilder->andWhere(
-                    $nativeQueryBuilder->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))),
-                ),
-                Role::AUTHORED_SHORT_URLS => $nativeQueryBuilder->andWhere(
-                    $nativeQueryBuilder->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())),
-                ),
-            });
-
-        // Apply API key specification to all sub-queries
         $this->applySpecification($subQb, new WithInlinedApiKeySpecsEnsuringJoin($apiKey), 't');
-        $applyApiKeyToNativeQuery($apiKey, $allVisitsSubQuery);
-        $applyApiKeyToNativeQuery($apiKey, $nonBotVisitsSubQuery);
 
         // A native query builder needs to be used here, because DQL and ORM query builders do not support
         // sub-queries at "from" and "join" level.
@@ -107,22 +71,29 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
             ->select(
                 't.id_0 AS id',
                 't.name_1 AS name',
-                'COALESCE(v.visits, 0) AS visits', // COALESCE required for postgres to properly order
-                'COALESCE(v2.non_bot_visits, 0) AS non_bot_visits', // COALESCE required for postgres to properly order
                 'COUNT(DISTINCT s.id) AS short_urls_count',
+                'COUNT(DISTINCT v.id) AS visits', // Native queries require snake_case for cross-db compatibility
+                'COUNT(DISTINCT v2.id) AS non_bot_visits',
             )
             ->from('(' . $subQb->getQuery()->getSQL() . ')', 't') // @phpstan-ignore-line
             ->leftJoin('t', 'short_urls_in_tags', 'st', $nativeQb->expr()->eq('t.id_0', 'st.tag_id'))
             ->leftJoin('st', 'short_urls', 's', $nativeQb->expr()->eq('s.id', 'st.short_url_id'))
-            ->leftJoin('t', '(' . $allVisitsSubQuery->getSQL() . ')', 'v', $nativeQb->expr()->eq('t.id_0', 'v.tag_id'))
-            ->leftJoin('t', '(' . $nonBotVisitsSubQuery->getSQL() . ')', 'v2', $nativeQb->expr()->eq(
-                't.id_0',
-                'v2.tag_id',
+            ->leftJoin('st', 'visits', 'v', $nativeQb->expr()->eq('st.short_url_id', 'v.short_url_id'))
+            ->leftJoin('st', 'visits', 'v2', $nativeQb->expr()->and( // @phpstan-ignore-line
+                $nativeQb->expr()->eq('st.short_url_id', 'v2.short_url_id'),
+                $nativeQb->expr()->eq('v2.potential_bot', $conn->quote('0')),
             ))
-            ->groupBy('t.id_0', 't.name_1', 'v.visits', 'v2.non_bot_visits');
+            ->groupBy('t.id_0', 't.name_1');
 
         // Apply API key role conditions to the native query too, as they will affect the amounts on the aggregates
-        $applyApiKeyToNativeQuery($apiKey, $nativeQb);
+        $apiKey?->mapRoles(static fn (Role $role, array $meta) => match ($role) {
+            Role::DOMAIN_SPECIFIC => $nativeQb->andWhere(
+                $nativeQb->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))),
+            ),
+            Role::AUTHORED_SHORT_URLS => $nativeQb->andWhere(
+                $nativeQb->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())),
+            ),
+        });
 
         if ($orderMainQuery) {
             $nativeQb
@@ -136,9 +107,9 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
 
         $rsm = new ResultSetMappingBuilder($this->getEntityManager());
         $rsm->addScalarResult('name', 'tag');
+        $rsm->addScalarResult('short_urls_count', 'shortUrlsCount');
         $rsm->addScalarResult('visits', 'visits');
         $rsm->addScalarResult('non_bot_visits', 'nonBotVisits');
-        $rsm->addScalarResult('short_urls_count', 'shortUrlsCount');
 
         return map(
             $this->getEntityManager()->createNativeQuery($nativeQb->getSQL(), $rsm)->getResult(),

module/Core/test-api/Action/RobotsTest.php

@@ -1,31 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace ShlinkioApiTest\Shlink\Core\Action;
-
-use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase;
-
-class RobotsTest extends ApiTestCase
-{
-    /** @test */
-    public function expectedListOfCrawlableShortCodesIsReturned(): void
-    {
-        $resp = $this->callShortUrl('robots.txt');
-        $body = $resp->getBody()->__toString();
-
-        self::assertEquals(200, $resp->getStatusCode());
-        self::assertEquals(
-            <<<ROBOTS
-            # For more information about the robots.txt standard, see:
-            # https://www.robotstxt.org/orig.html
-            
-            User-agent: *
-            Allow: /custom
-            Allow: /abc123
-            Disallow: /
-            ROBOTS,
-            $body,
-        );
-    }
-}

module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php

@@ -11,7 +11,7 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey;
 
 class WithInlinedApiKeySpecsEnsuringJoin extends BaseSpecification
 {
-    public function __construct(private readonly ?ApiKey $apiKey, private readonly string $fieldToJoin = 'shortUrls')
+    public function __construct(private ?ApiKey $apiKey, private string $fieldToJoin = 'shortUrls')
     {
         parent::__construct();
     }

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

@@ -122,7 +122,7 @@ class ListShortUrlsTest extends ApiTestCase
         ],
         'domain' => null,
         'title' => null,
-        'crawlable' => true,
+        'crawlable' => false,
         'forwardQuery' => false,
     ];
     private const SHORT_URL_CUSTOM_DOMAIN = [

module/Rest/test-api/Fixtures/ShortUrlsFixture.php

@@ -62,7 +62,6 @@ class ShortUrlsFixture extends AbstractFixture implements DependentFixtureInterf
             'maxVisits' => 2,
             'apiKey' => $authorApiKey,
             'longUrl' => 'https://shlink.io',
-            'crawlable' => true,
             'forwardQuery' => false,
         ])), '2019-01-01 00:00:20');
         $manager->persist($customShortUrl);