Conversation
|
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:
📝 WalkthroughWalkthroughLarge refactor: Query delegates to utopia-php/query BaseQuery; adapters, traits, hooks, and new domain types/enums (Attribute, Index, Relationship, Capability, etc.) migrate to value-object and capability models; CI, Docker, and composer updated to use a local query path and Paratest. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Pool
participant Adapter
participant WriteHooks as "Write Hooks\n(PermissionWrite/TenantWrite)"
participant Relationship as "RelationshipHandler"
Client->>Pool: createDocument(collection, Document)
Pool->>Adapter: createDocument(Document)
Note right of Adapter: decorate row via write hooks
Adapter->>WriteHooks: decorateRow(row, metadata)
WriteHooks-->>Adapter: decorated row
Adapter->>Adapter: persist row (DB)
Adapter->>Relationship: afterDocumentCreate(collection, document)
Relationship-->>Adapter: possibly update/populate related docs
Adapter-->>Pool: Document (created)
Pool-->>Client: Document (created)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Database/Query.php (1)
62-76: Align cursor helper docblocks with the actual parameter type.Lines 62 and 73 document
Document, but the signature ismixed $value(Lines 65 and 76). Please update phpdoc to match actual behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 62 - 76, The PHPDoc for Query::cursorAfter and Query::cursorBefore incorrectly types the parameter as Document while the method signatures accept mixed $value; update the `@param` annotations in both docblocks to "@param mixed $value" (and keep "@return Query" unchanged) so the docblocks match the actual signatures for the cursorAfter and cursorBefore methods on the Query class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Around line 62-66: Replace the SSH Git URL in the composer.json "repositories"
entry with its HTTPS equivalent to avoid CI SSH auth issues; locate the
"repositories" array and update the url value currently set to
"git@github.com:utopia-php/query.git" to use
"https://github.com/utopia-php/query.git" so installs succeed in non-interactive
CI environments.
In `@src/Database/Query.php`:
- Around line 167-183: The code is using $limit as a fallback for both $offset
and $cursor which is incorrect; update the assignments in the TYPE_OFFSET and
TYPE_CURSOR_AFTER/TYPE_CURSOR_BEFORE branches so they use the actual first value
or null instead of $limit (i.e. change $values[0] ?? $limit to $values[0] ??
null) while keeping the existing checks that ignore subsequent offsets/cursors
and preserving $cursorDirection assignment (references: TYPE_OFFSET,
TYPE_CURSOR_AFTER, TYPE_CURSOR_BEFORE, $offset, $cursor, $values, $limit,
$cursorDirection, Database::CURSOR_AFTER, Database::CURSOR_BEFORE).
---
Nitpick comments:
In `@src/Database/Query.php`:
- Around line 62-76: The PHPDoc for Query::cursorAfter and Query::cursorBefore
incorrectly types the parameter as Document while the method signatures accept
mixed $value; update the `@param` annotations in both docblocks to "@param mixed
$value" (and keep "@return Query" unchanged) so the docblocks match the actual
signatures for the cursorAfter and cursorBefore methods on the Query class.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
composer.jsonsrc/Database/Query.php
| "repositories": [ | ||
| { | ||
| "type": "vcs", | ||
| "url": "git@github.com:utopia-php/query.git" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n composer.json | head -70Repository: utopia-php/database
Length of output: 2871
🏁 Script executed:
python - <<'PY'
import json
from pathlib import Path
data = json.loads(Path("composer.json").read_text())
repos = data.get("repositories", [])
for idx, repo in enumerate(repos):
url = repo.get("url", "")
if url.startswith("git@"):
print(f"Repository[{idx}] uses SSH URL: {url}")
else:
print(f"Repository[{idx}] URL type: {url}")
PYRepository: utopia-php/database
Length of output: 126
Use HTTPS for the VCS repository URL to prevent install failures in CI.
Line 65 uses an SSH URL (git@github.com:utopia-php/query.git), which fails in non-interactive CI environments that lack SSH key configuration. Change to the HTTPS equivalent:
🔧 Proposed fix
"repositories": [
{
"type": "vcs",
- "url": "git@github.com:utopia-php/query.git"
+ "url": "https://github.com/utopia-php/query.git"
}
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "repositories": [ | |
| { | |
| "type": "vcs", | |
| "url": "git@github.com:utopia-php/query.git" | |
| } | |
| "repositories": [ | |
| { | |
| "type": "vcs", | |
| "url": "https://github.com/utopia-php/query.git" | |
| } | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` around lines 62 - 66, Replace the SSH Git URL in the
composer.json "repositories" entry with its HTTPS equivalent to avoid CI SSH
auth issues; locate the "repositories" array and update the url value currently
set to "git@github.com:utopia-php/query.git" to use
"https://github.com/utopia-php/query.git" so installs succeed in non-interactive
CI environments.
Add a PHPStan stub for Utopia\Query\Query that declares `@return static` on all factory methods, so PHPStan correctly resolves return types when called via the Utopia\Database\Query subclass. Also fix groupByType() param type and remove dead instanceof checks in parse/parseQuery. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Database/Query.php (1)
161-177:⚠️ Potential issue | 🟠 MajorFix fallback leakage from
limitinto offset/cursor state.Line 167 and Line 176 incorrectly use
$limitas fallback, which can corrupt grouped query output when offset/cursor values are missing.🔧 Proposed fix
case self::TYPE_OFFSET: @@ - $offset = $values[0] ?? $limit; + $offset = $values[0] ?? $offset; break; @@ case self::TYPE_CURSOR_AFTER: case self::TYPE_CURSOR_BEFORE: @@ - $cursor = $values[0] ?? $limit; + $cursor = $values[0] ?? $cursor; $cursorDirection = $method === self::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE; break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 161 - 177, In Query:: (switch handling TYPE_OFFSET / TYPE_CURSOR_AFTER / TYPE_CURSOR_BEFORE) remove the incorrect fallback to $limit so $offset and $cursor aren't polluted: instead of assigning $values[0] ?? $limit, only assign when an explicit value exists (e.g. check isset($values[0]) or array_key_exists) and otherwise leave $offset/$cursor as null; keep the first-occurrence guards intact and preserve setting $cursorDirection when handling TYPE_CURSOR_AFTER vs TYPE_CURSOR_BEFORE (Database::CURSOR_AFTER / Database::CURSOR_BEFORE).
🧹 Nitpick comments (1)
src/Database/Query.php (1)
53-73: Align cursor helper PHPDoc with the actualmixedparameter type.Both docblocks still document
@param Document $value, but the signatures now acceptmixed.📝 Suggested doc fix
- * `@param` Document $value + * `@param` mixed $value @@ - * `@param` Document $value + * `@param` mixed $value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 53 - 73, Update the PHPDoc for the Query helper methods to match the actual parameter type: in the Query class update the docblocks for cursorAfter and cursorBefore so the `@param` annotation is "@param mixed $value" (instead of "@param Document $value") and keep the `@return` annotation as "Query"; ensure these docblocks sit immediately above the corresponding methods cursorAfter and cursorBefore to maintain accurate IDE/typehinting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Database/Query.php`:
- Around line 161-177: In Query:: (switch handling TYPE_OFFSET /
TYPE_CURSOR_AFTER / TYPE_CURSOR_BEFORE) remove the incorrect fallback to $limit
so $offset and $cursor aren't polluted: instead of assigning $values[0] ??
$limit, only assign when an explicit value exists (e.g. check isset($values[0])
or array_key_exists) and otherwise leave $offset/$cursor as null; keep the
first-occurrence guards intact and preserve setting $cursorDirection when
handling TYPE_CURSOR_AFTER vs TYPE_CURSOR_BEFORE (Database::CURSOR_AFTER /
Database::CURSOR_BEFORE).
---
Nitpick comments:
In `@src/Database/Query.php`:
- Around line 53-73: Update the PHPDoc for the Query helper methods to match the
actual parameter type: in the Query class update the docblocks for cursorAfter
and cursorBefore so the `@param` annotation is "@param mixed $value" (instead of
"@param Document $value") and keep the `@return` annotation as "Query"; ensure
these docblocks sit immediately above the corresponding methods cursorAfter and
cursorBefore to maintain accurate IDE/typehinting.
Update Query overrides to use `: static` return types matching the base query package. Remove the phpstan.neon and stubs workaround since the query package now uses `: static` natively. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Database/Query.php (1)
159-169:⚠️ Potential issue | 🟠 MajorFix incorrect fallback variable usage for offset and cursor.
Lines 159 and 168 incorrectly fall back to
$limitinstead of the appropriate variable. This can silently corrupt the grouped query state.
- Line 159:
$offset = $values[0] ?? $limitshould use$offset(ornull)- Line 168:
$cursor = $values[0] ?? $limitshould use$cursor(ornull)🔧 Proposed fix
case self::TYPE_OFFSET: // Keep the 1st offset encountered and ignore the rest if ($offset !== null) { break; } - $offset = $values[0] ?? $limit; + $offset = $values[0] ?? null; break; case self::TYPE_CURSOR_AFTER: case self::TYPE_CURSOR_BEFORE: // Keep the 1st cursor encountered and ignore the rest if ($cursor !== null) { break; } - $cursor = $values[0] ?? $limit; + $cursor = $values[0] ?? null; $cursorDirection = $method === self::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE; break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 159 - 169, The switch handling in Query.php incorrectly falls back to $limit when assigning $offset and $cursor, corrupting grouped query state; update the assignments in the cases that set $offset and $cursor (the branches that assign $offset = $values[0] ?? $limit and $cursor = $values[0] ?? $limit) to use a proper null fallback instead (e.g. $offset = $values[0] ?? null and $cursor = $values[0] ?? null), keeping the surrounding logic for cursorDirection (Database::CURSOR_AFTER / Database::CURSOR_BEFORE) intact.
🧹 Nitpick comments (1)
src/Database/Query.php (1)
54-65: Consider tightening parameter type hint.The PHPDoc indicates
@param Document $value, but the signature usesmixed. Consider usingDocumentas the type hint for better type safety, or update the PHPDoc to reflect the actual accepted types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 54 - 65, The PHPDoc for cursorBefore indicates the parameter is a Document but the method signature (and cursorAfter) use mixed; update the signatures to use Document instead of mixed for cursorBefore and cursorAfter (i.e., change the parameter type from mixed to Document) to match the PHPDoc and improve type safety, or alternatively update the PHPDoc to reflect mixed if these methods truly accept other types—adjust the declarations in the Query class (cursorBefore, cursorAfter) so the docblock and method signatures are consistent.
🤖 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/Query.php`:
- Around line 42-49: The method parseQuery currently declares a return type of
static but calls parent::parseQuery() which returns BaseQuery; change the method
signature to return BaseQuery instead of static and keep the try/catch that
wraps BaseQueryException into QueryException (i.e., update the return type on
parseQuery to BaseQuery to match parent::parseQuery and ensure the thrown
QueryException still wraps the original BaseQueryException).
---
Duplicate comments:
In `@src/Database/Query.php`:
- Around line 159-169: The switch handling in Query.php incorrectly falls back
to $limit when assigning $offset and $cursor, corrupting grouped query state;
update the assignments in the cases that set $offset and $cursor (the branches
that assign $offset = $values[0] ?? $limit and $cursor = $values[0] ?? $limit)
to use a proper null fallback instead (e.g. $offset = $values[0] ?? null and
$cursor = $values[0] ?? null), keeping the surrounding logic for cursorDirection
(Database::CURSOR_AFTER / Database::CURSOR_BEFORE) intact.
---
Nitpick comments:
In `@src/Database/Query.php`:
- Around line 54-65: The PHPDoc for cursorBefore indicates the parameter is a
Document but the method signature (and cursorAfter) use mixed; update the
signatures to use Document instead of mixed for cursorBefore and cursorAfter
(i.e., change the parameter type from mixed to Document) to match the PHPDoc and
improve type safety, or alternatively update the PHPDoc to reflect mixed if
these methods truly accept other types—adjust the declarations in the Query
class (cursorBefore, cursorAfter) so the docblock and method signatures are
consistent.
| public static function parseQuery(array $query): static | ||
| { | ||
| try { | ||
| $query = \json_decode($query, true, flags: JSON_THROW_ON_ERROR); | ||
| } catch (\JsonException $e) { | ||
| throw new QueryException('Invalid query: ' . $e->getMessage()); | ||
| } | ||
|
|
||
| if (!\is_array($query)) { | ||
| throw new QueryException('Invalid query. Must be an array, got ' . \gettype($query)); | ||
| return parent::parseQuery($query); | ||
| } catch (BaseQueryException $e) { | ||
| throw new QueryException($e->getMessage(), $e->getCode(), $e); | ||
| } | ||
|
|
||
| return self::parseQuery($query); | ||
| } |
There was a problem hiding this comment.
Same return type issue as parse().
parent::parseQuery() will also return BaseQuery, not static. Apply the same fix pattern.
🔧 Proposed fix
public static function parseQuery(array $query): static
{
try {
- return parent::parseQuery($query);
+ $parsed = parent::parseQuery($query);
+ return new static($parsed->getMethod(), $parsed->getAttribute(), $parsed->getValues());
} catch (BaseQueryException $e) {
throw new QueryException($e->getMessage(), $e->getCode(), $e);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function parseQuery(array $query): static | |
| { | |
| try { | |
| $query = \json_decode($query, true, flags: JSON_THROW_ON_ERROR); | |
| } catch (\JsonException $e) { | |
| throw new QueryException('Invalid query: ' . $e->getMessage()); | |
| } | |
| if (!\is_array($query)) { | |
| throw new QueryException('Invalid query. Must be an array, got ' . \gettype($query)); | |
| return parent::parseQuery($query); | |
| } catch (BaseQueryException $e) { | |
| throw new QueryException($e->getMessage(), $e->getCode(), $e); | |
| } | |
| return self::parseQuery($query); | |
| } | |
| public static function parseQuery(array $query): static | |
| { | |
| try { | |
| $parsed = parent::parseQuery($query); | |
| return new static($parsed->getMethod(), $parsed->getAttribute(), $parsed->getValues()); | |
| } catch (BaseQueryException $e) { | |
| throw new QueryException($e->getMessage(), $e->getCode(), $e); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Query.php` around lines 42 - 49, The method parseQuery currently
declares a return type of static but calls parent::parseQuery() which returns
BaseQuery; change the method signature to return BaseQuery instead of static and
keep the try/catch that wraps BaseQueryException into QueryException (i.e.,
update the return type on parseQuery to BaseQuery to match parent::parseQuery
and ensure the thrown QueryException still wraps the original
BaseQueryException).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s in Mongo adapter
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/Database/Document.php (1)
232-243:⚠️ Potential issue | 🟡 MinorUpdate docblock to match the new parameter type.
The docblock still documents
@param string $typebut the actual parameter type is nowSetType. This could confuse IDE autocompletion and static analysis tools.📝 Proposed fix
/** * Set Attribute. * * Method for setting a specific field attribute * * `@param` string $key * `@param` mixed $value - * `@param` string $type + * `@param` SetType $type * * `@return` static */ public function setAttribute(string $key, mixed $value, SetType $type = SetType::Assign): static🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Document.php` around lines 232 - 243, Update the docblock for the setAttribute method to match the new parameter type: replace the incorrect "@param string $type" with "@param SetType $type" (or fully qualified \Namespace\SetType if necessary) so IDEs and static analyzers reflect the actual signature of setAttribute(string $key, mixed $value, SetType $type = SetType::Assign) and keep the rest of the description intact.src/Database/Mirror.php (1)
398-422:⚠️ Potential issue | 🟠 MajorUse the filtered key when mirroring attribute renames.
beforeUpdateAttribute()can rewrite the attribute document, but the destination update still uses the caller’s raw$newKey. Any filter that renames destination attributes will drift on update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Mirror.php` around lines 398 - 422, The destination update call is still using the original caller $newKey even though writeFilters->beforeUpdateAttribute may have rewritten the attribute document (including renaming the key); update the call in Mirror:: where updateAttribute is invoked to use the filtered key from the mutated $document (e.g., $document->getAttribute('key')) instead of $newKey, falling back to $newKey if the document does not contain a key, so attribute renames applied by beforeUpdateAttribute are honored; reference writeFilters, beforeUpdateAttribute, $document, updateAttribute and $newKey when making the change.src/Database/Adapter/Pool.php (1)
101-104:⚠️ Potential issue | 🟠 MajorPersist the timeout on the pool wrapper.
This override forwards the current call but never updates
$this->timeout. Laterdelegate()calls therefore won’t reapply the timeout to newly borrowed adapters, so the setting is lost across pool checkouts.⏱️ Proposed fix
public function setTimeout(int $milliseconds, string $event = Database::EVENT_ALL): void { + parent::setTimeout($milliseconds, $event); $this->delegate(__FUNCTION__, \func_get_args()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Pool.php` around lines 101 - 104, The setTimeout override in Pool::setTimeout(int $milliseconds, string $event = Database::EVENT_ALL) forwards the call to delegate() but fails to persist the value to the pool wrapper, so $this->timeout is never updated and newly borrowed adapters don't get the timeout; update the Pool::setTimeout implementation to assign the passed $milliseconds (and $event if you store per-event) to the pool's internal property ($this->timeout or a per-event map) before or after calling $this->delegate(__FUNCTION__, func_get_args()), ensuring future delegate() calls reapply the stored timeout to adapters checked out from the pool.src/Database/Query.php (1)
240-257:⚠️ Potential issue | 🟠 MajorCursor queries stop working after
toArray()/groupForDatabase()round-trips.
Documents::find()expects$cursorto be aDocumentso it can read order keys and$collection.toArray()collapses cursor documents to$id, andgroupForDatabase()returns that scalar unchanged, so any serialized cursor query will fail when pagination code callsgetAttribute()orgetCollection()on it.🔧 Proposed fix
- if ($value instanceof Document && in_array($this->method, [Method::CursorAfter, Method::CursorBefore])) { - $value = $value->getId(); + if ($value instanceof Document && \in_array($this->method, [Method::CursorAfter, Method::CursorBefore], true)) { + $value = $value->getArrayCopy(); } $array['values'][] = $value; } } @@ + $cursor = $grouped->cursor; + if (\is_array($cursor)) { + $cursor = new Document($cursor); + } + return [ 'filters' => $filters, 'selections' => $selections, 'limit' => $grouped->limit, 'offset' => $grouped->offset, 'orderAttributes' => $grouped->orderAttributes, 'orderTypes' => $orderTypes, - 'cursor' => $grouped->cursor, + 'cursor' => $cursor, 'cursorDirection' => $cursorDirection, ];Also applies to: 280-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 240 - 257, The cursor-serialization code in Query::toArray() currently replaces Document instances with their id for cursor methods (Method::CursorAfter, Method::CursorBefore), which breaks Documents::find() that expects full Document objects after groupForDatabase() round-trips; change the branch so that when $value instanceof Document you serialize it with $value->toArray() (not ->getId()) so the document structure is preserved and can be rehydrated on read (also apply the same fix in the analogous block around lines 280-317), making sure groupForDatabase()/unserialize logic continues to round-trip these document arrays back into Document objects so getAttribute() and getCollection() still work.src/Database/Adapter/Mongo.php (2)
1575-1592:⚠️ Potential issue | 🟠 MajorKeep replacement-style updates for schemaless collections.
This now always wraps the payload in
$set. When defined attributes are disabled, missing keys are supposed to disappear on update; with$setthey persist forever because the old document is never replaced.Suggested fix
$options = $this->getTransactionOptions(); - $updateQuery = [ - '$set' => $record, - ]; + $updateQuery = $this->supportForAttributes + ? ['$set' => $record] + : $record; $this->client->update($name, $filters, $updateQuery, $options);Based on learnings: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Mongo.php` around lines 1575 - 1592, The updateDocument method currently always wraps the payload in a $set which prevents replacement-style updates for schemaless collections; change updateDocument so that after preparing $record (and unsetting '_id') it checks $this->getSupportForAttributes(): if true keep $updateQuery = ['$set' => $record], but if false use $updateQuery = $record (a replacement document) before calling $this->client->update($name, $filters, $updateQuery, $options) so missing keys are removed for schemaless collections; refer to updateDocument and getSupportForAttributes() to locate where to branch.
1542-1553:⚠️ Potential issue | 🟠 MajorScope the post-insert readback.
In shared-table mode
_uidis only unique within_tenant. This follow-upfind()reads back by_uidalone, so a custom ID collision can hydrate the newly created document from another tenant's row instead of the one that was just inserted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Mongo.php` around lines 1542 - 1553, The post-insert readback in insertDocument uses a filter of ['_uid' => $document['_uid']], which can collide across tenants; update the filter used in the inner client->find call to scope by both '_uid' and '_tenant' (e.g. include '_tenant' => $document['_tenant'] when present) so the find() returns the document from the same tenant that was just inserted, adjusting any variable names (filters) used around client->find accordingly.
🟠 Major comments (19)
composer.json-64-72 (1)
64-72:⚠️ Potential issue | 🟠 MajorPath repository will fail in CI environments.
The path repository pointing to
../queryis only valid for local development where the siblingquerydirectory exists. In CI environments (GitHub Actions, etc.) and for other contributors cloning this repo, this path won't exist andcomposer installwill fail.For CI compatibility, either:
- Publish
utopia-php/queryto Packagist before merging- Use the official VCS repository URL as a fallback
🔧 Proposed fix using VCS repository
"repositories": [ { "type": "path", "url": "../query", "options": { "symlink": true } - } + }, + { + "type": "vcs", + "url": "https://github.com/utopia-php/query.git" + } ],Note: Composer will prefer the path repository if available, falling back to VCS otherwise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` around lines 64 - 72, The current "repositories" entry uses a path repo ("type": "path", "url": "../query") which will break CI because the sibling ../query won't exist; update composer.json to remove or guard the path-only repository and add a fallback VCS repository entry pointing to the public repo URL for utopia-php/query (or publish to Packagist and use its package name), so Composer can use the VCS source in CI while still preferring the local path when present; modify the "repositories" array to include the official VCS URL (or switch to Packagist) alongside or instead of the existing path entry.src/Database/Hook/MongoPermissionFilter.php-26-27 (1)
26-27:⚠️ Potential issue | 🟠 MajorEscape regex metacharacters in role names to prevent regex injection.
Role names are directly interpolated into the regex pattern without escaping. If a role contains regex metacharacters (e.g.,
(,),|,.,*), it could break the regex or enable ReDoS attacks.🛡️ Proposed fix to escape role names
- $roles = \implode('|', $this->authorization->getRoles()); + $roles = \implode('|', \array_map( + fn($role) => \preg_quote($role, '/'), + $this->authorization->getRoles() + )); $filters['_permissions']['$in'] = [new Regex("{$forPermission}\\(\".*(?:{$roles}).*\"\\)", 'i')];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Hook/MongoPermissionFilter.php` around lines 26 - 27, The code builds a Regex from role names without escaping them, risking regex injection; update the logic that creates $roles (from $this->authorization->getRoles()) to escape each role with a regex-escaping function (e.g., preg_quote) before joining them, then use the escaped string when constructing the Regex used in the $filters['_permissions']['$in'] assignment (the Regex("{$forPermission}\\(\".*(?:{$roles}).*\"\\)", 'i') instantiation) so special characters in role names are neutralized.src/Database/Traits/Attributes.php-998-1015 (1)
998-1015:⚠️ Potential issue | 🟠 MajorRollback the previous default when metadata persistence fails.
If the adapter update succeeds and
updateMetadata()then fails, the rollback model recreates the old column without its previous default. The physical schema can remain mutated while metadata rolls back.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Attributes.php` around lines 998 - 1015, The rollback Attribute is missing the original default value so if the adapter update succeeds but updateMetadata() fails the physical column loses its default; modify the rollback model creation (the Attribute instance assigned to $rollbackAttrModel) to include the previous default (e.g., $originalDefault) and ensure whatever variable holds the prior default is captured and passed into the Attribute constructor, then keep the rollback closure that calls $this->adapter->updateAttribute($collection, $rollbackAttrModel, $originalKey) unchanged so the adapter will restore the column including its original default when metadata persistence fails.src/Database/Traits/Relationships.php-893-902 (1)
893-902:⚠️ Potential issue | 🟠 MajorPreserve the original side during rollback.
This recreation hard-codes
RelationSide::Parent. If a child-side delete fails after the physical schema change, rollback will restore the relationship on the wrong side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Relationships.php` around lines 893 - 902, The rollback recreates the Relationship with a hard-coded RelationSide::Parent which can restore the relation on the wrong side; change the code that builds $recreateRelModel in the rollback to use the original relationship side (e.g., the existing side value from the original Relationship instance or the local $side variable) instead of RelationSide::Parent so the recreated Relationship preserves the original side; update the constructor call that sets side to use that original side source (RelationSide value obtained from the original model) when constructing $recreateRelModel.src/Database/Traits/Attributes.php-281-284 (1)
281-284:⚠️ Potential issue | 🟠 MajorRollback should only delete attributes created in this batch.
$attributeDocumentsalso contains schema-only orphans that already existed before this call. If one new attribute is created andupdateMetadata()fails,cleanupAttributes()will delete those pre-existing columns too.Also applies to: 317-323
src/Database/Traits/Attributes.php-546-559 (1)
546-559:⚠️ Potential issue | 🟠 MajorInvalidate both collection and metadata caches after these mutations.
createAttribute(),updateAttribute(), anddeleteAttribute()purge cached collection state and the metadata document.updateAttributeMeta()does neither, andrenameAttribute()still leaves the metadata document cache warm, so callers can read stale attribute definitions after a successful write.Also applies to: 1284-1292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Attributes.php` around lines 546 - 559, After performing attribute-mutating operations (specifically in updateAttributeMeta() and renameAttribute()), ensure you clear both the collection cache and the metadata document cache just like createAttribute()/updateAttribute()/deleteAttribute() do: call the same cache invalidation logic used elsewhere after the $this->updateMetadata(...) call (and before returning), and also ensure any cache keys for the metadata document are purged so callers don't read stale attribute definitions; reference updateAttributeMeta(), renameAttribute(), and the existing $this->updateMetadata(...) / self::EVENT_ATTRIBUTE_UPDATE flow to locate the correct spot to add the invalidation.src/Database/Adapter.php-685-697 (1)
685-697:⚠️ Potential issue | 🟠 MajorDon’t report unsupported relationship operations as success.
Traits\Relationshipsuses these return values to decide whether to update metadata. Returningtruehere lets an adapter without a real implementation claim success while leaving the physical schema unchanged.🧱 Proposed fix
public function createRelationship(Relationship $relationship): bool { - return true; + return false; } public function updateRelationship(Relationship $relationship, ?string $newKey = null, ?string $newTwoWayKey = null): bool { - return true; + return false; } public function deleteRelationship(Relationship $relationship): bool { - return true; + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter.php` around lines 685 - 697, The Adapter currently reports success for unsupported relationship operations; update the methods createRelationship(Relationship $relationship), updateRelationship(Relationship $relationship, ?string $newKey = null, ?string $newTwoWayKey = null) and deleteRelationship(Relationship $relationship) to not claim success — return false (or throw a clear UnsupportedOperationException) instead of returning true so Traits\Relationships won't assume metadata was updated; ensure the change is applied to those method bodies and keep the signatures unchanged.src/Database/Traits/Relationships.php-518-579 (1)
518-579:⚠️ Potential issue | 🟠 MajorUpdate both relationship metadata documents atomically.
These
updateAttributeMeta()calls commit independently. If the first collection update succeeds and the second one fails, the catch only reverts the adapter rename; it never restores the already-persisted metadata change on the first collection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Relationships.php` around lines 518 - 579, The metadata updates via updateAttributeMeta on the primary collection, the related collection (oldTwoWayKey), and the junction (when ManyToMany) must be performed atomically; currently if one update fails the others remain persisted. Fix by either wrapping all metadata updates and the adapter rename in a single DB transaction (begin/commit/rollback) so any failure rolls back all changes, or if transactions are not available, capture the existing attribute state for collection->getId()/$id, relatedCollection->getId()/oldTwoWayKey, and junction entries (if RelationType::ManyToMany) before applying updates and, inside the catch, call updateAttributeMeta to restore those saved states (and ensure purgeCachedCollection is similarly reverted). Update logic references: updateAttributeMeta, getJunctionCollection, withRetries->purgeCachedCollection, and the adapter->updateRelationship/Relationship model so both metadata and adapter rename are consistent.src/Database/Traits/Relationships.php-143-157 (1)
143-157:⚠️ Potential issue | 🟠 MajorCheck the related collection for
twoWayKeycollisions before creating the relationship.The duplicate scan only inspects the source collection.
checkAttribute()only enforces limits/width, so an existing attribute on the related collection with the same$twoWayKeyis not rejected before partial relationship creation starts.Also applies to: 191-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Relationships.php` around lines 143 - 157, The duplicate-check currently inspects only the source collection attributes (loop over $attributes) and misses collisions on the related collection; update the logic in the Relationships trait where you check for existing relationship attributes (the foreach that compares ColumnType::Relationship->value, twoWayKey and relatedCollection id) to also fetch and iterate the related collection's attributes (use $relatedCollection->getAttribute('attributes', [])) and compare their options['twoWayKey'] case-insensitively to $twoWayKey and their relatedCollection id to the source collection id, throwing DuplicateException('Related attribute already exists') on conflict; apply the same symmetric check to the other similar block referenced by checkAttribute() (the block around the other relationship-creation path) so both creation flows validate twoWayKey collisions on the opposite collection before proceeding.src/Database/Traits/Attributes.php-572-576 (1)
572-576:⚠️ Potential issue | 🟠 MajorKeep the metadata-only helpers behind the same invariants as
updateAttribute().
updateAttributeRequired()can mark an attribute required while leaving its old default in place,updateAttributeFilters()can remove mandatory filters likedatetime, andupdateAttributeDefault()never checks vector length against the configured dimension count. These helpers can persist attribute metadata that the full update path would reject.Also applies to: 627-631, 644-653
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Attributes.php` around lines 572 - 576, The three metadata-only helpers (updateAttributeRequired, updateAttributeFilters, updateAttributeDefault) bypass the same validation/invariants enforced by updateAttribute and can persist invalid metadata; change each helper to call the full update path or reuse its validation logic by delegating to updateAttribute (or to the common validation function used by updateAttribute) instead of directly mutating via updateAttributeMeta: ensure updateAttributeRequired validates compatibility between required flag and the existing default, updateAttributeFilters preserves/enforces mandatory filters (e.g., datetime) and validates any removed filters against attribute type, and updateAttributeDefault enforces vector length vs configured dimension count and any type-specific constraints before persisting. Ensure you reference and reuse updateAttribute or its shared validators so all invariants are consistently applied.src/Database/Hook/PermissionWrite.php-131-136 (1)
131-136:⚠️ Potential issue | 🟠 MajorRoute permission reads/deletes through
WriteContext::execute.The insert path uses the context executor, but these branches call
execute()directly and some of the delete helpers ignore the boolean result entirely. A failed permission mutation can therefore leave<collection>_permsout of sync with the document write.Also applies to: 187-192, 208-215, 227-231, 262-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Hook/PermissionWrite.php` around lines 131 - 136, The permission delete branches in PermissionWrite (use of ($context->newBuilder), $removeBuilder->delete(), and the subsequent ($context->executeResult) returning $deleteStmt) call $deleteStmt->execute() directly and ignore the WriteContext executor and result; change these to run deletes through the WriteContext executor (use WriteContext::execute on the prepared statement returned by ($context->executeResult) with Database::EVENT_PERMISSIONS_DELETE), check the boolean return value and propagate/handle failures (throw or abort the parent write) so permission mutations cannot silently fail; apply the same fix pattern to the other affected blocks that build/delete perms (the sections analogous to lines 187-192, 208-215, 227-231, 262-266).src/Database/Traits/Relationships.php-493-515 (1)
493-515:⚠️ Potential issue | 🟠 MajorDon’t treat the unchanged source key as proof that the rename already happened.
When only
$newTwoWayKeychanges,$actualNewKeystill points at the current source column. The orphan-recovery fallback will then mark$adapterUpdated = trueas soon as it sees that existing column, even if the related/junction rename never happened.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Relationships.php` around lines 493 - 515, The recovery logic in Relationships.php incorrectly treats finding $actualNewKey in schema as proof the rename completed; instead, when catching the Throwable you must verify the rename truly occurred by checking that the new column exists AND the original source column was removed (or that the two-way/junction column change also exists) — use getSchemaAttributes(), $this->adapter->filter($actualNewKey) and also compare against the filtered original/source key (e.g., filteredOldKey) and/or the filtered $newTwoWayKey to ensure the found column isn't simply the unchanged source; only set $adapterUpdated = true if the new name exists AND the old name does not (or the corresponding two-way column was updated), otherwise rethrow the DatabaseException (preserving $e as previous).src/Database/Traits/Documents.php-970-975 (1)
970-975:⚠️ Potential issue | 🟠 MajorUse the hook's returned document in bulk updates.
updateDocument()reassignsafterDocumentUpdate(), butupdateDocuments()ignores the returned value. Any relationship hook that rewrites the payload will be skipped on this path.🔧 Proposed fix
$hook = $this->relationshipHook; if ($hook?->isEnabled()) { - $this->silent(fn () => $hook->afterDocumentUpdate($collection, $document, $new)); + $new = $this->silent(fn () => $hook->afterDocumentUpdate($collection, $document, $new)); } $document = $new;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Documents.php` around lines 970 - 975, The bulk-update path is ignoring the relationship hook's return value: in the block using $this->relationshipHook and calling $hook->afterDocumentUpdate($collection, $document, $new), capture and use the hook's returned document (replace $new with the hook return when non-null) before assigning to $document so any payload rewrites by afterDocumentUpdate are preserved; update the logic around relationshipHook/isEnabled and the assignment to $document to use the hook's return value (from afterDocumentUpdate) instead of discarding it.src/Database/Traits/Documents.php-401-409 (1)
401-409:⚠️ Potential issue | 🟠 MajorRun
castingAfter()on single-document creates even when hooks are off.This path only normalizes adapter output inside the relationship-hook branch.
MariaDB::createDocument()returns the pre-cast document instance, so a single create can return DB-shaped values whilecreateDocuments()returns normalized ones.🔧 Proposed fix
$hook = $this->relationshipHook; if ($hook !== null && !$hook->isInBatchPopulation() && $hook->isEnabled()) { $fetchDepth = $hook->getWriteStackCount(); $documents = $this->silent(fn () => $hook->populateDocuments([$document], $collection, $fetchDepth)); - $document = $this->adapter->castingAfter($collection, $documents[0]); + $document = $documents[0]; } + $document = $this->adapter->castingAfter($collection, $document); $document = $this->casting($collection, $document); $document = $this->decode($collection, $document);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Documents.php` around lines 401 - 409, The current code only runs $this->adapter->castingAfter(...) when $this->relationshipHook is present and enabled, causing single-document creates to return DB-shaped values; move or duplicate the adapter post-processing so castingAfter is applied for single-document flows regardless of the relationship hook state: ensure that after the potential hook population block you call $this->adapter->castingAfter($collection, $document) (using the same normalized $documents[0] result when hook ran) before running $this->casting(...) and $this->decode(...), so both createDocument() and createDocuments() return consistently normalized documents; use the existing symbols relationshipHook, populateDocuments, castingAfter, casting, decode and silent to implement this change.src/Database/Traits/Documents.php-274-301 (1)
274-301:⚠️ Potential issue | 🟠 MajorPreserve relationship roots when pruning selected fields.
$attributesToKeeponly records the literal selector. A query likeselect(['author.name'])will therefore remove the populatedauthorattribute itself, and aliased projections are dropped for the same reason. Please retain the root segment of dotted paths, plus any projection alias, before removing attributes.Based on learnings: Repo utopia-php/database: Relationship selects are always evaluated in the main alias context (no per-collection aliasing). In Utopia\Database\Database::applySelectFiltersToDocuments, do not rely on Query::getAlias() for relationships; instead, preserve the root of dotted paths and any projection alias (Query::getAs()) when filtering attributes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Documents.php` around lines 274 - 301, The pruning logic in applySelectFiltersToDocuments builds $attributesToKeep from selectQueries by using literal selectors only, which drops relationship root attributes and aliased projections (e.g., select(['author.name']) or projections with Query::getAs()). Update the loop that populates $attributesToKeep (the foreach over $selectQueries and getValues()) to also: for each selector string preserve its root segment (the part before the first dot) and, if Query::getAs() is present for that query, preserve the alias name as well; do not rely on Query::getAlias() for relationship selects. Ensure these root keys and projection aliases are added to $attributesToKeep before the wildcard/internal-key checks so removeAttribute only strips truly unselected fields.src/Database/Traits/Documents.php-1174-1215 (1)
1174-1215:⚠️ Potential issue | 🟠 MajorDon't drop brand-new empty documents as a no-op.
When
$oldis empty and the incoming document has no user fields/operators,$hasChangesstays false and the item is removed from the batch.upsertDocument()then falls back togetDocument()and returns an empty document instead of creating a valid empty record.🔧 Proposed fix
- if (!$hasChanges) { + if (!$hasChanges && !$old->isEmpty()) { // If not updating a single attribute and the document is the same as the old one, skip it unset($documents[$key]); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Documents.php` around lines 1174 - 1215, The current change-detection logic in the Documents trait can wrongly treat brand-new empty incoming docs as unchanged and drop them from $documents[$key], because when $old is empty and regularUpdatesUserOnly and $operators are empty $hasChanges remains false; update the logic in the block that computes $hasChanges (the checks that reference $operators, $attribute, $skipPermissionsUpdate, $old, regularUpdatesUserOnly, self::INTERNAL_ATTRIBUTES) to explicitly mark a document as changed when $old is null/empty (new record) even if no user fields/operators are present so upsertDocument() will create the empty record instead of falling back to getDocument(); ensure this new case still respects internal attribute filtering and the subsequent unset($documents[$key]) skip only applies to truly identical existing documents.src/Database/Adapter/MariaDB.php-603-609 (1)
603-609:⚠️ Potential issue | 🟠 MajorUse side-aware junction naming when renaming many-to-many columns.
deleteRelationship()computes the junction table name differently for parent vs child sides, butupdateRelationship()always uses<collection>_<related>. Child-side renames will target the wrong junction table.🔧 Proposed fix
- $junctionName = '_' . $collection->getSequence() . '_' . $relatedCollection->getSequence(); + $junctionName = $side === RelationSide::Parent + ? '_' . $collection->getSequence() . '_' . $relatedCollection->getSequence() + : '_' . $relatedCollection->getSequence() . '_' . $collection->getSequence();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/MariaDB.php` around lines 603 - 609, updateRelationship() is using a fixed junction name ('_' . $collection->getSequence() . '_' . $relatedCollection->getSequence()) which ignores the relationship side and causes child-side renames to target the wrong junction table; change the junction name computation in updateRelationship() to mirror the logic used in deleteRelationship() (i.e., compute $junctionName based on the $side or whichever variable deleteRelationship() uses to decide order of $collection->getSequence() and $relatedCollection->getSequence()), then use that side-aware $junctionName when calling $renameCol for $key/$newKey and $twoWay/$newTwoWayKey so renames operate on the correct many-to-many table.src/Database/Adapter/MariaDB.php-1211-1228 (1)
1211-1228:⚠️ Potential issue | 🟠 MajorAdd handlers for
TYPE_COVERS,TYPE_NOT_COVERS,TYPE_SPATIAL_EQUALS, andTYPE_NOT_SPATIAL_EQUALSin the spatial query match statement.The validator in
src/Database/Validator/Queries.phpallows these four new spatial query types, but the match statement insrc/Database/Adapter/MariaDB.php(lines 1211-1228) does not handle them. These queries will throwUnknown spatial query methodat runtime when executed. The same issue exists insrc/Database/Adapter/Postgres.php.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/MariaDB.php` around lines 1211 - 1228, The match in MariaDB.php handling spatial query methods misses Query::TYPE_COVERS, Query::TYPE_NOT_COVERS, Query::TYPE_SPATIAL_EQUALS and Query::TYPE_NOT_SPATIAL_EQUALS; add match arms that map TYPE_COVERS to "ST_Covers({$alias}.{$attribute}, {$geom})", TYPE_NOT_COVERS to "NOT ST_Covers(...)", TYPE_SPATIAL_EQUALS to "ST_SpatialEquals({$alias}.{$attribute}, {$geom})" and TYPE_NOT_SPATIAL_EQUALS to "NOT ST_SpatialEquals(...)" (use the same pattern/templating as the existing arms such as Query::TYPE_CONTAINS and Query::TYPE_EQUAL), and apply the identical changes to the corresponding match in Postgres.php so both adapters support the four new Query types.src/Database/Adapter/Mongo.php-1064-1069 (1)
1064-1069:⚠️ Potential issue | 🟠 MajorResolve partial-index types by attribute name, not loop offset.
Line 1165 builds
$indexAttributeTypesas[$attrName => $type], but Line 1067 reads$indexAttributeTypes[$i]. That falls back tostringfor numeric/date fields and can exclude the real field type from the partial index entirely.Suggested fix
if (in_array($type, [IndexType::Unique, IndexType::Key])) { $partialFilter = []; foreach ($attributes as $i => $attr) { - $attrType = $indexAttributeTypes[$i] ?? ColumnType::String->value; // Default to string if type not provided + $sourceAttribute = $index->attributes[$i] ?? $attr; + $attrType = $indexAttributeTypes[$sourceAttribute] ?? ColumnType::String->value; $attrType = $this->getMongoTypeCode($attrType); $partialFilter[$attr] = ['$exists' => true, '$type' => $attrType]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Mongo.php` around lines 1064 - 1069, The partial index builder in Mongo.php is using the loop index to look up attribute types but $indexAttributeTypes is keyed by attribute name; update the lookup in the loop that constructs $partialFilter (in the block handling IndexType::Unique and IndexType::Key) to use the attribute name ($attr) as the key instead of $i, e.g. fetch the type with $indexAttributeTypes[$attr] ?? ColumnType::String->value and then pass that into $this->getMongoTypeCode so the correct field type is used for the partial index filter.
🟡 Minor comments (12)
docker-compose.yml-6-7 (1)
6-7:⚠️ Potential issue | 🟡 MinorDocument local development setup requirement for sibling
queryrepository.The volume mount
../query/src:/usr/src/code/vendor/utopia-php/query/srcand build context..assume a specific local directory structure where thequeryrepository exists as a sibling. This dependency is not documented in CONTRIBUTING.md. While the volume mount failure is non-fatal in CI (the container starts regardless), local development withdocker compose up -d --buildwill fail without the siblingqueryrepository, making setup unclear for new contributors.Also applies to: 21-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 6 - 7, The docker-compose configuration uses a sibling repo via the volume mount '../query/src:/usr/src/code/vendor/utopia-php/query/src' and a build context '..' (with dockerfile database/Dockerfile), which must be documented; update CONTRIBUTING.md to state that the `query` repository must exist as a sibling (or provide steps to clone it) before running `docker compose up -d --build`, note the mount is optional for CI and point to the alternative (remove/disable the volume or use a docker-compose.override.yml) for contributors who cannot have the sibling repo, and mirror this documentation for the other occurrence referenced (line 21) so local dev setup is explicit.README.md-636-638 (1)
636-638:⚠️ Potential issue | 🟡 MinorAdd import statement for
ForeignKeyActionin the example.The example shows
ForeignKeyAction::Cascade->valuebut doesn't include the necessaryusestatement. This could confuse developers trying to use the code.📝 Suggested addition before line 636
use Utopia\Database\ForeignKeyAction;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 636 - 638, Add a use/import for the ForeignKeyAction enum in the example so the symbols like ForeignKeyAction::Cascade->value, ForeignKeyAction::SetNull->value, and ForeignKeyAction::Restrict->value resolve; specifically, add the statement to import the class (e.g., use Utopia\Database\ForeignKeyAction;) near the top of the snippet where the example code begins so ForeignKeyAction is available in that example context.README.md-758-776 (1)
758-776:⚠️ Potential issue | 🟡 MinorAdd import statement for
SetTypein the example.Similar to
ForeignKeyAction, theSetTypeenum usage examples should include the import statement for clarity.📝 Suggested addition before line 758
use Utopia\Database\SetType;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 758 - 776, Add the missing import for the SetType enum used in the README examples by inserting the statement importing SetType (e.g., use Utopia\Database\SetType;) near the other example imports so the usages of SetType::Assign / SetType::Append / SetType::Prepend in the setAttribute examples resolve; ensure it appears alongside existing imports like Permission and Role so the sample snippet is self-contained and clear.src/Database/Relationship.php-21-31 (1)
21-31:⚠️ Potential issue | 🟡 Minor
toDocument()omitscollectionandkeyproperties.The
toDocument()method doesn't serialize$this->collectionor$this->key, butfromDocument()accepts both as inputs (collection as parameter, key from attribute). This asymmetry may cause data loss during round-trip serialization if these fields need to be preserved.If this is intentional (because these fields come from external context), consider documenting this behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Relationship.php` around lines 21 - 31, The toDocument() method on Relationship currently omits $this->collection and $this->key while fromDocument() (and the static fromDocument method) accept collection and key, causing asymmetrical serialization; modify Relationship::toDocument() to include 'collection' => $this->collection and 'key' => $this->key in the returned Document (or, if omission is intentional, add a clear docblock on Relationship::toDocument() and fromDocument() explaining that collection and key are provided externally and therefore not persisted) so round-trip serialization is consistent.src/Database/Hook/RelationshipHandler.php-243-243 (1)
243-243:⚠️ Potential issue | 🟡 MinorUnused loop variable
$index.The
$indexvariable from the foreach loop is declared but never used. This was correctly flagged by static analysis.🔧 Proposed fix
- foreach ($relationships as $index => $relationship) { + foreach ($relationships as $relationship) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Hook/RelationshipHandler.php` at line 243, The foreach in RelationshipHandler.php declares an unused loop variable $index; change the loop signature from foreach ($relationships as $index => $relationship) to foreach ($relationships as $relationship) (or use foreach ($relationships as $_ => $relationship) if you prefer an explicit unused key) so the unused $index is removed, keeping the loop body using $relationship unchanged.src/Database/Hook/RelationshipHandler.php-1803-1814 (1)
1803-1814:⚠️ Potential issue | 🟡 MinorSame shadowing issue:
$documentin deleteCascade.The loop variable
$documentshadows the method parameter, same issue as indeleteSetNull. The loop iterates over junction documents while the original parameter is the document being deleted.🔧 Rename loop variable
- foreach ($junctions as $document) { + foreach ($junctions as $junctionDoc) { if ($side === RelationSide::Parent->value) { $this->db->deleteDocument( $relatedCollection->getId(), - $document->getAttribute($key) + $junctionDoc->getAttribute($key) ); } $this->db->deleteDocument( $junction, - $document->getId() + $junctionDoc->getId() ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Hook/RelationshipHandler.php` around lines 1803 - 1814, In deleteCascade, the foreach loop uses $document which shadows the method parameter $document; rename the loop variable (e.g., to $junctionDocument or $junctionDoc) wherever it's used within the loop so the parameter $document remains the original document being deleted, and update the two deleteDocument calls to reference the renamed loop variable ($junctionDocument->getAttribute($key) and $junctionDocument->getId()) while leaving the method parameter untouched.src/Database/Attribute.php-76-92 (1)
76-92:⚠️ Potential issue | 🟡 Minor
fromArray()omitsstatusandoptionsfields.Unlike
fromDocument(), thefromArray()method doesn't extractstatusandoptionsfrom the input array. This asymmetry could cause data loss when attributes with these fields are serialized viatoDocument()and then reconstructed viafromArray().🔧 Proposed fix to include status and options
public static function fromArray(array $data): self { $type = $data['type'] ?? 'string'; return new self( key: $data['$id'] ?? $data['key'] ?? '', type: $type instanceof ColumnType ? $type : ColumnType::from($type), size: $data['size'] ?? 0, required: $data['required'] ?? false, default: $data['default'] ?? null, signed: $data['signed'] ?? true, array: $data['array'] ?? false, format: $data['format'] ?? null, formatOptions: $data['formatOptions'] ?? [], filters: $data['filters'] ?? [], + status: $data['status'] ?? null, + options: $data['options'] ?? null, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Attribute.php` around lines 76 - 92, fromArray() currently ignores the 'status' and 'options' keys so reconstructing an Attribute via Attribute::fromArray loses data present in toDocument()/fromDocument(); update Attribute::fromArray to read 'status' and 'options' from the input array (e.g. $data['status'] ?? <sensible default> and $data['options'] ?? <default empty array>) and pass them into the Attribute constructor (matching how fromDocument() does), ensuring defaults mirror fromDocument/toDocument behavior.src/Database/Hook/RelationshipHandler.php-272-283 (1)
272-283:⚠️ Potential issue | 🟡 MinorLoose equality comparison may cause unexpected behavior.
Using
==for comparing$oldValueand$value(Line 272) can lead to unexpected type coercion, especially when comparing Documents, arrays, or mixed types. This could cause relationship updates to be incorrectly skipped.🔧 Consider strict comparison or dedicated comparison logic
- if ($oldValue == $value) { + if ($this->areRelationshipValuesEqual($oldValue, $value)) {Alternatively, if loose comparison is intentional for this specific use case, add a comment explaining why.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Hook/RelationshipHandler.php` around lines 272 - 283, The loose comparison using $oldValue == $value in RelationshipHandler (around the block handling RelationType and RelationSide) can produce incorrect skips; change this to strict comparison or explicit comparison logic: use === for scalar checks, if both values are Document instances compare their IDs (e.g., $oldValue->getId() === $value->getId()), and for arrays use a deep equality check (or serialize/sort then compare) before deciding to set or remove attributes via $document->setAttribute/$document->removeAttribute; if loose comparison was intentional, replace the == with a clear comment explaining why and what cases rely on coercion.src/Database/Hook/RelationshipHandler.php-1727-1732 (1)
1727-1732:⚠️ Potential issue | 🟡 MinorParameter shadowing:
$documentreused in loop.The foreach loop variable
$documentshadows the method parameter$document(from Line 1640), which could cause confusion or unintended behavior if the original document is needed after the loop.🔧 Rename loop variable
- foreach ($junctions as $document) { - $this->db->skipRelationships(fn () => $this->db->deleteDocument( - $junction, - $document->getId() - )); + foreach ($junctions as $junctionDoc) { + $this->db->skipRelationships(fn () => $this->db->deleteDocument( + $junction, + $junctionDoc->getId() + )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Hook/RelationshipHandler.php` around lines 1727 - 1732, The foreach loop reuses the variable name $document which shadows the method parameter $document; rename the loop variable (e.g., $junctionDocument or $junctionItem) and update its usage inside the loop so the method parameter remains available unchanged, keeping the existing calls to $this->db->skipRelationships(fn () => $this->db->deleteDocument($junction, <loop-var>->getId())); ensure only the loop variable name is changed (no other logic altered).src/Database/Traits/Indexes.php-359-366 (1)
359-366:⚠️ Potential issue | 🟡 MinorFix rollback index TTL default: should be
0, not1, to match TTL semantics elsewhere.The rollback index uses
getAttribute('ttl', 1)as default, but throughout the codebase TTL semantics treat0(or missing attribute) as "no TTL configured". InDocuments.php:247, the code explicitly checksif ($ttlSeconds <= 0 || !$ttlAttr)to determine whether to skip TTL processing. Additionally, theValidator/Index.php:810usesgetAttribute('ttl', 0)as default. Using1as the default means if the original index had no TTL attribute, the rollback would incorrectly create an index with a 1-second TTL, causing documents to expire prematurely. Change the default to0to preserve the "no TTL" state when the attribute is missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Traits/Indexes.php` around lines 359 - 366, The rollback index creation uses getAttribute('ttl', 1) which incorrectly sets a 1s TTL when the attribute is missing; change the default to 0 so $rollbackIndex is created with no TTL when absent by updating the call on $indexDeleted->getAttribute('ttl', 1) to use 0 (the code building the Index instance with IndexType::from(...) and attributes/lengths/orders should remain the same) to match Validator/Index.php and Documents.php TTL semantics.src/Database/Adapter/SQLite.php-43-70 (1)
43-70:⚠️ Potential issue | 🟡 MinorSQLite advertises it does not support
Capability::Upsertsbut still implements upsert functionality.Line 62 removes
Capability::Upsertsfrom the advertised capabilities, yet SQLite inheritsupsertDocuments()from SQL and overridesexecuteUpsertBatch()to handle SQLite's ON CONFLICT syntax. This creates an inconsistency: the capability advertisement does not match the runtime behavior. While this doesn't currently cause dead code (no code gate upsert calls onsupports(Capability::Upserts)), the mismatch violates the contract that removed capabilities should not be implemented. Either re-addCapability::Upsertsto SQLite's capabilities, or remove theexecuteUpsertBatch()override if SQLite should not support upserts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/SQLite.php` around lines 43 - 70, The SQLite adapter currently removes Capability::Upserts in capabilities() while still implementing upsertDocuments() and overriding executeUpsertBatch(), causing a contract mismatch; fix by either (A) re-adding support: remove Capability::Upserts from the $remove array in capabilities() so parent::capabilities() reports Upserts, or (B) disable runtime support: delete or revert the SQLite::executeUpsertBatch() override (and any SQLite-specific upsert helpers) so the adapter no longer implements upsert behavior — choose one approach and make the changes consistently (references: capabilities(), Capability::Upserts, upsertDocuments(), executeUpsertBatch(), parent::capabilities()).src/Database/Adapter/Mongo.php-1081-1081 (1)
1081-1081:⚠️ Potential issue | 🟡 MinorRemove the
->valueaccessor to fix the unreachable readiness loop.At line 1081,
$typeis anIndexTypeenum case (assigned from$index->typeat line 44), but the comparison$type === IndexType::Unique->valuecompares an enum case object to its backing scalar value. This will never match in PHP—enum cases and their backing values are distinct types. As a result, the readiness loop is unreachable and the code never waits for unique indexes to be fully built.The fix is to compare the enum case directly:
Fix
- if ($type === IndexType::Unique->value) { + if ($type === IndexType::Unique) {This is consistent with all other enum comparisons in the same function (lines 1020, 1042, 1050, 1055, 1060), which use enum cases without the
->valueaccessor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Mongo.php` at line 1081, The readiness loop is unreachable because it compares the enum case object $type to the backing scalar of IndexType::Unique (using ->value); locate the comparison of $type and IndexType::Unique in the readiness/wait loop and remove the ->value accessor so the code compares the enum case directly (i.e., use IndexType::Unique) to match how other enum checks in this function handle $type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3dfcd406-2dfe-4455-a2f0-baf7fa17d195
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (128)
.github/workflows/tests.ymlDockerfileREADME.mdbin/tasks/relationships.phpcomposer.jsondocker-compose.ymlsrc/Database/Adapter.phpsrc/Database/Adapter/Feature/Attributes.phpsrc/Database/Adapter/Feature/Collections.phpsrc/Database/Adapter/Feature/ConnectionId.phpsrc/Database/Adapter/Feature/Databases.phpsrc/Database/Adapter/Feature/Documents.phpsrc/Database/Adapter/Feature/Indexes.phpsrc/Database/Adapter/Feature/InternalCasting.phpsrc/Database/Adapter/Feature/Relationships.phpsrc/Database/Adapter/Feature/SchemaAttributes.phpsrc/Database/Adapter/Feature/Spatial.phpsrc/Database/Adapter/Feature/Timeouts.phpsrc/Database/Adapter/Feature/Transactions.phpsrc/Database/Adapter/Feature/UTCCasting.phpsrc/Database/Adapter/Feature/Upserts.phpsrc/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/Mongo/RetryClient.phpsrc/Database/Adapter/MySQL.phpsrc/Database/Adapter/Pool.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Adapter/SQLite.phpsrc/Database/Attribute.phpsrc/Database/Capability.phpsrc/Database/CursorDirection.phpsrc/Database/Database.phpsrc/Database/Document.phpsrc/Database/Helpers/Permission.phpsrc/Database/Hook/MongoPermissionFilter.phpsrc/Database/Hook/MongoTenantFilter.phpsrc/Database/Hook/PermissionFilter.phpsrc/Database/Hook/PermissionWrite.phpsrc/Database/Hook/Read.phpsrc/Database/Hook/Relationship.phpsrc/Database/Hook/RelationshipHandler.phpsrc/Database/Hook/TenantFilter.phpsrc/Database/Hook/TenantWrite.phpsrc/Database/Hook/Write.phpsrc/Database/Hook/WriteContext.phpsrc/Database/Index.phpsrc/Database/Mirror.phpsrc/Database/Operator.phpsrc/Database/OperatorType.phpsrc/Database/OrderDirection.phpsrc/Database/PermissionType.phpsrc/Database/Query.phpsrc/Database/RelationSide.phpsrc/Database/RelationType.phpsrc/Database/Relationship.phpsrc/Database/SetType.phpsrc/Database/Traits/Attributes.phpsrc/Database/Traits/Collections.phpsrc/Database/Traits/Databases.phpsrc/Database/Traits/Documents.phpsrc/Database/Traits/Indexes.phpsrc/Database/Traits/Relationships.phpsrc/Database/Traits/Transactions.phpsrc/Database/Validator/Attribute.phpsrc/Database/Validator/Datetime.phpsrc/Database/Validator/Index.phpsrc/Database/Validator/IndexedQueries.phpsrc/Database/Validator/Operator.phpsrc/Database/Validator/Permissions.phpsrc/Database/Validator/Queries.phpsrc/Database/Validator/Queries/Document.phpsrc/Database/Validator/Queries/Documents.phpsrc/Database/Validator/Query/Filter.phpsrc/Database/Validator/Query/Limit.phpsrc/Database/Validator/Query/Offset.phpsrc/Database/Validator/Sequence.phpsrc/Database/Validator/Spatial.phpsrc/Database/Validator/Structure.phptests/e2e/Adapter/Base.phptests/e2e/Adapter/MariaDBTest.phptests/e2e/Adapter/MirrorTest.phptests/e2e/Adapter/MongoDBTest.phptests/e2e/Adapter/MySQLTest.phptests/e2e/Adapter/PoolTest.phptests/e2e/Adapter/PostgresTest.phptests/e2e/Adapter/SQLiteTest.phptests/e2e/Adapter/Schemaless/MongoDBTest.phptests/e2e/Adapter/Scopes/AttributeTests.phptests/e2e/Adapter/Scopes/CollectionTests.phptests/e2e/Adapter/Scopes/CustomDocumentTypeTests.phptests/e2e/Adapter/Scopes/DocumentTests.phptests/e2e/Adapter/Scopes/GeneralTests.phptests/e2e/Adapter/Scopes/IndexTests.phptests/e2e/Adapter/Scopes/ObjectAttributeTests.phptests/e2e/Adapter/Scopes/OperatorTests.phptests/e2e/Adapter/Scopes/PermissionTests.phptests/e2e/Adapter/Scopes/RelationshipTests.phptests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.phptests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.phptests/e2e/Adapter/Scopes/Relationships/OneToManyTests.phptests/e2e/Adapter/Scopes/Relationships/OneToOneTests.phptests/e2e/Adapter/Scopes/SchemalessTests.phptests/e2e/Adapter/Scopes/SpatialTests.phptests/e2e/Adapter/Scopes/VectorTests.phptests/e2e/Adapter/SharedTables/MariaDBTest.phptests/e2e/Adapter/SharedTables/MongoDBTest.phptests/e2e/Adapter/SharedTables/MySQLTest.phptests/e2e/Adapter/SharedTables/PostgresTest.phptests/e2e/Adapter/SharedTables/SQLiteTest.phptests/unit/DocumentTest.phptests/unit/OperatorTest.phptests/unit/PermissionTest.phptests/unit/QueryTest.phptests/unit/Validator/AttributeTest.phptests/unit/Validator/AuthorizationTest.phptests/unit/Validator/DocumentQueriesTest.phptests/unit/Validator/DocumentsQueriesTest.phptests/unit/Validator/IndexTest.phptests/unit/Validator/IndexedQueriesTest.phptests/unit/Validator/OperatorTest.phptests/unit/Validator/QueriesTest.phptests/unit/Validator/Query/FilterTest.phptests/unit/Validator/Query/OrderTest.phptests/unit/Validator/Query/SelectTest.phptests/unit/Validator/QueryTest.phptests/unit/Validator/SpatialTest.phptests/unit/Validator/StructureTest.php
| protected function applyReadFilters(array $filters, string $collection, string $forPermission = 'read'): array | ||
| { | ||
| foreach ($this->readHooks as $hook) { | ||
| $filters = $hook->applyFilters($filters, $collection, $forPermission); | ||
| } | ||
| return $filters; |
There was a problem hiding this comment.
Initialize read hooks before applying filters.
Line 1583, Line 1623, Line 1888, Line 1932, and Line 1958 call this helper without a prior syncReadHooks(). On a fresh adapter instance $readHooks is still empty, so tenant and permission predicates are skipped on update/delete paths.
Suggested fix
protected function applyReadFilters(array $filters, string $collection, string $forPermission = 'read'): array
{
+ $this->syncReadHooks();
foreach ($this->readHooks as $hook) {
$filters = $hook->applyFilters($filters, $collection, $forPermission);
}
return $filters;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function applyReadFilters(array $filters, string $collection, string $forPermission = 'read'): array | |
| { | |
| foreach ($this->readHooks as $hook) { | |
| $filters = $hook->applyFilters($filters, $collection, $forPermission); | |
| } | |
| return $filters; | |
| protected function applyReadFilters(array $filters, string $collection, string $forPermission = 'read'): array | |
| { | |
| $this->syncReadHooks(); | |
| foreach ($this->readHooks as $hook) { | |
| $filters = $hook->applyFilters($filters, $collection, $forPermission); | |
| } | |
| return $filters; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/Mongo.php` around lines 127 - 132, applyReadFilters is
invoked before syncReadHooks is guaranteed to have run, so on a fresh adapter
$readHooks can be empty and tenant/permission predicates are skipped; modify
applyReadFilters (method name applyReadFilters in class Mongo) to ensure hooks
are initialized by calling $this->syncReadHooks() at the start when
$this->readHooks is empty (or unconditionally) before iterating, or
alternatively ensure every caller invokes syncReadHooks() first (update/delete
paths that call applyReadFilters); update references to the $readHooks property
and the syncReadHooks() method accordingly so tenant and permission filters are
always applied.
| public static function fromDocument(Document $document): self | ||
| { | ||
| return new self( | ||
| key: $document->getAttribute('key', $document->getId()), | ||
| type: IndexType::from($document->getAttribute('type', 'index')), | ||
| attributes: $document->getAttribute('attributes', []), | ||
| lengths: $document->getAttribute('lengths', []), | ||
| orders: $document->getAttribute('orders', []), | ||
| ttl: $document->getAttribute('ttl', 1), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
utopia-php/query IndexType enum cases
💡 Result:
As of utopia-php/query v0.1.1 (tag dated Mar 3, 2026), the repository only contains src/Query/Query.php and src/Query/Exception.php—there is no IndexType enum in this package, so there are no enum cases to list. [1][2]
If what you’re after is “index types” in the Utopia ecosystem, those are currently exposed in utopia-php/database as constants (not an enum): INDEX_KEY, INDEX_FULLTEXT, INDEX_UNIQUE, INDEX_SPATIAL, INDEX_ARRAY. [3]
Sources:
[1] https://github.com/utopia-php/query/tree/main/src/Query
[2] https://github.com/utopia-php/query/tags
[3] https://packagist.org/packages/utopia-php/database
🏁 Script executed:
# Search for IndexType definition in the codebase
rg "enum IndexType|class IndexType" -A 10Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check the imports in the Index.php file to see where IndexType comes from
head -30 src/Database/Index.phpRepository: utopia-php/database
Length of output: 797
🏁 Script executed:
# Search for all IndexType usages and definitions
rg "IndexType" -B 2 -A 2Repository: utopia-php/database
Length of output: 50376
🏁 Script executed:
# Find the IndexType enum definition
find . -type f -name "*.php" -exec grep -l "enum IndexType\|class IndexType" {} \;Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look in vendor if the enum is from utopia-php/query
find . -path "*/vendor/utopia-php/query*" -type f -name "*.php" 2>/dev/null | head -20Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Search for IndexType enum directly with ast-grep
ast-grep --pattern 'enum IndexType { $$$ }'Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check composer.json to see the dependency version
cat composer.json | grep -A 5 "utopia-php/query"Repository: utopia-php/database
Length of output: 233
🏁 Script executed:
# Search for 'type' => being set with string values in test files to see actual values used
rg "'type'\s*=>" tests/ -A 1 | grep -E "(type.*=>|IndexType)" | head -30Repository: utopia-php/database
Length of output: 3224
🏁 Script executed:
# Look at the toDocument method again and search for fromDocument usage
rg "fromDocument|'index'" src/ tests/ -B 2 -A 2Repository: utopia-php/database
Length of output: 11016
🏁 Script executed:
# Check if vendor contains utopia-php/query source files
find . -path "*/vendor/utopia-php/query/src*" -name "*.php" 2>/dev/null | head -10Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Search for any enum definition patterns with 'index' or case definitions
rg "case.*[Ii]ndex|'index'|\"index\"" --type php | grep -i "enum\|case\|const" | head -20Repository: utopia-php/database
Length of output: 1358
🏁 Script executed:
# Try searching for IndexType in any PHP files that might have definitions
rg "IndexType::" src/ tests/ --no-heading | grep -o "IndexType::[A-Za-z]*" | sort -uRepository: utopia-php/database
Length of output: 287
🏁 Script executed:
# Check if there's any special handling for 'index' string in the codebase
rg "'index'" src/Database/Index.php -B 5 -A 5Repository: utopia-php/database
Length of output: 556
🏁 Script executed:
# Look for IndexType backing values - check if it's a backed enum and what values it has
rg "enum IndexType|string.*IndexType" --type php -A 15 | head -40Repository: utopia-php/database
Length of output: 3567
🏁 Script executed:
# Search in vendor for IndexType definition
find . -type f -name "*.php" -exec grep -l "enum IndexType" {} \; 2>/dev/nullRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Search for the actual enum definition with values
rg "enum IndexType" -A 20 --type phpRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check what value IndexType::Key has - look at test assertions
rg "IndexType::Key->value" -B 2 -A 2 | head -20Repository: utopia-php/database
Length of output: 1371
🏁 Script executed:
# Look for any documentation or comments about the default index type
rg -i "default.*index.*type|index.*type.*default" src/ -B 2 -A 2Repository: utopia-php/database
Length of output: 2696
🏁 Script executed:
# Check the actual enum values by looking at how they're used in string context
rg "IndexType::[A-Z][a-z]*->value" src/ | head -5 | while read line; do echo "$line"; doneRepository: utopia-php/database
Length of output: 525
🏁 Script executed:
# Check if there's any test that exercises fromDocument with a missing 'type' attribute
rg "fromDocument" tests/ -B 3 -A 10 | grep -E "fromDocument|'type'|Document\(\[" | head -30Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Try to find what the actual string values are for IndexType cases
rg "IndexType::[A-Za-z]+->value" src/Database/Validator/Index.php -A 1 -B 1Repository: utopia-php/database
Length of output: 3870
🏁 Script executed:
# Look for any test that calls Index::fromDocument to see what values are passed
rg "Index::fromDocument|->fromDocument\(" tests/ -B 5 -A 5 | head -40Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Search for where documents are created with 'type' attribute for indexes
rg "Document\(\[" tests/ -A 10 | grep -B 3 -A 3 "'type'" | grep -E "type.*=>|Document" | head -20Repository: utopia-php/database
Length of output: 1718
🏁 Script executed:
# Look in the Mongo adapter where it uses tryFrom with fallback to Key - this shows the pattern
rg "IndexType::tryFrom.*IndexType::Key" src/ -B 3 -A 3Repository: utopia-php/database
Length of output: 891
🏁 Script executed:
# We need to find the actual backed enum values - let's check the utopia-php/query package
# Try looking at package.json or composer info to understand the structure better
cat composer.json | grep -A 3 "repositories"Repository: utopia-php/database
Length of output: 153
🏁 Script executed:
# Check if there are any error messages or exceptions that would be thrown with invalid index type
rg "from.*index" -i --type php src/ | head -10Repository: utopia-php/database
Length of output: 1005
🏁 Script executed:
# Let's check what string value IndexType::Key actually has by looking at how it's serialized
rg "->value" src/Database/Index.php -B 3 -A 3Repository: utopia-php/database
Length of output: 467
🏁 Script executed:
# Most importantly, let's see if there are any tests that would fail if 'index' is passed
rg "getAttribute.*type.*index" tests/ -iRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check the .. /query directory structure since it's a local path repository
ls -la ../query/src/ 2>/dev/null || echo "Query package not accessible"Repository: utopia-php/database
Length of output: 91
🏁 Script executed:
# Let's verify the critical issue: does IndexType::from() with 'index' string actually work or throw?
# Look for any error handling around IndexType::from() calls
rg "IndexType::from" src/ -B 2 -A 2Repository: utopia-php/database
Length of output: 2141
🏁 Script executed:
# Check if 'index' is handled specially anywhere
rg "'index'|\"index\"" src/Database/Index.php -C 5Repository: utopia-php/database
Length of output: 556
🏁 Script executed:
# Most importantly - look at the toDocument method to see what actual values get stored
cat src/Database/Index.php | head -50Repository: utopia-php/database
Length of output: 1305
🏁 Script executed:
# Look for what the actual backing value of IndexType::Key is by checking conversions
rg "IndexType::Key" src/ -A 1 -B 1 | grep -E "Key|->value|case" | head -15Repository: utopia-php/database
Length of output: 1907
🏁 Script executed:
# Check if there's any error handling around fromDocument calls
rg "fromDocument" src/ -B 3 -A 3 | grep -E "try|catch|fromDocument"Repository: utopia-php/database
Length of output: 1071
🏁 Script executed:
# Most importantly - does IndexType::from() throw an exception for invalid values?
# Check if there's any documentation or testing of what happens with invalid types
rg "ValueError|from\(" tests/ --type php | head -10Repository: utopia-php/database
Length of output: 404
🏁 Script executed:
# Let's look at what string values IndexType enum actually uses - check the Validator error messages
rg "Must be one of" src/ -A 1 | head -20Repository: utopia-php/database
Length of output: 1681
🏁 Script executed:
# Final verification: check if 'index' would actually cause an error by looking at test coverage
# Search for tests that test the default behavior of fromDocument
rg "fromDocument" tests/ -B 5 -A 5 | grep -E "fromDocument|Document\(\[" | head -20Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check what string value IndexType::Key actually is by looking at how stored values are compared
rg "getAttribute.*type" src/Database/Validator/Index.php | head -5Repository: utopia-php/database
Length of output: 620
🏁 Script executed:
# Verify the enum backing values - look for any case statements with string literals
python3 << 'EOF'
# Based on the evidence:
# 1. IndexType is a backed enum (PHP 8.1+)
# 2. Cases include: Key, Fulltext, Unique, Object, Trigram, Ttl, Spatial, HnswEuclidean, HnswCosine, HnswDot
# 3. The code uses IndexType::from() which requires exact string match
# 4. 'index' is not in the list of valid cases
# 5. If it's not a valid backing value, IndexType::from('index') will throw ValueError
print("Analysis: The default 'index' string is NOT a valid IndexType enum case.")
print("Valid cases found: Key, Fulltext, Unique, Object, Trigram, Ttl, Spatial, HnswEuclidean, HnswCosine, HnswDot")
print("\nIndexType::from('index') will throw ValueError because 'index' doesn't match any enum backing value.")
print("The likely intended default should be IndexType::Key or IndexType::Key->value")
EOFRepository: utopia-php/database
Length of output: 423
Fix the invalid default index type in fromDocument() method.
The fromDocument method defaults to 'index' when deserializing the type, but 'index' is not a valid IndexType enum case. Valid cases are: Key, Fulltext, Unique, Object, Trigram, Ttl, Spatial, HnswEuclidean, HnswCosine, and HnswDot.
IndexType::from('index') will throw a ValueError when called on a Document without a type attribute. Change the default to a valid backing value, such as IndexType::Key->value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Index.php` around lines 33 - 43, The fromDocument() factory
currently uses an invalid default string 'index' for IndexType causing
IndexType::from('index') to throw; update the default to a valid enum backing
value (e.g., use IndexType::Key->value) so the call becomes
IndexType::from($document->getAttribute('type', IndexType::Key->value)); keep
the rest of the parameters unchanged and ensure you reference the
Index::fromDocument method and IndexType enum when making the change.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Database/Adapter/Pool.php (1)
80-88: Use the short class name for consistency.Line 80 uses the fully qualified class name
\Utopia\Database\Capabilityeven thoughCapabilityis already imported at line 7. Use the short name for consistency with the rest of the file.Suggested fix
- public function supports(\Utopia\Database\Capability $feature): bool + public function supports(Capability $feature): bool { return $this->delegate(__FUNCTION__, \func_get_args()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Pool.php` around lines 80 - 88, Change the fully-qualified parameter type in the supports method to the short imported class name: replace "\Utopia\Database\Capability $feature" with "Capability $feature" in the supports method signature so it matches the rest of the file and uses the imported Capability symbol; ensure the method still calls $this->delegate(__FUNCTION__, func_get_args()) unchanged.src/Database/Adapter/SQLite.php (1)
247-248: Remove the duplicated_permsindex creation.These same two indexes are already created on Lines 247-248. Repeating them here just adds extra catalog probes and will become observable if index-create hooks/events are added later.
♻️ Proposed cleanup
- $this->createIndex("{$id}_perms", new Index(key: '_index_1', type: IndexType::Unique, attributes: ['_document', '_type', '_permission'])); - $this->createIndex("{$id}_perms", new Index(key: '_index_2', type: IndexType::Key, attributes: ['_permission', '_type']));Also applies to: 265-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/SQLite.php` around lines 247 - 248, The code calls $this->createIndex twice for the same "{$id}_perms" table with identical Index(...) definitions (using Index(key: '_index_1', type: IndexType::Unique, attributes: ['_document','_type','_permission']) and Index(key: '_index_2', type: IndexType::Key, attributes: ['_permission','_type'])), causing duplicate index creation; remove the duplicated createIndex calls (the repeated "{$id}_perms" invocations) so each index is created only once, leaving a single call to createIndex for each Index definition (references: createIndex, Index, IndexType).
🤖 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/Adapter/SQLite.php`:
- Around line 45-65: The capabilities() method currently removes
Capability::Upserts from the $remove list which causes
supports(Capability::Upserts) to return false even though this class implements
SQLite-specific ON CONFLICT expressions and executeUpsertBatch(); fix by
removing Capability::Upserts from the $remove array (so Upserts is reported as
supported) and ensure any related logic in executeUpsertBatch() and
conflict-expression generation remains intact to be exercised when
supports(Capability::Upserts) is checked.
In `@src/Database/Mirror.php`:
- Around line 1074-1086: The setRelationshipHook method in Mirror currently
discards the caller's RelationshipHook by always installing a new
RelationshipHandler on the source/destination; instead, forward the provided
$hook (or null) directly to the underlying databases. In the
Mirror::setRelationshipHook(?RelationshipHook $hook) implementation replace the
calls that pass new RelationshipHandler($this->source)/new
RelationshipHandler($this->destination) with the original $hook (or null) so the
caller's custom RelationshipHook is preserved when calling
$this->source->setRelationshipHook(...) and
$this->destination->setRelationshipHook(...).
- Around line 321-333: The code calls Attribute::fromDocument($document) after
running filters in $this->writeFilters, but Filter::beforeCreateAttribute() can
return null to indicate the attribute should be skipped; update the blocks that
call beforeCreateAttribute (e.g., the loop using $this->writeFilters and the
code that assigns $filteredAttribute) to check if $document is null after the
loop and, if so, skip further processing (do not call Attribute::fromDocument) —
apply the same null-check/skipping logic to the other symmetric block that
processes attributes (the second occurrence mentioned around the attribute
handling).
---
Nitpick comments:
In `@src/Database/Adapter/Pool.php`:
- Around line 80-88: Change the fully-qualified parameter type in the supports
method to the short imported class name: replace "\Utopia\Database\Capability
$feature" with "Capability $feature" in the supports method signature so it
matches the rest of the file and uses the imported Capability symbol; ensure the
method still calls $this->delegate(__FUNCTION__, func_get_args()) unchanged.
In `@src/Database/Adapter/SQLite.php`:
- Around line 247-248: The code calls $this->createIndex twice for the same
"{$id}_perms" table with identical Index(...) definitions (using Index(key:
'_index_1', type: IndexType::Unique, attributes:
['_document','_type','_permission']) and Index(key: '_index_2', type:
IndexType::Key, attributes: ['_permission','_type'])), causing duplicate index
creation; remove the duplicated createIndex calls (the repeated "{$id}_perms"
invocations) so each index is created only once, leaving a single call to
createIndex for each Index definition (references: createIndex, Index,
IndexType).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6220a957-b838-4386-98ab-e1a755c06239
📒 Files selected for processing (7)
.gitignoresrc/Database/Adapter/Pool.phpsrc/Database/Adapter/SQLite.phpsrc/Database/Database.phpsrc/Database/Mirror.phptests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.phptests/e2e/Adapter/Scopes/SchemalessTests.php
✅ Files skipped from review due to trivial changes (1)
- .gitignore
| $remove = [ | ||
| Capability::Schemas, | ||
| Capability::Fulltext, | ||
| Capability::MultipleFulltextIndexes, | ||
| Capability::Regex, | ||
| Capability::PCRE, | ||
| Capability::UpdateLock, | ||
| Capability::AlterLock, | ||
| Capability::BatchCreateAttributes, | ||
| Capability::QueryContains, | ||
| Capability::Hostname, | ||
| Capability::AttributeResizing, | ||
| Capability::SpatialIndexOrder, | ||
| Capability::OptionalSpatial, | ||
| Capability::SchemaAttributes, | ||
| Capability::Spatial, | ||
| Capability::Relationships, | ||
| Capability::Upserts, | ||
| Capability::Timeouts, | ||
| Capability::ConnectionId, | ||
| ]; |
There was a problem hiding this comment.
Keep Capability::Upserts aligned with the new ON CONFLICT path.
capabilities() removes Capability::Upserts, but this class now adds SQLite-specific conflict expressions and executeUpsertBatch(). Any caller that checks supports(Capability::Upserts) will still treat SQLite as unsupported and bypass this new path.
🔧 Minimal fix
- Capability::Upserts,Also applies to: 1465-1673
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/SQLite.php` around lines 45 - 65, The capabilities()
method currently removes Capability::Upserts from the $remove list which causes
supports(Capability::Upserts) to return false even though this class implements
SQLite-specific ON CONFLICT expressions and executeUpsertBatch(); fix by
removing Capability::Upserts from the $remove array (so Upserts is reported as
supported) and ensure any related logic in executeUpsertBatch() and
conflict-expression generation remains intact to be exercised when
supports(Capability::Upserts) is checked.
| $document = $attribute->toDocument(); | ||
|
|
||
| foreach ($this->writeFilters as $filter) { | ||
| $document = $filter->beforeCreateAttribute( | ||
| source: $this->source, | ||
| destination: $this->destination, | ||
| collectionId: $collection, | ||
| attributeId: $id, | ||
| attributeId: $attribute->key, | ||
| attribute: $document, | ||
| ); | ||
| } | ||
|
|
||
| $result = $this->destination->createAttribute( | ||
| $collection, | ||
| $document->getId(), | ||
| $document->getAttribute('type'), | ||
| $document->getAttribute('size'), | ||
| $document->getAttribute('required'), | ||
| $document->getAttribute('default'), | ||
| $document->getAttribute('signed'), | ||
| $document->getAttribute('array'), | ||
| $document->getAttribute('format'), | ||
| $document->getAttribute('formatOptions'), | ||
| $document->getAttribute('filters'), | ||
| ); | ||
| $filteredAttribute = Attribute::fromDocument($document); |
There was a problem hiding this comment.
Handle null from beforeCreateAttribute() before deserializing.
src/Database/Mirroring/Filter.php:96-104 makes beforeCreateAttribute() nullable, but these paths always call Attribute::fromDocument($document). A filter that returns null to skip mirroring an attribute will now fail with a type error instead of being skipped.
Proposed fix
foreach ($this->writeFilters as $filter) {
$document = $filter->beforeCreateAttribute(
source: $this->source,
destination: $this->destination,
collectionId: $collection,
attributeId: $attribute->key,
attribute: $document,
);
}
- $filteredAttribute = Attribute::fromDocument($document);
- $result = $this->destination->createAttribute($collection, $filteredAttribute);
+ if ($document !== null) {
+ $filteredAttribute = Attribute::fromDocument($document);
+ $result = $this->destination->createAttribute($collection, $filteredAttribute);
+ } foreach ($attributes as $attribute) {
$document = $attribute->toDocument();
foreach ($this->writeFilters as $filter) {
$document = $filter->beforeCreateAttribute(
source: $this->source,
destination: $this->destination,
collectionId: $collection,
attributeId: $attribute->key,
attribute: $document,
);
}
- $filteredAttributes[] = Attribute::fromDocument($document);
+ if ($document !== null) {
+ $filteredAttributes[] = Attribute::fromDocument($document);
+ }
}
- $result = $this->destination->createAttributes(
- $collection,
- $filteredAttributes,
- );
+ if ($filteredAttributes !== []) {
+ $result = $this->destination->createAttributes(
+ $collection,
+ $filteredAttributes,
+ );
+ }Also applies to: 351-365
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Mirror.php` around lines 321 - 333, The code calls
Attribute::fromDocument($document) after running filters in $this->writeFilters,
but Filter::beforeCreateAttribute() can return null to indicate the attribute
should be skipped; update the blocks that call beforeCreateAttribute (e.g., the
loop using $this->writeFilters and the code that assigns $filteredAttribute) to
check if $document is null after the loop and, if so, skip further processing
(do not call Attribute::fromDocument) — apply the same null-check/skipping logic
to the other symmetric block that processes attributes (the second occurrence
mentioned around the attribute handling).
| public function setRelationshipHook(?RelationshipHook $hook): self | ||
| { | ||
| parent::setRelationshipHook($hook); | ||
|
|
||
| if (isset($this->source)) { | ||
| $this->source->setRelationshipHook( | ||
| $hook !== null ? new RelationshipHandler($this->source) : null | ||
| ); | ||
| } | ||
| if (isset($this->destination)) { | ||
| $this->destination->setRelationshipHook( | ||
| $hook !== null ? new RelationshipHandler($this->destination) : null | ||
| ); |
There was a problem hiding this comment.
Don't replace the caller's relationship hook with a new handler.
This setter only uses $hook as a null check. Lines 1079-1085 always install a fresh RelationshipHandler on the source/destination databases, so any custom RelationshipHook passed into Mirror::setRelationshipHook() never reaches the real databases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Mirror.php` around lines 1074 - 1086, The setRelationshipHook
method in Mirror currently discards the caller's RelationshipHook by always
installing a new RelationshipHandler on the source/destination; instead, forward
the provided $hook (or null) directly to the underlying databases. In the
Mirror::setRelationshipHook(?RelationshipHook $hook) implementation replace the
calls that pass new RelationshipHandler($this->source)/new
RelationshipHandler($this->destination) with the original $hook (or null) so the
caller's custom RelationshipHook is preserved when calling
$this->source->setRelationshipHook(...) and
$this->destination->setRelationshipHook(...).
…patch compatibility
…jects Convert the Index validator from Document-based getAttribute() calls to typed Attribute and Index value objects. This eliminates all mixed-type errors from ColumnType::tryFrom() and IndexType::tryFrom() which require int|string, and fixes nullsafe property access warnings on ->value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
Documentation