Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalidate old query cache format #6510

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions src/Cache/ArrayResult.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
use Doctrine\DBAL\Exception\InvalidColumnIndex;

use function array_combine;
use function array_keys;
use function array_map;
use function array_values;
use function count;

/** @internal The class is internal to the caching layer implementation. */
Expand All @@ -21,8 +24,10 @@ final class ArrayResult implements Result
* @param list<list<mixed>> $rows The rows of the result. Each row must have the same number of columns
* as the number of column names.
*/
public function __construct(private readonly array $columnNames, private array $rows)
{
public function __construct(
private readonly array $columnNames,
private array $rows,
) {
}

public function fetchNumeric(): array|false
Expand Down Expand Up @@ -96,6 +101,29 @@ public function free(): void
$this->rows = [];
}

/** @return array{list<string>, list<list<mixed>>} */
public function __serialize(): array
{
return [$this->columnNames, $this->rows];
}

/** @param mixed[] $data */
public function __unserialize(array $data): void
{
// Handle objects serialized with DBAL 4.1 and earlier.
if (isset($data["\0" . self::class . "\0data"])) {
/** @var list<array<string, mixed>> $legacyData */
$legacyData = $data["\0" . self::class . "\0data"];

$this->columnNames = array_keys($legacyData[0] ?? []);
$this->rows = array_map(array_values(...), $legacyData);

return;
}

[$this->columnNames, $this->rows] = $data;
}

/** @return list<mixed>|false */
private function fetch(): array|false
{
Expand Down
10 changes: 4 additions & 6 deletions src/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -810,10 +810,8 @@ public function executeCacheQuery(string $sql, array $params, array $types, Quer
$value = [];
}

if (isset($value[$realKey])) {
[$columnNames, $rows] = $value[$realKey];

return new Result(new ArrayResult($columnNames, $rows), $this);
if (isset($value[$realKey]) && $value[$realKey] instanceof ArrayResult) {
return new Result($value[$realKey], $this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes made here in Connection introduced a BC Break

Before this PR, the values in the cache were uncoupled from the ArrayResult.
Since this PR, the cache and the Result/ArrayResult are now coupled: hitting the \Doctrine\DBAL\Result::free method erases all the cache content as well 😱

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please file a bug report with clear steps to reproduce.

}
} else {
$value = [];
Expand All @@ -828,7 +826,7 @@ public function executeCacheQuery(string $sql, array $params, array $types, Quer

$rows = $result->fetchAllNumeric();

$value[$realKey] = [$columnNames, $rows];
$value[$realKey] = new ArrayResult($columnNames, $rows);

$item->set($value);

Expand All @@ -839,7 +837,7 @@ public function executeCacheQuery(string $sql, array $params, array $types, Quer

$resultCache->save($item);

return new Result(new ArrayResult($columnNames, $rows), $this);
return new Result($value[$realKey], $this);
}

/**
Expand Down
66 changes: 66 additions & 0 deletions tests/Cache/ArrayResultTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,15 @@

use Doctrine\DBAL\Cache\ArrayResult;
use Doctrine\DBAL\Exception\InvalidColumnIndex;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\TestWith;
use PHPUnit\Framework\TestCase;

use function assert;
use function file_get_contents;
use function serialize;
use function unserialize;

class ArrayResultTest extends TestCase
{
private ArrayResult $result;
Expand Down Expand Up @@ -105,4 +111,64 @@ public function testSameColumnNames(): void

self::assertEquals([1, 2], $result->fetchNumeric());
}

public function testSerialize(): void
{
$result = unserialize(serialize($this->result));

self::assertSame([
[
'username' => 'jwage',
'active' => true,
],
[
'username' => 'romanb',
'active' => false,
],
], $result->fetchAllAssociative());

self::assertSame(2, $result->columnCount());
self::assertSame('username', $result->getColumnName(0));
}

public function testRowPointerIsNotSerialized(): void
{
$this->result->fetchAssociative();
$result = unserialize(serialize($this->result));

self::assertSame([
'username' => 'jwage',
'active' => true,
], $result->fetchAssociative());
}

#[DataProvider('provideSerializedResultFiles')]
public function testUnserialize(string $file): void
{
$serialized = file_get_contents($file);
assert($serialized !== false);
$result = unserialize($serialized);

self::assertInstanceOf(ArrayResult::class, $result);
self::assertSame([
[
'username' => 'jwage',
'active' => true,
],
[
'username' => 'romanb',
'active' => false,
],
], $result->fetchAllAssociative());

self::assertSame(2, $result->columnCount());
self::assertSame('username', $result->getColumnName(0));
}

/** @return iterable<string, array{string}> */
public static function provideSerializedResultFiles(): iterable
{
yield '4.1 format' => [__DIR__ . '/Fixtures/array-result-4.1.txt'];
yield '4.2 format' => [__DIR__ . '/Fixtures/array-result-4.2.txt'];
}
}
Binary file added tests/Cache/Fixtures/array-result-4.1.txt
Binary file not shown.
1 change: 1 addition & 0 deletions tests/Cache/Fixtures/array-result-4.2.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
O:31:"Doctrine\DBAL\Cache\ArrayResult":2:{i:0;a:2:{i:0;s:8:"username";i:1;s:6:"active";}i:1;a:2:{i:0;a:2:{i:0;s:5:"jwage";i:1;b:1;}i:1;a:2:{i:0;s:6:"romanb";i:1;b:0;}}}
23 changes: 20 additions & 3 deletions tests/Connection/CachedQueryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,25 @@ public function testCachedQueryWithChangedImplementationIsExecutedTwice(): void
)->fetchAllAssociative());
}

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

[$cacheKey, $realKey] = $qcp->generateCacheKeys('SELECT 1', [], [], []);
$cache->save(
$cache->getItem($cacheKey)->set([$realKey => [['foo' => 'bar']]]),
);

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

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

/**
* @param list<string> $columnNames
* @param list<list<mixed>> $rows
Expand All @@ -56,9 +75,7 @@ private function createConnection(int $expectedQueryCount, array $columnNames, a
$connection = $this->createMock(Driver\Connection::class);
$connection->expects(self::exactly($expectedQueryCount))
->method('query')
->willReturnCallback(static function () use ($columnNames, $rows): ArrayResult {
return new ArrayResult($columnNames, $rows);
});
->willReturnCallback(static fn (): ArrayResult => new ArrayResult($columnNames, $rows));

$driver = $this->createMock(Driver::class);
$driver->method('connect')
Expand Down
Loading