Merge pull request #2363 from acelaya-forks/feature/find-url-perf

Fix unique_short_code_plus_domain index in Microsoft SQL

CHANGELOG.md

@@ -4,7 +4,7 @@ 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]
+## [4.4.3] - 2025-02-15
 ### Added
 * *Nothing*
 
@@ -27,6 +27,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
   In the future, Shlink will allow you to define trusted proxies, to avoid other potential side effects because of this reversing of the list.
 
 * [#2354](https://github.com/shlinkio/shlink/issues/2354) Fix error "NOSCRIPT No matching script. Please use EVAL" thrown when creating a lock in redis.
+* [#2319](https://github.com/shlinkio/shlink/issues/2319) Fix unique index for `short_code` and `domain_id` in `short_urls` table not being used in Microsoft SQL engines for rows where `domain_id` is `null`.
 
 ## [4.4.2] - 2025-01-29
 ### Added

module/Core/migrations/Version20250215100756.php

@@ -0,0 +1,43 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkMigrations;
+
+use Doctrine\DBAL\Platforms\SQLServerPlatform;
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * Fix an incorrectly generated unique index in Microsoft SQL, on short_urls table, for short_code + domain_id columns.
+ * The index was generated only for rows where both columns were not null, which is not the desired behavior, as
+ * domain_id can be null.
+ * This is due to a bug in doctrine/dbal: https://github.com/doctrine/dbal/issues/3671
+ *
+ * FIXME DO NOT DELETE THIS MIGRATION! IT IS NOT POSSIBLE TO DO THIS IN ENTITY CONFIG CODE WHILE THE BUG EXISTS
+ */
+final class Version20250215100756 extends AbstractMigration
+{
+    public function up(Schema $schema): void
+    {
+        $this->skipIf(! $this->isMicrosoftSql());
+
+        // Drop the existing unique index
+        $shortUrls = $schema->getTable('short_urls');
+        $shortUrls->dropIndex('unique_short_code_plus_domain');
+    }
+
+    public function postUp(Schema $schema): void
+    {
+        // The only way to get the index properly generated is by hardcoding the SQL.
+        // Since this migration is run Microsoft SQL only, it is safe to use this approach.
+        $this->connection->executeStatement(
+            'CREATE UNIQUE INDEX unique_short_code_plus_domain ON short_urls (short_code, domain_id);',
+        );
+    }
+
+    private function isMicrosoftSql(): bool
+    {
+        return $this->connection->getDatabasePlatform() instanceof SQLServerPlatform;
+    }
+}

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

@@ -31,24 +31,33 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU
         $ordering = $dbPlatform instanceof PostgreSQLPlatform ? 'ASC' : 'DESC';
         $isStrict = $shortUrlMode === ShortUrlMode::STRICT;
 
-        $qb = $this->createQueryBuilder('s');
-        $qb->leftJoin('s.domain', 'd')
-           ->where($qb->expr()->eq($isStrict ? 's.shortCode' : 'LOWER(s.shortCode)', ':shortCode'))
-           ->setParameter('shortCode', $isStrict ? $identifier->shortCode : strtolower($identifier->shortCode))
-           ->andWhere($qb->expr()->orX(
-               $qb->expr()->isNull('s.domain'),
-               $qb->expr()->eq('d.authority', ':domain'),
-           ))
-           ->setParameter('domain', $identifier->domain);
+        // FIXME The `LOWER(s.shortCode)` condition in non-strict mode drops performance dramatically.
+        //       Investigate if the case-insensitive check can be done natively by the DB engine.
 
-        // Since we order by domain, we will have first the URL matching provided domain, followed by the one
-        // with no domain (if any), so it is safe to fetch 1 max result, and we will get:
-        //  * The short URL matching both the short code and the domain, or
-        //  * The short URL matching the short code but without any domain, or
-        //  * No short URL at all
-        $qb->orderBy('s.domain', $ordering)
+        $qb = $this->createQueryBuilder('s');
+        $qb->where($qb->expr()->eq($isStrict ? 's.shortCode' : 'LOWER(s.shortCode)', ':shortCode'))
+           ->setParameter('shortCode', $isStrict ? $identifier->shortCode : strtolower($identifier->shortCode))
            ->setMaxResults(1);
 
+        // If $domain is null, do not join with domains nor do $qb->expr()->eq('d.authority', ':domain')
+        $domain = $identifier->domain;
+        if ($domain === null) {
+            $qb->andWhere($qb->expr()->isNull('s.domain'));
+        } else {
+            $qb->leftJoin('s.domain', 'd')
+               ->andWhere($qb->expr()->orX(
+                   $qb->expr()->isNull('s.domain'),
+                   $qb->expr()->eq('d.authority', ':domain'),
+               ))
+               ->setParameter('domain', $domain)
+               // Since we order by domain, we will have first the URL matching provided domain, followed by the one
+               // with no domain (if any), so it is safe to fetch 1 max result, and we will get:
+               //  * The short URL matching both the short code and the domain, or
+               //  * The short URL matching the short code but without any domain, or
+               //  * No short URL at all
+               ->orderBy('s.domain', $ordering);
+        }
+
         return $qb->getQuery()->getOneOrNullResult();
     }