Skip to content

Commit

Permalink
[Bug] Query Cache mangled if saved by-reference (doctrine#6552)
Browse files Browse the repository at this point in the history
Bug emerged in
doctrine#6510 (comment)

The current implementation of query cache relies on the cache **not**
saved by-reference; the bug has never been seen before because by
default `new ArrayAdapter()` saves the cache with serialization, hence
breaking the by-reference pointer.

Once the _by-reference_ tecnique is used, two issues pop up:

1. `\Doctrine\DBAL\Cache\ArrayResult::$num` is never reset, so once it
gets incremented in the first `\Doctrine\DBAL\Cache\ArrayResult::fetch`
call, the following calls will always fail
2. Even considering fixing the `$num` property reset, a manual call on
`\Doctrine\DBAL\Result::free` will by cascade call the
`\Doctrine\DBAL\Cache\ArrayResult::free` method erasing all the saved
results

I think that the `ArrayResult` implementation is not the culprit, but
rather the doctrine#6510 giving to the cache backend the internal object by
reference instead of giving it a copy.
  • Loading branch information
Slamdunk authored Oct 21, 2024
1 parent 173d1c6 commit 8f60369
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 10 deletions.
4 changes: 2 additions & 2 deletions src/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,7 @@ public function executeCacheQuery(string $sql, array $params, array $types, Quer
}

if (isset($value[$realKey]) && $value[$realKey] instanceof ArrayResult) {
return new Result($value[$realKey], $this);
return new Result(clone $value[$realKey], $this);
}
} else {
$value = [];
Expand All @@ -837,7 +837,7 @@ public function executeCacheQuery(string $sql, array $params, array $types, Quer

$resultCache->save($item);

return new Result($value[$realKey], $this);
return new Result(clone $value[$realKey], $this);
}

/**
Expand Down
36 changes: 28 additions & 8 deletions tests/Connection/CachedQueryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,49 +8,60 @@
use Doctrine\DBAL\Cache\QueryCacheProfile;
use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Driver;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\TestCase;
use Psr\Cache\CacheItemPoolInterface;
use Symfony\Component\Cache\Adapter\ArrayAdapter;

class CachedQueryTest extends TestCase
{
public function testCachedQuery(): void
#[DataProvider('providePsrCacheImplementations')]
public function testCachedQuery(callable $psrCacheProvider): void
{
$cache = new ArrayAdapter();
$cache = $psrCacheProvider();

$connection = $this->createConnection(1, ['foo'], [['bar']]);
$qcp = new QueryCacheProfile(0, __FUNCTION__, $cache);

self::assertSame([['foo' => 'bar']], $connection->executeCacheQuery('SELECT 1', [], [], $qcp)
$firstResult = $connection->executeCacheQuery('SELECT 1', [], [], $qcp);
self::assertSame([['foo' => 'bar']], $firstResult
->fetchAllAssociative());
$firstResult->free();
$secondResult = $connection->executeCacheQuery('SELECT 1', [], [], $qcp);
self::assertSame([['foo' => 'bar']], $secondResult
->fetchAllAssociative());
$secondResult->free();
self::assertSame([['foo' => 'bar']], $connection->executeCacheQuery('SELECT 1', [], [], $qcp)
->fetchAllAssociative());

self::assertCount(1, $cache->getItem(__FUNCTION__)->get());
}

public function testCachedQueryWithChangedImplementationIsExecutedTwice(): void
#[DataProvider('providePsrCacheImplementations')]
public function testCachedQueryWithChangedImplementationIsExecutedTwice(callable $psrCacheProvider): void
{
$connection = $this->createConnection(2, ['baz'], [['qux']]);

self::assertSame([['baz' => 'qux']], $connection->executeCacheQuery(
'SELECT 1',
[],
[],
new QueryCacheProfile(0, __FUNCTION__, new ArrayAdapter()),
new QueryCacheProfile(0, __FUNCTION__, $psrCacheProvider()),
)->fetchAllAssociative());

self::assertSame([['baz' => 'qux']], $connection->executeCacheQuery(
'SELECT 1',
[],
[],
new QueryCacheProfile(0, __FUNCTION__, new ArrayAdapter()),
new QueryCacheProfile(0, __FUNCTION__, $psrCacheProvider()),
)->fetchAllAssociative());
}

public function testOldCacheFormat(): void
#[DataProvider('providePsrCacheImplementations')]
public function testOldCacheFormat(callable $psrCacheProvider): void
{
$connection = $this->createConnection(1, ['foo'], [['bar']]);
$cache = new ArrayAdapter();
$cache = $psrCacheProvider();
$qcp = new QueryCacheProfile(0, __FUNCTION__, $cache);

[$cacheKey, $realKey] = $qcp->generateCacheKeys('SELECT 1', [], [], []);
Expand Down Expand Up @@ -83,4 +94,13 @@ private function createConnection(int $expectedQueryCount, array $columnNames, a

return new Connection([], $driver);
}

/** @return array<non-empty-string, list<callable():CacheItemPoolInterface>> */
public static function providePsrCacheImplementations(): array
{
return [
'serialized' => [static fn () => new ArrayAdapter(0, true)],
'by-reference' => [static fn () => new ArrayAdapter(0, false)],
];
}
}

0 comments on commit 8f60369

Please sign in to comment.