Skip to content

perf: replace str_replace with substr + cache in getPermissionsByType #858

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
loks0n wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf: replace str_replace with substr + cache in getPermissionsByType #858

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions src/Database/Document.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ class Document extends ArrayObject
public const SET_TYPE_PREPEND = 'prepend';
public const SET_TYPE_APPEND = 'append';

/** @var array<string, array<string>> */
private array $permissionCache = [];

/**
* Construct.
*
Expand Down Expand Up @@ -144,16 +147,21 @@ public function getWrite(): array
*/
public function getPermissionsByType(string $type): array
{
$typePermissions = [];
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);
Comment on lines +150 to +164
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Stale cache when $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);
}

}

/**
Expand Down Expand Up @@ -243,6 +251,10 @@ public function getAttribute(string $name, mixed $default = null): mixed
*/
public function setAttribute(string $key, mixed $value, string $type = self::SET_TYPE_ASSIGN): static
{
if ($key === '$permissions') {
$this->permissionCache = [];
}

Comment on lines 252 to +257
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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 -30

Repository: 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 -20

Repository: utopia-php/database

Length of output: 237

🏁 Script executed:

# Check for offsetSet/offsetUnset methods in Document
rg 'offsetSet|offsetUnset' src/Database/Document.php

Repository: 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.php

Repository: utopia-php/database

Length of output: 603

🏁 Script executed:

# Check context around SQL.php line 3213
sed -n '3209,3225p' src/Database/Adapter/SQL.php

Repository: 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 -50

Repository: 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.php

Repository: utopia-php/database

Length of output: 1449

🏁 Script executed:

# Check getPermissions and getPermissionsByType implementation
sed -n '148,170p' src/Database/Document.php

Repository: utopia-php/database

Length of output: 710

🏁 Script executed:

# Look for the full getPermissions method
rg -A10 'public function getPermissions' src/Database/Document.php

Repository: utopia-php/database

Length of output: 701

🏁 Script executed:

# Check what test does after ArrayAccess mutations
sed -n '35,50p' tests/unit/Validator/PermissionsTest.php

Repository: 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.php

Repository: 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 -60

Repository: 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 -40

Repository: 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 cache

Repository: utopia-php/database

Length of output: 45

🏁 Script executed:

# Verify the actual cache behavior - when is permissionCache populated?
rg 'permissionCache\[' src/Database/Document.php

Repository: 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.

switch ($type) {
case self::SET_TYPE_ASSIGN:
$this[$key] = $value;
Expand Down
Loading