Add list cache support#894
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a generic ChangesCache-aside helper and find cache-key infrastructure
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Cache as Cache Adapter
participant Callback
Client->>Database: cachedFind(collection, queries)
Database->>Database: getFindCacheKey()
Database->>Database: getFindCacheField()
Database->>Cache: load(key, hash)
alt Cache hit and value exists
Cache-->>Database: cached document arrays
else Cache miss or false
Cache-->>Database: null
Database->>Callback: invoke callback via withCache()
Callback->>Database: find() results
end
Database->>Database: recreate documents + authorize
Database-->>Client: documents
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR introduces a
Confidence Score: 5/5The change is additive and the new public methods are well-covered by a comprehensive test suite. The new withCache, getQueryCacheKey, getQueryCacheField, and purgeCachedQueries methods handle all edge cases verified by the 1,034-line test file. The only findings are minor inconsistencies that do not affect correctness in expected usage patterns. The withCache method in src/Database/Database.php is the most complex new addition; in particular the shouldRefreshCache purge path and schema-hash concatenation. Important Files Changed
Reviews (47): Last reviewed commit: "Merge branch 'main' into feat/cached-fin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Database/Database.php`:
- Around line 8625-8642: The cache hit path in the conditional block starting
with if ($cached !== null && $cached !== false && \is_array($cached)) only wraps
cached arrays into Document instances using createDocumentInstance, but does not
hydrate relationship fields that may have been serialized as arrays. This
results in different document shapes being returned compared to the cache miss
path which uses find() that returns fully processed documents. Either ensure the
cached hit path recursively hydrates relationship fields to match the shape
returned by find(), or add logic to bypass caching for finds that request
relationship-resolved data to avoid returning inconsistent types between cache
hits and misses.
- Around line 9563-9564: The cache key generation logic in the findCached method
is incorrectly dropping the serialized queries when a $key is provided. Change
the ternary expression that sets the queries field to always call
serializeFindCacheQueries($queries) instead of returning null when $key is set.
The caller key should add an additional cache dimension alongside the query
fingerprint, not replace it, so both the serialized queries and the key must be
included in the cache identifier to prevent different queries with the same key
from returning incorrect cached results.
- Around line 9572-9584: The getFindCacheSchemaHash method currently only
includes collection attributes and indexes in the hash calculation, but the
find() authorization check also validates collection permissions and
documentSecurity settings. When collection authorization policies change, cached
results remain valid incorrectly because the schema hash doesn't reflect these
changes. Modify the getFindCacheSchemaHash method to include the collection's
permissions and documentSecurity flag in the hash payload alongside the existing
attributes and indexes. Additionally, replace the MD5 hash algorithm with a
stronger digest function such as SHA256 to address the static analysis flag
regarding weak hashing.
In `@tests/unit/FindCacheTest.php`:
- Around line 140-144: The `HashMemoryCache::save` method incorrectly prevents
caching of empty arrays because the condition `empty($data)` returns true for
empty arrays and rejects them. Modify the validation logic to only reject empty
strings while allowing empty arrays to be cached as valid results. Change the
condition to specifically check if `$data` is an empty string or if `$key` is
empty, rather than using `empty()` which treats empty arrays the same as empty
strings.
- Around line 83-99: The test testFindCachedTriggersFindEventOnCacheHit does not
actually verify a cache hit because both cache hits and cache misses emit
EVENT_DOCUMENT_FIND. To fix this, add data mutation between the two findCached
calls to force a true cache hit scenario. After the first findCached call on
line 92, modify the seeded project data in the database, then verify that the
second findCached call on line 93 returns the stale cached result from the first
call rather than the modified data, which would definitively prove the second
call was a cache hit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0eed82b-9eca-489f-a0fd-19748bc2694c
📒 Files selected for processing (4)
src/Database/Database.phptests/e2e/Adapter/Scopes/DocumentTests.phptests/unit/CacheKeyTest.phptests/unit/FindCacheTest.php
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Database.php (2)
8611-8617:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate queries before serializing the cache key.
findCached()reachesserializeQueriesForFindCache()beforefind()runscheckQueryTypes(), so malformed query arrays can fail outside the normalQueryExceptionpath on cached calls.Proposed fix
if ($collectionDocument->isEmpty()) { throw new NotFoundException('Collection not found'); } + $this->checkQueryTypes($queries); + [$findKey, $findField] = $this->getFindCacheKeys($collectionDocument->getId(), $queries, $key, $forPermission, $collectionDocument);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Database.php` around lines 8611 - 8617, In the findCached() method, validate the queries before they are serialized for the cache key. Call checkQueryTypes() on the queries parameter before invoking getFindCacheKeys(), which calls serializeQueriesForFindCache(). This ensures malformed query arrays are caught early with proper QueryException handling rather than failing during serialization.
8617-8639:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid serving document-security caches after permission revocation.
The cache hit path returns serialized documents without re-reading current document permissions. If a cached document later loses
$permissions, the same role can still receive it until the find TTL expires. Until permission-changing writes purge affected find variants, bypass caching fordocumentSecuritycollections.Conservative fix
if ($collectionDocument->isEmpty()) { throw new NotFoundException('Collection not found'); } + if ($collectionDocument->getAttribute('documentSecurity', false)) { + return $this->find($collectionDocument->getId(), $queries, $forPermission); + } + [$findKey, $findField] = $this->getFindCacheKeys($collectionDocument->getId(), $queries, $key, $forPermission, $collectionDocument);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Database.php` around lines 8617 - 8639, The current caching logic in the find method returns cached documents without re-validating permissions, which creates a security vulnerability for documentSecurity collections where permissions may have been revoked. Add a check using the $collectionDocument to determine if the collection is a documentSecurity collection, and if so, skip the entire cache loading and cache return logic (the try-catch block for $this->cache->load() and the conditional check for $cached !== null) to force a fresh permission check on every query. This ensures that permission changes are respected immediately rather than serving stale cached results until the TTL expires.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Database/Database.php`:
- Around line 8611-8617: In the findCached() method, validate the queries before
they are serialized for the cache key. Call checkQueryTypes() on the queries
parameter before invoking getFindCacheKeys(), which calls
serializeQueriesForFindCache(). This ensures malformed query arrays are caught
early with proper QueryException handling rather than failing during
serialization.
- Around line 8617-8639: The current caching logic in the find method returns
cached documents without re-validating permissions, which creates a security
vulnerability for documentSecurity collections where permissions may have been
revoked. Add a check using the $collectionDocument to determine if the
collection is a documentSecurity collection, and if so, skip the entire cache
loading and cache return logic (the try-catch block for $this->cache->load() and
the conditional check for $cached !== null) to force a fresh permission check on
every query. This ensures that permission changes are respected immediately
rather than serving stale cached results until the TTL expires.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29c2ae6a-250e-4626-8ae6-59effc86c769
📒 Files selected for processing (5)
src/Database/Database.phptests/e2e/Adapter/Base.phptests/e2e/Adapter/RedisTest.phptests/e2e/Adapter/Scopes/DocumentTests.phptests/unit/FindCacheTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/Adapter/Base.php
- tests/e2e/Adapter/RedisTest.php
- tests/e2e/Adapter/Scopes/DocumentTests.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Database/Database.php`:
- Line 8659: The condition on line 8659 checks `if ($touchOnHit &&
!$hasExpiredDocuments)`, but the `!$hasExpiredDocuments` part is redundant
because the code already guarantees this value through an early return statement
in the preceding lines (8646-8657). Remove the `&& !$hasExpiredDocuments` clause
from the condition, leaving only `if ($touchOnHit)`, to eliminate the
always-true condition that is causing the PHPStan error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 647e586e-bf40-4233-b042-568a6b0b5436
📒 Files selected for processing (4)
src/Database/Database.phptests/e2e/Adapter/Scopes/DocumentTests.phptests/unit/CacheKeyTest.phptests/unit/FindCacheTest.php
💤 Files with no reviewable changes (1)
- tests/unit/CacheKeyTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/Adapter/Scopes/DocumentTests.php
- tests/unit/FindCacheTest.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Database/Database.php`:
- Around line 8608-8615: The cachedFind method caches document payloads without
filtering TTL-expired documents, causing expired documents to remain visible
until the cache entry expires. Filter out TTL-expired documents in two places
within cachedFind: first, in the callback function shown (after the find call
returns documents but before they are array-mapped and stored in cache), and
second, in the cache hit scenario (around lines 8635-8647, before returning the
cached payload). Use the isTtlExpired method to identify and exclude expired
documents from both the cache miss result being saved and the cache hit payload
being returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc6fd113-7a02-40de-8fe9-c1c00576e805
📒 Files selected for processing (3)
src/Database/Database.phptests/unit/CacheKeyTest.phptests/unit/ListCacheTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/CacheKeyTest.php
abnegate
reviewed
Jun 23, 2026
| * @param string|null $namespace | ||
| * @return bool | ||
| */ | ||
| public function purgeQueryCache(string $collection, ?string $namespace = null): bool |
Member
There was a problem hiding this comment.
Let's call this purgeCachedQueries for consistency with the other purge methods
abnegate
approved these changes
Jun 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Database::withCache()as a generic cache-aside helper that keeps caller usage tokey,callback, and optionalhashwithCache():Documentvalues are stored as arrays and restored on cache hitsDocument[]values are stored as arrays and restored on cache hitsDocumentpayloads are not cached, so the callback shape is preservednullfromgetQueryCacheField()for non-read permission modes so callers can bypass cache and run the callback directlygetQueryCacheKey()getQueryCacheField()purgeQueryCache()Query::orderRandom()cache bypass as caller policy: callers should skipwithCache()for random orderingUsage
Callers cache read query-derived values by composing the Database-owned key helpers and
withCache():For variable permission modes, bypass cache when the field is
null:Invalidate cached query fields for the same collection namespace after writes with:
For scalar values such as
count,sum, or plain arrays, callers use the samewithCache()API. Document payload serialization and restoration are internal.Cache Shape
Query-cache entries use:
The hash field is:
The query-state hash includes the active authorization roles, database id, query serialization, relationship state, and active filter signatures. This follows Appwrite's list-cache TTL model: cached query entries are read-oriented and role-scoped, so callers with different authorization contexts do not share entries.
The cached value envelope is:
or, for documents:
[ 'collection' => 'wafRules', 'type' => 'document' | 'documents', 'value' => $payload, ]Tests
composer formatcomposer lintvendor/bin/phpunit tests/unit/QueryCacheTest.php tests/unit/CacheKeyTest.phpcomposer checkis currently blocked locally by the existingConnection.phpPHPStan issue whereSwoole\\Database\\DetectsLostConnectionsis not discovered by the local analyzer setup.