Add cache-aside helper#900
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR replaces
Confidence Score: 3/5The change needs the removed authorization guard addressed before merging — callers can silently cache authorization-filtered rows and serve them to users with different permissions. The old src/Database/Database.php — specifically the
|
| Filename | Overview |
|---|---|
| src/Database/Database.php | Replaces findCached with withCache/withCachedPayload; removes the authorization-active guard that prevented caching user-specific query results, shifting that responsibility entirely to callers without documenting the contract. |
| tests/unit/ListCacheTest.php | Replaces integration-style findCached tests with focused unit tests for withCache; covers miss/hit, empty array, null, hash field separation, and false-value non-caching. touchOnHit branch has zero coverage (flagged in prior thread). |
Reviews (7): Last reviewed commit: "Add cache-aside helper" | Re-trigger Greptile
| public function testWithCacheReturnsValidatedCachedValue(): void | ||
| { | ||
| $cache = new HashMemoryCache(); | ||
| $database = $this->createDatabase($cache); | ||
| $cache->save('key', ['value' => 'cached'], 'field'); | ||
|
|
||
| $callbackCalls = 0; | ||
| $hitPayload = null; | ||
|
|
||
| $value = $database->withCache( | ||
| key: 'key', | ||
| callback: function () use (&$callbackCalls): array { | ||
| $callbackCalls++; | ||
| return ['value' => 'fresh']; | ||
| }, | ||
| ttl: 3600, | ||
| hash: 'field', | ||
| fromCache: static fn (mixed $cached): array|false => \is_array($cached) ? $cached : false, | ||
| onCacheHit: static function (array $value, mixed $cached) use (&$hitPayload): void { | ||
| $hitPayload = [$value, $cached]; | ||
| }, | ||
| ); | ||
|
|
||
| $this->assertSame(['value' => 'cached'], $value); | ||
| $this->assertSame(0, $callbackCalls); | ||
| $this->assertSame([['value' => 'cached'], ['value' => 'cached']], $hitPayload); | ||
| } | ||
|
|
||
| public function testWithCacheRefreshesRejectedCachedValue(): void | ||
| { | ||
| $cache = new HashMemoryCache(); | ||
| $database = $this->createDatabase($cache); | ||
| $cache->save('key', ['invalid' => true], 'field'); | ||
|
|
||
| $callbackCalls = 0; | ||
|
|
||
| $value = $database->withCache( | ||
| key: 'key', | ||
| callback: function () use (&$callbackCalls): array { | ||
| $callbackCalls++; | ||
| return ['valid' => true]; | ||
| }, | ||
| ttl: 3600, | ||
| hash: 'field', | ||
| fromCache: static fn (mixed $cached): array|false => \is_array($cached) && ($cached['valid'] ?? false) === true ? $cached : false, | ||
| toCache: static fn (array $value): array => $value, | ||
| ); | ||
|
|
||
| $this->assertSame(['valid' => true], $value); | ||
| $this->assertSame(1, $callbackCalls); | ||
|
|
||
| $value = $database->withCache( | ||
| key: 'key', | ||
| callback: function () use (&$callbackCalls): array { | ||
| $callbackCalls++; | ||
| return ['valid' => false]; | ||
| }, | ||
| ttl: 3600, | ||
| hash: 'field', | ||
| fromCache: static fn (mixed $cached): array|false => \is_array($cached) && ($cached['valid'] ?? false) === true ? $cached : false, | ||
| ); | ||
|
|
||
| $this->assertSame(['valid' => true], $value); | ||
| $this->assertSame(1, $callbackCalls); | ||
| } |
There was a problem hiding this comment.
touchOnHit path is not covered by any test
The new $touchOnHit parameter triggers $this->cache->touch() on a cache hit and is the only branch in withCache with zero test coverage. A test that verifies the TTL is extended (i.e., a near-expired entry is still returned after a touch) would confirm the feature works as advertised and guard against regressions if touch logic or the cache adapter changes.
f4085bc to
29f1628
Compare
29f1628 to
4af25ad
Compare
Summary
Database::withCache(string $key, callable $callback, string $hash = '')as the public cache-aside API for keyed callback executionhashoptional: callers can omit it for a single cached value, or pass it to keep multiple field-level payloads under one cache keynullcan be cached while literalfalseremains the cache-miss sentinelwithCache()find()flows without a dedicatedfindCached()APInull, hash-field separation, and non-cacheablefalsevaluesFlow
withCache()is the generic cache boundary. For a single cached value, callers only needkeyandcallback:When callers need field-level cache entries under the same key, they can pass the optional
hashfield:Callers that need cached
find()results compose the key and field explicitly, and return the cache-safe shape they want from the callback:This keeps
withCache()reusable for reads and writes while leaving find-specific query and payload decisions at the call site.Cache Behavior
withCache()reads and writes through the configured Utopia cache adapter using the provided key and optional hash field. If the hash is omitted, cache adapters keep their existing behavior and use the key as the hash field.The helper stores callback results in an internal
valuewrapper. That lets callers cache empty arrays andnullwithout those values being confused with cache misses. A literalfalsecallback result is not cached becausefalseis the existing cache miss sentinel.For cached find-style flows,
getFindCacheKey()builds the collection-level key andgetFindCacheField()builds the query-specific hash field. The field includes schema, roles, query state, and field type so different query shapes do not collide.Tests
composer lintcomposer formatvendor/bin/phpunit --configuration phpunit.xml tests/unit/ListCacheTest.php tests/unit/CacheKeyTest.phpvendor/bin/phpstan analyse --level 7 src/Database/Database.php tests/unit/ListCacheTest.php tests/unit/CacheKeyTest.php --memory-limit 2Ggit diff --check