[Cache] Fix regressions in Psr16Adapter, Psr16Cache

symfony · Apr 27, 2024 · fb44d0e · fb44d0e
1 parent ca1bc5e
commit fb44d0e
Show file tree

Hide file tree

Showing 4 changed files with 152 additions and 3 deletions.
diff --git a/src/Symfony/Component/Cache/Adapter/Psr16Adapter.php b/src/Symfony/Component/Cache/Adapter/Psr16Adapter.php
@@ -12,6 +12,7 @@
 namespace Symfony\Component\Cache\Adapter;
 
 use Psr\SimpleCache\CacheInterface;
+use Symfony\Component\Cache\CacheItem;
 use Symfony\Component\Cache\PruneableInterface;
 use Symfony\Component\Cache\ResettableInterface;
 use Symfony\Component\Cache\Traits\ProxyTrait;
@@ -40,6 +41,17 @@ public function __construct(CacheInterface $pool, string $namespace = '', int $d
         $this->miss = new \stdClass();
     }
 
+    public function getItem(mixed $key): CacheItem {
+        CacheItem::validateKey($key);
+        return parent::getItem($key);
+    }
+
+    public function getItems(array $keys = []): iterable
+    {
+        CacheItem::validateKeys($keys);
+        return parent::getItems($keys);
+    }
+
     protected function doFetch(array $ids): iterable
     {
         foreach ($this->pool->getMultiple($ids, $this->miss) as $key => $value) {
@@ -64,6 +76,11 @@ protected function doDelete(array $ids): bool
         return $this->pool->deleteMultiple($ids);
     }
 
+    public function deleteItems(array $keys): bool {
+        CacheItem::validateKeys($keys);
+        return parent::deleteItems($keys);
+    }
+
     protected function doSave(array $values, int $lifetime): array|bool
     {
         return $this->pool->setMultiple($values, 0 === $lifetime ? null : $lifetime);

diff --git a/src/Symfony/Component/Cache/CacheItem.php b/src/Symfony/Component/Cache/CacheItem.php
@@ -143,6 +143,16 @@ public static function validateKey($key, string $reservedChars = self::RESERVED_
         return $key;
     }
 
+    /**
+     * @param mixed[] $keys The keys to validate
+     */
+    public static function validateKeys(array $keys): void
+    {
+        foreach ($keys as $key) {
+            CacheItem::validateKey($key);
+        }
+    }
+
     /**
      * Internal logging helper.
      *

diff --git a/src/Symfony/Component/Cache/Psr16Cache.php b/src/Symfony/Component/Cache/Psr16Cache.php
@@ -47,7 +47,7 @@ static function ($key, $value, $allowInt = false) use (&$cacheItemPrototype) {
                 if ($allowInt && \is_int($key)) {
                     $item->key = (string) $key;
                 } else {
-                    \assert('' !== CacheItem::validateKey($key));
+                    CacheItem::validateKey($key);
                     $item->key = $key;
                 }
                 $item->value = $value;
@@ -79,6 +79,7 @@ static function (CacheItem $item) {
 
     public function get($key, $default = null): mixed
     {
+        CacheItem::validateKey($key);
         try {
             $item = $this->pool->getItem($key);
         } catch (SimpleCacheException $e) {
@@ -96,6 +97,7 @@ public function get($key, $default = null): mixed
 
     public function set($key, $value, $ttl = null): bool
     {
+        CacheItem::validateKey($key);
         try {
             if (null !== $f = $this->createCacheItem) {
                 $item = $f($key, $value);
@@ -117,6 +119,7 @@ public function set($key, $value, $ttl = null): bool
     public function delete($key): bool
     {
         try {
+            CacheItem::validateKey($key);
             return $this->pool->deleteItem($key);
         } catch (SimpleCacheException $e) {
             throw $e;
@@ -139,6 +142,7 @@ public function getMultiple($keys, $default = null): iterable
         }
 
         try {
+            CacheItem::validateKeys($keys);
             $items = $this->pool->getItems($keys);
         } catch (SimpleCacheException $e) {
             throw $e;
@@ -179,14 +183,17 @@ public function setMultiple($values, $ttl = null): bool
             } elseif ($valuesIsArray) {
                 $items = [];
                 foreach ($values as $key => $value) {
-                    $items[] = (string) $key;
+                    $key = (string) $key;
+                    CacheItem::validateKey($key);
+                    $items[] = $key;
                 }
                 $items = $this->pool->getItems($items);
             } else {
                 foreach ($values as $key => $value) {
                     if (\is_int($key)) {
                         $key = (string) $key;
                     }
+                    CacheItem::validateKey($key);
                     $items[$key] = $this->pool->getItem($key)->set($value);
                 }
             }
@@ -198,6 +205,7 @@ public function setMultiple($values, $ttl = null): bool
         $ok = true;
 
         foreach ($items as $key => $item) {
+            CacheItem::validateKey((string)$key);
             if ($valuesIsArray) {
                 $item->set($values[$key]);
             }
@@ -219,6 +227,7 @@ public function deleteMultiple($keys): bool
         }
 
         try {
+            CacheItem::validateKeys($keys);
             return $this->pool->deleteItems($keys);
         } catch (SimpleCacheException $e) {
             throw $e;
@@ -230,6 +239,7 @@ public function deleteMultiple($keys): bool
     public function has($key): bool
     {
         try {
+            CacheItem::validateKey($key);
             return $this->pool->hasItem($key);
         } catch (SimpleCacheException $e) {
             throw $e;

diff --git a/src/Symfony/Component/Cache/Tests/Adapter/Psr16AdapterTest.php b/src/Symfony/Component/Cache/Tests/Adapter/Psr16AdapterTest.php
@@ -11,10 +11,13 @@
 
 namespace Symfony\Component\Cache\Tests\Adapter;
 
+use Exception;
 use Psr\Cache\CacheItemPoolInterface;
+use Psr\Log\AbstractLogger;
 use Symfony\Component\Cache\Adapter\ArrayAdapter;
 use Symfony\Component\Cache\Adapter\FilesystemAdapter;
 use Symfony\Component\Cache\Adapter\Psr16Adapter;
+use Symfony\Component\Cache\Exception\InvalidArgumentException;
 use Symfony\Component\Cache\Psr16Cache;
 
 /**
@@ -29,9 +32,104 @@ class Psr16AdapterTest extends AdapterTestCase
         'testClearPrefix' => 'SimpleCache cannot clear by prefix',
     ];
 
+    private TestLogger $testLogger;
+
     public function createCachePool(int $defaultLifetime = 0): CacheItemPoolInterface
     {
-        return new Psr16Adapter(new Psr16Cache(new FilesystemAdapter()), '', $defaultLifetime);
+        $this->testLogger = new TestLogger();
+        $psr16Adapter = new Psr16Adapter(new Psr16Cache(new FilesystemAdapter()), '', $defaultLifetime);
+        $psr16Adapter->setLogger($this->testLogger);
+        return $psr16Adapter;
+    }
+
+    /**
+     * @dataProvider invalidKeys
+     */
+    public function testGetItemInvalidKeys($key)
+    {
+        if (isset($this->skippedTests[__FUNCTION__])) {
+            $this->markTestSkipped($this->skippedTests[__FUNCTION__]);
+        }
+
+        try {
+            $this->cache->getItem($key);
+        } catch (Exception $exception) {
+            $this->assertInstanceOf(InvalidArgumentException::class, $exception);
+
+            return;
+        }
+
+        $this->assertNotEmpty($this->testLogger->records);
+        $record = $this->testLogger->records[0];
+
+        $this->assertSame('warning', $record['level']);
+        $this->assertSame(sprintf('Failed to fetch key "{key}": Cache key "%s" contains reserved characters "{}()/\\@:".', $key), $record['message']);
+        $this->assertSame('Symfony\\Component\\Cache\\Adapter\\Psr16Adapter', $record['context']['cache-adapter']);
+        $this->assertSame($key, $record['context']['key']);
+
+        $exception = $record['context']['exception'];
+        $this->assertInstanceOf(InvalidArgumentException::class, $exception);
+        $this->assertSame(sprintf('Cache key "%s" contains reserved characters "{}()/\\@:".', $key), $exception->getMessage());
+    }
+
+    /**
+     * @dataProvider invalidKeys
+     */
+    public function testHasItemInvalidKeys($key)
+    {
+        if (isset($this->skippedTests[__FUNCTION__])) {
+            $this->markTestSkipped($this->skippedTests[__FUNCTION__]);
+        }
+
+        try {
+            $this->cache->hasItem($key);
+        } catch (Exception $exception) {
+            $this->assertInstanceOf(InvalidArgumentException::class, $exception);
+
+            return;
+        }
+
+        $this->assertNotEmpty($this->testLogger->records);
+        $record = $this->testLogger->records[0];
+
+        $this->assertSame('warning', $record['level']);
+        $this->assertSame(sprintf('Failed to check if key "{key}" is cached: Cache key "%s" contains reserved characters "{}()/\@:".', $key), $record['message']);
+        $this->assertSame('Symfony\\Component\\Cache\\Adapter\\Psr16Adapter', $record['context']['cache-adapter']);
+        $this->assertSame($key, $record['context']['key']);
+
+        $exception = $record['context']['exception'];
+        $this->assertInstanceOf(InvalidArgumentException::class, $exception);
+        $this->assertSame(sprintf('Cache key "%s" contains reserved characters "{}()/\\@:".', $key), $exception->getMessage());
+    }
+
+    /**
+     * @dataProvider invalidKeys
+     */
+    public function testDeleteItemInvalidKeys($key)
+    {
+        if (isset($this->skippedTests[__FUNCTION__])) {
+            $this->markTestSkipped($this->skippedTests[__FUNCTION__]);
+        }
+
+        try {
+            $this->cache->deleteItem($key);
+        } catch (Exception $exception) {
+            $this->assertInstanceOf(InvalidArgumentException::class, $exception);
+
+            return;
+        }
+
+        $this->assertNotEmpty($this->testLogger->records);
+        $record = $this->testLogger->records[0];
+
+        $this->assertSame('warning', $record['level']);
+        $this->assertSame(sprintf('Failed to delete key "{key}": Cache key "%s" contains reserved characters "{}()/\@:".', $key), $record['message']);
+        $this->assertSame('Symfony\\Component\\Cache\\Adapter\\Psr16Adapter', $record['context']['cache-adapter']);
+        $this->assertSame($key, $record['context']['key']);
+
+        $exception = $record['context']['exception'];
+        $this->assertInstanceOf(InvalidArgumentException::class, $exception);
+        $this->assertSame(sprintf('Cache key "%s" contains reserved characters "{}()/\\@:".', $key), $exception->getMessage());
     }
 
     public function testValidCacheKeyWithNamespace()
@@ -44,3 +142,17 @@ public function testValidCacheKeyWithNamespace()
         $this->assertTrue($cache->getItem('my_key')->isHit(), 'Stored item is successfully retrieved.');
     }
 }
+
+final class TestLogger extends AbstractLogger
+{
+    public array $records = [];
+
+    public function log($level, $message, array $context = []): void
+    {
+        $this->records[] = [
+            'level' => $level,
+            'message' => $message,
+            'context' => $context,
+        ];
+    }
+}