perf: replace str_replace with substr + cache in getPermissionsByType#858
perf: replace str_replace with substr + cache in getPermissionsByType#858
Conversation
Eliminates repeated str_replace calls (294K+ per profiling run) by using
str_starts_with + substr on the predictable type("role") format, and caches
results per Document instance. Cache is invalidated on setAttribute('$permissions').
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 optimises
Confidence Score: 4/5Safe to merge if the offsetSet/offsetUnset invalidation gap is addressed; the core substr optimisation is correct. One P1 finding: the cache can go stale when src/Database/Document.php — cache invalidation coverage in Important Files Changed
Reviews (1): Last reviewed commit: "perf: replace str_replace with substr + ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (isset($this->permissionCache[$type])) { | ||
| return $this->permissionCache[$type]; | ||
| } | ||
|
|
||
| $prefix = $type . '("'; | ||
| $prefixLen = \strlen($prefix); | ||
| $result = []; | ||
|
|
||
| foreach ($this->getPermissions() as $permission) { | ||
| if (!\str_starts_with($permission, $type)) { | ||
| continue; | ||
| if (\str_starts_with($permission, $prefix)) { | ||
| $result[] = \substr($permission, $prefixLen, -2); | ||
| } | ||
| $typePermissions[] = \str_replace([$type . '(', ')', '"', ' '], '', $permission); | ||
| } | ||
|
|
||
| return \array_unique($typePermissions); | ||
| return $this->permissionCache[$type] = \array_unique($result); |
There was a problem hiding this comment.
$permissions is mutated via direct array access
Document extends ArrayObject, so callers can bypass setAttribute entirely with $doc['$permissions'] = [...]. The cache is only invalidated in setAttribute, so any direct offset write leaves permissionCache holding the old results. The test file PermissionsTest.php already demonstrates this pattern (e.g. lines 37–48), repeatedly assigning $document['$permissions'] = [...] on a live Document instance — if getRead() were called before and after one of those assignments, it would return stale data.
To fully guard against this, override offsetSet and offsetUnset in Document:
public function offsetSet(mixed $key, mixed $value): void
{
if ($key === '$permissions') {
$this->permissionCache = [];
}
parent::offsetSet($key, $value);
}
public function offsetUnset(mixed $key): void
{
if ($key === '$permissions') {
$this->permissionCache = [];
}
parent::offsetUnset($key);
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Database/Document.php (1)
156-164: Redundantarray_uniqueon the result.
getPermissions()(line 98) already returnsarray_values(array_unique(...)), and since every matched entry here shares the same fixed prefix ($type(") and suffix (")), duplicate inner values would require duplicate full permission strings — which have already been deduplicated. The trailing\array_unique($result)can be dropped.♻️ Proposed simplification
- return $this->permissionCache[$type] = \array_unique($result); + return $this->permissionCache[$type] = $result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Document.php` around lines 156 - 164, The final \array_unique($result) is redundant because getPermissions() already returns deduplicated permission strings; update Document:: (the block that iterates over $this->getPermissions() and builds $result) to simply assign the unique-permission-derived array to $this->permissionCache[$type] without calling \array_unique(), i.e. remove the trailing \array_unique call and return the $result after building it (still keep \substr and prefix/suffix logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Document.php`:
- Around line 252-257: Document's permissionCache can become stale because
ArrayObject's ArrayAccess methods can mutate '$permissions' without going
through setAttribute; override offsetSet() and offsetUnset() in the Document
class to clear $this->permissionCache when the mutated key is '$permissions'
(then call parent::offsetSet/offsetUnset to preserve existing behavior). Ensure
the overrides handle string keys (and null keys for append) and mirror any
existing side-effects of setAttribute so
getRead()/getCreate()/getUpdate()/getDelete()/getPermissionsByType() see the
cleared cache.
---
Nitpick comments:
In `@src/Database/Document.php`:
- Around line 156-164: The final \array_unique($result) is redundant because
getPermissions() already returns deduplicated permission strings; update
Document:: (the block that iterates over $this->getPermissions() and builds
$result) to simply assign the unique-permission-derived array to
$this->permissionCache[$type] without calling \array_unique(), i.e. remove the
trailing \array_unique call and return the $result after building it (still keep
\substr and prefix/suffix logic).
🪄 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: 9a984586-5488-4fce-b044-8db246d5ba28
📒 Files selected for processing (1)
src/Database/Document.php
| public function setAttribute(string $key, mixed $value, string $type = self::SET_TYPE_ASSIGN): static | ||
| { | ||
| if ($key === '$permissions') { | ||
| $this->permissionCache = []; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find direct ArrayAccess writes to $permissions on Document instances.
rg -nP --type=php -C2 '\[[\x27"]\$permissions[\x27"]\]\s*(\[\]\s*)?='
rg -nP --type=php -C2 '->offsetSet\(\s*[\x27"]\$permissions[\x27"]'Repository: utopia-php/database
Length of output: 12782
🏁 Script executed:
# Check Document class structure
cat -n src/Database/Document.php | head -30Repository: utopia-php/database
Length of output: 953
🏁 Script executed:
# Check if Document extends ArrayObject and if offsetSet is overridden
rg -A5 'class Document' src/Database/Document.php | head -20Repository: utopia-php/database
Length of output: 237
🏁 Script executed:
# Check for offsetSet/offsetUnset methods in Document
rg 'offsetSet|offsetUnset' src/Database/Document.phpRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check what happens in SQL.php after the ArrayAccess mutation on line 424
sed -n '420,435p' src/Database/Adapter/SQL.phpRepository: utopia-php/database
Length of output: 603
🏁 Script executed:
# Check context around SQL.php line 3213
sed -n '3209,3225p' src/Database/Adapter/SQL.phpRepository: utopia-php/database
Length of output: 672
🏁 Script executed:
# Verify if getPermissions is called right after ArrayAccess mutations in realistic scenarios
rg -B3 -A3 "getPermissions\(\)" src/Database/Adapter/SQL.php | head -50Repository: utopia-php/database
Length of output: 1901
🏁 Script executed:
# Check Document constructor to see if/how permissionCache is initialized
sed -n '30,80p' src/Database/Document.phpRepository: utopia-php/database
Length of output: 1449
🏁 Script executed:
# Check getPermissions and getPermissionsByType implementation
sed -n '148,170p' src/Database/Document.phpRepository: utopia-php/database
Length of output: 710
🏁 Script executed:
# Look for the full getPermissions method
rg -A10 'public function getPermissions' src/Database/Document.phpRepository: utopia-php/database
Length of output: 701
🏁 Script executed:
# Check what test does after ArrayAccess mutations
sed -n '35,50p' tests/unit/Validator/PermissionsTest.phpRepository: utopia-php/database
Length of output: 1176
🏁 Script executed:
# Check if tests call getRead/getCreate/getUpdate/getDelete (which use cached getPermissionsByType)
rg 'getRead|getCreate|getUpdate|getDelete' tests/unit/Validator/PermissionsTest.phpRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check if actual production code in Database.php uses these methods on Documents after mutations
rg -B5 -A5 'getRead\(\)|getCreate\(\)|getUpdate\(\)|getDelete\(\)' src/Database/Database.php | head -60Repository: utopia-php/database
Length of output: 2185
🏁 Script executed:
# Check if Document instances have offsetSet behavior issues in actual usage
# Look for patterns where ArrayAccess mutation happens then getRead/getCreate/getUpdate/getDelete called
rg -B10 -A10 '\[\s*[\x27"]\$permissions[\x27"]\s*\].*=' tests/ --type=php | grep -A10 'getRead\|getCreate\|getUpdate\|getDelete' | head -40Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Double-check: does cache initialization in constructor populate cache?
grep -A30 'public function __construct' src/Database/Document.php | grep -i cacheRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Verify the actual cache behavior - when is permissionCache populated?
rg 'permissionCache\[' src/Database/Document.phpRepository: utopia-php/database
Length of output: 235
Document's permission cache can become stale if $permissions is modified via ArrayAccess after getRead()/getCreate()/getUpdate()/getDelete() are called.
Document extends ArrayObject without overriding offsetSet() or offsetUnset(), so callers can bypass setAttribute() — e.g. $doc['$permissions'] = [...]. Since getPermissionsByType() caches results by type, any ArrayAccess mutation after a call to one of the typed permission getters will leave the cache stale.
This pattern does not currently occur in the codebase (tests that mutate via ArrayAccess only call getPermissions(), which doesn't cache; production code loads documents from the database or uses setAttribute()). However, to prevent future bugs, consider overriding offsetSet()/offsetUnset() to clear the cache, or document that direct ArrayAccess mutations to $permissions are unsupported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Document.php` around lines 252 - 257, Document's permissionCache
can become stale because ArrayObject's ArrayAccess methods can mutate
'$permissions' without going through setAttribute; override offsetSet() and
offsetUnset() in the Document class to clear $this->permissionCache when the
mutated key is '$permissions' (then call parent::offsetSet/offsetUnset to
preserve existing behavior). Ensure the overrides handle string keys (and null
keys for append) and mirror any existing side-effects of setAttribute so
getRead()/getCreate()/getUpdate()/getDelete()/getPermissionsByType() see the
cleared cache.
1 participant
Summary
str_replacewithstr_starts_with+substringetPermissionsByType(), exploiting the predictabletype("role")format to use a single C-levelsubstrcall instead of multiple string replacements$permissionCacheso repeated calls for the same type on the same document (e.g.getRead()called multiple times) return immediatelysetAttribute()when$permissionsis mutated to prevent stale readsProfiling showed ~294K
str_replacecalls attributable to this method. This patch eliminates them entirely.Test plan
getRead(),getCreate(),getUpdate(),getDelete(),getWrite()return correct rolessetAttribute('$permissions', ...)🤖 Generated with Claude Code
Summary by CodeRabbit