Skip to content

Commit 5608004

Browse files
abnegateabnegate
authored
Merge pull request #888 from utopia-php/fix/for-update-cache-bypass
Bypass cache for locking document reads
2 parents 2ae2e6c + 8e59397 commit 5608004

2 files changed

Lines changed: 123 additions & 7 deletions

File tree

‎src/Database/Database.php‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4840,16 +4840,26 @@ public function getDocument(string $collection, string $id, array $queries = [],
48404840
$selections
48414841
);
48424842

4843-
try {
4844-
$cached = $this->cache->load($documentKey, self::TTL, $hashKey);
4845-
} catch (Exception $e) {
4846-
Console::warning('Warning: Failed to get document from cache: ' . $e->getMessage());
4847-
$cached = null;
4843+
// A locking read must observe the current row, not a cached copy:
4844+
// updateDocument merges the changes into this read and writes the result
4845+
// back, so serving it from a stale cache would persist the staleness.
4846+
$cached = null;
4847+
if (!$forUpdate) {
4848+
try {
4849+
$cached = $this->cache->load($documentKey, self::TTL, $hashKey);
4850+
} catch (Exception $e) {
4851+
Console::warning('Warning: Failed to get document from cache: ' . $e->getMessage());
4852+
}
48484853
}
48494854

48504855
if ($cached) {
48514856
$document = $this->createDocumentInstance($collection->getId(), $cached);
48524857

4858+
// JSON serialization in cache backends collapses floats with zero
4859+
// fractions to ints. Re-cast so cached and freshly-loaded documents
4860+
// compare equal under strict equality (e.g. in updateDocument).
4861+
$document = $this->casting($collection, $document);
4862+
48534863
if ($collection->getId() !== self::METADATA) {
48544864

48554865
if (!$this->authorization->isValid(new Input(self::PERMISSION_READ, [
@@ -4916,8 +4926,10 @@ public function getDocument(string $collection, string $id, array $queries = [],
49164926
fn ($attribute) => $attribute['type'] === Database::VAR_RELATIONSHIP
49174927
);
49184928

4919-
// Don't save to cache if it's part of a relationship
4920-
if (empty($relationships)) {
4929+
// Don't save to cache if it's part of a relationship, or if this is a
4930+
// locking read: a forUpdate read happens inside an open transaction, and
4931+
// caching the pre-commit row would poison the cache for other readers.
4932+
if (!$forUpdate && empty($relationships)) {
49214933
try {
49224934
$this->cache->save($documentKey, $document->getArrayCopy(), $hashKey);
49234935
$this->cache->save($collectionKey, 'empty', $documentKey);

‎tests/unit/ForUpdateCacheTest.php‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
<?php
2+
3+
namespace Tests\Unit;
4+
5+
use PHPUnit\Framework\TestCase;
6+
use Utopia\Cache\Adapter\Memory as CacheMemory;
7+
use Utopia\Cache\Cache;
8+
use Utopia\Database\Adapter\Memory as DatabaseMemory;
9+
use Utopia\Database\Database;
10+
use Utopia\Database\Document;
11+
use Utopia\Database\Helpers\Permission;
12+
use Utopia\Database\Helpers\Role;
13+
14+
class ForUpdateCacheTest extends TestCase
15+
{
16+
private DatabaseMemory $adapter;
17+
18+
private Database $database;
19+
20+
protected function setUp(): void
21+
{
22+
$this->adapter = new DatabaseMemory();
23+
$this->database = new Database($this->adapter, new Cache(new CacheMemory()));
24+
$this->database
25+
->setDatabase('utopiaTests')
26+
->setNamespace('for_update_' . \uniqid());
27+
28+
$this->database->create();
29+
$this->database->createCollection('projects');
30+
$this->database->createAttribute('projects', 'name', Database::VAR_STRING, 255, false);
31+
$this->database->createAttribute('projects', 'description', Database::VAR_STRING, 255, false);
32+
$this->database->createDocument('projects', new Document([
33+
'$id' => 'project',
34+
'$permissions' => [
35+
Permission::read(Role::any()),
36+
Permission::update(Role::any()),
37+
],
38+
'name' => 'stale',
39+
'description' => 'stale',
40+
]));
41+
}
42+
43+
/**
44+
* Write to the row through the adapter, bypassing Database and therefore
45+
* the cache purge — the cache now holds a stale copy, exactly as if a
46+
* concurrent reader had re-cached the old row after a writer's purge.
47+
*/
48+
private function staleCache(string $attribute, string $value): void
49+
{
50+
$collection = $this->database->getCollection('projects');
51+
$document = $this->adapter->getDocument($collection, 'project');
52+
$document->setAttribute($attribute, $value);
53+
$this->adapter->updateDocument($collection, 'project', $document, true);
54+
}
55+
56+
public function testForUpdateReadBypassesStaleCache(): void
57+
{
58+
$cached = $this->database->getDocument('projects', 'project');
59+
$this->assertSame('stale', $cached->getAttribute('name'));
60+
61+
$this->staleCache('name', 'fresh');
62+
63+
$cached = $this->database->getDocument('projects', 'project');
64+
$this->assertSame('stale', $cached->getAttribute('name'));
65+
66+
$fresh = $this->database->getDocument('projects', 'project', forUpdate: true);
67+
$this->assertSame('fresh', $fresh->getAttribute('name'));
68+
}
69+
70+
public function testForUpdateReadDoesNotPopulateCache(): void
71+
{
72+
$this->database->getDocument('projects', 'project');
73+
$this->staleCache('name', 'fresh');
74+
75+
$this->database->getDocument('projects', 'project', forUpdate: true);
76+
77+
// The locking read happens inside an open transaction; caching what it
78+
// saw would publish a pre-commit row. The cached copy must be untouched.
79+
$cached = $this->database->getDocument('projects', 'project');
80+
$this->assertSame('stale', $cached->getAttribute('name'));
81+
}
82+
83+
public function testUpdateDocumentDoesNotResurrectStaleCachedAttributes(): void
84+
{
85+
$this->database->getDocument('projects', 'project');
86+
$this->staleCache('name', 'fresh');
87+
88+
// updateDocument merges the changes into its locking read of the current
89+
// row. Served from the stale cache, that merge would durably revert
90+
// name to 'stale' — the lost-update behind flaky project config tests.
91+
$this->database->updateDocument('projects', 'project', new Document([
92+
'description' => 'updated',
93+
]));
94+
95+
$collection = $this->database->getCollection('projects');
96+
$row = $this->adapter->getDocument($collection, 'project');
97+
$this->assertSame('fresh', $row->getAttribute('name'));
98+
$this->assertSame('updated', $row->getAttribute('description'));
99+
100+
$document = $this->database->getDocument('projects', 'project');
101+
$this->assertSame('fresh', $document->getAttribute('name'));
102+
$this->assertSame('updated', $document->getAttribute('description'));
103+
}
104+
}

0 commit comments

Comments
 (0)