Merge pull request #1343 from acelaya-forks/feature/inline-specs-improvements

Feature/inline specs improvements

module/Core/src/Repository/TagRepository.php

@@ -13,22 +13,15 @@ use Shlinkio\Shlink\Core\Tag\Model\TagsListFiltering;
 use Shlinkio\Shlink\Core\Tag\Spec\CountTagsWithName;
 use Shlinkio\Shlink\Rest\ApiKey\Role;
 use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin;
+use Shlinkio\Shlink\Rest\ApiKey\Spec\WithInlinedApiKeySpecsEnsuringJoin;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
 
 use function Functional\map;
-use function is_object;
-use function method_exists;
-use function sprintf;
-use function strlen;
-use function strpos;
-use function substr_replace;
 
 use const PHP_INT_MAX;
 
 class TagRepository extends EntitySpecificationRepository implements TagRepositoryInterface
 {
-    private const PARAM_PLACEHOLDER = '?';
-
     public function deleteByName(array $names): int
     {
         if (empty($names)) {
@@ -60,24 +53,11 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
         }
 
         $apiKey = $filtering?->apiKey();
-        $this->applySpecification($subQb, $apiKey?->spec(false, 'shortUrls'), 't');
+        $this->applySpecification($subQb, new WithInlinedApiKeySpecsEnsuringJoin($apiKey, 'shortUrls'), 't');
 
         $subQuery = $subQb->getQuery();
         $subQuerySql = $subQuery->getSQL();
 
-        // Sadly, we need to manually interpolate the params in the query replacing the placeholders, as this is going
-        // to be used as a sub-query in a native query. There's no need to sanitize, though.
-        foreach ($subQuery->getParameters() as $param) {
-            $value = $param->getValue();
-            $pos = strpos($subQuerySql, self::PARAM_PLACEHOLDER);
-            $subQuerySql = substr_replace(
-                $subQuerySql,
-                sprintf('\'%s\'', is_object($value) && method_exists($value, 'getId') ? $value->getId() : $value),
-                $pos === false ? -1 : $pos,
-                strlen(self::PARAM_PLACEHOLDER),
-            );
-        }
-
         // 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.
         // If no sub-query is used, the whole list is loaded even with pagination, making it very inefficient.

module/Core/src/Repository/VisitRepository.php

@@ -105,7 +105,7 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo
         $shortUrlRepo = $this->getEntityManager()->getRepository(ShortUrl::class);
         $shortUrlId = $shortUrlRepo->findOne($identifier, $filtering->apiKey()?->spec())?->getId() ?? '-1';
 
-        // Parameters in this query need to be part of the query itself, as we need to use it a sub-query later
+        // Parameters in this query need to be part of the query itself, as we need to use it as sub-query later
         // Since they are not provided by the caller, it's reasonably safe
         $qb = $this->getEntityManager()->createQueryBuilder();
         $qb->from(Visit::class, 'v')
@@ -142,14 +142,14 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo
         $qb->from(Visit::class, 'v')
            ->join('v.shortUrl', 's')
            ->join('s.tags', 't')
-           ->where($qb->expr()->eq('t.name', '\'' . $tag . '\'')); // This needs to be concatenated, not bound
+           ->where($qb->expr()->eq('t.name', $this->getEntityManager()->getConnection()->quote($tag)));
 
         if ($filtering->excludeBots()) {
             $qb->andWhere($qb->expr()->eq('v.potentialBot', 'false'));
         }
 
         $this->applyDatesInline($qb, $filtering->dateRange());
-        $this->applySpecification($qb, $filtering->apiKey()?->spec(true), 'v');
+        $this->applySpecification($qb, $filtering->apiKey()?->inlinedSpec(), 'v');
 
         return $qb;
     }
@@ -174,7 +174,7 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo
         $qb = $this->createAllVisitsQueryBuilder($filtering);
         $qb->andWhere($qb->expr()->isNotNull('v.shortUrl'));
 
-        $this->applySpecification($qb, $filtering->apiKey()?->spec(true));
+        $this->applySpecification($qb, $filtering->apiKey()?->inlinedSpec());
 
         return $this->resolveVisitsWithNativeQuery($qb, $filtering->limit(), $filtering->offset());
     }

module/Core/src/ShortUrl/Spec/BelongsToApiKeyInlined.php

@@ -17,6 +17,7 @@ class BelongsToApiKeyInlined implements Filter
     public function getFilter(QueryBuilder $qb, string $dqlAlias): string
     {
         // Parameters in this query need to be inlined, not bound, as we need to use it as sub-query later
-        return $qb->expr()->eq('s.authorApiKey', '\'' . $this->apiKey->getId() . '\'')->__toString();
+        $conn = $qb->getEntityManager()->getConnection();
+        return $qb->expr()->eq('s.authorApiKey', $conn->quote($this->apiKey->getId()))->__toString();
     }
 }

module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php

@@ -16,6 +16,7 @@ class BelongsToDomainInlined implements Filter
     public function getFilter(QueryBuilder $qb, string $context): string
     {
         // Parameters in this query need to be inlined, not bound, as we need to use it as sub-query later
-        return $qb->expr()->eq('s.domain', '\'' . $this->domainId . '\'')->__toString();
+        $conn = $qb->getEntityManager()->getConnection();
+        return $qb->expr()->eq('s.domain', $conn->quote($this->domainId))->__toString();
     }
 }

module/Core/test-db/Repository/TagRepositoryTest.php

@@ -63,19 +63,27 @@ class TagRepositoryTest extends DatabaseTestCase
         foreach ($names as $name) {
             $this->getEntityManager()->persist(new Tag($name));
         }
+
+        $apiKey = $filtering?->apiKey();
+        if ($apiKey !== null) {
+            $this->getEntityManager()->persist($apiKey);
+        }
+
         $this->getEntityManager()->flush();
 
         [$firstUrlTags] = array_chunk($names, 3);
         $secondUrlTags = [$names[0]];
-        $metaWithTags = fn (array $tags) => ShortUrlMeta::fromRawData(['longUrl' => '', 'tags' => $tags]);
+        $metaWithTags = fn (array $tags, ?ApiKey $apiKey) => ShortUrlMeta::fromRawData(
+            ['longUrl' => '', 'tags' => $tags, 'apiKey' => $apiKey],
+        );
 
-        $shortUrl = ShortUrl::fromMeta($metaWithTags($firstUrlTags), $this->relationResolver);
+        $shortUrl = ShortUrl::fromMeta($metaWithTags($firstUrlTags, $apiKey), $this->relationResolver);
         $this->getEntityManager()->persist($shortUrl);
         $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()));
         $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()));
         $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()));
 
-        $shortUrl2 = ShortUrl::fromMeta($metaWithTags($secondUrlTags), $this->relationResolver);
+        $shortUrl2 = ShortUrl::fromMeta($metaWithTags($secondUrlTags, null), $this->relationResolver);
         $this->getEntityManager()->persist($shortUrl2);
         $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl2, Visitor::emptyInstance()));
         $this->getEntityManager()->flush();
@@ -181,6 +189,23 @@ class TagRepositoryTest extends DatabaseTestCase
                 self::assertEquals($tagNames[0], $result[0]->tag()->__toString());
             },
         ];
+        yield 'api key' => [new TagsListFiltering(null, null, null, null, ApiKey::fromMeta(
+            ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls()),
+        )), static function (array $result, array $tagNames): void {
+            /** @var TagInfo[] $result */
+            self::assertCount(3, $result);
+            self::assertEquals(1, $result[0]->shortUrlsCount());
+            self::assertEquals(3, $result[0]->visitsCount());
+            self::assertEquals($tagNames[1], $result[0]->tag()->__toString());
+
+            self::assertEquals(1, $result[1]->shortUrlsCount());
+            self::assertEquals(3, $result[1]->visitsCount());
+            self::assertEquals($tagNames[2], $result[1]->tag()->__toString());
+
+            self::assertEquals(1, $result[2]->shortUrlsCount());
+            self::assertEquals(3, $result[2]->visitsCount());
+            self::assertEquals($tagNames[0], $result[2]->tag()->__toString());
+        }];
     }
 
     /** @test */

module/Rest/src/ApiKey/Role.php

@@ -21,21 +21,22 @@ class Role
         self::DOMAIN_SPECIFIC => 'Domain only',
     ];
 
-    public static function toSpec(ApiKeyRole $role, bool $inlined, ?string $context = null): Specification
+    public static function toSpec(ApiKeyRole $role, ?string $context = null): Specification
     {
-        if ($role->name() === self::AUTHORED_SHORT_URLS) {
-            $apiKey = $role->apiKey();
-            return $inlined ? Spec::andX(new BelongsToApiKeyInlined($apiKey)) : new BelongsToApiKey($apiKey, $context);
-        }
+        return match ($role->name()) {
+            self::AUTHORED_SHORT_URLS => new BelongsToApiKey($role->apiKey(), $context),
+            self::DOMAIN_SPECIFIC => new BelongsToDomain(self::domainIdFromMeta($role->meta()), $context),
+            default => Spec::andX(),
+        };
+    }
 
-        if ($role->name() === self::DOMAIN_SPECIFIC) {
-            $domainId = self::domainIdFromMeta($role->meta());
-            return $inlined
-                ? Spec::andX(new BelongsToDomainInlined($domainId))
-                : new BelongsToDomain($domainId, $context);
-        }
-
-        return Spec::andX();
+    public static function toInlinedSpec(ApiKeyRole $role): Specification
+    {
+        return match ($role->name()) {
+            self::AUTHORED_SHORT_URLS => Spec::andX(new BelongsToApiKeyInlined($role->apiKey())),
+            self::DOMAIN_SPECIFIC => Spec::andX(new BelongsToDomainInlined(self::domainIdFromMeta($role->meta()))),
+            default => Spec::andX(),
+        };
     }
 
     public static function domainIdFromMeta(array $meta): string

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

@@ -20,7 +20,7 @@ class WithApiKeySpecsEnsuringJoin extends BaseSpecification
     {
         return $this->apiKey === null || $this->apiKey->isAdmin() ? Spec::andX() : Spec::andX(
             Spec::join($this->fieldToJoin, 's'),
-            $this->apiKey->spec(false, $this->fieldToJoin),
+            $this->apiKey->spec($this->fieldToJoin),
         );
     }
 }

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

@@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Rest\ApiKey\Spec;
+
+use Happyr\DoctrineSpecification\Spec;
+use Happyr\DoctrineSpecification\Specification\BaseSpecification;
+use Happyr\DoctrineSpecification\Specification\Specification;
+use Shlinkio\Shlink\Rest\Entity\ApiKey;
+
+class WithInlinedApiKeySpecsEnsuringJoin extends BaseSpecification
+{
+    public function __construct(private ?ApiKey $apiKey, private string $fieldToJoin = 'shortUrls')
+    {
+        parent::__construct();
+    }
+
+    protected function getSpec(): Specification
+    {
+        return $this->apiKey === null || $this->apiKey->isAdmin() ? Spec::andX() : Spec::andX(
+            Spec::join($this->fieldToJoin, 's'),
+            $this->apiKey->inlinedSpec(),
+        );
+    }
+}

module/Rest/src/Entity/ApiKey.php

@@ -96,9 +96,15 @@ class ApiKey extends AbstractEntity
         return $this->key;
     }
 
-    public function spec(bool $inlined = false, ?string $context = null): Specification
+    public function spec(?string $context = null): Specification
     {
-        $specs = $this->roles->map(fn (ApiKeyRole $role) => Role::toSpec($role, $inlined, $context))->getValues();
+        $specs = $this->roles->map(fn (ApiKeyRole $role) => Role::toSpec($role, $context))->getValues();
+        return Spec::andX(...$specs);
+    }
+
+    public function inlinedSpec(): Specification
+    {
+        $specs = $this->roles->map(fn (ApiKeyRole $role) => Role::toInlinedSpec($role))->getValues();
         return Spec::andX(...$specs);
     }
 

module/Rest/test/ApiKey/RoleTest.php

@@ -21,39 +21,50 @@ class RoleTest extends TestCase
      * @test
      * @dataProvider provideRoles
      */
-    public function returnsExpectedSpec(ApiKeyRole $apiKeyRole, bool $inlined, Specification $expected): void
+    public function returnsExpectedSpec(ApiKeyRole $apiKeyRole, Specification $expected): void
     {
-        self::assertEquals($expected, Role::toSpec($apiKeyRole, $inlined));
+        self::assertEquals($expected, Role::toSpec($apiKeyRole));
     }
 
     public function provideRoles(): iterable
     {
         $apiKey = ApiKey::create();
 
-        yield 'inline invalid role' => [new ApiKeyRole('invalid', [], $apiKey), true, Spec::andX()];
-        yield 'not inline invalid role' => [new ApiKeyRole('invalid', [], $apiKey), false, Spec::andX()];
-        yield 'inline author role' => [
+        yield 'invalid role' => [new ApiKeyRole('invalid', [], $apiKey), Spec::andX()];
+        yield 'author role' => [
             new ApiKeyRole(Role::AUTHORED_SHORT_URLS, [], $apiKey),
-            true,
-            Spec::andX(new BelongsToApiKeyInlined($apiKey)),
-        ];
-        yield 'not inline author role' => [
-            new ApiKeyRole(Role::AUTHORED_SHORT_URLS, [], $apiKey),
-            false,
             new BelongsToApiKey($apiKey),
         ];
-        yield 'inline domain role' => [
-            new ApiKeyRole(Role::DOMAIN_SPECIFIC, ['domain_id' => '123'], $apiKey),
-            true,
-            Spec::andX(new BelongsToDomainInlined('123')),
-        ];
-        yield 'not inline domain role' => [
+        yield 'domain role' => [
             new ApiKeyRole(Role::DOMAIN_SPECIFIC, ['domain_id' => '456'], $apiKey),
-            false,
             new BelongsToDomain('456'),
         ];
     }
 
+    /**
+     * @test
+     * @dataProvider provideInlinedRoles
+     */
+    public function returnsExpectedInlinedSpec(ApiKeyRole $apiKeyRole, Specification $expected): void
+    {
+        self::assertEquals($expected, Role::toInlinedSpec($apiKeyRole));
+    }
+
+    public function provideInlinedRoles(): iterable
+    {
+        $apiKey = ApiKey::create();
+
+        yield 'invalid role' => [new ApiKeyRole('invalid', [], $apiKey), Spec::andX()];
+        yield 'author role' => [
+            new ApiKeyRole(Role::AUTHORED_SHORT_URLS, [], $apiKey),
+            Spec::andX(new BelongsToApiKeyInlined($apiKey)),
+        ];
+        yield 'domain role' => [
+            new ApiKeyRole(Role::DOMAIN_SPECIFIC, ['domain_id' => '123'], $apiKey),
+            Spec::andX(new BelongsToDomainInlined('123')),
+        ];
+    }
+
     /**
      * @test
      * @dataProvider provideMetasWithDomainId