Add cached find support#894
Conversation
|
Warning Review limit reached
More reviews will be available in 41 minutes and 57 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds ChangesfindCached Caching Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR introduces
Confidence Score: 4/5Safe to merge for callers that treat findCached() results as opaque blobs, but callers that read typed attributes or nested relationship objects from cache hits can receive different PHP types or plain arrays where fresh find() would return Document instances. The cache-hit reconstruction path in decodeFindCachePayload() skips the castingAfter/casting/decode pipeline that find() applies to every result, so float attributes serialized as integers in JSON and nested relationship documents serialized as arrays are returned as-is to callers on every cache hit. This is a real behavioral difference between the first (cold) call and subsequent (warm) calls for the same query. The rest of the new logic — cursor payload serialization, random-order bypass, payloadKey inclusion in the field hash, non-array entry refresh — is correctly implemented. src/Database/Database.php — specifically the decodeFindCachePayload() method and the cache-hit branch of findCached(), which return documents without the type-casting and decoding steps that find() applies Important Files Changed
Reviews (17): Last reviewed commit: "Harden find cache validation" | 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}
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
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
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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Database/Database.php (1)
8831-8838:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHydrate cached hits through the same post-processing as
find().
decodeCachedFindPayload()only wraps top-level arrays, while the miss path returns fully cast/decoded documents and may include populated relationshipDocumentinstances. JSON-backed cache adapters can therefore make cache hits return a different shape than misses; recursively hydrate relationship fields or bypass relationship-resolved cached finds until hit-path hydration matchesfind().🤖 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 8831 - 8838, The decodeCachedFindPayload() method only wraps top-level arrays but does not perform the same post-processing and hydration of relationship fields that the find() method applies to documents from the miss path. To fix this, ensure that documents retrieved from the cache in this code section (where isTtlExpired() is checked) are passed through the same post-processing/hydration logic that is applied to non-cached results, so that relationship Document instances and field casting match the behavior of the find() method. This will ensure cache hits return the same shape as cache misses and prevent JSON-backed cache adapters from returning inconsistently structured data.
🤖 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 9788-9793: Replace the MD5 hashing algorithm with SHA-256 for
collision resistance in the cache key generation. In the sprintf call that
builds the cache key, replace both instances of \md5(\json_encode($roles) ?: '')
and \md5(\json_encode($queryPayload) ?: '') with hash('sha256', ...) calls
instead. Also apply the same SHA-256 replacement at the location around line
9822 as indicated in the review comment.
---
Duplicate comments:
In `@src/Database/Database.php`:
- Around line 8831-8838: The decodeCachedFindPayload() method only wraps
top-level arrays but does not perform the same post-processing and hydration of
relationship fields that the find() method applies to documents from the miss
path. To fix this, ensure that documents retrieved from the cache in this code
section (where isTtlExpired() is checked) are passed through the same
post-processing/hydration logic that is applied to non-cached results, so that
relationship Document instances and field casting match the behavior of the
find() method. This will ensure cache hits return the same shape as cache misses
and prevent JSON-backed cache adapters from returning inconsistently structured
data.
🪄 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: 25343a0e-0838-4824-bd18-b9cd2fd72e4b
📒 Files selected for processing (3)
src/Database/Database.phptests/unit/CacheKeyTest.phptests/unit/FindCacheTest.php
| return \sprintf( | ||
| '%s:%s:%s:%s', | ||
| $this->getCachedFindSchemaHash($collection), | ||
| \md5(\json_encode($roles) ?: ''), | ||
| \md5(\json_encode($queryPayload) ?: ''), | ||
| $field, |
There was a problem hiding this comment.
Use a collision-resistant digest for cache-field hashes.
These hashes decide which cached result is returned. Replace MD5 with SHA-256 for the role/query/find payload fingerprints to avoid aliasing distinct cache variants.
Proposed fix
return \sprintf(
'%s:%s:%s:%s',
$this->getCachedFindSchemaHash($collection),
- \md5(\json_encode($roles) ?: ''),
- \md5(\json_encode($queryPayload) ?: ''),
+ \hash('sha256', \json_encode($roles) ?: ''),
+ \hash('sha256', \json_encode($queryPayload) ?: ''),
$field,
);
@@
- return [$findKey, \md5(\json_encode($payload) ?: '')];
+ return [$findKey, \hash('sha256', \json_encode($payload) ?: '')];Also applies to: 9822-9822
🧰 Tools
🪛 ast-grep (0.43.0)
[error] 9787-9793: Do not use a weak hash algorithm
Context: \sprintf(
'%s:%s:%s:%s',
$this->getCachedFindSchemaHash($collection),
\md5(\json_encode($roles) ?: ''),
\md5(\json_encode($queryPayload) ?: ''),
$field,
)
Note: [CWE-328] Use of Weak Hash.
(weak-hash-algorithm)
[error] 9790-9790: Do not use a weak hash algorithm
Context: \md5(\json_encode($roles) ?: '')
Note: [CWE-328] Use of Weak Hash.
(weak-hash-algorithm)
[error] 9791-9791: Do not use a weak hash algorithm
Context: \md5(\json_encode($queryPayload) ?: '')
Note: [CWE-328] Use of Weak Hash.
(weak-hash-algorithm)
🤖 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 9788 - 9793, Replace the MD5 hashing
algorithm with SHA-256 for collision resistance in the cache key generation. In
the sprintf call that builds the cache key, replace both instances of
\md5(\json_encode($roles) ?: '') and \md5(\json_encode($queryPayload) ?: '')
with hash('sha256', ...) calls instead. Also apply the same SHA-256 replacement
at the location around line 9822 as indicated in the review comment.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 8616-8627: The code attempts to load from cache before performing
full query validation against the collection structure. Add the complete
semantic validation logic from the full DocumentsValidator (not just
checkQueryTypes() which only validates that values are Query objects) before the
cache->load() call to ensure that cached hits cannot bypass standard
QueryException behavior for invalid query semantics. This validation block
should validate the queries parameter against the collectionDocument before
attempting to retrieve from the cache identified by the cacheKey and cacheField
variables.
In `@tests/unit/CacheKeyTest.php`:
- Around line 284-293: The PHPDoc annotation on the $queries variable in the
testFindCacheFieldValidatesQueryTypes method incorrectly declares the type as
array<Query>, but the actual value assigned is ['invalid'], which is an array of
strings. Update the PHPDoc comment to accurately reflect the actual data type by
changing the annotation to array<string> to match what is actually being
assigned to the variable.
🪄 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: eeaf2dfe-7928-4818-8c18-4d69cbbec6f8
📒 Files selected for processing (3)
src/Database/Database.phptests/unit/CacheKeyTest.phptests/unit/ListCacheTest.php
1 participant
Summary
Database::findCached()for trusted internal reads that can tolerate bounded staleness:findsuffix so it does not collide with Appwrite's native list cachegetFindCacheKey()andgetFindCacheField()so callers can build and purge the same find-cache key/field shape explicitlyfind()QueryExceptionconsistently withfind()Database::TTL(24h), withttl: 0disabling caching for a callQuery::orderRandom()Usage
findCached()executes the normalfind()query on cache miss and stores the result under a find-cache key derived from the Appwrite list-cache shape.Callers that need invalidation can purge the whole find-cache key they own:
Cache Key Generation
Cached entries are stored under:
Example:
The
:findsuffix keeps this separate from Appwrite's native list-cache key for the same collection.The hash field keeps the Appwrite list-cache field shape:
The query-state hash includes serialized queries, database, relationship mode, and active filter signatures. Cursor queries include the cursor document payload so pagination boundaries do not collide when ordered values change.
The
payloadKeyis included in the field identity and cached values are stored under that configured key, for examplerulesfor WAF.Tests
php vendor/bin/phpunit tests/unit/ListCacheTest.php tests/unit/CacheKeyTest.phpcomposer lintcomposer formatphp vendor/bin/phpstan analyse --level 7 src/Database/Database.php tests/unit/ListCacheTest.php tests/unit/CacheKeyTest.php --memory-limit 2GSummary by CodeRabbit