Standardized how inlined or regular specs are applied to query builders

module/Core/src/Repository/TagRepository.php

@@ -16,12 +16,6 @@ use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin;
 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;
 
@@ -60,24 +54,11 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
         }
 
         $apiKey = $filtering?->apiKey();
-        $this->applySpecification($subQb, $apiKey?->spec(false, 'shortUrls'), 't');
+        $this->applySpecification($subQb, new WithApiKeySpecsEnsuringJoin($apiKey, 'shortUrls', true), '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')
@@ -149,7 +149,7 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo
         }
 
         $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/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) {
+        return match ($role->name()) {
-            $apiKey = $role->apiKey();
+            self::AUTHORED_SHORT_URLS => new BelongsToApiKey($role->apiKey(), $context),
-            return $inlined ? Spec::andX(new BelongsToApiKeyInlined($apiKey)) : new BelongsToApiKey($apiKey, $context);
+            self::DOMAIN_SPECIFIC => new BelongsToDomain(self::domainIdFromMeta($role->meta()), $context),
-        }
+            default => Spec::andX(),
+        };
+    }
 
-        if ($role->name() === self::DOMAIN_SPECIFIC) {
+    public static function toInlinedSpec(ApiKeyRole $role): Specification
-            $domainId = self::domainIdFromMeta($role->meta());
+    {
-            return $inlined
+        return match ($role->name()) {
-                ? Spec::andX(new BelongsToDomainInlined($domainId))
+            self::AUTHORED_SHORT_URLS => Spec::andX(new BelongsToApiKeyInlined($role->apiKey())),
-                : new BelongsToDomain($domainId, $context);
+            self::DOMAIN_SPECIFIC => Spec::andX(new BelongsToDomainInlined(self::domainIdFromMeta($role->meta()))),
-        }
+            default => Spec::andX(),
-
+        };
-        return Spec::andX();
     }
 
     public static function domainIdFromMeta(array $meta): string

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

@@ -11,8 +11,11 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey;
 
 class WithApiKeySpecsEnsuringJoin extends BaseSpecification
 {
-    public function __construct(private ?ApiKey $apiKey, private string $fieldToJoin = 'shortUrls')
+    public function __construct(
-    {
+        private ?ApiKey $apiKey,
+        private string $fieldToJoin = 'shortUrls',
+        private bool $inlined = false,
+    ) {
         parent::__construct();
     }
 
@@ -20,7 +23,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->inlined ? $this->apiKey->inlinedSpec() : $this->apiKey->spec($this->fieldToJoin),
         );
     }
 }

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 'invalid role' => [new ApiKeyRole('invalid', [], $apiKey), Spec::andX()];
-        yield 'not inline invalid role' => [new ApiKeyRole('invalid', [], $apiKey), false, Spec::andX()];
+        yield 'author role' => [
-        yield 'inline 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' => [
+        yield 'domain role' => [
-            new ApiKeyRole(Role::DOMAIN_SPECIFIC, ['domain_id' => '123'], $apiKey),
-            true,
-            Spec::andX(new BelongsToDomainInlined('123')),
-        ];
-        yield 'not inline 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