Joins #763
Joins #763
Conversation
# Conflicts: # composer.lock # src/Database/Database.php # tests/e2e/Adapter/Base.php
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
4980-5013: Nested relationship selects are incorrectly pruned inapplySelectFiltersToDocuments, breaking multi‑level relationship loading.
applySelectFiltersToDocuments()treats every select’sgetAttribute()as a concrete field name (Lines 4987‑4991). For nested paths like"employee.company.name"the first‑level relationship batch receives a select with attribute"company.name". That key never exists on the related documents, so this logic removes all non‑internal attributes, including the relationship attribute (company), and subsequent relationship population for deeper levels cannot work.Example:
- User selects
project.employee.company.name.- For the
employeerelationship,selectQueriescontainsQuery::select('company.name').attributesToKeep['company.name'] = true, but employee docs only havecompany, socompanyis pruned.- The next breadth‑first step can no longer traverse
companyfrom employee.Restricting this pruning to top‑level attributes (no dots) fixes the issue while leaving deeper filtering to subsequent levels.
Suggested fix:
- // Collect all attributes to keep from select queries - $attributesToKeep = []; - foreach ($selectQueries as $selectQuery) { - $attributesToKeep[$selectQuery->getAttribute()] = true; - } + // Collect all attributes to keep from select queries + // Only consider top‑level attributes here; nested paths (with ".") + // are handled when processing deeper relationship levels. + $attributesToKeep = []; + foreach ($selectQueries as $selectQuery) { + $attribute = $selectQuery->getAttribute(); + if (\str_contains($attribute, '.')) { + continue; + } + $attributesToKeep[$attribute] = true; + }This preserves behavior for simple selects and
*, while allowing multi‑level relationship selects to work correctly.
♻️ Duplicate comments (8)
tests/e2e/Adapter/Scopes/JoinsTests.php (2)
79-84: Remove unused variable assignments.The variables
$sessionNoPermissions(line 79) and$session3(line 122) are assigned but never referenced. Since only the document creation side effect is needed, drop the assignments.Apply this diff:
- $sessionNoPermissions = $db->createDocument('__sessions', new Document([ + $db->createDocument('__sessions', new Document([ 'user_id' => $user1->getId(), 'user_internal_id' => $user1->getSequence(), '$permissions' => [], ]));- $session3 = $db->createDocument('__sessions', new Document([ + $db->createDocument('__sessions', new Document([ 'user_id' => $user2->getId(), 'user_internal_id' => $user2->getSequence(), '$permissions' => [ Permission::read(Role::any()), ], 'boolean' => true, 'float' => 5.5, ]));Also applies to: 122-130
771-771: Fix spacing in return type declaration.Missing space before the colon in the return type declaration. PSR-12 requires a space between the closing parenthesis and the colon.
Apply this diff:
- public function testJoinsScopeOrder():void + public function testJoinsScopeOrder(): voidsrc/Database/Adapter/SQL.php (2)
3116-3118: Quote identifiers in right join permission clause.Line 3117 uses raw
{$alias}._uid IS NULLwithout quoting, which is inconsistent with the permission condition that uses$this->quote($alias)ingetSQLPermissionsCondition(). This was flagged in a previous review.Apply this diff to fix the quoting:
$skipAuth = $context->skipAuth($name, $forPermission, $this->authorization); if (! $skipAuth) { $permissionsCondition = $this->getSQLPermissionsCondition($name, $roles, $alias, $forPermission); if ($rightJoins) { - $permissionsCondition = "($permissionsCondition OR {$alias}._uid IS NULL)"; + $permissionsCondition = sprintf( + '(%s OR %s.%s IS NULL)', + $permissionsCondition, + $this->quote($alias), + $this->quote('_uid') + ); } $where[] = $permissionsCondition; }
3275-3281: Quote identifiers in right join permission clause.Line 3279 uses raw
{$alias}._uid IS NULLwithout quoting, identical to the issue in thefindmethod. This is inconsistent with the quoted identifiers ingetSQLPermissionsCondition().Apply this diff to fix the quoting:
$skipAuth = $context->skipAuth($name, Database::PERMISSION_READ, $this->authorization); if (! $skipAuth) { $permissionsCondition = $this->getSQLPermissionsCondition($name, $roles, $alias, Database::PERMISSION_READ); if ($rightJoins) { - $permissionsCondition = "($permissionsCondition OR {$alias}._uid IS NULL)"; + $permissionsCondition = sprintf( + '(%s OR %s.%s IS NULL)', + $permissionsCondition, + $this->quote($alias), + $this->quote('_uid') + ); } $where[] = $permissionsCondition; }src/Database/Database.php (1)
8712-8788: Remove unused$idAddedinprocessRelationshipQueriesand keep$idauto‑select relationship‑only.The tail of
processRelationshipQueries()still captures$idAddedfromQuery::addSelect(...)but never uses it (Lines 8783‑8785), which PHPMD correctly flags and which we’ve previously discussed.Given the intent is to:
- Always ensure
$idis selected when there are relationship attributes, and- Avoid unused locals,
you can safely drop
$idAddedand only capture the first returned element:- /** - * In order to populateDocumentRelationships we need $id - */ - if (!empty($relationships)) { - [$queries, $idAdded] = Query::addSelect($queries, Query::select('$id', system: true)); - } + /** + * In order to populateDocumentsRelationships we need $id + */ + if (!empty($relationships)) { + [$queries] = Query::addSelect($queries, Query::select('$id', system: true)); + }Behavior (auto‑adding
$idonly when relationships exist) remains unchanged, while satisfying PHPMD.
Based on learnings, this matches the prior guidance for this method.src/Database/Validator/Queries/V2.php (3)
676-691: Validate alias and guard schema access forSELECT *.When
$query->getAttribute() === '*', the code fetches the collection by alias but doesn't verify it exists before accessing the schema. If the alias is unknown,getCollectionByAlias()returns an empty Document,$collection->getId()is null, and$this->schema[null]triggers undefined index errors.Add validation and a guard:
if ($query->getAttribute() === '*') { $collection = $this->context->getCollectionByAlias($query->getAlias()); + if ($collection->isEmpty()) { + throw new \Exception('Invalid query: Unknown alias "'.$query->getAlias().'"'); + } + - $attributes = $this->schema[$collection->getId()]; + $attributes = $this->schema[$collection->getId()] ?? []; + if (empty($attributes)) { + throw new \Exception('Invalid query: No schema found for collection "'.$collection->getId().'"'); + } + foreach ($attributes as $attribute) {This ensures
SELECT *fails fast for invalid aliases and prevents iteration over null/undefined data.
532-536: Join-scope alias check is too broad and will reject valid queries.This block runs for every query when
$scope === 'joins', not justTYPE_RELATION_EQUAL. For normal filter or order queries within a join,getRightAlias()is typically empty, soin_array('', $this->joinsAliasOrder)fails and valid queries are incorrectly rejected.Since you removed the dedicated
validateJoinAliasOrder()helper (per your earlier comment), you have two options:Option 1 (recommended): Remove this block entirely. The
isRelationExist()check at line 638 already ensures at least one relation query is present in each join, and relation queries validate both aliases in theTYPE_RELATION_EQUALcase (lines 648–649).-if ($scope === 'joins') { - if (!in_array($query->getAlias(), $this->joinsAliasOrder) || !in_array($query->getRightAlias(), $this->joinsAliasOrder)) { - throw new \Exception('Invalid query: '.\ucfirst($query->getMethod()).' alias reference in join has not been defined.'); - } -}Option 2: Restrict this check to
TYPE_RELATION_EQUALqueries only:-if ($scope === 'joins') { +if ($scope === 'joins' && $query->getMethod() === Query::TYPE_RELATION_EQUAL) { if (!in_array($query->getAlias(), $this->joinsAliasOrder) || !in_array($query->getRightAlias(), $this->joinsAliasOrder)) {
210-225: Guard against undefined array access in schemaless mode.When
$supportForAttributesisfalseand the attribute is not in the schema,$attributeis an empty array. Lines 219 and 223 then access$attribute['type']without a guard, which will trigger undefined index notices and potentially throw incorrect "Only nested relationships allowed" errors.Adopt the same early-return pattern used in
validateValues()(line 292–294):$attribute = $this->schema[$collection->getId()][$attributeId] ?? []; -if (empty($attribute) && $this->supportForAttributes) { - throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); -} - -if (\in_array('encrypt', $attribute['filters'] ?? [])) { + +if (empty($attribute)) { + if ($this->supportForAttributes) { + throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); + } + // Schemaless: skip attribute-based checks for unknown attributes + return; +} + +if (\in_array('encrypt', $attribute['filters'] ?? [])) { throw new \Exception('Cannot query encrypted attribute: ' . $attributeId); }This makes schemaless behavior consistent and avoids dereferencing an empty array.
🧹 Nitpick comments (5)
src/Database/Adapter/SQL.php (2)
375-375: Clarify the$queriesparameter type in getDocument.The
getDocumentmethod signature showsQuery[] $queries = [](line 359), but line 375 passes$queriesdirectly togetAttributeProjection(array $selects). The method expects an array of Query objects with select semantics, but the parameter name and usage are ambiguous.Consider renaming the parameter to
$selectsfor clarity, or add a PHPDoc comment clarifying that$queriesshould contain Query objects of typeTYPE_SELECT.
2346-2346: Address the TODO comment for spatial attributes.The TODO comment on line 2346 indicates that spatial attributes handling is incomplete. Spatial attributes may require special SQL functions (like
ST_AsTextorST_AsBinary) in the SELECT projection to properly return geometry data.Do you want me to help implement spatial attribute handling in the projection, or should this be tracked in a separate issue?
src/Database/Database.php (2)
34-35: QueryContext + V2 DocumentsValidator wiring forgetDocumentlooks coherent; consider documenting the “selects‑only” contract.The new
QueryContext/DocumentsValidatorflow ingetDocument()(context creation, select extraction, relationship processing, caching keyed by$selects, and context‑awarecasting()/decode()) is consistent with the new join model and looks solid.One behavioral change is that
$queriesare now enforced to be onlyTYPE_SELECT(mismatch throwsQueryExceptionat Lines 4300‑4303). It would be helpful to update the docblock forgetDocument()to state that$queriesare select projections only, not filters/orders/limits, so callers don’t accidentally trip the validator.Also applies to: 4275-4417
8086-8137:sum()usesQueryContext/V2 validator but doesn’t yet build join context; verify whether joins should be supported.
sum()now validates queries against aQueryContext(Lines 8088‑8103) but:
- Only adds the base collection to the context (Lines 8088‑8091).
- Does not collect join queries into the context or propagate joins into the adapter call, unlike
find()/count().As a result, any join/alias‑based queries passed into
sum()are likely to fail validation (unknown alias) or be ignored by adapters.If
sum()is intentionally restricted to single‑collection queries, that’s fine; it may be worth documenting this. If the intent is “joins work everywhere,” consider mirroring the join/context wiring fromcount()and, if needed, extending the adaptersum()signature to acceptQueryContextand joins.src/Database/Validator/Queries/V2.php (1)
740-741: Remove commented-out debug code.The
var_dump()calls are commented out but should be deleted entirely for cleaner code.} catch (\Throwable $e) { $this->message = $e->getMessage(); - //var_dump($this->message); - //var_dump($e); - return false; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Adapter/SQL.php(12 hunks)src/Database/Database.php(55 hunks)src/Database/Validator/Queries/V2.php(1 hunks)tests/e2e/Adapter/Scopes/JoinsTests.php(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Database.php
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Database.phptests/e2e/Adapter/Scopes/JoinsTests.php
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Adapter/SQL.phpsrc/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Database.phptests/e2e/Adapter/Scopes/JoinsTests.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: 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.
Applied to files:
src/Database/Validator/Queries/V2.php
🧬 Code graph analysis (2)
src/Database/Validator/Queries/V2.php (4)
src/Database/Database.php (1)
Database(38-9411)src/Database/Document.php (1)
Document(12-470)src/Database/Query.php (7)
Query(8-1599)getAttribute(255-258)getAlias(277-280)getMethod(247-250)getValues(263-266)parse(487-500)getAs(292-295)src/Database/QueryContext.php (3)
QueryContext(8-89)getCollections(32-35)getCollectionByAlias(37-55)
src/Database/Database.php (2)
src/Database/Validator/Queries/V2.php (3)
V2(23-748)isValid(498-747)getDescription(135-138)src/Database/QueryContext.php (6)
QueryContext(8-89)add(60-68)getCollections(32-35)skipAuth(75-86)addSkipAuth(70-73)getCollectionByAlias(37-55)
🪛 GitHub Actions: Linter
tests/e2e/Adapter/Scopes/JoinsTests.php
[error] 1-1: PSR-12: return_type_declaration style issue detected by Pint lint. File contains a return type declaration that violates PSR-12 rules.
🪛 PHPMD (2.15.0)
src/Database/Database.php
8784-8784: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
tests/e2e/Adapter/Scopes/JoinsTests.php
79-79: Avoid unused local variables such as '$sessionNoPermissions'. (undefined)
(UnusedLocalVariable)
122-122: Avoid unused local variables such as '$session3'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (16)
src/Database/Adapter/SQL.php (6)
2302-2335: LGTM! Enhanced tenant query flexibility.The added
$conditionand$forceIsNullparameters allow proper handling of right joins and flexible query composition. The empty string option for$conditionenables clean integration when the caller manages the AND/OR logic.
1800-1806: LGTM! Join type mapping correctly implemented.The join type constants are properly mapped to their SQL keywords. This addresses the previous review concern about hardcoded INNER JOIN usage.
3017-3079: LGTM! Cursor pagination properly handles join aliases.The ordering and cursor pagination logic correctly uses
getAlias()from order queries, enabling ordering by attributes from joined collections. The tie-breaking logic for multi-attribute cursors is preserved.
3131-3141: LGTM! Vector ordering properly integrated.Vector distance calculations are correctly prepended to the ORDER BY clause, ensuring similarity search results are ordered by relevance first before any secondary ordering.
3170-3177: LGTM! Float precision handling prevents data loss.Binding floats as strings with
getFloatPrecision()preserves full precision during SQL parameter binding, avoiding floating-point rounding issues.
3234-3329: LGTM! Count method properly implements QueryContext and join support.The count method correctly:
- Uses QueryContext for collection and auth information
- Implements join support with proper SQL construction
- Handles right joins with tenant query adjustments
- Provides consistent error handling with try-catch
src/Database/Database.php (8)
5118-5123: Context‑aware casting/decoding after create paths is consistent with read paths.Creating a
QueryContext, adding the collection, and then runningcasting($context, …)anddecode($context, …)in bothcreateDocument()andcreateDocuments()aligns the write return types withgetDocument()/find(). This should keep consumers from seeing different scalar types depending on which CRUD path they use.Also applies to: 5171-5173, 5234-5238
5591-5831: Update/upsert paths only call adapter‑level casting; ensure this keeps return types consistent with create/find.
updateDocument(),updateDocuments(), andupsertDocumentsWithIncrease()now:
- Run
$this->adapter->castingAfter($collection, $document)/$docafter persistence.- Then decode via
decode($context, …)but do not callDatabase::casting($context, …)on their results (unlikegetDocument(),createDocument(),createDocuments(), andfind()).If all adapters now perform full scalar normalization (bool/int/float) inside
castingAfter()for these paths, this is fine. If any adapter still relies onDatabase::casting()for normalization (especially SQL adapters), these update/upsert return values could diverge in type from create/find results.It’s worth double‑checking adapter implementations to confirm that:
castingAfter()is doing the same scalar normalization for updates/upserts as reads, or- No adapter depends on
Database::casting()for normalization in these code paths.If not, consider adding a
casting($context, …)call beforedecode()here for parity.Also applies to: 5854-6076, 6544-6831
7496-7588:deleteDocumentsjoins/context/validation wiring matchesupdateDocumentsand looks good.The new
QueryContext+DocumentsValidatorusage indeleteDocuments()is consistent withupdateDocuments()andfind():
- Context collects base + joined collections, with per‑collection auth (via
addSkipAuth).- Queries are type‑checked, validated with V2, then grouped.
- Required attributes
$permissionsand$sequenceare explicitly enforced before proceeding (Lines 7581‑7588), which is critical for permission cleanup and engine‑level deletes.This batch delete pipeline looks coherent with the rest of the new query model.
7742-7893: Join‑awarefind()pipeline usingQueryContextand V2 validator is well‑structured.The new
find()flow:
- Builds a
QueryContextwith base collection + all join targets/aliases (Lines 7750‑7761).- Computes per‑collection
skipAuthand records it in the context (Lines 7762‑7771), so adapters can make permission‑aware decisions.- Validates the full query set (including joins/nested) with
DocumentsValidator V2using that context (Lines 7775‑7784).- Converts queries through
convertQueries($context, …)(fixing the earlier static call issue), then fans outselects,filters,joins,vectors,ordersand passes them, along withQueryContext, intoadapter->find()(Lines 7795‑7865).- Runs adapter
castingAfter, then context‑awarecasting()anddecode()per document (Lines 7874‑7878).Overall, this matches the intended context‑first/joins model and looks correct.
7992-8071:count()now mirrorsfind()’s join/context model;maxguard is sensible.The updated
count():
- Validates
$maxto benullor>= 1(Lines 7994‑7997), preventing silent misuse of negative/zero values.- Uses
QueryContext+ join discovery exactly likefind()(Lines 8004‑8015), including per‑collection READ auth andaddSkipAuth.- Validates queries with
DocumentsValidator V2against the same context (Lines 8030‑8038).- Runs
convertQueries($context, …), extracts filters, converts relationship filters to subqueries, and passes everything, plus joins and context, intoadapter->count()(Lines 8049‑8066).This is a clean, join‑aware implementation of count and matches the rest of the new query pipeline.
8240-8446: Context‑awaredecode()andcasting()correctly handle aliases,AS, and multi‑collection schemas.The new implementations:
- Build an internal + per‑collection schema once per call (Lines 8242‑8255, 8344‑8357).
- For each field in the raw document, resolve:
- The effective alias and attribute name, honoring
AS(getAs()) and adapter‑filtered column names (Lines 8263‑8277, 8365‑8379).- The target collection via
QueryContext::getCollectionByAlias()(Lines 8279‑8283, 8381‑8385).- The attribute metadata from either internal or per‑collection schema (Lines 8284‑8288, 8386‑8390).
- In
decode(), apply registered filters in reverse order per attribute and rebuild a new document keyed by logical attribute IDs / aliases (Lines 8313‑8324).- In
casting(), short‑circuit when the adapter doesn’t support casting (Lines 8340‑8342) and otherwise perform scalar normalization based on attribute type, including array handling and JSON decoding for array‑typed columns (Lines 8405‑8438).This design should handle joined projections, aliased columns, and multi‑collection result sets cleanly.
8547-8615:convertQueries/convertQuerynow correctly useQueryContextand respect aliases.The refactor to:
- Thread
QueryContextthroughconvertQueries()andconvertQuery()(Lines 8547‑8567).- Treat nested/join queries by recursively converting their value arrays (Lines 8549‑8553).
- Resolve the right collection via
getCollectionByAlias($query->getAlias())and throw if the alias is unknown (Lines 8572‑8576).- Attach
onArrayandattributeTypehints from the resolved attribute metadata and normalize datetime values using adapter UTC behavior (Lines 8595‑8611),brings these helpers in line with the new join/alias model and fixes the earlier static‑call/
$thisissue.
8806-9180: Nested relationship filter → subquery conversion is sophisticated but generally consistent; rely on tests to guard edge cases.The new
processNestedRelationshipPath()andconvertRelationshipFiltersToSubqueries():
- Parse dot‑path filters, group them by relationship chain, and walk the relationship graph from the deepest collection back to the start to derive matching IDs (Lines 8808‑8952).
- Use existing relationship metadata (
relationType,side,twoWayKey) to decide when reverse lookups or junction traversals are needed, including many‑to‑many via junction collections.- Convert complex relationship filters into equivalent
Query::equal(...)constraints on either$idor the appropriate relationship key, returningnullearly when no matches are possible (Lines 8977‑9180).Given the complexity and number of cases (1‑1, 1‑n, n‑1, n‑n, multi‑level chains), this is the right shape but heavily relies on tests for safety. It would be good to ensure you have explicit coverage for:
- Multi‑hop paths (e.g.
project.employee.company.name),- Each relation type/side,
- Filters that intentionally match nothing (confirm they short‑circuit to
null/0 quickly).src/Database/Validator/Queries/V2.php (2)
56-128: LGTM! Schema construction is solid.The constructor correctly clones collections from the context to avoid mutations and systematically adds system attributes to each collection's schema. The indexing approach (by collection ID and attribute key) aligns well with the validation logic downstream.
323-323: Sequence validator logic corrected.The Sequence validator now correctly uses
$attributeId === '$sequence'(not$attribute === '$sequence') to set the primary flag. This aligns with the pattern inStructure.phpline 343 and properly validates ID-like attributes.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Adapter/SQL.php (1)
3017-3077: Cursor pagination ignores per-order aliases in tie‑break conditionsIn the cursor block, equality predicates for “previous” order attributes always use
$orderAlias(the current order’s alias):for ($j = 0; $j < $i; $j++) { $prevQuery = $orderQueries[$j]; $prevOriginal = $prevQuery->getAttribute(); $prevAttr = $this->filter($this->getInternalKeyForAttribute($prevOriginal)); ... $conditions[] = "{$this->quote($orderAlias)}.{$this->quote($prevAttr)} = {$bindName}"; }If different
orderQueriesuse different aliases (e.g., main vs joined collection), the cursor WHERE clause will compare the wrong table alias for earlier attributes, breaking lexicographic pagination and potentially producing incorrect result windows.You can fix this by using each previous query’s own alias:
- // Add equality conditions for previous attributes - for ($j = 0; $j < $i; $j++) { - $prevQuery = $orderQueries[$j]; - $prevOriginal = $prevQuery->getAttribute(); - $prevAttr = $this->filter($this->getInternalKeyForAttribute($prevOriginal)); - - $bindName = ":cursor_{$j}"; - $binds[$bindName] = $cursor[$prevOriginal]; - - $conditions[] = "{$this->quote($orderAlias)}.{$this->quote($prevAttr)} = {$bindName}"; - } + // Add equality conditions for previous attributes, respecting aliases per order query + for ($j = 0; $j < $i; $j++) { + /** @var Query $prevQuery */ + $prevQuery = $orderQueries[$j]; + $prevAlias = $prevQuery->getAlias(); + $prevOriginal = $prevQuery->getAttribute(); + $prevAttr = $this->filter($this->getInternalKeyForAttribute($prevOriginal)); + + $bindName = ":cursor_{$j}"; + $binds[$bindName] = $cursor[$prevOriginal]; + + $conditions[] = "{$this->quote($prevAlias)}.{$this->quote($prevAttr)} = {$bindName}"; + }
♻️ Duplicate comments (3)
tests/e2e/Adapter/Scopes/JoinsTests.php (2)
79-84: Remove unused variable assignments.
$sessionNoPermissionsand$session3are assigned but never referenced. Since only the side-effect of inserting documents is needed, drop the assignments.Also applies to: 122-130
514-589: Commented-out tests for ambiguous/duplicate column selection.This block was previously flagged. Consider tracking these as a separate issue if the validation logic is planned for future implementation.
src/Database/Adapter/SQL.php (1)
3085-3125: RIGHT JOIN permission/tenant handling: alias quoting + skipAuth keyingTwo related points in the JOIN + permissions paths:
- RIGHT JOIN permission clause uses unquoted alias/column
In both
find()andcount()you extend the main collection’s permission condition for right joins:$permissionsCondition = $this->getSQLPermissionsCondition($name, $roles, $alias, $forPermission); // ... if ($rightJoins) { $permissionsCondition = "($permissionsCondition OR {$alias}._uid IS NULL)"; }and similarly in
count().
getSQLPermissionsCondition()itself uses quoted identifiers ({$this->quote($alias)}.{$this->quote('_uid')}), but the addedORbranch uses raw{$alias}._uid. This is inconsistent and may break if alias quoting ever changes or if aliases contain special characters.You can build the OR branch with the same quoting:
- $skipAuth = $context->skipAuth($name, $forPermission, $this->authorization); - if (! $skipAuth) { - $permissionsCondition = $this->getSQLPermissionsCondition($name, $roles, $alias, $forPermission); - if ($rightJoins) { - $permissionsCondition = "($permissionsCondition OR {$alias}._uid IS NULL)"; - } - $where[] = $permissionsCondition; - } + $skipAuth = $context->skipAuth($name, $forPermission, $this->authorization); + if (! $skipAuth) { + $permissionsCondition = $this->getSQLPermissionsCondition($name, $roles, $alias, $forPermission); + if ($rightJoins) { + $permissionsCondition = \sprintf( + '(%s OR %s.%s IS NULL)', + $permissionsCondition, + $this->quote($alias), + $this->quote('_uid'), + ); + } + $where[] = $permissionsCondition; + }and analogously in
count()(without$forPermissionargument).
- skipAuth() is called with filtered collection names
For both joins and the root collection you do:
$collection = $join->getCollection(); $collection = $this->filter($collection); $skipAuth = $context->skipAuth($collection, $forPermission, $this->authorization); ... $name = $context->getCollections()[0]->getId(); $name = $this->filter($name); ... $skipAuth = $context->skipAuth($name, $forPermission, $this->authorization);If
QueryContext::addSkipAuth()is keyed by the raw collection id (e.g.$collection->getId()in Database), using the filtered value here would causeskipAuth()to miss entries and accidentally apply permission predicates where skip-auth was requested.A safer pattern would be to keep the raw id for context lookups and use the filtered string only for SQL identifiers, for example:
- foreach ($joins as $join) { - $permissions = ''; - $collection = $join->getCollection(); - $collection = $this->filter($collection); - - $skipAuth = $context->skipAuth($collection, $forPermission, $this->authorization); + foreach ($joins as $join) { + $permissions = ''; + $collectionId = $join->getCollection(); + $collection = $this->filter($collectionId); + + $skipAuth = $context->skipAuth($collectionId, $forPermission, $this->authorization);and similarly for the root collection in both
find()andcount()(keep$rootCollectionId = $context->getCollections()[0]->getId();forskipAuth(), pass the filtered$nameonly into SQL helpers).Would you double‑check how
QueryContext::addSkipAuth()is keyed (raw vs filtered id) and confirm that changingskipAuth()calls to use the raw collection id won’t break existing behaviour?To help verify, you can run something like:
#!/bin/bash # Inspect QueryContext skipAuth/addSkipAuth usage and keys. rg -n "class QueryContext|addSkipAuth\(|skipAuth\(" src/Database -C3Also applies to: 3249-3287
🧹 Nitpick comments (2)
tests/e2e/Adapter/Scopes/JoinsTests.php (1)
44-44: Remove commented-out code.This commented-out
Authorization::setRolecall appears to be leftover debugging/development code and should be removed for cleanliness.- //Authorization::setRole('user:bob');src/Database/Adapter/SQL.php (1)
364-379: ClarifygetDocument()’s$queriescontract vsgetAttributeProjection()
getDocument()now builds its projection via:$sql = " SELECT {$this->getAttributeProjection($queries)} FROM {$this->getSQLTable($name)} AS {$this->quote($alias)} WHERE {$this->quote($alias)}._uid = :_uid {$this->getTenantQuery($collection, $alias)} ";and
getAttributeProjection()expects an array of selectionQueryobjects ($selects), mapping$id/$sequence/$tenant/...to internal columns and supporting*andASaliases.Two follow‑ups:
- If callers ever pass non‑SELECT queries (filters, orders, etc.) into
getDocument()’s$queriesparameter, those would now be misinterpreted as projected columns.- The naming mismatch (
$queriesvs$selects) and the new behaviour are not obvious from thegetDocument()docblock.To make this safer and clearer, consider:
- Narrowing
getDocument()to accept a$selectsarray (or pulling selects directly fromQueryContext), and updating the docblock accordingly.- Optionally asserting that each entry in
$selectshasgetMethod() === Query::TYPE_SELECTbefore using it ingetAttributeProjection().Example adjustment in the signature/docblock:
- * @param Query[] $queries + * @param array<Query> $selects ... -public function getDocument(Document $collection, string $id, array $queries = [], bool $forUpdate = false): Document +public function getDocument(Document $collection, string $id, array $selects = [], bool $forUpdate = false): Documentand pass
$selectsintogetAttributeProjection().Please confirm that all current call sites of
SQL::getDocument()only ever pass SELECT queries into this parameter; if not, we should tighten the API as above.Also applies to: 2340-2391
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter/SQL.php(12 hunks)tests/e2e/Adapter/Scopes/JoinsTests.php(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/JoinsTests.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
tests/e2e/Adapter/Scopes/JoinsTests.php
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Adapter/SQL.php
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/JoinsTests.php (2)
src/Database/Query.php (7)
Query(8-1599)join(1003-1006)relationEqual(1041-1044)equal(645-648)orderAsc(810-813)orderDesc(799-802)select(788-791)src/Database/Adapter/SQL.php (1)
getSupportForRelationships(1536-1539)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/JoinsTests.php
79-79: Avoid unused local variables such as '$sessionNoPermissions'. (undefined)
(UnusedLocalVariable)
122-122: Avoid unused local variables such as '$session3'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Unit Test
🔇 Additional comments (6)
tests/e2e/Adapter/Scopes/JoinsTests.php (6)
1-21: LGTM!The namespace, imports, and trait declaration are properly structured. All imported classes are used within the test methods.
165-287: Comprehensive exception testing.Good coverage of invalid query scenarios: unknown aliases, ambiguous aliases, missing relation queries, invalid ON clause queries, and SQL injection attempts in alias names. The try/catch pattern with
fail()ensures exceptions are properly thrown.
348-512: Thorough select/projection testing.Excellent coverage of:
- Selecting from multiple aliases (
main, joined collections)- Attribute renaming with
asparameter- Type preservation for float/boolean attributes
- Internal attribute selection (
$permissions,$id,$sequence,$createdAt)- Validation of invalid
asvalues (SQL injection attempts, invalidason wildcards)
592-680: Well-structured left join tests.Good coverage of left join semantics:
- Proper null handling for unmatched rows (lines 651, 679)
- Skip permissions scenario with
getAuthorization()->skip()- Additional ON clause conditions with
Query::equal- Ordering across joined collections
682-769: Right join tests with documented edge case.The test structure appropriately mirrors the left join tests. The comment at lines 764-766 correctly documents the edge case behavior when right join returns nulls for the main collection that are then filtered by permissions.
771-855: Multi-join and aggregation tests.Good coverage of:
- Chained joins across three tables with proper alias referencing
- Error detection for undefined alias references in join chains
- Integration of
count()with join queries to verify aggregation compatibility
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Database/Database.php (3)
4295-4331: Align$permissionsauto-select behavior between cache hits and misses ingetDocument().You correctly track whether
$permissionswas system-added via$permissionsAdded, and you strip it from the result on cache misses. On cache hits, you return early from the$cachedbranch and never apply that cleanup, so callers can see$permissionseven when they didn’t select it, while the miss path hides it.To keep behavior consistent without affecting internal permission checks, apply the same cleanup inside the
$cachedbranch:if ($cached) { $document = $this->createDocumentInstance($collection->getId(), $cached); @@ $this->trigger(self::EVENT_DOCUMENT_READ, $document); - return $document; + if ($permissionsAdded) { + $document->removeAttribute('$permissions'); + } + + return $document; } @@ - if ($permissionsAdded) { // Or remove all queries added by system - $document->removeAttribute('$permissions'); - } + if ($permissionsAdded) { + $document->removeAttribute('$permissions'); + }This way, the projection semantics for
$permissionsdon’t depend on whether the document came from cache.Also applies to: 4387-4388, 4411-4413
8977-9180: Avoid turning all errors inconvertRelationshipFiltersToSubqueries()into “no matches”.Inside the per-relationship loop you wrap the entire subquery-conversion flow in a broad
try { … } catch (\Exception $e) { return null; }. Callers then interpretnullas “no documents can match this relationship filter”.That means:
- Adapter/database errors (timeouts, connection failures),
- Validation/logic bugs in nested
find()/processNestedRelationshipPath(), and- Any unexpected exception
will be silently converted into an empty result set instead of surfacing as failures. This makes real outages or bugs indistinguishable from legitimate “no matches” and will be painful to debug.
I’d strongly recommend either:
- Removing the catch and letting exceptions bubble up, or
- Narrowing it to the specific, anticipated case you truly want to treat as “no matches” (e.g. a dedicated exception type, or at minimum a well-scoped
QueryException), while letting other errors propagate.As written, this is a significant observability/behavior change.
8086-8137:sum()doesn't handle join queries likefind()andcount()do.You create a
QueryContextwith only the main collection and don't extract join queries. Unlikefind()andcount(), which callQuery::getJoinQueries()and populate the context with each join's collection and alias,sum()skips this step entirely.Any join query passed to
sum()will be ignored or cause an error—join aliases won't exist in the context when queries are converted, resulting in "Unknown Alias context" exceptions.Two options:
- Add join support by extracting and populating joins like
find()/count()do, then passQueryContextand$joinsto the adapter (requires updating the adapter'ssum()signature), or- Detect and reject join queries up front with a clear error message if joins aren't meant to be supported yet.
src/Database/Adapter/Mongo.php (1)
1442-1469: Confirm schemaless Mongo updates should use replacement, not$set
updateDocument()always performs a$setoperation:$updateQuery = [ '$set' => $record, ]; $this->client->update($name, $filters, $updateQuery, $options);Per design intent for schemaless mode (
getSupportForAttributes() === false), Mongo should perform replacement-style updates without$set, allowing fields omitted from the update to be removed from the stored record. This aligns with schemaless semantics where the document becomes exactly what is provided.Always using
$setmeans removed fields persist in the database—a regression from intended behavior. BothupdateDocument()andupdateDocuments()need branching:+ if (!$this->getSupportForAttributes()) { + // Schemaless: full replacement + $this->client->update($name, $filters, $record, $options); + } else { + // Schema mode: partial update + $updateQuery = ['$set' => $record]; + $this->client->update($name, $filters, $updateQuery, $options); + }Apply the same fix to
updateDocuments()method as well.
♻️ Duplicate comments (10)
src/Database/Database.php (2)
5818-5822: Update/upsert still skipcasting(QueryContext, …), so result types can differ from create/get/find.Right now:
createDocument(),createDocuments(),getDocument(), andfind()all call$this->casting($context, $document, $selects)beforedecode().updateDocument()only does$this->decode($context, $document).updateDocuments()only does$this->decode($context, $doc)in the batch loop.upsertDocumentsWithIncrease()only calls$this->decode($context, $doc)when!$hasOperators.For adapters that rely on
Database::casting()for scalar normalization, that means update/upsert can return documents whose scalar attributes are still in DB-native form (e.g. strings for ints/bools) while create/get/find return normalized PHP types. This is the same concern raised earlier and appears still unresolved.Unless
getSupportForCasting()semantics were intentionally inverted, consider mirroring the other CRUD paths, e.g.:- $context = new QueryContext(); - $context->add($collection); - - $document = $this->decode($context, $document); + $context = new QueryContext(); + $context->add($collection); + + $document = $this->casting($context, $document); + $document = $this->decode($context, $document);and similarly call
casting()beforedecode()inupdateDocuments()and the non-operator branch ofupsertDocumentsWithIncrease().If this divergence is intentional, it’d be good to document that update/upsert may return raw DB types where other methods don’t.
Also applies to: 5974-5981, 6048-6053, 6565-6567, 6796-6800
8712-8788: Remove unused$idAddedinprocessRelationshipQueries()and keep$idinjection relationship-only.At the tail of
processRelationshipQueries()you still assign the second element ofQuery::addSelect()to$idAddedbut never use it, which PHPMD flags and which isn’t needed now that you always require$idonly when there are relationship attributes:if (!empty($relationships)) { [$queries, $idAdded] = Query::addSelect($queries, Query::select('$id', system: true)); }You can simplify and silence the warning by only capturing the updated query list:
- $queries = array_values($queries); - - /** - * In order to populateDocumentRelationships we need $id - */ - if (!empty($relationships)) { - [$queries, $idAdded] = Query::addSelect($queries, Query::select('$id', system: true)); - } - - return [$queries, $nestedSelections]; + $queries = array_values($queries); + + /** + * In order to populateDocumentsRelationships we need $id + */ + if (!empty($relationships)) { + [$queries] = Query::addSelect($queries, Query::select('$id', system: true)); + } + + return [$queries, $nestedSelections];This matches the prior guidance to only auto-select
$idwhen the collection has relationship attributes and avoids the unused local.Based on learnings, this keeps
$idauto-selection restricted to relationship-bearing collections.src/Database/Validator/Queries/V2.php (3)
179-226: Avoid dereferencing$attribute['type']for unknown attributes in schemaless modeWhen
$supportForAttributesisfalseand the attribute is not in$this->schema,$attributeis empty but you still access$attribute['type']for nested checks:$attribute = $this->schema[$collection->getId()][$attributeId] ?? []; if (empty($attribute) && $this->supportForAttributes) { throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); } if ($isNested && $attribute['type'] != Database::VAR_RELATIONSHIP) { throw new \Exception('Only nested relationships allowed'); }In schemaless mode this can trigger undefined index notices and “Only nested relationships allowed” for arbitrary nested fields, instead of behaving permissively.
Consider short‑circuiting when the attribute is unknown in schemaless mode, mirroring
validateValues():- $attribute = $this->schema[$collection->getId()][$attributeId] ?? []; - if (empty($attribute) && $this->supportForAttributes) { - throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); - } - - if (\in_array('encrypt', $attribute['filters'] ?? [])) { + $attribute = $this->schema[$collection->getId()][$attributeId] ?? []; + + if (empty($attribute)) { + if ($this->supportForAttributes) { + throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); + } + + // Schemaless: attribute not in schema, skip further attribute-based checks + return; + } + + if (\in_array('encrypt', $attribute['filters'] ?? [])) { throw new \Exception('Cannot query encrypted attribute: ' . $attributeId); } - if ($isNested && $attribute['type'] != Database::VAR_RELATIONSHIP) { + if ($isNested && $attribute['type'] != Database::VAR_RELATIONSHIP) { throw new \Exception('Only nested relationships allowed'); }This keeps strict behavior when schema support is enabled while avoiding bogus errors and notices in schemaless mode.
526-537: Relax join-scope alias validation; current check rejects valid non-relation queriesThe generic join-scope alias check:
if ($scope === 'joins') { if (!in_array($query->getAlias(), $this->joinsAliasOrder) || !in_array($query->getRightAlias(), $this->joinsAliasOrder)) { throw new \Exception('Invalid query: '.\ucfirst($query->getMethod()).' alias reference in join has not been defined.'); } }runs for every nested query under a join. For normal filter/order queries inside a join,
getRightAlias()is usually an empty string, so this block incorrectly throws even when the query is otherwise valid.You already enforce relation semantics in the
TYPE_RELATION_EQUALcase (viavalidateAttributeExist()on both sides andisRelationExist()on the join). To avoid false positives, restrict this alias-order check to relation queries only, and make it strict:- if ($scope === 'joins') { - if (!in_array($query->getAlias(), $this->joinsAliasOrder) || !in_array($query->getRightAlias(), $this->joinsAliasOrder)) { - throw new \Exception('Invalid query: '.\ucfirst($query->getMethod()).' alias reference in join has not been defined.'); - } - } + if ($scope === 'joins' && $method === Query::TYPE_RELATION_EQUAL) { + if ( + !\in_array($query->getAlias(), $this->joinsAliasOrder, true) + || !\in_array($query->getRightAlias(), $this->joinsAliasOrder, true) + ) { + throw new \Exception('Invalid query: '.\ucfirst($query->getMethod()).' alias reference in join has not been defined.'); + } + }This keeps the alias-order safety for relation predicates while allowing non-relation filters and orders inside joins.
666-705: GuardSELECT *against unknown aliases and missing schema entriesIn the
TYPE_SELECTbranch, whengetAttribute() === '*'you directly resolve the collection and schema:if ($query->getAttribute() === '*') { $collection = $this->context->getCollectionByAlias($query->getAlias()); $attributes = $this->schema[$collection->getId()]; foreach ($attributes as $attribute) { … } }If the alias is unknown,
getCollectionByAlias()can return an emptyDocument, so$collection->getId()isnull;$this->schema[$collection->getId()]then raises an undefined index andforeachruns onnull. The query effectively passes validation even though the alias context is invalid.To make this robust and consistent with other paths (e.g.
validateAttributeExist()), validate the alias and guard the schema lookup:- if ($query->getAttribute() === '*') { - $collection = $this->context->getCollectionByAlias($query->getAlias()); - $attributes = $this->schema[$collection->getId()]; + if ($query->getAttribute() === '*') { + $collection = $this->context->getCollectionByAlias($query->getAlias()); + if ($collection->isEmpty()) { + throw new \Exception('Invalid query: Unknown Alias context'); + } + + $attributes = $this->schema[$collection->getId()] ?? []; foreach ($attributes as $attribute) { if (($duplications[$query->getAlias()][$attribute['$id']] ?? false) === true) { //throw new \Exception('Ambiguous column using "*" for "'.$query->getAlias().'.'.$attribute['$id'].'"'); }This ensures
SELECT *fails fast for unknown aliases and avoids undefined index/foreach-on-null issues.src/Database/Validator/Query/Filter.php (1)
1-422: Remove the commented-out legacy Filter validator (and fix Pint violation)This file is now just a 400+ line commented
Filterclass plus the<?phptag, and Pint is failing with a PSR‑12blank_line_after_opening_tagerror here.Given that V2/DVV-based validation has replaced this class, it’s better to delete the old implementation instead of keeping it commented out:
- It removes the Pint/PSR‑12 issue.
- Avoids confusion about which validator is active.
- Keeps the repo smaller and easier to navigate; the old code remains in Git history if needed.
A minimal fix is to drop the entire commented block and, if you still want a placeholder, replace it with a short comment pointing to the new validator:
-<?php -// -//namespace Utopia\Database\Validator\Query; -// ... -//class Filter extends Base -//{ -// ... -//} +<?php + +// Legacy Filter validator has been superseded by Utopia\Database\Validator\Queries\V2. +// This file is kept empty intentionally; see V2 for current query validation logic.Or simply delete
Filter.phpentirely if no longer referenced.src/Database/Adapter/Mongo.php (1)
1875-1887: Guard Mongofind()against unsupportedorderRandomqueriesIn
find()you iterate$orderQueriesand call:foreach ($orderQueries as $i => $order) { $attribute = $order->getAttribute(); // ... $direction = $order->getOrderDirection(); // possibly flipped for cursorBefore $options['sort'][$attribute] = $this->getOrder($direction); // ... }
Query::getOrderDirection()only supports ASC/DESC orders and throws forTYPE_ORDER_RANDOM. This adapter returnsfalsefromgetSupportForOrderRandom(), so a random-order query reaching this code would currently blow up with “Invalid method: Get order direction on "orderRandom" Query”.If you expect higher layers (
Database::find()/ validator) to prevent random ordering for Mongo, that’s fine; otherwise, add a defensive guard here to fail fast and clearly:- foreach ($orderQueries as $i => $order) { + foreach ($orderQueries as $i => $order) { + if ($order->getMethod() === Query::TYPE_ORDER_RANDOM) { + throw new DatabaseException('Random order is not supported by Mongo adapter'); + } $attribute = $order->getAttribute(); // ... $direction = $order->getOrderDirection();This makes the behavior explicit and avoids surprising internal exceptions if a random-order query slips through.
Also applies to: 1938-1988
src/Database/Adapter/SQL.php (2)
3076-3116: Use quoted identifiers for the RIGHT JOINIS NULLcheck.When
$rightJoinsis true, the permission condition appends{$alias}._uid IS NULLusing unquoted identifiers. This is inconsistent withgetSQLPermissionsCondition()which uses$this->quote()for identifiers.Apply this diff to use consistent quoting:
if (! $skipAuth) { $permissionsCondition = $this->getSQLPermissionsCondition($name, $roles, $alias, $forPermission); if ($rightJoins) { - $permissionsCondition = "($permissionsCondition OR {$alias}._uid IS NULL)"; + $permissionsCondition = sprintf( + '(%s OR %s.%s IS NULL)', + $permissionsCondition, + $this->quote($alias), + $this->quote('_uid') + ); } $where[] = $permissionsCondition; }
3266-3277: Same quoting inconsistency incount()for RIGHT JOIN permission check.The
count()method has the same unquoted identifier issue asfind().Apply this diff:
if (! $skipAuth) { $permissionsCondition = $this->getSQLPermissionsCondition($name, $roles, $alias); if ($rightJoins) { - $permissionsCondition = "($permissionsCondition OR {$alias}._uid IS NULL)"; + $permissionsCondition = sprintf( + '(%s OR %s.%s IS NULL)', + $permissionsCondition, + $this->quote($alias), + $this->quote('_uid') + ); } $where[] = $permissionsCondition; }src/Database/Query.php (1)
1559-1599:addSelect()does not add a select when no existing selects are present.When
$queriescontains noTYPE_SELECTqueries,$foundremainsfalse, and the new select query is never added. This seems counterintuitive for a method namedaddSelect.If the first select should always be added, consider adjusting the logic:
- if ($found && $merge) { + if ($merge) { $queries = [ ...$queries, $query ]; return [$queries, true]; } return [$queries, false];This preserves the duplicate/wildcard protection while allowing the first select to be added.
🧹 Nitpick comments (5)
src/Database/Database.php (2)
7992-8066:count()gains join-aware validation, butmaxargument semantics change.The new
count()flow (building aQueryContext, wiring joins, usingDocumentsValidator, then callingadapter->count($context, …)with relationship filters converted to subqueries) looks solid.One behavioral change:
if (!is_null($max) && $max < 1)now rejects$max = 0outright. If existing call sites ever used$max = 0as “no limit” (similar to howsum()still documents$max = 0as unlimited), this will start throwingDatabaseExceptioninstead of delegating to the adapter.If backward compatibility matters, consider normalizing
$max === 0tonull(or documenting the new constraint clearly) so consumers aren’t surprised by exceptions from previously-valid calls.
8645-8685: Be cautious about serializing fullQueryobjects into cache keys.Using
\md5(\serialize($selects))means the cache hash now depends on the entireQueryobjects (method, alias, system flag, etc.), not just the logical projection. That’s OK as long asQuerystays a simple data holder, but if you ever add non-deterministic fields (e.g. runtime metadata) you could unintentionally fragment the cache.If you only care about the projection, consider hashing a normalized description instead, e.g. an array of
[$alias, $attribute, $as]per select.src/Database/Adapter/Postgres.php (1)
1725-1787: Filter and quote right-hand join alias consistentlyIn
getSQLCondition()you now supportTYPE_RELATION_EQUAL:$attributeRight = $this->quote($this->filter($query->getAttributeRight())); $aliasRight = $this->quote($query->getRightAlias()); return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}";The left alias goes through
filter()thenquote(), but the right alias is only quoted. For consistency and a bit more defense-in-depth (even though aliases are validated elsewhere), it’s better to apply the same filtering:- case Query::TYPE_RELATION_EQUAL: - $attributeRight = $this->quote($this->filter($query->getAttributeRight())); - $aliasRight = $this->quote($query->getRightAlias()); - - return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}"; + case Query::TYPE_RELATION_EQUAL: + $attributeRight = $this->quote($this->filter($query->getAttributeRight())); + $aliasRight = $this->quote($this->filter($query->getRightAlias())); + + return "{$alias}.{$attribute} = {$aliasRight}.{$attributeRight}";Keeps both sides of the relation predicate normalized the same way as other SQL fragments.
src/Database/Adapter/Mongo.php (1)
1875-1887: Silence PHPMD “unused parameter” warnings for$joins/$vectorsBoth
find()andcount()accept$joins/$vectorsto match the adapter interface, but Mongo doesn’t support relationships or vectors and the parameters are unused:public function find( QueryContext $context, ?int $limit = 25, ?int $offset = null, array $cursor = [], string $cursorDirection = Database::CURSOR_AFTER, string $forPermission = Database::PERMISSION_READ, array $selects = [], array $filters = [], array $joins = [], array $vectors = [], array $orderQueries = [] ): array { … } public function count(QueryContext $context, ?int $max, array $filters = [], array $joins = []): int { … }PHPMD flags these as unused. A simple, non-behavioral way to clean this up is to explicitly unset them at the start of each method:
public function find( QueryContext $context, @@ - array $joins = [], - array $vectors = [], + array $joins = [], + array $vectors = [], array $orderQueries = [] ): array { + unset($joins, $vectors); @@ public function count(QueryContext $context, ?int $max, array $filters = [], array $joins = []): int { + unset($joins);This keeps the signature stable while satisfying static analysis.
Also applies to: 2140-2141
src/Database/Query.php (1)
338-366: Consider returning the direction constant from the class instead of Database.The
getCursorDirection()andgetOrderDirection()methods throw generic\Exceptioninstead ofQueryException. For consistency with other query-related errors, consider usingQueryException.- throw new \Exception('Invalid method: Get cursor direction on "'.$this->method.'" Query'); + throw new QueryException('Invalid method: Get cursor direction on "'.$this->method.'" Query');- throw new \Exception('Invalid method: Get order direction on "'.$this->method.'" Query'); + throw new QueryException('Invalid method: Get order direction on "'.$this->method.'" Query');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Database/Adapter/Mongo.php(14 hunks)src/Database/Adapter/Postgres.php(2 hunks)src/Database/Adapter/SQL.php(12 hunks)src/Database/Database.php(55 hunks)src/Database/Query.php(27 hunks)src/Database/QueryContext.php(1 hunks)src/Database/Validator/Queries/V2.php(1 hunks)src/Database/Validator/Query/Filter.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/QueryContext.php
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Query.phpsrc/Database/Database.phpsrc/Database/Validator/Queries/V2.phpsrc/Database/Adapter/SQL.php
📚 Learning: 2025-08-14T06:35:30.429Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 661
File: tests/e2e/Adapter/Scopes/SpatialTests.php:180-186
Timestamp: 2025-08-14T06:35:30.429Z
Learning: Query::distance method in Utopia\Database\Query expects an array of values parameter, where each value is a [geometry, distance] pair. So the correct format is Query::distance('attribute', [[[lat, lng], distance]]) where the outer array contains the values and each value is [geometry, distance].
Applied to files:
src/Database/Query.php
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.phpsrc/Database/Validator/Queries/V2.phpsrc/Database/Adapter/Mongo.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.phpsrc/Database/Validator/Queries/V2.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Database.phpsrc/Database/Validator/Queries/V2.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: 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.
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Adapter/Mongo.php
🧬 Code graph analysis (3)
src/Database/Adapter/Postgres.php (2)
src/Database/Query.php (6)
setAttributeRight(331-336)getAttributeRight(287-290)getAttribute(255-258)getAlias(277-280)Query(8-1600)getRightAlias(282-285)src/Database/Adapter.php (1)
quote(491-491)
src/Database/Validator/Queries/V2.php (9)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Query.php (8)
Query(8-1600)__construct(198-233)isNested(1301-1308)getMethod(247-250)getFilterQueries(1183-1186)isSpatialQuery(456-473)isVectorQuery(479-482)parse(491-504)src/Database/QueryContext.php (4)
QueryContext(8-95)__construct(25-27)getCollections(32-35)getCollectionByAlias(45-63)src/Database/Validator/Alias.php (5)
Alias(7-70)getDescription(15-18)isArray(54-57)getType(66-69)isValid(26-45)src/Database/Validator/AsQuery.php (6)
AsQuery(7-84)__construct(14-18)getDescription(24-27)isArray(68-71)getType(80-83)isValid(35-59)src/Database/Validator/Datetime.php (1)
Datetime(7-152)src/Database/Validator/Query/Cursor.php (1)
Cursor(10-56)src/Database/Validator/Query/Limit.php (1)
Limit(9-63)src/Database/Validator/Query/Offset.php (3)
Offset(9-59)__construct(16-19)isValid(25-53)
src/Database/Adapter/Mongo.php (2)
src/Database/QueryContext.php (3)
QueryContext(8-95)getMainCollection(40-43)skipAuth(83-94)src/Database/Document.php (2)
removeAttribute(287-293)Document(12-470)
🪛 GitHub Actions: Linter
src/Database/Validator/Query/Filter.php
[error] 1-1: Pint lint failed. PSR-12 violation: 'blank_line_after_opening_tag' in src/Database/Validator/Query/Filter.php.
🪛 PHPMD (2.15.0)
src/Database/Database.php
8784-8784: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
src/Database/Adapter/Mongo.php
1884-1884: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
1885-1885: Avoid unused parameters such as '$vectors'. (undefined)
(UnusedFormalParameter)
2140-2140: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (25)
src/Database/Database.php (6)
5118-5123: QueryContext-based casting/decoding on create/createDocuments looks consistent.Creating a
QueryContextfor the collection and routing fresh documents throughcasting($context, …)anddecode($context, …)afteradapter->castingAfter()mirrors thefind()/getDocument()behavior and should keep type normalization uniform across read/write paths. I don’t see issues in these additions.Also applies to: 5171-5173, 5234-5238
4988-5013:applySelectFiltersToDocuments()’s projection logic looks sound for post-batch relationship filtering.Collecting
attributesToKeepfrom select queries, special-casing'*', and always preserving internal ($*) attributes gives you a clean way to trim batch-fetched relationship documents back down to the requested projection. Given this is only invoked with relationship-level select queries, the behavior here matches the intent.
7496-7551: Bulk delete path’s newQueryContext/validation and required-attribute guard look consistent.
deleteDocuments()now:
- Builds a
QueryContext, wires all join collections into it, and records per-collectionskipAuthflags (viaaddSkipAuth), just likefind().- Validates the original queries with
DocumentsValidator.- Ensures
$permissionsand$sequenceare present when an explicit projection is used, throwing aQueryExceptionif the select list is insufficient for the internal delete flow.That matches what the adapter and permission cleanup need and should fail fast instead of producing partial/deceptive behavior. No functional issues spotted here.
Also applies to: 7581-7588
7750-7771: Join-awarefind()integration withQueryContextlooks coherent.Seeding
QueryContextwith the main collection plus all join targets, computing per-collectionskipAuthflags, validating withDocumentsValidator, then running$this->convertQueries($context, $queries)before passingselects,filters,joins,vectors, andorderQueriesdown to the adapter gives a clear and consistent story for join handling. Authorization, context, and projection all line up with the new model.Also applies to: 7795-7806, 7841-7865, 7874-7889
6090-6466: TargetedQuery::select('$id')usages in relationship helpers are a good fit for the new constraints.Switching internal
getDocument/findOne/findcalls inupdateDocumentRelationships(),deleteRestrict(),deleteSetNull(), anddeleteCascade()to include explicitQuery::select('$id')(and sometimes only$id) is a nice refinement:
- It respects the new rule that
getDocument()’s$queriesargument must be composed solely of select queries.- It keeps payloads minimal while still providing the only attribute these helpers actually need for existence/uniqueness checks and cascading behavior.
I don’t see regressions introduced by these changes.
Also applies to: 7203-7375, 7395-7477
8547-8561:convertQueries()/convertQuery()withQueryContextgive consistent alias-aware typing.The new
convertQueries(QueryContext $context, array $queries)recursion and the alias-based lookup inconvertQuery()(with a clearQueryExceptionon unknown aliases) provide a clean, centralized place to attach attribute metadata (array flag, type, datetime conversion) across joins and nested queries. SkippingTYPE_SELECThere also avoids mutating simple projections. This looks correct and nicely factored.Also applies to: 8566-8615
src/Database/Adapter/SQL.php (7)
20-20: LGTM!The import of
QueryContextis correctly added to support the new context-driven query API.
375-378: LGTM!The
getDocumentmethod correctly uses the newgetAttributeProjectionsignature and properly quotes the alias in the WHERE clause.
1800-1805: LGTM!The
getSQLOperatorcorrectly maps join type constants to their SQL equivalents. Note thatTYPE_INNER_JOINmaps to'JOIN'(which is semantically equivalent toINNER JOIN).
2306-2334: LGTM!The
getTenantQuerysignature is correctly extended with theforceIsNullparameter to support RIGHT JOIN scenarios where NULL tenant values should be allowed in the WHERE clause.
2344-2381: LGTM!The rewritten
getAttributeProjectioncorrectly:
- Returns quoted default alias with
.*when no selects are provided- Maps special attributes (
$id,$collection, etc.) to internal keys- Filters and quotes attribute names properly
- Handles the
ASaliasing for selected columns
2981-3211: Thefind()method implementation with QueryContext is well-structured.The method correctly:
- Extracts collection from context
- Handles join permissions via
context->skipAuth()- Builds join SQL with proper operator mapping from
getSQLOperator()- Tracks RIGHT JOINs for permission adjustments
- Handles cursor pagination with proper tie-breaking logic
- Includes vector distance ordering
3225-3320: Thecount()method implementation with QueryContext looks correct.The method properly:
- Uses QueryContext for collection and skip-auth checks
- Handles joins with proper permission logic
- Tracks RIGHT JOINs for NULL handling
- Uses a subquery approach with LIMIT support
src/Database/Query.php (12)
44-46: LGTM!The new join-related constants are properly defined and organized:
TYPE_RELATION_EQUALfor join condition queriesTYPE_INNER_JOIN,TYPE_LEFT_JOIN,TYPE_RIGHT_JOINfor join operationsAlso applies to: 69-73
133-173: LGTM!The new
JOINS_TYPESandFILTER_TYPESarrays are well-organized and include all relevant query types.TYPE_RELATION_EQUALis correctly included inFILTER_TYPESsince it's used in join ON conditions.
176-183: LGTM!The new metadata properties and protected constructor correctly:
- Add fields for join metadata (
collection,alias,aliasRight,attributeRight,as,system)- Default
aliasandaliasRighttoQuery::DEFAULT_ALIASwhen empty- Wire all fields through the constructor
Also applies to: 198-233
277-300: LGTM!The new getters are straightforward and provide proper access to the metadata fields.
441-449: LGTM!The
isMethod()function correctly includes the new join and relation types, allowing them to be recognized as valid query methods for parsing and validation.
475-482: LGTM!The new
isVectorQuery()static helper method provides a clean way to check if a method is a vector query type.
517-555: LGTM!The
parseQuery()method correctly:
- Extracts the new metadata fields from the input array
- Handles nested queries for both logical types AND join types
- Passes all metadata fields to the constructor using named parameters
588-620: LGTM!The
toArray()method correctly:
- Emits new metadata fields only when they differ from defaults
- Handles nested serialization for both logical and join queries via
isJoin()
997-1044: LGTM!The join factory methods are well-designed:
join()andinnerJoin()are aliases for inner joinleftJoin()andrightJoin()create appropriate join typesrelationEqual()creates join ON conditions with proper alias tracking
1066-1195: LGTM!The new public helper methods (
getSelectQueries,getJoinQueries,getLimitQueries,getLimitQuery,getOffsetQueries,getOffsetQuery,getOrderQueries,getCursorQueries,getFilterQueries,getVectorQueries) provide a clean public API that replaces directgetByType()calls.
1310-1321: LGTM!The
isJoin()andisFilter()methods provide clean type-checking utilities for query categorization.
1053-1064: No breaking change —getByType()is an internal implementation detail.The method is not tested, not documented, and has no usages within the repository. Changing its visibility from public to protected aligns with its actual role as an internal helper method rather than part of the public API surface.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Adapter/SQL.php (1)
2965-3074: Fix cursor pagination conditions to use each order query’s own aliasIn
find(), the cursor WHERE clauses for multi‑column ordering currently use the current order’s alias for all previous attributes:foreach ($orderQueries as $i => $order) { $orderAlias = $order->getAlias(); … for ($j = 0; $j < $i; $j++) { $prevQuery = $orderQueries[$j]; $prevOriginal = $prevQuery->getAttribute(); $prevAttr = $this->filter($this->getInternalKeyForAttribute($prevOriginal)); … $conditions[] = "{$this->quote($orderAlias)}.{$this->quote($prevAttr)} = {$bindName}"; } … $conditions[] = "{$this->quote($orderAlias)}.{$this->quote($attribute)} {$this->getSQLOperator($operator)} {$bindName}"; }If different order clauses use different aliases (e.g. main vs join alias), this generates equality predicates against the wrong table alias and can yield incorrect filters or invalid SQL.
Use each previous query’s alias instead:
- for ($j = 0; $j < $i; $j++) { - $prevQuery = $orderQueries[$j]; - $prevOriginal = $prevQuery->getAttribute(); - $prevAttr = $this->filter($this->getInternalKeyForAttribute($prevOriginal)); - - $bindName = ":cursor_{$j}"; - $binds[$bindName] = $cursor[$prevOriginal]; - - $conditions[] = "{$this->quote($orderAlias)}.{$this->quote($prevAttr)} = {$bindName}"; - } + for ($j = 0; $j < $i; $j++) { + $prevQuery = $orderQueries[$j]; + $prevAlias = $prevQuery->getAlias(); + $prevOriginal = $prevQuery->getAttribute(); + $prevAttr = $this->filter($this->getInternalKeyForAttribute($prevOriginal)); + + $bindName = ":cursor_{$j}"; + $binds[$bindName] = $cursor[$prevOriginal]; + + $conditions[] = "{$this->quote($prevAlias)}.{$this->quote($prevAttr)} = {$bindName}"; + }The comparison for the current attribute can continue to use
$orderAliasas it already does. This keeps cursor pagination correct when ordering across fields from different aliases.src/Database/Database.php (1)
4295-4335: System$permissionsselect is stripped only on non‑cached reads, causing inconsistent results.In
getDocument()you compute$permissionsAddedviaQuery::addSelect()and later remove$permissionsonly in the non‑cache branch (Lines 4411–4413). When the same document is served from cache,$permissionsremains present even when it was added implicitly, so callers see different shapes depending on cache hit vs miss.Consider also stripping
$permissionsin the cache‑hit branch, after auth checks and before returning:if ($cached) { $document = $this->createDocumentInstance($collection->getId(), $cached); if ($collection->getId() !== self::METADATA) { if (!$this->authorization->isValid(new Input(self::PERMISSION_READ, [ ...$collection->getRead(), ...($documentSecurity ? $document->getRead() : []) ]))) { return $this->createDocumentInstance($collection->getId(), []); } } + if ($permissionsAdded) { + $document->removeAttribute('$permissions'); + } + $this->trigger(self::EVENT_DOCUMENT_READ, $document); return $document; }Also applies to: 4401-4413
♻️ Duplicate comments (4)
src/Database/Validator/Query/Filter.php (1)
2-422: Delete the commented Filter implementation (or replace with a minimal stub) and fix PSR‑12 blank-line errorThe entire legacy
Filtervalidator is kept as commented code and line 1 violatesPSR-12: blank_line_after_opening_tag. This both clutters the repo and currently breaks the linter.Recommend either:
- Remove this file entirely if
Filteris deprecated in favor of the V2/Documents validator, or- Replace the block with a short comment explaining that
Utopia\Database\Validator\Query\Filterwas removed in favor ofUtopia\Database\Validator\Queries\V2(DocumentsValidator), and ensure there is a blank line after<?php>.In either case, drop the 400+ lines of commented implementation.
src/Database/Validator/Queries/V2.php (1)
176-226: Short‑circuitvalidateAttributeExist()for unknown attributes in schemaless modeWhen
$supportForAttributesisfalseand an attribute is not in$this->schema,$attributeis empty but the method still executes type-based checks:$attribute = $this->schema[$collection->getId()][$attributeId] ?? []; if (empty($attribute) && $this->supportForAttributes) { throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); } if (\in_array('encrypt', $attribute['filters'] ?? [])) { ... } if ($isNested && $attribute['type'] != Database::VAR_RELATIONSHIP) { ... }In schemaless mode this both:
- Accesses
$attribute['type']on an empty array, and- Incorrectly throws “Only nested relationships allowed” for arbitrary nested fields, instead of behaving permissively as in
validateValues().Align this with
validateValues()by returning early when the attribute is unknown and schema support is disabled:- $attribute = $this->schema[$collection->getId()][$attributeId] ?? []; - if (empty($attribute) && $this->supportForAttributes) { - throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); - } + $attribute = $this->schema[$collection->getId()][$attributeId] ?? []; + + if (empty($attribute)) { + if ($this->supportForAttributes) { + throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); + } + + // Schemaless: unknown attribute, skip attribute-based checks + return; + }This avoids undefined-index notices and keeps schemaless behavior consistent with the rest of the validator.
src/Database/Adapter/Mongo.php (1)
1938-1950: Guard against unsupported ORDER_RANDOM before calling getOrderDirection()This issue was flagged in a previous review but remains unaddressed. At line 1944,
$order->getOrderDirection()is called without checking forTYPE_ORDER_RANDOM. Since this adapter reportsgetSupportForOrderRandom() === falseandgetOrderDirection()throws an exception for random-order queries, a defensive check is needed.Apply this diff to add the guard:
foreach ($orderQueries as $i => $order) { $attribute = $order->getAttribute(); $originalAttribute = $attribute; $attribute = $this->getInternalKeyForAttribute($originalAttribute); $attribute = $this->filter($attribute); + // Reject unsupported random order queries + if ($order->getMethod() === Query::TYPE_ORDER_RANDOM) { + throw new DatabaseException('MongoDB adapter does not support random ordering'); + } + $direction = $order->getOrderDirection();src/Database/Query.php (1)
1560-1599:addSelectstill never appends when there are no existingTYPE_SELECTqueriesThe logic here remains the same as in the earlier review: because the append is guarded by
if ($found && $merge), callingaddSelect()on a list that contains noTYPE_SELECTqueries will never add the new select —$foundstaysfalse, so you always return the original$queriesunchanged.That means the first select is silently dropped, which is surprising for a method named
addSelectand can break callers that rely on it to inject selects into an initially empty query list.You can preserve the duplicate/wildcard protection while always allowing the first select to be appended by dropping the
$foundcondition, e.g.:- if ($found && $merge) { + if ($merge) { $queries = [ ...$queries, $query ]; return [$queries, true]; }This still prevents merging when there is a conflicting
*or identical select, but no longer blocks the initial select from being added.
🧹 Nitpick comments (5)
src/Database/Adapter/SQL.php (1)
374-379: ConfirmgetDocument()always passes only SELECT queries intogetAttributeProjection()
getDocument()now builds its projection via:SELECT {$this->getAttributeProjection($queries)} FROM … WHERE {$this->quote($alias)}._uid = :_uid
getAttributeProjection()assumes its input is a list ofTYPE_SELECTqueries. If callers ever pass a mixed$queriesarray (filters, limits, etc.), those will be treated as selects and end up in the projection list.If the surrounding codepath guarantees
$queriesis already filtered down to selects (e.g. viaQuery::getSelectQueries()), this is fine; otherwise consider either:
- Passing only the select subset into
getAttributeProjection(), or- Filtering
$selectsinsidegetAttributeProjection()togetMethod() === Query::TYPE_SELECT.Tagging for verification rather than as a bug, since the upstream contract isn’t visible here.
src/Database/Adapter/Mongo.php (2)
1875-1887: Document or validate unsupported features in find() signatureThe
$joinsand$vectorsparameters are unused since this adapter reportsgetSupportForRelationships() === falseandgetSupportForVectors() === false. While these parameters maintain interface consistency, consider adding:
- PHPDoc annotations like
@param array<Query> $joins (unused - MongoDB does not support joins)- Defensive validation at the start of the method to throw a clear exception if non-empty arrays are passed
This would improve maintainability and prevent silent failures if these parameters are mistakenly populated.
Apply this diff to add defensive checks:
public function find( QueryContext $context, ?int $limit = 25, ?int $offset = null, array $cursor = [], string $cursorDirection = Database::CURSOR_AFTER, string $forPermission = Database::PERMISSION_READ, array $selects = [], array $filters = [], array $joins = [], array $vectors = [], array $orderQueries = [] ): array { + if (!empty($joins)) { + throw new DatabaseException('MongoDB adapter does not support joins'); + } + if (!empty($vectors)) { + throw new DatabaseException('MongoDB adapter does not support vector queries'); + } + $collection = $context->getMainCollection();
2140-2162: Unused $joins parameter in count() signatureSimilar to
find(), the$joinsparameter at line 2140 is unused since MongoDB doesn't support joins (getSupportForRelationships() === false). For interface consistency this is acceptable, but consider adding PHPDoc annotation or a defensive check as suggested for thefind()method.The count implementation correctly uses QueryContext for collection retrieval and authorization, and properly handles the aggregation pipeline with timeout support.
src/Database/Query.php (2)
1212-1281: Confirm whether join queries should be included in thefiltersbucket ofgroupByType
groupByType()’sswitchstill routes everything that is not order/limit/offset/cursor/select into the'filters'bucket. With the addition of join methods (TYPE_INNER_JOIN,TYPE_LEFT_JOIN,TYPE_RIGHT_JOIN) and the newisJoin()helper, join queries will currently fall into'filters'here.If
groupByType()is still used in places that now support joins, you may want to either:
- Explicitly skip join methods in this switch, or
- Add a dedicated
'joins'bucket alongside'filters'and'selections'.If
groupByType()is only used in legacy, non-join code paths, this may be fine as-is, but it’s worth double-checking call sites to ensure joins are not accidentally treated as regular filters.
338-366: Consider throwing a domain-specific exception ingetCursorDirection/getOrderDirectionThe new helpers correctly map cursor and order query methods to
Databasedirection constants, but on misuse they throw a generic\Exception. For consistency with the rest of this class, consider throwing a more specific exception (e.g.QueryExceptionor\InvalidArgumentException) so callers can distinguish programming mistakes from other failures more easily. This is non-critical but would tighten the API surface.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Database/Adapter/Mongo.php(14 hunks)src/Database/Adapter/Postgres.php(2 hunks)src/Database/Adapter/SQL.php(12 hunks)src/Database/Database.php(55 hunks)src/Database/Query.php(27 hunks)src/Database/QueryContext.php(1 hunks)src/Database/Validator/Queries/V2.php(1 hunks)src/Database/Validator/Query/Filter.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Database/Adapter/Postgres.php
- src/Database/QueryContext.php
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Database.php
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Database.php
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Adapter/SQL.phpsrc/Database/Query.phpsrc/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Database.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: 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.
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Adapter/Mongo.php
📚 Learning: 2025-08-14T06:35:30.429Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 661
File: tests/e2e/Adapter/Scopes/SpatialTests.php:180-186
Timestamp: 2025-08-14T06:35:30.429Z
Learning: Query::distance method in Utopia\Database\Query expects an array of values parameter, where each value is a [geometry, distance] pair. So the correct format is Query::distance('attribute', [[[lat, lng], distance]]) where the outer array contains the values and each value is [geometry, distance].
Applied to files:
src/Database/Query.php
🧬 Code graph analysis (5)
src/Database/Validator/Queries/V2.php (9)
src/Database/Document.php (3)
Document(12-470)getId(63-66)getArrayCopy(423-458)src/Database/Query.php (12)
Query(8-1600)__construct(198-233)getAttribute(255-258)isNested(1301-1308)getAlias(277-280)getMethod(247-250)getFilterQueries(1183-1186)getValues(263-266)isSpatialQuery(456-473)isVectorQuery(479-482)parse(491-504)getAs(292-295)src/Database/QueryContext.php (4)
QueryContext(8-95)__construct(25-27)getCollections(32-35)getCollectionByAlias(45-63)src/Database/Validator/Alias.php (5)
Alias(7-70)getDescription(15-18)isArray(54-57)getType(66-69)isValid(26-45)src/Database/Validator/AsQuery.php (6)
AsQuery(7-84)__construct(14-18)getDescription(24-27)isArray(68-71)getType(80-83)isValid(35-59)src/Database/Validator/Datetime.php (1)
Datetime(7-152)src/Database/Validator/Query/Cursor.php (1)
Cursor(10-56)src/Database/Validator/Query/Limit.php (1)
Limit(9-63)src/Database/Adapter/SQL.php (2)
__construct(60-63)count(3225-3320)
src/Database/Adapter/SQL.php (3)
src/Database/QueryContext.php (3)
QueryContext(8-95)getMainCollection(40-43)skipAuth(83-94)src/Database/Query.php (10)
Query(8-1600)select(788-791)getAttribute(255-258)getAlias(277-280)getAs(292-295)getOrderDirection(351-366)join(1003-1006)getCollection(297-300)getMethod(247-250)getValues(263-266)src/Database/Validator/Authorization.php (1)
getRoles(105-108)
src/Database/Query.php (2)
src/Database/Database.php (2)
__construct(447-676)Database(38-9411)src/Database/QueryContext.php (1)
__construct(25-27)
src/Database/Adapter/Mongo.php (3)
src/Database/QueryContext.php (3)
QueryContext(8-95)getMainCollection(40-43)skipAuth(83-94)src/Database/Adapter/SQL.php (1)
renameAttribute(310-327)src/Database/Validator/Authorization.php (1)
getRoles(105-108)
src/Database/Database.php (4)
src/Database/Validator/Queries/V2.php (2)
V2(23-716)getDescription(135-138)src/Database/QueryContext.php (7)
QueryContext(8-95)add(68-76)getCollections(32-35)skipAuth(83-94)addSkipAuth(78-81)getMainCollection(40-43)getCollectionByAlias(45-63)src/Database/Query.php (12)
Query(8-1600)getSelectQueries(1070-1075)addSelect(1566-1599)select(788-791)getAttribute(255-258)join(1003-1006)getCollection(297-300)limit(831-834)offset(842-845)setAttribute(321-326)isNested(1301-1308)isJoin(1313-1316)src/Database/Document.php (4)
getAttribute(224-231)Document(12-470)getCollection(85-88)setAttribute(244-261)
🪛 GitHub Actions: Linter
src/Database/Validator/Query/Filter.php
[error] 1-1: PSR-12: blank_line_after_opening_tag
🪛 PHPMD (2.15.0)
src/Database/Validator/Queries/V2.php
513-513: Avoid unused local variables such as '$ambiguous'. (undefined)
(UnusedLocalVariable)
514-514: Avoid unused local variables such as '$duplications'. (undefined)
(UnusedLocalVariable)
src/Database/Adapter/Mongo.php
1884-1884: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
1885-1885: Avoid unused parameters such as '$vectors'. (undefined)
(UnusedFormalParameter)
2140-2140: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
src/Database/Database.php
8784-8784: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (8)
src/Database/Adapter/SQL.php (4)
1770-1805: Join type mapping ingetSQLOperator()looks correctThe new cases for
TYPE_INNER_JOIN,TYPE_LEFT_JOIN, andTYPE_RIGHT_JOINreturningJOIN/LEFT JOIN/RIGHT JOINare a clean way to centralize join keyword selection and are correctly reused from the join-building code infind()/count().
2302-2335: ExtendedgetTenantQuery()contract is consistent with new right‑join handlingAdding
$conditionand$forceIsNulland then usingforceIsNullwhen any right join is present:$orIsNull = ''; if ($collection === Database::METADATA || $forceIsNull) { $orIsNull = " OR {$alias}{$dot}_tenant IS NULL"; } … return "{$condition} ({$alias}{$dot}_tenant IN ({$bindings}) {$orIsNull})";matches the updated
find()/count()calls and keeps existing call sites (e.g. permissions subqueries) working via the default'AND'. The emptycondition: ''pattern for top‑level$where[]entries is also consistent with howsum()already uses this helper.
3147-3210: Result fetching and mapping infind()looks solid after context/join refactorThe new
find()implementation:
- Uses
getAttributeProjection($selects)consistently for projections.- Correctly merges vector-based ORDER BY expressions ahead of normal orders.
- Binds floats via
getFloatPrecision()andPDO::PARAM_STR, leaving other types togetPDOType().- Maps internal columns (
_uid,_id,_tenant,_createdAt,_updatedAt,_permissions) back to their$…counterparts before wrapping each row inDocument, and reverses results when usingCURSOR_BEFORE.This aligns well with the QueryContext-driven API and looks coherent.
3216-3320:count()’s QueryContext/joins integration matchesfind()semanticsThe updated
count()mirrors the newfind()behavior:
- Accepts
QueryContextandarray<Query> $joins.- Builds
$sqlJoinusinggetSQLOperator($join->getMethod())andgetSQLConditions()for ON clauses.- Applies per-join permission filters and tenant conditions, and tracks right joins to relax main‑alias permissions (
OR alias._uid IS NULL) and tenant filters viaforceIsNull.- Applies an optional
$maxvia innerLIMIT :limit.This keeps count semantics aligned with the data returned by
find(), including joins and right‑join behavior.src/Database/Adapter/Mongo.php (4)
19-19: LGTM: QueryContext integration and renameAttribute signature updateThe import of
QueryContextand the updatedrenameAttributesignature align with the context-driven architecture changes. The internal key mapping is correctly applied to both the old and new attribute names.Also applies to: 711-722
2565-2598: LGTM: getAttributeProjection implementationThe projection mapping is correctly implemented:
- Returns empty array when no selects or wildcard (
*) is present (MongoDB returns all fields)- Properly maps internal attributes (
$id→_uid,$sequence→_id, etc.)- Skips unsupported
$collectionattribute- Returns MongoDB projection format (
{attribute: 1})This aligns with the conditional
$sequenceremoval logic infind()andgetDocument()where MongoDB's always-returned_idfield is cleaned up when not requested.
1790-1792: LGTM: Updated deleteDocuments to use Query::equalThe change from direct filter construction to using
Query::equal('$sequence', $sequences)aligns with the new query-building pattern and ensures consistent internal key mapping throughbuildFilters().
1125-1136: No issue found. The Database.php wrapper (lines 4275-4289) explicitly validates and filters queries before passing them to the Mongo adapter. It extracts SELECT queries viaQuery::getSelectQueries($queries)and throws aQueryExceptionif any non-SELECT queries are present. Consequently,getAttributeProjection()is guaranteed to receive only SELECT Query objects, making the filtering unnecessary at the adapter level.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
8807-8994: Many‑to‑many relationship filters:skipRelationshipslikely breaks parent ID derivation.
processNestedRelationshipPath()andconvertRelationshipFiltersToSubqueries()rely on:
- Calling
find()on the related collection,- Selecting both
'$id'and the relationship’stwoWayKey,- Then deriving parent IDs from the
twoWayKeyattribute on those related documents.- For many‑to‑many relationships in this codebase, those reverse links are populated via relationship resolution (
populateDocumentsRelationships), not as plain scalar columns.- Both in
processNestedRelationshipPath()and in the directneedsParentResolutionbranch ofconvertRelationshipFiltersToSubqueries(), the innerfind()calls are wrapped inskipRelationships(...), which disables relationship population; this contradicts the earlier, working pattern for many‑to‑many filtering that depended on not skipping relationships to materialize the two‑way keys. Based on the prior learning, that will tend to produce empty$parentIdsand cause such filters to short‑circuit to “no matches” instead of returning the correct parent documents.I’d suggest:
- For
relationType === RELATION_MANY_TO_MANY, drop theskipRelationships()wrapper when fetching the related collection used to derive parent IDs (and ensure the necessary selects are present), or- Factor out a dedicated many‑to‑many branch that reuses the earlier, proven approach for resolving parent IDs from populated relationship attributes instead of from raw columns.
Based on learnings, many‑to‑many filtering previously relied on
find()withoutskipRelationshipsto populate the reverse relationship before deriving parent IDs.
♻️ Duplicate comments (8)
src/Database/Validator/Query/Filter.php (1)
3-423: The commented-out Filter validator class should be removed.This concern has already been raised in a previous review. The entire 423-line Filter validator class remains commented out. As noted in the prior review, if this validator has been superseded by the V2 validation approach (DocumentsValidator with QueryContext), the commented code should be deleted entirely rather than kept as a non-functional placeholder.
src/Database/Query.php (2)
515-555: Validate new metadata fields inparseQuery()to avoid rawTypeErrors
parseQuery()now extracts additional metadata:$method = $query['method'] ?? ''; $attribute = $query['attribute'] ?? ''; $attributeRight = $query['attributeRight'] ?? ''; $values = $query['values'] ?? []; $alias = $query['alias'] ?? ''; $aliasRight = $query['aliasRight'] ?? ''; $as = $query['as'] ?? ''; $collection = $query['collection'] ?? '';You validate only
method,attribute, andvaluestypes before calling the constructor. If any of the new fields is not a string (e.g.alias: []in user JSON), the strict-typed constructor will throw aTypeErrorinstead of the expectedQueryException, which is inconsistent with the rest of the parsing API.Consider mirroring the existing validation for these keys, for example:
- if (!\is_array($values)) { + if (!\is_array($values)) { throw new QueryException('Invalid query values. Must be an array, got ' . \gettype($values)); } + + foreach (['alias' => $alias, 'aliasRight' => $aliasRight, 'attributeRight' => $attributeRight, 'collection' => $collection, 'as' => $as] as $key => $meta) { + if (!\is_string($meta)) { + throw new QueryException('Invalid query ' . $key . '. Must be a string, got ' . \gettype($meta)); + } + }This keeps
parse()/parseQuery()failures predictable and contained toQueryExceptioneven for malformed JSON.
76-120: Confirm whether join/relation types should be exposed viaTYPES
isMethod()has been updated to recognize join and relation-equal types:case self::TYPE_RELATION_EQUAL, self::TYPE_INNER_JOIN, self::TYPE_LEFT_JOIN, self::TYPE_RIGHT_JOIN, self::TYPE_VECTOR_DOT, ...but the public
TYPESconstant still omits these:public const TYPES = [ self::TYPE_EQUAL, ... self::TYPE_CURSOR_BEFORE, self::TYPE_AND, self::TYPE_OR, ];If external consumers (or internal validators) treat
Query::TYPESas the canonical list of supported methods, joins andrelationEqualwill be invisible there despite being accepted byisMethod()/parse(). That asymmetry can be surprising.If joins are intended as first-class public query methods, consider appending them to
TYPES:public const TYPES = [ ... self::TYPE_AND, self::TYPE_OR, + self::TYPE_RELATION_EQUAL, + self::TYPE_INNER_JOIN, + self::TYPE_LEFT_JOIN, + self::TYPE_RIGHT_JOIN, ];If instead joins are meant to be internal-only, a short docblock on
TYPESexplaining that it intentionally excludes join methods would help clarify expectations.src/Database/Validator/Queries/V2.php (2)
188-225: Avoid undefinedtypeaccess for unknown nested attributes in schemaless modeWhen
$supportForAttributesisfalseand an attribute is not in$this->schema,$attributebecomes[]. For nested attributes, the later checks still dereference$attribute['type']:$attribute = $this->schema[$collection->getId()][$attributeId] ?? []; if (empty($attribute) && $this->supportForAttributes) { throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); } if ($isNested && $attribute['type'] != Database::VAR_RELATIONSHIP) { throw new \Exception('Only nested relationships allowed'); }In schemaless mode (supportForAttributes = false) and
empty($attribute), this will access an undefined index and can also incorrectly throw “Only nested relationships allowed” for arbitrary nested fields. This is inconsistent withvalidateValues(), which short-circuits when schema support is disabled and the attribute is unknown.Recommend short-circuiting here as well:
- $attribute = $this->schema[$collection->getId()][$attributeId] ?? []; - if (empty($attribute) && $this->supportForAttributes) { - throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); - } + $attribute = $this->schema[$collection->getId()][$attributeId] ?? []; + + if (empty($attribute)) { + if ($this->supportForAttributes) { + throw new \Exception('Invalid query: Attribute not found in schema: ' . $attributeId); + } + + // Schemaless: attribute not in schema, skip further attribute-based checks + return; + } if (\in_array('encrypt', $attribute['filters'] ?? [])) { throw new \Exception('Cannot query encrypted attribute: ' . $attributeId); } - if ($isNested && $attribute['type'] != Database::VAR_RELATIONSHIP) { + if ($isNested && $attribute['type'] != Database::VAR_RELATIONSHIP) { throw new \Exception('Only nested relationships allowed'); }This aligns schemaless behavior and avoids undefined index notices.
664-674:SELECT '*'doesn’t validate alias context againstQueryContextIn the
TYPE_SELECTbranch, you validate theASclause and then only check attribute existence when it isn’t'*':$asValidator = new AsValidator($query->getAttribute()); ... if ($query->getAttribute() !== '*') { $this->validateAttributeExist($query->getAttribute(), $query->getAlias()); }For
SELECT '*', this means the alias is never validated againstQueryContext. A query likeQuery::select('*', alias: 'unknown')will passisValid()as long as the alias string passesAliasValidator, but later fail when adapters try to resolve the alias.You can at least enforce that the alias maps to a known collection when
'*'is used:case Query::TYPE_SELECT: $asValidator = new AsValidator($query->getAttribute()); if (! $asValidator->isValid($query->getAs())) { throw new \Exception('Invalid query: '.\ucfirst($method).' '.$asValidator->getDescription()); } - if ($query->getAttribute() !== '*') { - $this->validateAttributeExist($query->getAttribute(), $query->getAlias()); - } + if ($query->getAttribute() === '*') { + $collection = $this->context->getCollectionByAlias($query->getAlias()); + if ($collection->isEmpty()) { + throw new \Exception('Unknown Alias context'); + } + + break; + } + + $this->validateAttributeExist($query->getAttribute(), $query->getAlias()); break;This keeps
SELECT '*'lightweight (no schema expansion) while still rejecting aliases that are not defined in the context.src/Database/Database.php (3)
5118-5123: Context‑aware casting/decode added for create paths; update/upsert still only decode.
- Wiring
QueryContextintocreateDocumentandcreateDocumentsand then calling$this->casting($context, …); $this->decode($context, …);keeps result typing consistent withfind()andgetDocument(). This is aligned with the new context‑driven API.updateDocument,updateDocuments, andupsertDocumentsWithIncreasenow only calldecode($context, …)(nocasting()), so their return types may still differ fromcreate*/getDocument/findon adapters where normalization is handled inDatabase::casting()rather than the adapter. This was already flagged earlier; your new context plumbing doesn’t change that gap.To re‑check this across the repo, you can run:
#!/bin/bash rg -n 'function (createDocument|createDocuments|getDocument|find|updateDocument|updateDocuments|upsertDocumentsWithIncrease)\(' src/Database/Database.php -n -C5Also applies to: 5171-5173, 5234-5238
5818-5822: Decode now context‑aware in update/upsert paths; casting gap remains.
- Adding
QueryContextinupdateDocument,updateDocuments, andupsertDocumentsWithIncrease, and switching the final decode calls to$this->decode($context, …)is consistent with the rest of the context‑driven API and should make join/alias handling uniform.- The previous concern about missing
$this->casting($context, …)before decode on these code paths still applies; without it, adapters that rely onDatabase::casting()for scalar normalization will continue to return raw DB types here while other methods return normalized values.Also applies to: 5879-5893, 5974-5981, 6050-6053, 6565-6567, 6796-6800
8713-8788: Remove unused$idAddedand keep$idauto‑select relationship‑only.
- The tail of
processRelationshipQueries()correctly:
- Collects
nestedSelectionsforrelationship.pathselectors,- Re‑maps top‑level select attributes according to relation type/side,
- And only auto‑adds
$idwhen the collection actually has relationship attributes (!empty($relationships)), which matches prior guidance.- PHPMD is right that
$idAddedis never used here; you only care about the updated$queries.You can simplify and satisfy static analysis by discarding the second return value:
- $queries = array_values($queries); - - /** - * In order to populateDocumentRelationships we need $id - */ - if (!empty($relationships)) { - [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); - } - - return [$queries, $nestedSelections]; + $queries = array_values($queries); + + /** + * In order to populateDocumentsRelationships we need $id + */ + if (!empty($relationships)) { + [$queries] = QueryContext::addSelect($queries, Query::select('$id', system: true)); + } + + return [$queries, $nestedSelections];Based on learnings, this also aligns with the earlier guidance to only add
$idfor relationship‑bearing collections and avoid unused locals.
🧹 Nitpick comments (6)
src/Database/Database.php (6)
4295-4414:getDocument’s new QueryContext + select‑only contract: check for BC and alias coverage.
- The move to
QueryContext+DocumentsValidatorand deriving$selects/$nestedSelectionsbefore hitting the adapter looks coherent and matches the new context‑driven design.- Enforcing
Only Select queries are permittedfor$queriesis a behavior change: any existing callers that passed non‑select filters togetDocumentwill now throwQueryException. Please confirm that public usage already restrictsgetDocumentto projections only, or document this as an intentional tightening.- Passing
$selectsintogetCacheKeys()and into$this->casting()/decode()should prevent projection mix‑ups on cache; this looks correct.$permissionsauto‑select plus conditional removal via$permissionsAddedis a nice touch and avoids leaking system fields when user didn’t ask for them.
7231-7256: Internal$id/$permissions/$sequenceselects for delete paths are consistent but strict.
- Using
Query::select('$id')in the internalfind/findOnecalls and explicitly validating the presence of['$permissions', '$sequence']in the first batch document forupdateDocuments()anddeleteDocuments()makes these bulk operations safer and more predictable with the new validators.- It does, however, make these methods sensitive to user‑supplied select lists: if callers aggressively limit projections, they’ll now see
QueryException("Missing required attribute … in select query"). That seems acceptable, but it’s worth calling out in API docs or migration notes.Also applies to: 7293-7303, 7340-7366, 7433-7456, 7531-7538, 7581-7588
7520-7534: QueryContext + joins wiring forfind/count/sumis well‑structured; watch a couple of edge behaviors.
- The pattern of:
- Building a
QueryContextwith main + joined collections,- Running
DocumentsValidatorV2 over the original queries,- Then calling
$this->convertQueries($context, $queries)and, for relationships,convertRelationshipFiltersToSubqueries(...),- And finally delegating to the adapter with explicit
selects/filters/joins/orderQueries
is coherent and matches the new join design.- Per‑collection auth in
find()/count()viacontext->addSkipAuth()andgetPermissionsByType($forPermission)/getRead()looks correct for enforcing permissions across joins.count()now rejects$max < 1, whilesum()still documents$max = 0as “unlimited”; if callers relied oncount(..., max: 0)this is a behavior change. Consider either harmonizing semantics withsum()or clearly documenting the new expectation.- The default
Query::orderAsc()injection infind()is unchanged, but the comment now says “In joins we should not add a default order”; if that’s the new policy, you may want to guard that append byif (empty($joins))to avoid surprising default ordering on joined queries.Also applies to: 7750-7771, 7795-7806, 7815-7824, 7841-7865, 7992-8006, 8009-8025, 8049-8066, 8088-8103, 8121-8133
8004-8013: UsegetPermissionsByTypeconsistently incount()like infind().In
count(), skip‑auth is based on$_collection->getRead(), whilefind()usesgetPermissionsByType($forPermission); for read counts this is usually equivalent but less future‑proof if permission derivation changes. Consider switching togetPermissionsByType(self::PERMISSION_READ)here for parity.
8234-8327: Context‑awaredecode()/casting()refactor looks solid; error type could be more specific.
- Building
$schemaonce perQueryContextacross all collections and resolving aliases (asandalias.attribute) per select to determine the right collection and attribute is a good approach for joins.- Schemaless adapters falling back to
setAttribute($key, $value)when no schema entry is found preserves old behavior.- Returning a new instance of the main collection’s document type via
createDocumentInstance($context->getMainCollection()->getId(), [])ensures casting/decoding respects custom document subclasses.- Throwing a bare
\Exception('Invalid query: Unknown Alias context')when alias lookup fails makes this path harder to catch than aQueryException; consider switching toQueryExceptionfor consistency with other query‑validation failures.Also applies to: 8332-8446
8978-9181:convertRelationshipFiltersToSubqueriesnull‑on‑failure semantics may mask bad paths.
- When a relationship path cannot be resolved or the chained lookups yield no matching IDs, the helper returns
null, whichfind()/count()/sum()interpret as “no matches” (empty result / 0) rather than signaling a bad query.- Given
DocumentsValidatorV2 should already reject structurally invalid paths, this may be acceptable, but if the validator misses a case (e.g., typo in a relationship key), this will quietly degrade into “0 results” instead of aQueryException.If possible, consider distinguishing “validator passed but relationship resolution failed structurally” from “no matching related documents” and turning the former into a
QueryExceptionfor easier debugging.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Database.php(55 hunks)src/Database/Query.php(26 hunks)src/Database/QueryContext.php(1 hunks)src/Database/Validator/Queries/V2.php(1 hunks)src/Database/Validator/Query/Filter.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/QueryContext.php
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Database.phpsrc/Database/Validator/Queries/V2.phpsrc/Database/Query.php
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.phpsrc/Database/Validator/Queries/V2.phpsrc/Database/Query.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.phpsrc/Database/Validator/Queries/V2.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Database.phpsrc/Database/Validator/Queries/V2.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: 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.
Applied to files:
src/Database/Validator/Queries/V2.php
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
Repo: utopia-php/database PR: 733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
src/Database/Validator/Queries/V2.php
📚 Learning: 2025-08-14T06:35:30.429Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 661
File: tests/e2e/Adapter/Scopes/SpatialTests.php:180-186
Timestamp: 2025-08-14T06:35:30.429Z
Learning: Query::distance method in Utopia\Database\Query expects an array of values parameter, where each value is a [geometry, distance] pair. So the correct format is Query::distance('attribute', [[[lat, lng], distance]]) where the outer array contains the values and each value is [geometry, distance].
Applied to files:
src/Database/Query.php
🪛 PHPMD (2.15.0)
src/Database/Database.php
8785-8785: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (5)
src/Database/Database.php (5)
34-36: Validator alias switch toDocumentsValidatorlooks consistent.The import of
Queries\V2asDocumentsValidatoraligns with its usage throughout the file; no issues spotted here.
4532-4560: Nested relationship select processing and post‑batch select filtering appear sound.
- Using
processRelationshipQueries()again for nested breadth‑first traversal, and thenapplySelectFiltersToDocuments()to reduce related docs to explicitly selected fields while preserving internal$*attributes, is consistent with the rest of the relationship pipeline.- The
attributesToKeepmap keyed byselectQuery->getAttribute()plus wildcard handling (*) gives predictable behavior and preserves internals via$id/other$attributes.Also applies to: 4988-5013
6141-6171: Ensuring$idis explicitly selected in relationship maintenance helpers is a good hardening.
- Adding
Query::select('$id')to the variousgetDocument/findOne/findcalls insideupdateDocumentRelationships()prevents the new validators from rejecting these internal lookups and makes the required projection explicit.- The changes respect
skipRelationships()and existing authorization behavior and look consistent across the different relation types.Also applies to: 6202-6204, 6283-6299, 6327-6339, 6407-6413
8542-8615:convertQueries/convertQueryalias‑aware conversion matches the new context model.
- Recursing into nested/join queries first and then using
QueryContext::getCollectionByAlias($query->getAlias())to locate the schema before settingonArray,attributeType, and date‑normalized values is the right direction for joins.- Short‑circuiting for
TYPE_SELECTavoids touching projection queries, which is desirable.- Using the adapter’s UTC casting support in the datetime branch keeps temporal behavior consistent across adapters.
8651-8684: Select‑sensitive cache keys are correct; purge path still uses the unparameterized key.
- Extending
getCacheKeys()to accept$selectsand fold them into$documentHashKeyviamd5(serialize($selects))will prevent cross‑pollution between different projections of the same document, which is good.purgeCachedDocumentInternal()intentionally callsgetCacheKeys($collectionId, $id)without selects, ensuring it blows away the entire cache entry for that document, regardless of projection. This is the desired behavior; no issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
4235-4238: Decode still operates on the original$documentinstead of the encoded resultHere you build a
QueryContextand then call:$context = new QueryContext(); $context->add($collection); $result = $database->decode($context, $document);Given the prior
$result = $database->encode($collection, $document);, this continues to decode the original logical$documentrather than the encoded$result, so the test doesn’t exercise the actual encode→decode round‑trip path. This is the same concern raised in an earlier review.A clearer and more realistic pipeline would be:
- $result = $database->encode($collection, $document); + $encoded = $database->encode($collection, $document); … - $context = new QueryContext(); - $context->add($collection); - - $result = $database->decode($context, $document); + $context = new QueryContext(); + $context->add($collection); + + $result = $database->decode($context, $encoded);(or alternatively introduce a
$decodedvariable and update the following assertions to use it).If decoding the raw
$documentis intentional (to unit‑testdecodeindependently ofencode), an inline comment to that effect would avoid confusion.src/Database/Validator/Queries/V2.php (3)
179-226: Guard schemaless attribute lookups to avoid undefined index and over-strict checksWhen
$supportForAttributesisfalseand the attribute is not present in$this->schema,$attributeis an empty array. The subsequent checks still dereference$attribute['type'], which will trigger undefined index notices and also enforce relationship / order-by rules in schemaless mode where no type information is available.You already short‑circuit in
validateValues()for this case; do the same here so schemaless behaves consistently and safely:- $attribute = $this->schema[$collection->getId()][$attributeId] ?? []; - if (empty($attribute) && $this->supportForAttributes) { - throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); - } - - if (\in_array('encrypt', $attribute['filters'] ?? [])) { + $attribute = $this->schema[$collection->getId()][$attributeId] ?? []; + + if (empty($attribute)) { + if ($this->supportForAttributes) { + throw new \Exception('Invalid query: Attribute not found in schema: '.$attributeId); + } + + // Schemaless: attribute not in schema, skip further attribute-based checks + return; + } + + if (\in_array('encrypt', $attribute['filters'] ?? [])) { throw new \Exception('Cannot query encrypted attribute: ' . $attributeId); } - if ($isNested && $attribute['type'] != Database::VAR_RELATIONSHIP) { + if ($isNested && $attribute['type'] != Database::VAR_RELATIONSHIP) { throw new \Exception('Only nested relationships allowed'); } if ($isNested && \in_array($method, [Query::TYPE_ORDER_ASC, Query::TYPE_ORDER_DESC])) { throw new \Exception('Cannot order by nested attribute: ' . $attributeId); }Based on learnings, this keeps schemaless mode permissive while preserving strict validation when attributes are supported.
530-533: Restrict join-scope alias validation to relation queries to avoid rejecting valid filtersThe generic
scope === 'joins'alias check currently runs for all nested queries under a join:if ($scope === 'joins') { if (!in_array($query->getAlias(), $this->joinsAliasOrder) || !in_array($query->getRightAlias(), $this->joinsAliasOrder)) { throw new \Exception('Invalid query: '.\ucfirst($query->getMethod()).' alias reference in join has not been defined.'); } }For normal filter/order queries inside a join,
getRightAlias()is typically empty, soin_array('', $this->joinsAliasOrder)fails and valid queries get rejected. Only relation queries really need this alias-order validation.Narrow the check to relation queries and use strict comparisons:
- if ($scope === 'joins') { - if (!in_array($query->getAlias(), $this->joinsAliasOrder) || !in_array($query->getRightAlias(), $this->joinsAliasOrder)) { - throw new \Exception('Invalid query: '.\ucfirst($query->getMethod()).' alias reference in join has not been defined.'); - } - } + if ($scope === 'joins' && $query->getMethod() === Query::TYPE_RELATION_EQUAL) { + if ( + ! \in_array($query->getAlias(), $this->joinsAliasOrder, true) + || ! \in_array($query->getRightAlias(), $this->joinsAliasOrder, true) + ) { + throw new \Exception( + 'Invalid query: ' . \ucfirst($query->getMethod()) . ' alias reference in join has not been defined.' + ); + } + }This preserves alias validation for relations while allowing non‑relation filters and orders within joins.
664-674: EnsureSELECT *also validates alias existence against the contextIn the
TYPE_SELECTbranch, attribute-specific selects go throughvalidateAttributeExist(), which verifies that the alias maps to a collection in theQueryContext. ForSELECT '*', you now skip that check entirely, so a query with an unknown alias can pass validation here and fail later in the pipeline.You can keep the simplified
'*'handling while still validating the alias context:- case Query::TYPE_SELECT: - $asValidator = new AsValidator($query->getAttribute()); - if (! $asValidator->isValid($query->getAs())) { - throw new \Exception('Invalid query: '.\ucfirst($method).' '.$asValidator->getDescription()); - } - - if ($query->getAttribute() !== '*') { - $this->validateAttributeExist($query->getAttribute(), $query->getAlias()); - } - - break; + case Query::TYPE_SELECT: + $asValidator = new AsValidator($query->getAttribute()); + if (! $asValidator->isValid($query->getAs())) { + throw new \Exception('Invalid query: '.\ucfirst($method).' '.$asValidator->getDescription()); + } + + if ($query->getAttribute() === '*') { + $collection = $this->context->getCollectionByAlias($query->getAlias()); + if ($collection->isEmpty()) { + throw new \Exception('Invalid query: Unknown Alias context'); + } + + break; + } + + $this->validateAttributeExist($query->getAttribute(), $query->getAlias()); + + break;This restores the “unknown alias is invalid” guarantee for
SELECT *without reintroducing the previous schema-iteration logic.
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
3153-3162: Consider asserting selected attribute presence intestOrNestedAdding
Query::select('director')ahead of the logical queries is a good way to ensure projections interact correctly with nestedorconditions and that unselected fields (name) are omitted.You might strengthen this test slightly by also asserting that
directoris present (e.g.assertArrayHasKey('director', $documents[0])), so the test verifies both exclusion of unselected attributes and inclusion of the explicitly selected one.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Validator/Queries/V2.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(18 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Validator/Queries/V2.php
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Validator/Queries/V2.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: 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.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Validator/Queries/V2.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Validator/Queries/V2.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Validator/Queries/V2.php
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
Repo: utopia-php/database PR: 733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
src/Database/Validator/Queries/V2.php
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
src/Database/QueryContext.php (2)
QueryContext(8-136)add(68-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Unit Test
🔇 Additional comments (7)
tests/e2e/Adapter/Scopes/DocumentTests.php (7)
22-22: Import ofQueryContextmatches new context-aware decode usageBringing
QueryContextinto scope is consistent with the updateddecode(QueryContext $context, ...)API used later intestEncodeDecode; no issues here.
1397-1435: Per‑fieldselectand internal field assertions forgetDocumentlook correctThe switch to
Query::select('string')/Query::select('integer_signed')and the explicit assertions that$id,$sequence,$createdAt,$updatedAt, and$permissionsare absent unless explicitly selected (while$collectionis always present) accurately exercise the new select semantics forgetDocument. The newtestGetDocumentOnlySelectQueriesalso cleanly enforces that onlyselectqueries are accepted in the third parameter, with clear error messaging.
1437-1470: GuardinggetDocumentagainst non‑select queries is well covered
testGetDocumentOnlySelectQueriescorrectly verifies that passingQuery::equalorQuery::limitintogetDocumenttriggers aDatabaseExceptionwith the expected message, ensuring callers can’t accidentally mix filtering/paging with the “projection only” API. Test structure and expectations are solid.
1955-1959:notSearchextension in full‑text test aligns with new operatorAdding the
Query::notSearch('name', 'captain')branch here complements the positivesearchcheck and matches the dataset (2 captain movies + 4 non‑captain); this is a good, minimal sanity check that the negative full‑text operator is wired correctly.
3748-3885: ExpandedtestFindSelectthoroughly validates internal field projection rulesThe refactored
testFindSelectnow exercises many combinations of:
- User attributes (
name,year)- System attributes (
$id,$sequence,$collection,$createdAt,$updatedAt,$permissions)via repeated
Query::select('field')calls, with assertions that:
- Only explicitly selected internal attributes appear.
$collectionremains present even when not selected.- Non‑selected internal attributes are absent.
This directly tests and documents the new behavior that internal fields are not auto‑selected apart from
$collection, which aligns with the QueryContext/relationship‑processing learning about not adding$idunless needed. Based on learnings, this coverage looks appropriate and consistent.
4981-4988: Usingselectprojections in bulk delete tests is consistent with new semanticsIn the bulk delete tests you now prepend
Query::select(...)(for$createdAt,$sequence,$id,$collection,$permissions,$updatedAt) to thedeleteDocumentsquery arrays. That:
- Exercises the new projection behavior with cursor and ordering queries (
cursorAfter,greaterThan,orderAsc,limit).- Ensures the adapter has all necessary system fields for paging/deletion while still validating that projection queries are accepted in non‑read operations.
The patterns and field sets you’re selecting match what deletion and cursor logic need; the tests remain clear and should be stable across adapters.
Also applies to: 5190-5196, 5213-5220
5465-5467:select('*')intestEmptyTenantshared‑tables path is reasonableSwitching the shared‑tables branch to query via
Query::select('*')(rather than a filtering predicate) to work around the Mongo integer UID bug is compatible with the new per‑field select API and keeps the core assertions about$tenantunchanged. As long as'*'is treated as “all fields” by the query layer, this is a straightforward and appropriate adjustment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/unit/Validator/Query/FilterTest.php (4)
8-81: V2 validator wiring and QueryContext setup looks consistentThe metadata collection, attributes (including
search), fulltext index, andQueryContextwiring all line up with howDocumentsValidator(V2) expects schema/context, andmaxValuesCountis passed in explicitly. This gives the tests a realistic environment without overcomplicating setup. Only nit: if multiple test classes need a similar context, consider extracting a small factory/helper to avoid duplication, but fine to defer.
110-124: Failure-path coverage for invalid attributes and ranges is appropriate
testFailure()targets invalid attribute names, invalid limits/offsets, cursor misuse, and type/shape mismatches (e.g.containson scalar,equalon array attribute). This gives good negative coverage of the new V2 behavior. If you later tighten specific error messages for limits/offsets or cursors, you might add explicitassertEqualsfor those descriptions as well, but it’s not strictly necessary now.
168-184: Clarify comments intestNotSearch()The assertions themselves look correct (valid on
searchwith FT index, invalid on non-indexed and array attributes, and enforcing exactly one value). However:
- Line 172’s comment “Test valid notSearch queries” is followed by failing assertions.
- The later comments are accurate in behavior but could be clearer about what is being validated (index requirement vs shape vs value-count).
Consider tightening the comments, for example:
- // Test valid notSearch queries + // Missing fulltext index should be rejected - // Test that arrays cannot use notSearch + // Arrays cannot be used with notSearch - // Test multiple values not allowed + // NotSearch must receive exactly one valueThis keeps future readers from misinterpreting the scenario.
186-202: Search tests mirror NotSearch well; fix copy-paste commentsThe
testSearch()logic correctly mirrorstestNotSearch()(valid onsearchwith FT index, rejected for non-indexed and array attributes, and enforcing exactly one value). The only nits are copy-paste comments that still mentionnotSearch:- // Test valid notSearch queries + // Missing fulltext index should be rejected - // Test that arrays cannot use notSearch + // Arrays cannot be used with search - // Test multiple values not allowed + // Search must receive exactly one valueUpdating these will make the test intent clearer without changing behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/Validator/Query/FilterTest.php(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/unit/Validator/Query/FilterTest.php
🧬 Code graph analysis (1)
tests/unit/Validator/Query/FilterTest.php (2)
src/Database/QueryContext.php (2)
QueryContext(8-136)add(68-76)src/Database/Validator/Queries/V2.php (2)
V2(23-714)getDescription(135-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (4)
tests/unit/Validator/Query/FilterTest.php (4)
84-107: Positive-case coverage for filters and non-filter queries is solidThe
testSuccess()method exercises a good spread of filter methods and also asserts that non-filter queries (limit,offset,orderAsc,orderDesc) are accepted. This aligns well with the intent that the validator now allows these through. All calls use the new array-of-queries contract correctly.
126-154: Type-mismatch and max-values checks match V2maxValuesCountsemantics
testTypeMismatch(),testEmptyValues(), andtestMaxValuesCount()now assert theInvalid query: ...-prefixed messages and use the local$maxValuesCountproperty, which matches the documented V2 behavior and the constructor signature. This should keep the tests stable if the global defaults change.
156-166: NotContains behavior is well covered
testNotContains()verifies both valid and invalid (empty-values) cases and aligns the expected message with the newInvalid query:prefix. This addresses prior feedback and clearly documents the minimum-values constraint fornotContains.
204-251: NotStartsWith / NotEndsWith / NotBetween coverage looks completeThe three tests cover:
- Happy-path usage on scalar attributes.
- Rejection when targeting array attributes.
- Rejection when the number of values is wrong for
notBetween, with precise error messages.These are well aligned with the V2 semantics and use the array-of-queries contract consistently.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Database.php (2)
8234-8327: Use query-specific exceptions instead of generic\Exceptionindecode/castingwhen alias context is unknownBoth
decode()andcasting()now rely onQueryContextand select metadata to resolve collection aliases:$collection = $context->getCollectionByAlias($alias); if ($collection->isEmpty()) { throw new \Exception('Invalid query: Unknown Alias context'); }Throwing bare
\Exceptionhere makes it harder for callers to distinguish:
- genuine query-construction errors (bad alias, malformed select),
- from other runtime failures.
Since this is clearly a query-level problem, using
QueryException(or another domain-specific exception already used elsewhere) would be more consistent and easier to handle upstream.Targeted change
- if ($collection->isEmpty()) { - throw new \Exception('Invalid query: Unknown Alias context'); - } + if ($collection->isEmpty()) { + throw new QueryException('Invalid query: Unknown Alias context'); + }Apply in both
decode()andcasting()where this pattern appears.Also applies to: 8338-8446
8978-9181:convertRelationshipFiltersToSubqueriesswallows all exceptions and turns them into “no matches”
convertRelationshipFiltersToSubqueries()wraps the entire relationship-filter resolution in a broadtry { … } catch (\Exception $e) { return null; }. Callers then interpretnullas “no documents can match”:
find()treatsnullas “return empty$results”.count()returns0.sum()returns0.This means that any error encountered while resolving relationship filters — including:
- authorization failures,
- timeouts,
- malformed queries / invalid attributes,
- adapter/SQL errors —
will silently degrade into “no results”, which is a significant behavior change and makes operational debugging very hard.
I’d strongly recommend narrowing this:
- Let most exceptions bubble (AuthorizationException, TimeoutException, DatabaseException, QueryException, etc.).
- If you specifically want to treat “relationship filter finds no IDs” as “no matches”, handle that by checking for an empty
$matchingIds/$parentIds, not by catching all \Exception.For example (conceptually):
try { // current logic } catch (QueryException $e) { // rethrow as-is so clients see a query error throw $e; } catch (DatabaseException|AuthorizationException|TimeoutException $e) { // propagate real runtime failures throw $e; }and only return
nullwhen the algorithm logically determines there are no matching IDs.
♻️ Duplicate comments (4)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
4253-4281: Decode still operates on the original$documentinstead of the encoded$resultThis test currently:
$result = $database->encode($collection, $document); // … $context = new QueryContext(); $context->add($collection); $result = $database->decode($context, $document);So the decode step is applied to the original
$document, not to the encoded$result. That means you’re testingencodeanddecodelargely independently rather than validating the full encode→decode pipeline on the same payload (including any system‑injected selects influenced byQueryContext).To exercise the round‑trip more realistically, consider decoding the encoded document instead, or add a brief comment clarifying that the intent is to test
decodein isolation.Suggested change to round‑trip the encoded document
- $result = $database->encode($collection, $document); + $result = $database->encode($collection, $document); @@ - $context = new QueryContext(); - $context->add($collection); - - $result = $database->decode($context, $document); + $context = new QueryContext(); + $context->add($collection); + + $result = $database->decode($context, $result);src/Database/Adapter/Mongo.php (1)
1859-2065: Context-drivenfind()implementation is solid; consider guards and unused params
The new
find(QueryContext $context, …, array $selects, array $filters, array $joins, array $vectors, array $orderQueries)correctly:
- pulls the main collection from
QueryContext,- clones and runs
$filtersthroughbuildFilters()and tenant filters,- applies auth via
QueryContext::skipAuth()and_permissionsregex,- derives projections from
getAttributeProjection($selects)and hides$sequencewhen_idis not projected,- builds cursor-based pagination filters from
$orderQueriesand applies sort and$orlogic,- iterates the Mongo cursor in batches with transaction options and cleans up cursors.
Two follow-ups to consider:
$joinsand$vectorsare intentionally unused (Mongo doesn’t support relationships/vectors), but they trigger static analysis warnings. You can optionally silence those by explicitly unsetting them at the top of the method:This makes intent clear without changing behavior.unset($joins, $vectors);- The prior review note about
orderRandomstill applies:getOrderDirection()will throw if aTYPE_ORDER_RANDOMquery ever reaches this method, whilegetSupportForOrderRandom()returnsfalsefor Mongo. If higher layers don’t already filter such queries, adding a small guard here (e.g., reject/skip random-order queries up front with a clearer exception) would make this more robust.src/Database/Database.php (2)
5818-5822: Update/upsert paths still skipcasting(QueryContext, …), leading to inconsistent type normalizationCompared to
createDocument,createDocuments,getDocument, andfind, these paths only calldecode()and notcasting():
- Line 5818–5822:
updateDocument()builds aQueryContextand then calls only$this->decode($context, $document);.- Line 6048–6053:
updateDocuments()calls$doc = $this->adapter->castingAfter(...);and then only$this->decode($context, $doc);.- Line 6796–6800:
upsertDocumentsWithIncrease()decodes outputs when!$hasOperatorsbut never callscasting().Given that:
casting()is where scalar types are normalized according to the schema (bool/int/float/id), and- All other CRUD read paths already run
casting($context, …)beforedecode(),these update/upsert methods can return different PHP types for the same attributes (typically raw DB types) compared to create/get/find, depending on the adapter. This is exactly the inconsistency flagged in the earlier review and still present in the current code.
I’d recommend aligning these paths by inserting
casting()beforedecode()in all three places, e.g.:Suggested diffs to normalize types on update/upsert
@@ 5818-5822 - $context = new QueryContext(); - $context->add($collection); - - $document = $this->decode($context, $document); + $context = new QueryContext(); + $context->add($collection); + + $document = $this->casting($context, $document); + $document = $this->decode($context, $document);@@ 6048-6053 - foreach ($batch as $index => $doc) { - $doc = $this->adapter->castingAfter($collection, $doc); - $doc->removeAttribute('$skipPermissionsUpdate'); - $this->purgeCachedDocument($collection->getId(), $doc->getId()); - $doc = $this->decode($context, $doc); + foreach ($batch as $index => $doc) { + $doc = $this->adapter->castingAfter($collection, $doc); + $doc->removeAttribute('$skipPermissionsUpdate'); + $this->purgeCachedDocument($collection->getId(), $doc->getId()); + $doc = $this->casting($context, $doc); + $doc = $this->decode($context, $doc);@@ 6796-6800 - foreach ($batch as $index => $doc) { - $doc = $this->adapter->castingAfter($collection, $doc); - if (!$hasOperators) { - $doc = $this->decode($context, $doc); - } + foreach ($batch as $index => $doc) { + $doc = $this->adapter->castingAfter($collection, $doc); + if (!$hasOperators) { + $doc = $this->casting($context, $doc); + $doc = $this->decode($context, $doc); + }Also applies to: 6048-6053, 6796-6800
8713-8788: Remove unused$idAddedinprocessRelationshipQueriesand discard second return value
processRelationshipQueries()’ tail currently is:$queries = array_values($queries); /** * In order to populateDocumentRelationships we need $id */ if (!empty($relationships)) { [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); }
$idAddedis never used, which PHPMD correctly flags, and the logic you want is just to ensure$idis present when there are relationship attributes.You can both satisfy static analysis and match the earlier guidance (“only when there are relationships, don’t keep an unused var”) by discarding the second return element:
Suggested fix
- $queries = array_values($queries); - - /** - * In order to populateDocumentRelationships we need $id - */ - if (!empty($relationships)) { - [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); - } - - return [$queries, $nestedSelections]; + $queries = array_values($queries); + + /** + * In order to populateDocumentRelationships we need $id + */ + if (!empty($relationships)) { + [$queries] = QueryContext::addSelect($queries, Query::select('$id', system: true)); + } + + return [$queries, $nestedSelections];This keeps the behavior (auto-select
$idonly when relationships exist) and removes the unused local, in line with the stored learning.
🧹 Nitpick comments (5)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
1439-1476: Projection behaviour intestGetDocumentSelectlooks correct; consider tightening the float assertionThe new assertions around
$id,$sequence,$createdAt,$updatedAt, and$permissionsalign well with the new “only explicitly‑selected system fields are exposed” behaviour and look sound.One small improvement: Line 1449 currently asserts that the attribute
'float'is absent, but the attributes defined intestCreateDocumentarefloat_signedandfloat_unsigned. As written, the'float'check is effectively a no‑op even if either float field slipped into the projection. Updating this to assert on the concrete attribute names would make the test stricter and more representative of the schema.Example tightening for the float assertion
- $this->assertArrayNotHasKey('float', $document->getAttributes()); + $this->assertArrayNotHasKey('float_signed', $document->getAttributes()); + $this->assertArrayNotHasKey('float_unsigned', $document->getAttributes());
1979-2002:notSearchassertions are duplicated betweentestFindFulltextandtestFindNotSearchThe new
Query::notSearch('name', 'captain')block intestFindFulltextis functionally very similar to the primary assertions intestFindNotSearchbelow. That’s not wrong, but it does couple two tests to the same behaviour and index setup.If you want to keep the suite lean, consider moving this assertion (and any future
notSearchscenarios) intotestFindNotSearchso all negative full‑text behaviour is exercised in one place.
5505-5510: Clarify the “Mongo bug with Integer UID” comment now that the query is onlyQuery::select('*')In the shared‑tables branch of
testEmptyTenant, the query was changed to:$documents = $database->find( 'documents', [Query::select('*')] // Mongo bug with Integer UID );The inline comment still references a “Mongo bug with Integer UID”, but the query no longer has any obvious filter related to that bug (unlike the non‑shared‑tables branch below, which uses
Query::notEqual('$id', '56000')with the same comment).For future readers, it might be worth either updating the comment to explain why an explicit
select('*')is required here, or adjusting the query/comment pair so it’s clear how this line mitigates the referenced Mongo issue.src/Database/Adapter/Mongo.php (2)
1125-1156: Projection ingetDocument()assumes$queriesare selects — please verify
getDocument()now derives$projectionsviagetAttributeProjection($queries)and uses$removeSequenceto hide$sequencewhen_idis not projected. This is sound if$querieshere are strictly TYPE_SELECT queries; if callers still pass filter queries in$queries, you’ll end up with a projection limited to attributes mentioned in filters, which could be a behavior change.If that’s not intended, consider passing only select queries into
getAttributeProjection()(mirroring howfind()uses$selects).
2131-2226:count()aligns with newQueryContextflow; joins unused but harmless
count(QueryContext $context, ?int $max, array $filters = [], array $joins = [])now:
- Uses
getMainCollection()and clones$filtersbefore passing them throughbuildFilters().- Applies tenant filters and READ-permission auth via
QueryContext::skipAuth()and a_permissionsregex.- Respects
$this->timeoutviamaxTimeMSin$options.- Uses an aggregation pipeline with
$match, optional$limit, and either$group+$sumor$count, which is appropriate for accurate counting (including sharded clusters).The
$joinsparameter is currently unused (Mongo doesn’t support joins), which is expected but flagged by static analysis. If you want to quiet those warnings without affecting behavior, you can explicitlyunset($joins);at the top of the method.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Database/Adapter.php(3 hunks)src/Database/Adapter/MariaDB.php(2 hunks)src/Database/Adapter/Mongo.php(14 hunks)src/Database/Adapter/Pool.php(4 hunks)src/Database/Adapter/Postgres.php(2 hunks)src/Database/Database.php(55 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Adapter/Postgres.php
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Adapter.phpsrc/Database/Database.phptests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Adapter/Mongo.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: 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.
Applied to files:
src/Database/Adapter.phptests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Adapter/Mongo.php
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Adapter/MariaDB.phpsrc/Database/Database.phptests/e2e/Adapter/Scopes/DocumentTests.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (3)
src/Database/Adapter/Pool.php (2)
src/Database/QueryContext.php (1)
QueryContext(8-136)src/Database/Adapter.php (3)
find(829-841)count(865-870)getAttributeProjection(1256-1256)
src/Database/Adapter/MariaDB.php (1)
src/Database/Query.php (5)
setAttributeRight(331-336)getAttributeRight(287-290)getAttribute(255-258)Query(8-1559)getRightAlias(282-285)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
src/Database/QueryContext.php (2)
QueryContext(8-136)add(68-76)src/Database/Query.php (2)
Query(8-1559)select(788-791)src/Database/Adapter/SQL.php (1)
getDocument(364-429)
🪛 PHPMD (2.15.0)
src/Database/Database.php
8785-8785: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
src/Database/Adapter/Mongo.php
1884-1884: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
1885-1885: Avoid unused parameters such as '$vectors'. (undefined)
(UnusedFormalParameter)
2140-2140: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SQLite)
🔇 Additional comments (9)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
1479-1512: Good guard thatgetDocument’s third argument is projection‑only
testGetDocumentOnlySelectQueriesclearly enforces that passing non‑select queries (e.g.equal,limit) into the$queriesargument ofgetDocumentfails with the expectedDatabaseExceptionand message. This is a nice, targeted regression test for the new projection‑only contract.
3785-3927: ExpandedtestFindSelectcoverage for system fields and projections looks solidThe additional
find('movies', [... Query::select(...) ...])cases exercising$id,$sequence,$collection,$createdAt,$updatedAt, and$permissionsbeing included or excluded only when explicitly selected give very good coverage of the new projection rules, across both instance ($database) and static (static::getDatabase()) access paths. The expectations on which system keys are present in each scenario are consistent and should catch regressions in the projection logic.src/Database/Adapter/MariaDB.php (1)
1548-1603: Relation/equality condition handling looks consistent and safeNormalizing both left and right attributes via
getInternalKeyForAttribute(), then filtering and quoting aliases/columns (including the newTYPE_RELATION_EQUALbranch) keeps join-style predicates consistent with other conditions and prevents unsafe identifiers. I don’t see correctness or security issues here.src/Database/Adapter/Pool.php (1)
9-9: QueryContext-based signatures and delegation are correctly wired
Pool::find,Pool::count, andgetAttributeProjectionnow match the updated Adapter signatures (includingQueryContextand the new select/filter/join arrays) while still delegating transparently viadelegate(). No issues spotted with type hints, defaults, or argument forwarding.Also applies to: 271-283, 292-299, 511-514
src/Database/Adapter.php (1)
815-841: Context-firstfind/countAPI and projection contract look coherentThe abstract
find/countmethods now correctly pivot toQueryContextplus explicit$selects/$filters/$joins/$orderQueries, andgetAttributeProjection(array $selects)provides a clear, narrower contract for per-field projection. The signatures and docblocks are consistent and line up with the adapter implementations in this PR.Also applies to: 855-870, 1253-1256
src/Database/Adapter/Mongo.php (3)
711-723:renameAttributecorrectly maps public names to internal Mongo keysRenaming now goes through
getInternalKeyForAttribute($old|$new), so callers can pass public names like$id/$sequenceand still have the right_uid/_idfields renamed internally. The additionalfilter()calls keep collection and field identifiers safe.
1790-1799: UsingbuildFilters()for bulk delete by$sequencekeeps ID handling consistentSwitching
deleteDocuments()to build filters viabuildFilters([Query::equal('$sequence', $sequences)])means the$sequence→_idmapping and operator semantics are centralized inbuildFilter(). This reduces duplication and keeps Mongo’s internal ID handling aligned with other query paths.
2561-2595:getAttributeProjection()behavior matches the new projection contractThe new
getAttributeProjection(array $selects)correctly:
- Returns
[]when there are no selects or when a*select is present, signalling “no projection override”.- Skips
$collection, which isn’t a stored field.- Maps public attributes (
$id,$sequence,$tenant,$createdAt,$updatedAt,$permissions) to their internal Mongo field names and marks them as included.- Produces a simple
['fieldName' => 1, …]projection array that’s compatible with bothgetDocument()andfind()’s$removeSequencelogic.This is consistent with the rest of the adapter and the shared Adapter contract.
src/Database/Database.php (1)
4295-4331:getDocumentnow enforces that all queries must be SELECT queries
getDocument()now rejects non-SELECT queries (filters, limits, ordering) by comparingQuery::getSelectQueries($queries)against the input and throwingQueryException('Only Select queries are permitted')if any non-SELECT query is present. This behavioral change is intentional and has dedicated test coverage intestGetDocumentOnlySelectQueries(verifying thatQuery::equal()andQuery::limit()are rejected).This is a breaking change for any callers passing non-SELECT queries, though
getDocumentsignature has never documented support for such queries. The implementation is correct: queries are filtered to SELECT-only before validation occurs.
| /** | ||
| * Check and tests for required attributes | ||
| */ | ||
| foreach (['$permissions', '$sequence'] as $required) { | ||
| if (!$batch[0]->offsetExists($required)) { | ||
| throw new QueryException("Missing required attribute {$required} in select query"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bulk update/delete require $permissions & $sequence in selects but don’t auto-add them
In both bulk paths:
updateDocuments()(Lines 5974–5981)deleteDocuments()(Lines 7581–7588)
you enforce:
foreach (['$permissions', '$sequence'] as $required) {
if (!$batch[0]->offsetExists($required)) {
throw new QueryException("Missing required attribute {$required} in select query");
}
}However, find() does not automatically add system selects for $permissions and $sequence the way getDocument() now does for $permissions. That means:
- Callers who add any
Query::select(...)to the bulk update/delete queries must remember to also select both$permissionsand$sequence, or they’ll get a runtimeQueryException. - This is a bit non-obvious and asymmetric with
getDocument()’s behavior.
Consider either:
- Automatically injecting these system selects (similar to how you auto-inject
$idfor relationships and$permissionsingetDocument()), or - Explicitly documenting that any custom SELECTs used with
updateDocuments/deleteDocumentsmust include$permissionsand$sequence.
Also applies to: 7581-7588
| public function count(string $collection, array $queries = [], ?int $max = null): int | ||
| { | ||
| if (!is_null($max) && $max < 1) { | ||
| throw new DatabaseException('Invalid max value, must be a valid integer and greater than 0'); | ||
| } | ||
|
|
||
| $collection = $this->silent(fn () => $this->getCollection($collection)); | ||
| $attributes = $collection->getAttribute('attributes', []); | ||
| $indexes = $collection->getAttribute('indexes', []); | ||
|
|
||
| if ($collection->isEmpty()) { | ||
| throw new NotFoundException('Collection not found'); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
if [ -f "src/Database/Database.php" ]; then
echo "File found"
wc -l src/Database/Database.php
else
echo "File not found at src/Database/Database.php"
# Try to find it
find . -name "Database.php" -type f | head -20
fiRepository: utopia-php/database
Length of output: 104
🏁 Script executed:
# Read the count() method around lines 7992-8003
echo "=== COUNT METHOD (lines 7992-8003) ==="
sed -n '7980,8015p' src/Database/Database.php | cat -n
echo -e "\n=== SUM METHOD (lines 8086-8091) ==="
sed -n '8075,8115p' src/Database/Database.php | cat -nRepository: utopia-php/database
Length of output: 3394
🏁 Script executed:
# Search for other methods that have a $max parameter to check for consistency
echo "=== Methods with \$max parameter ==="
rg 'public function.*\$max' src/Database/Database.php -A 5 | head -100
# Also check what the adapter does with $max in sum
echo -e "\n=== Search for how adapter handles max in sum context ==="
rg 'adapter.*sum' src/Database/Database.php -A 3 -B 3 | head -50Repository: utopia-php/database
Length of output: 1080
🏁 Script executed:
# Let's look for all methods that might use $max to see if there's a pattern
echo "=== All public methods with \$max parameter (more thorough search) ==="
rg 'public function.*\$max\s*=' src/Database/Database.php | grep -v setMaxQueryValues
# Check the full sum() method to see if $max is used anywhere else
echo -e "\n=== Full sum() method implementation ==="
sed -n '8075,8150p' src/Database/Database.php
# Check if there are other similar aggregate methods like avg, min, max
echo -e "\n=== Search for other aggregate methods ==="
rg 'public function (avg|min|max)\(' src/Database/Database.php -A 1Repository: utopia-php/database
Length of output: 3225
🏁 Script executed:
# Let's check the adapter implementations to understand the $max semantics better
echo "=== Search for adapter sum method ==="
find . -name "*.php" -type f -exec grep -l "public function sum" {} \; | grep -i adapter | head -5
# Also check if there's documentation about $max semantics
echo -e "\n=== Search for all docblocks mentioning 'max' ==="
rg '\$max' src/Database/Database.php -B 2 | head -80Repository: utopia-php/database
Length of output: 2844
🏁 Script executed:
# Let's check the Adapter base class to see the sum method signature
echo "=== Adapter base class sum() method ==="
sed -n '/public function sum/,/^[[:space:]]*}/p' src/Database/Adapter.php | head -50
# Check SQL adapter sum implementation
echo -e "\n=== SQL Adapter sum() method ==="
sed -n '/public function sum/,/^[[:space:]]*}/p' src/Database/Adapter/SQL.php | head -50Repository: utopia-php/database
Length of output: 1841
🏁 Script executed:
# Let's check if there are other aggregate methods and their $max handling
echo "=== Check for avg, min, max aggregate methods ==="
rg 'public function (avg|minimum|maximum)\(' src/Database/Database.php -A 10 | head -50
# Let's also verify the count() method's full implementation to see how it uses $max
echo -e "\n=== Full count() method implementation ==="
sed -n '7992,8060p' src/Database/Database.phpRepository: utopia-php/database
Length of output: 2917
🏁 Script executed:
# Let's check the SQL adapter's count method too to see if it handles $max differently
echo "=== SQL Adapter count() method ==="
sed -n '/public function count/,/^[[:space:]]*}/p' src/Database/Adapter/SQL.php | head -80
# And check the full sum implementation in Database.php
echo -e "\n=== Full sum() method in Database.php (complete) ==="
sed -n '8075,8150p' src/Database/Database.php | grep -A 20 "return \$sum"Repository: utopia-php/database
Length of output: 1367
Align $max parameter semantics between count() and sum()
count() validates and rejects $max < 1, but sum() has no validation and forwards $max directly to the adapter. The docblock for sum() claims "$max=0 for unlimited count," yet the SQL adapter would apply LIMIT 0 (limiting to 0 results), not treat 0 as a sentinel for unlimited. This inconsistency could break existing callers that rely on the stated semantics.
Recommend:
- Clarify whether
$max=0should mean "unlimited" or "invalid" - Apply consistent validation across both methods
- Update docblocks to explicitly document accepted values and their meanings
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 7992 to 8003, the count() method
rejects $max < 1 while sum() accepts $max (docblock says 0 means unlimited)
which is inconsistent and will cause adapters (e.g. SQL) to apply LIMIT 0;
decide which semantics you want (treat 0 as "unlimited" or treat 0 as invalid),
then make both methods consistent: either validate and reject non-positive $max
in sum() the same way count() does, or accept 0 as unlimited in both by
converting $max === 0 to null/omitting LIMIT before calling the adapter. Update
both methods' docblocks to explicitly state the accepted values and behavior
(e.g. "$max must be > 0; pass null for unlimited" or "$max = 0 means no limit
and will be treated as unlimited") and ensure adapters receive the adjusted
value (no LIMIT 0).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
7497-7534: BothdeleteDocuments()andupdateDocuments()require explicit$permissionsand$sequencein custom SELECT queries.When users provide custom select queries via the
$queriesparameter, they must include both$permissionsand$sequenceattributes. These system attributes are not auto-injected and are required by the batch delete/update logic (lines 7584-7588 for delete, 5977-5981 for update). Currently there is no auto-injection mechanism and no prominent documentation of this requirement. Consider auto-injecting these system selects when custom projections are present, or document the requirement prominently in the method docblocks.
♻️ Duplicate comments (6)
src/Database/Database.php (6)
8338-8446: casting(QueryContext,…): schema‑driven scalar casting is consistent with decode()The new
casting()mirrorsdecode()’s schema resolution but only performs scalar type normalization (bool/int/float, plus array JSON decode when needed) into a fresh main‑collection Document, and early‑outs whenadapter->getSupportForCasting()is false.This design is coherent and keeps casting concerns separate from filter decoding. Just ensure callers always use
casting()beforedecode()when type normalization is required (see earlier comments on update*/upsert* paths).
5818-5822: Update/upsert return paths still skip casting(context, …) (type normalization inconsistency)
createDocument(),createDocuments(),getDocument(), andfind()now all pass results throughcasting($context, …)beforedecode($context, …), but:
updateDocument()only callsdecode($context, $document)(nocasting()).updateDocuments()only callsdecode($context, $doc)for each batch element.upsertDocumentsWithIncrease()only callsdecode($context, $doc)when no operators.That means for adapters relying on
Database::casting()for scalar normalization, update/upsert responses can return different PHP types (e.g. strings instead of ints/bools/floats) than the other CRUD methods.You should either:
- Add
casting($context, …)before eachdecode(...)in these three flows, or- Explicitly document that only create/get/find responses are guaranteed to be cast, and keep adapter behavior consistent.
Also applies to: 6048-6053, 6796-6800
6565-6567: upsertDocumentsWithIncrease: context‑aware decode only on non‑operator batchesThe introduction of a shared
QueryContextand conditionaldecode($context, $doc)when$hasOperators === falsemakes sense: operator‑based upserts refetch documents viarefetchDocuments(), and those refetched copies will go through the normal find/get decode pipeline.Once you add
casting($context, …)consistently to update/upsert flows (see earlier comment), mirror it here as well so non‑operator upsert results follow the same type normalization path as create/get/find.Also applies to: 6796-6800
7990-8006: count(): context + joins + relationship filters are wired well; clarify$maxsemanticsPositives:
- Using
QueryContextandDocumentsValidatorforcount()aligns its query semantics withfind().- Auth checks are properly enforced across all collections in the context, with
addSkipAuth()mirroringfind().convertQueries($context, $queries)andconvertRelationshipFiltersToSubqueries()ensure relationship filters are normalized before reaching the adapter, andadapter->count($context, $max, $filters, $joins)is a sensible new adapter contract.However, you now reject
$max < 1incount()whilesum():
- Still documents
$max=0as “unlimited count,” and- Forwards
$maxdirectly to the adapter without validation.This mismatch can confuse callers; decide on a single semantic (e.g. “
$maxmust be null or >=1; null means unlimited”) and apply it consistently to bothcount()andsum(), updating both docblocks and adapter usage.Also applies to: 8016-8025, 8031-8038, 8049-8067
8086-8087: sum(): context & relationship filters OK, but$maxhandling still inconsistent with count() and docblock
- Introducing
QueryContext+DocumentsValidatorand runningconvertQueries()plusconvertRelationshipFiltersToSubqueries()before callingadapter->sum(...)is symmetric with the newfind()/count()flows, which is good.- Auth logic (
$skipAuthvsauthorization->skip) remains the same, so behavior is preserved.But as noted above,
sum()’s docblock still says “Pass$max=0for unlimited count,” while:
sum()does not treat 0 specially, andcount()now rejects$max < 1.Please align
sum()’s$maxsemantics and documentation withcount()(or vice‑versa) so callers have a single, well‑defined contract.Also applies to: 8088-8091, 8095-8103, 8121-8133
8710-8789: Remove unused$idAddedvariable destructuring at line 8785The
$idAddedvariable is captured fromQueryContext::addSelect()but never used. Update the destructuring to capture only$queries:if (!empty($relationships)) { [$queries] = QueryContext::addSelect($queries, Query::select('$id', system: true)); }This eliminates the PHPMD warning and ensures
$idis auto-selected only when relationships exist, as intended.
🧹 Nitpick comments (7)
src/Database/Database.php (7)
4295-4331: getDocument(): SELECT‑only contract and context‑aware decoding
- Enforcing that
$queriescontain onlyQuery::select(...)and validating them viaDocumentsValidatorplusQueryContextis coherent with using the new projection/join machinery.- Auto‑adding
$permissionsviaQueryContext::addSelect()only when user already supplies selects, then stripping it back out via$permissionsAddedpreserves both correctness (auth checks) and API expectations (projection results).Consider updating the PHPDoc for
getDocument()to explicitly say: only SELECT queries are allowed and others will result inQueryException, to make this behavioral constraint obvious to callers.Also applies to: 4361-4363, 4387-4388, 4411-4413
4513-4516: Nested relationship select handling in populateDocumentsRelationshipsUsing
processRelationshipQueries()to derive$nestedSelections, then threading those down the BFS queue (and re‑invokingprocessRelationshipQueries()per level) is a reasonable way to support dotted relationship selects while keeping operator logic centralized.One nuance: the
$selectsvalue returned byprocessRelationshipQueries()in the inner call is discarded (only$nextSelectsis used). This is fine today because relationship fetch helpers don’t need per‑depth SELECTs beyond whatapplySelectFiltersToDocuments()does after grouping, but if you later want tighter projection at each depth, you may want to start honoring that first tuple element there.Also applies to: 4532-4545
4988-5003: applySelectFiltersToDocuments: logic is sound, but keep filters in sync with decode/castingPost‑filtering relationship batches by building an
attributesToKeepmap from select queries, honoring*, and always preserving internal$attributes (both fromINTERNAL_ATTRIBUTESand by leading$) is correct and efficient.Just ensure that any future changes to
INTERNAL_ATTRIBUTESor decode/casting behavior are mirrored here so internal/system attributes aren’t accidentally dropped from nested relationship documents.Also applies to: 5005-5012
5879-5893: Bulk update: validator/context wiring is good; required$permissions/$sequenceselects are sharp edgesPositives:
- Building a
QueryContextand validating$querieswithDocumentsValidatoris consistent withfind()/count()/sum().- The late decode via
decode($context, $doc)on each batch element is coherent with the new context‑aware decoding pipeline.However, the new guard:
foreach (['$permissions', '$sequence'] as $required) { if (!$batch[0]->offsetExists($required)) { throw new QueryException("Missing required attribute {$required} in select query"); } }forces callers who use custom SELECTs in
find()for bulk update to remember to include both$permissionsand$sequence, but bulk paths themselves don’t auto‑inject those system attributes (unlike howgetDocument()now auto‑injects$permissionswhen selects are present).Consider either:
- Automatically adding
Query::select('$permissions', system: true)andQuery::select('$sequence', system: true)for bulk update flows when any user SELECTs are present, or- Clearly documenting this requirement in the public API docs for
updateDocuments()so consumers aren’t surprised by runtimeQueryExceptions.Also applies to: 5974-5981, 6048-6053
8240-8327: decode(QueryContext,…): alias‑aware decoding looks correct; tighten exception typeThe new context‑aware
decode()implementation:
- Builds an internal+schema map per collection (filtered via adapter->filter) and iterates result keys, mapping them back to:
- The right collection (using alias resolution from
$selectsandQueryContext), and- The right attribute definition (internal vs schema).
- Falls back to schemaless behavior when adapter doesn’t support attributes, by copying unknown fields directly to the output document.
- Applies decode filters in reverse order per attribute and writes canonical attribute IDs (or
ASnames) into a fresh document instance for the main collection.Logic is sound overall. Two small suggestions:
- You currently throw a generic
\Exception('Invalid query: Unknown Alias context')on bad aliases; prefer throwingQueryExceptionfor consistency with other query‑validation paths.- If you later add join‑result projection that surfaces non‑main‑collection attributes, consider whether those should be nested or flattened; right now everything goes onto the main collection’s Document, which is fine, but should be consciously documented.
8651-8684: getCacheKeys(): projection‑aware cache keying looks correctExtending
getCacheKeys()to accept$selectsand includemd5(serialize($selects))in the document hash key when projections are present ensures that different select lists don’t collide in the cache. This is important now that per‑field selects are first‑class.Just be aware that
serialize($selects)includes fullQueryobjects; if their internal representation ever grows, you may want a more compact, stable representation (e.g., normalized attribute+alias list) for the hash.Also applies to: 8675-8677
8807-8954: convertRelationshipFiltersToSubqueries/processNestedRelationshipPath: multi‑level filter lowering looks correct but could be expensiveThe new relationship filter lowering logic:
- Groups dot‑path filters by top‑level relationship key.
- For each group, builds equivalent queries against the related collection (using
Query::parseQuery()), optionally handling multi‑level paths viaprocessNestedRelationshipPath().- For “virtual parent” sides (ONE_TO_MANY parent, MANY_TO_ONE child, MANY_TO_MANY), walks from leaf collections back up using
twoWayKeyand junction collections to derive parent IDs.- Replaces the original dot‑path filters with either:
Query::equal('$id', $parentIds)for virtual parents, orQuery::equal($relationshipKey, $matchingIds)for direct sides.- Returns
nullearly when no matches are possible, letting callers short‑circuit to empty results.This is a solid, general approach to translating relationship filters into SQL‑safe subqueries. The main trade‑off is potential extra
find()calls per relationship group, especially with deep nesting and large fan‑outs.If you see performance issues under heavy relational filtering, consider:
- Caching intermediate ID sets per path within a single top‑level request, and/or
- Bounding the size of
PHP_INT_MAX‑style subqueries for pathological cases.Functionally, though, the implementation looks consistent with the rest of the relationship system.
Also applies to: 8978-9181
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (2)
src/Database/Validator/Queries/V2.php (2)
V2(23-714)getDescription(135-138)src/Database/QueryContext.php (8)
QueryContext(8-136)add(68-76)addSelect(102-135)getCollections(32-35)skipAuth(83-94)addSkipAuth(78-81)getMainCollection(40-43)getCollectionByAlias(45-63)
🪛 PHPMD (2.15.0)
src/Database/Database.php
8785-8785: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (6)
src/Database/Database.php (6)
34-36: Switch to V2 DocumentsValidator looks appropriateAliasing
Utopia\Database\Validator\Queries\V2asDocumentsValidatoris clear and consistent with its new role across the file. No issues here.
5118-5123: createDocument/createDocuments: consistent casting + decode pathThreading a
QueryContextthroughcreateDocument()andcreateDocuments(), then applying$this->casting($context, ...)followed by$this->decode($context, ...)afteradapter->castingAfter()keeps type normalization and filter decoding consistent withgetDocument()/find().This alignment across create/read paths is good; no issues spotted.
Also applies to: 5171-5173, 5234-5238
6141-6170: Relationship maintenance: minimal$idprojections on nested reads look correctThe various relationship maintenance paths in
updateDocumentRelationships()now wrap nestedgetDocument()/findOne()andfind()calls withQuery::select('$id')where only the ID is needed. That’s a good optimization and keeps the new V2 query validator happy while minimizing projection size.These call‑sites all preserve existing behavior while cutting payload; they look consistent across one‑to‑one, one‑to‑many, many‑to‑one, and many‑to‑many branches.
Also applies to: 6181-6184, 6201-6205, 6283-6284, 6296-6299, 6325-6327, 6337-6338, 6407-6412
6853-6855: New$id‑only selects in increase/decrease/delete relationship helpersAdding
Query::select('$id')(and for some cases key attributes) to the internalfind()/findOne()/getDocument()calls used by increase/decrease and delete relationship helpers is consistent with the new V2 validator and reduces data transferred for these internal maintenance queries.These changes look safe and keep behavior while tightening projections.
Also applies to: 7231-7233, 7254-7256, 7292-7293, 7340-7343, 7363-7366, 7433-7436, 7553-7559
7750-7771: find(): QueryContext, joins, and adapter contract look coherentKey points in the updated
find():
- Correctly building
QueryContextwith the main collection plus all join targets fromQuery::getJoinQueries($queries).- Per‑collection auth: iterating
context->getCollections(), checking read/update/delete permission viaInput($forPermission, ...), and storing skipAuth flags in context for the adapter.- Switching to
$this->convertQueries($context, $queries)fixes the priorself::convertQueriesinstance‑method bug and lets conversion access the right alias/collection mapping.- Using
processRelationshipQueries()to split top‑level relationship selects from nested selects and auto‑inject$idonly when relationships exist.- Feeding the adapter a richer call:
$this->adapter->find( $context, $limit ?? 25, $offset ?? 0, $cursor, $cursorDirection, $forPermission, selects: $selects, filters: $filters, joins: $joins, vectors: $vectors, orderQueries: $orders );is a clean contract for join‑ and projection‑aware backends.
The overall flow is solid; no correctness issues obvious here beyond the bulk update/delete SELECT constraints already called out.
Also applies to: 7795-7806, 7841-7845, 7852-7865, 7874-7878
8547-8561: convertQueries/convertQuery: context‑aware conversion and datetime handling look good
- Accepting
QueryContextand recursing through nested/join queries while re‑invokingconvertQueries($context, $query->getValues())is a clean way to propagate alias/collection semantics down the tree.- In
convertQuery(), cloning the collection referenced bygetAlias(), resolving attribute metadata (including internal attributes), and mutating the query withsetOnArray()andsetAttributeType()preserves existing behavior.- Datetime normalization honors
adapter->getSupportForUTCCasting()and falls back toDateTime::setTimezone, wrapped inQueryExceptionon failure.The logic is solid; no issues beyond the already‑fixed static/instance call site in
find().Also applies to: 8566-8615
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
8890-9037: Nested many‑to‑many relationship filters likely never match due totwoWayKeyresolution logic.In
processNestedRelationshipPath()andconvertRelationshipFiltersToSubqueries()the reverse lookup for parent IDs uses:
find($link['toCollection'], …)withskipRelationships()(Lines 8968–8988).- Extraction of parent IDs via
$doc->getAttribute($link['twoWayKey'])(Lines 8993–9013 and 9208–9230).For many‑to‑many relationships, the
twoWayKeycolumn exists on the junction collection, not on the related collection documents themselves; and withskipRelationships()enabled inprocessNestedRelationshipPath(), relationship attributes won’t be populated either. That means for multi‑level paths involving a many‑to‑many hop (e.g.project.employees.skills.name),parentIdswill stay empty and these helpers will returnnull, causingfind()/count()/sum()to treat valid filters as “no matches”.You’ll likely need a special‑case for many‑to‑many in the nested path logic:
- Either walk through the junction via
getJunctionCollection()(mirroringpopulateManyToManyRelationshipsBatch()), or- Avoid
skipRelationships()on many‑to‑many and derive parent IDs from populated relationship attributes, as in the existing non‑nested filtering behavior.Also applies to: 9061-9264
♻️ Duplicate comments (1)
src/Database/Database.php (1)
5818-5822: Align update/upsert return casting with create/get/find to avoid type inconsistencies.
createDocument(),createDocuments(),getDocument(), andfind()all callcasting($context, …)followed bydecode($context, …)before returning documents, but:
updateDocument()only callsdecode($context, $document)(Lines 5818–5822).updateDocuments()only callsdecode($context, $doc)inside the batch loop (Lines 6048–6053).upsertDocumentsWithIncrease()only callsdecode($context, $doc)when!$hasOperators(Lines 6796–6800).For adapters relying on
Database::casting()for scalar normalization, these paths will return different PHP types (string vs int/bool/float) than the other CRUD methods using the same collection and queries, which is a behavioral inconsistency.Consider also invoking
casting($context, …)in these paths beforedecode()to keep the return shape uniform across read/write operations.Suggested diff (conceptual)
@@ public function updateDocument(…) - $context = new QueryContext(); - $context->add($collection); - - $document = $this->decode($context, $document); + $context = new QueryContext(); + $context->add($collection); + + $document = $this->casting($context, $document); + $document = $this->decode($context, $document); @@ public function updateDocuments(…) - $doc = $this->adapter->castingAfter($collection, $doc); - $doc->removeAttribute('$skipPermissionsUpdate'); - $this->purgeCachedDocument($collection->getId(), $doc->getId()); - $doc = $this->decode($context, $doc); + $doc = $this->adapter->castingAfter($collection, $doc); + $doc->removeAttribute('$skipPermissionsUpdate'); + $this->purgeCachedDocument($collection->getId(), $doc->getId()); + $doc = $this->casting($context, $doc); + $doc = $this->decode($context, $doc); @@ public function upsertDocumentsWithIncrease(…) - $doc = $this->adapter->castingAfter($collection, $doc); - if (!$hasOperators) { - $doc = $this->decode($context, $doc); - } + $doc = $this->adapter->castingAfter($collection, $doc); + if (!$hasOperators) { + $doc = $this->casting($context, $doc); + $doc = $this->decode($context, $doc); + }Also applies to: 6048-6053, 6565-6567, 6796-6800
🧹 Nitpick comments (1)
src/Database/Database.php (1)
8864-8869: Drop unused$idAddedinprocessRelationshipQueries()and keep$idauto‑select relationship‑only.PHPMD correctly flags
$idAddedas unused here:if (!empty($relationships)) { [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); }You only care about the updated
$queries, and the intent (per prior guidance) is to auto‑inject$idonly when relationship attributes exist.You can simplify and satisfy static analysis by discarding the second tuple element:
Proposed change
- /** - * In order to populateDocumentRelationships we need $id - */ - if (!empty($relationships)) { - [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); - } + /** + * In order to populateDocumentsRelationships we need $id + */ + if (!empty($relationships)) { + [$queries] = QueryContext::addSelect($queries, Query::select('$id', system: true)); + }This preserves behavior and removes the PHPMD warning.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (2)
src/Database/Validator/Queries/V2.php (3)
V2(23-714)isEmpty(163-174)getDescription(135-138)src/Database/QueryContext.php (8)
QueryContext(8-136)add(68-76)addSelect(102-135)getCollections(32-35)skipAuth(83-94)addSkipAuth(78-81)getCollectionByAlias(45-63)getMainCollection(40-43)
🪛 GitHub Actions: Linter
src/Database/Database.php
[error] 1-1: PSR-12: statement_indentation violation detected by Pint lint.
🪛 PHPMD (2.15.0)
src/Database/Database.php
8868-8868: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
4980-5013: Nested select paths currently drop intermediate relationship attributes (breaks multi-level population)
applySelectFiltersToDocuments()builds$attributesToKeepfromQuery::getAttribute()verbatim. For nested paths likecomments.author.name, the top-level related documents (e.g. comments) only have anauthorattribute, notauthor.name. As a result, when you call this helper from the batch population methods, theauthorattribute is removed, so deeper relationships (e.g. author → company) never get populated.To keep multi-level selections working, this method should treat only the top-level segment of the select path as the attribute to keep.
Proposed fix: normalize dotted paths to their top-level segment
// Collect all attributes to keep from select queries - $attributesToKeep = []; - - foreach ($selectQueries as $selectQuery) { - $attributesToKeep[$selectQuery->getAttribute()] = true; - } - - // Early return if wildcard selector present - if (isset($attributesToKeep['*'])) { - return; - } + $attributesToKeep = []; + + foreach ($selectQueries as $selectQuery) { + $attribute = $selectQuery->getAttribute(); + + // Wildcard: keep everything + if ($attribute === '*') { + $attributesToKeep['*'] = true; + break; + } + + // For dotted paths like "author.name", keep the top-level key "author" + $topLevel = \explode('.', $attribute)[0]; + $attributesToKeep[$topLevel] = true; + } + + // Early return if wildcard selector present + if (isset($attributesToKeep['*'])) { + return; + }This preserves:
- Simple selects (e.g.
select('name')→ keepname),- Nested selects (e.g.
select('author.name')→ keepauthoron this level and let deeper traversal handle the innername), and- Wildcard behavior.
♻️ Duplicate comments (3)
src/Database/Database.php (3)
5818-5822: Update/upsert return paths still skipcasting()(inconsistent with create/get/find)
updateDocument(),updateDocuments(), andupsertDocumentsWithIncrease()now create aQueryContextand calldecode($context, ...)on the documents they return, but they never pass those documents throughcasting($context, ...)the waycreateDocument(),createDocuments(),getDocument(), andfind()do.If
Database::casting()is responsible for normalizing scalar types (bool/int/float/id) for adapters that don’t fully cast on their own, these methods can still return un-normalized field values while the other CRUD methods return normalized ones.Consider aligning the pipelines by inserting
casting($context, $document, $selects)(or without$selectswhere appropriate) beforedecode()in these methods, mirroringcreateDocument()/find(). Please verify current adapter semantics forgetSupportForCasting()andcastingAfter()to decide whether this is required for the SQL/Mongo adapters you care about.Also applies to: 5879-5881, 6052-6053, 6565-6567, 6799-6800
7992-8003: Count/sum: semantics of$maxare still inconsistent
count()now explicitly rejects$max < 1, whilesum()’s docblock still advertises$max = 0as “unlimited count” and forwards$maxstraight to the adapter. This leaves the old inconsistency in place: callers can’t use$max = 0withcount(), and adapters may treat$max = 0forsum()asLIMIT 0rather than “no limit”.It would be good to decide on one set of semantics (e.g. “
$maxmust benullor> 0;nullmeans unlimited”) and align both methods’ validation, adapter calls, and docblocks accordingly.Also applies to: 8086-8087
8863-8869: Remove unused$idAddedinprocessRelationshipQueries()and ignore merge flag
processRelationshipQueries()now usesQueryContext::addSelect()to auto-add$idwhen relationships exist, but still captures the unused$idAddedflag. This matches the PHPMD warning and the prior guidance for this method.You can drop the unused variable and only capture the updated queries:
- /** - * In order to populateDocumentRelationships we need $id - */ - if (!empty($relationships)) { - [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); - } + /** + * In order to populateDocumentsRelationships we need $id + */ + if (!empty($relationships)) { + [$queries] = QueryContext::addSelect($queries, Query::select('$id', system: true)); + }This preserves behavior (only auto-adding
$idwhen there are relationship attributes) and satisfies PHPMD.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (2)
src/Database/Validator/Queries/V2.php (2)
V2(23-714)getDescription(135-138)src/Database/QueryContext.php (8)
QueryContext(8-136)add(68-76)addSelect(102-135)getCollections(32-35)skipAuth(83-94)addSkipAuth(78-81)getCollectionByAlias(45-63)getMainCollection(40-43)
🪛 PHPMD (2.15.0)
src/Database/Database.php
8868-8868: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (8)
src/Database/Database.php (8)
4306-4310: Context-aware DocumentsValidator wiring looks consistent across entry pointsUsing
Queries\V2asDocumentsValidatorwithQueryContext(ingetDocument(),updateDocuments(),deleteDocuments(),find(),count(), andsum()) matches the new context-based design and parameter contract (id type, max values, date bounds, attribute support, UID length). The constructor usage looks type-safe and consistent.Please just double-check against the actual
Queries\V2signature that the omitted optional arguments (e.g.maxQueriesCount,maxLimit,maxOffset) rely on defaults you’re happy with for all these call sites.Also applies to: 5885-5893, 7526-7534, 7776-7784, 8030-8038, 8095-8103
4295-4304: ConfirmgetDocument()’s “select-only” API andaddSelect()semanticsLimiting
$queriesingetDocument()to pureTYPE_SELECT(and throwing if any other method appears) is a reasonable tightening, but it’s a behavioral change if callers previously passed non‑select queries (even if they were ignored). Also,QueryContext::addSelect($selects, Query::select('$permissions', system: true))must not inject a$permissionsselect when there were no explicit selects originally, otherwisegetDocument()would only fetch$permissions(and whatever system selects you add later) instead of “all attributes”.Can you confirm:
- That existing public usage of
getDocument()never relied on non‑select queries, and- That
QueryContext::addSelect()is a no‑op when$selectsis empty (so the “no selects == all attributes” behavior is preserved)?Also applies to: 4321-4324
4327-4331: Select-aware cache keying looks good; verify purge clears all variantsIncluding
$selectsin the document hash key viamd5(serialize($selects))is a nice way to distinguish different projections.purgeCachedDocumentInternal()still purges by the base$documentKey, which should remove all select-variants as long as the underlyingCacheimplementation treatspurge($documentKey)as “delete all subkeys”.Please confirm that:
Cache::load/save/purgeinterpret the third parameter as a subkey and thatpurge($documentKey)clears all hashes for that key, and- There is no path where
purgeCachedDocumentInternal()would need to know the select set explicitly to invalidate a specific variant.Also applies to: 7691-7701, 8731-8767
5118-5123: QueryContext-based casting/decoding in create paths looks correctUsing a fresh
QueryContext(with only the main collection) forcreateDocument()andcreateDocuments()and then runningcasting($context, ...)followed bydecode($context, ...)aligns these write paths with the new read/join pipeline and keeps type normalization/decoding consistent.Also applies to: 5171-5173, 5235-5238
7750-7771: Join-awarefind()flow (context, auth, conversion, adapter call) is well-structuredThe updated
find():
- Builds a
QueryContextwith the main and joined collections and records per-collection skip-auth flags.- Validates queries via
DocumentsValidatorusing that context.- Runs
convertQueries($context, …)so alias-based attributes get proper type/array hints.- Processes relationship selects via
processRelationshipQueries()and converts relationship filters withconvertRelationshipFiltersToSubqueries().- Delegates to
adapter->find($context, … selects, filters, joins, vectors, orderQueries)and then appliescasting($context, …, $selects)anddecode($context, …, $selects).This is a coherent, join-capable pipeline that keeps authorization and query conversion centralized.
Given the new
Adapter::findsignature, please confirm all adapters (SQL, Mongo, etc.) are updated to accept theQueryContextand the splitselects/filters/joins/vectors/orderQueriescontract.Also applies to: 7795-7806, 7841-7842, 7853-7865, 7875-7878
8234-8241: New context-awaredecode()/casting()implementations look coherent for joinsThe reworked
decode(QueryContext $context, Document $document, array $selects = [])andcasting(QueryContext $context, Document $document, array $selects = []):
- Build per-collection schema maps keyed by adapter-filtered attribute names.
- Resolve aliasing (
ASand per-join alias) from$selectsto map keys back to the right collection/attribute.- Respect internal attributes and schemaless operation when
getSupportForAttributes() === false.- In
casting(), construct a new document of the main collection type with correctly cast scalars, whiledecode()decorates the existing document with filter-decoded values.This design matches the new join/query-context model and should behave correctly both for single-collection and multi-collection queries.
Also applies to: 8257-8299, 8314-8317, 8421-8429, 8443-8447, 8480-8485, 8523-8526
8630-8644:convertQueries()/convertQuery()refactor to useQueryContextlooks soundSwitching
convertQueries()/convertQuery()to accept aQueryContextand resolving attributes via alias:
- Properly handles nested and join queries by recursing on
isNested()/isJoin()values and reusing the same context.- Ensures non-SELECT queries get their
onArray, attribute type, and datetime normalization set based on the aliased collection’s schema, while SELECTs pass through untouched.- Throws a
QueryExceptionon unknown aliases instead of silently mis-interpreting attributes.This should make attribute-type handling for joins more robust without changing behavior for single-collection queries.
Also applies to: 8649-8697
8890-9037: Multi-level relationship filter → subquery conversion appears correct but is complex
processNestedRelationshipPath()plusconvertRelationshipFiltersToSubqueries()introduce a fairly sophisticated pipeline:
- Parse dotted attributes into relationship chains.
- Walk from the deepest collection back to the start, resolving parent IDs via
$twoWayKey(and for many-to-many via populated relationship attributes).- Replace filters like
author.company.name = 'X'with equivalent$id/relationship-key filters on the starting collection, returningnullearly when no matches are possible.The overall structure looks logically consistent for
oneToOne,oneToMany,manyToOne, andmanyToMany, and the early-returnnullbehavior is a good optimization for impossible filters.Given the number of relation-type/side combinations, it would be worth validating this against your e2e relationship tests (especially multi-hop cases like
project.employee.company.*and many-to-many chains) to ensure every combination yields the expected ID sets.Also applies to: 9061-9264
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/Database/Database.php (4)
5974-5981: Bulk update/delete still hard-require$permissions&$sequencein selects without auto-injectionBoth
updateDocuments()anddeleteDocuments()enforce that the first batch document contains$permissionsand$sequence, and throwQueryExceptionif those system attributes are missing from the select projection. Sincefind()does not automatically add these system selects (unlike how$idis auto-added for relationships and$permissionsis injected ingetDocument()), callers must remember to always include both fields whenever they customize selects for bulk ops. That’s easy to miss and somewhat asymmetric. Consider either auto-injecting these system selects in the bulk paths (similar to$idinjection inprocessRelationshipQueries()and$permissionsingetDocument()), or clearly documenting this requirement on theupdateDocuments()/deleteDocuments()PHPDocs.Also applies to: 7581-7588
7992-8067:count()vssum()$maxsemantics remain inconsistent
count()now validates$maxand rejects values< 1, which is clear and safe.sum()still accepts$maxunchecked and the docblock claims “$max=0 for unlimited count”, but the underlying adapter is likely to treat0asLIMIT 0, not “unlimited”. This mismatch between documentation and behavior (and betweencount()andsum()) can surprise callers. It would be better to standardize semantics (e.g. “nullmeans no limit,$maxmust be > 0”) across both methods and update docblocks and adapter plumbing accordingly.Also applies to: 8086-8137
5818-5822: Update / bulk-update / upsert paths still skipcasting()→ possible type-normalization inconsistencyThese write flows now call
decode($context, …)on the returned documents but still don’t invokecasting($context, …)(unlikecreateDocument(),createDocuments()batch,getDocument(), andfind(), which callcasting()beforedecode()). For adapters that rely onDatabase::casting()for scalar normalization (string → int/bool/float based on schema), this can leaveupdateDocument(),updateDocuments()callbacks, andupsertDocumentsWithIncrease()callbacks returning different PHP types for the same attributes compared to create/get/find. Unless the adapter’scastingAfter()already guarantees identical normalization, consider aligning these paths by addingcasting($context, …)beforedecode()in the post-write section of each method.// Example pattern (updateDocument tail) $context = new QueryContext(); $context->add($collection); // Optionally normalize scalars before decode, matching other CRUD flows $document = $this->casting($context, $document); $document = $this->decode($context, $document);Also applies to: 6048-6053, 6565-6567, 6796-6800
8800-8875: Remove unused$idAddedinprocessRelationshipQueries()and keep$idauto-select relationship-onlyPHPMD is right here:
$idAddedfromQueryContext::addSelect()is never used—only the updated$queriesmatter. Given the intended behavior (“auto-add$idonly when the collection has relationship attributes”), you can safely drop the second tuple element and avoid the unused-local warning while preserving semantics. This also aligns with the prior guidance captured in the learnings for this method.Proposed fix
- /** - * In order to populateDocumentRelationships we need $id - */ - if (!empty($relationships)) { - [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); - } + /** + * In order to populateDocumentsRelationships we need $id + */ + if (!empty($relationships)) { + [$queries] = QueryContext::addSelect($queries, Query::select('$id', system: true)); + }
🧹 Nitpick comments (1)
src/Database/Database.php (1)
8240-8325:decode()now mutates in-place whilecasting()builds a new instance—watch for mixed-key stateThe new
decode(QueryContext, …)implementation decodes directly into the original$document, whilecasting()still creates a freshDocumentand copies attributes. This means a casted-then-decoded flow (e.g.find()/getDocument()) will end up with a newDocumentfromcasting()thatdecode()then mutates in place, potentially leaving both canonical keys and any pre-existing raw keys present. That’s probably fine, but if you ever rely ongetArrayCopy()shape (e.g. for serialization or equality checks), consider either (a) havingdecode()also build a newDocumentlikecasting(), or (b) explicitly unsetting raw keys when$attributeKey !== $key.Also applies to: 8425-8533
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (3)
src/Database/QueryContext.php (8)
QueryContext(8-136)add(68-76)addSelect(102-135)getCollections(32-35)skipAuth(83-94)addSkipAuth(78-81)getCollectionByAlias(45-63)getMainCollection(40-43)src/Database/Document.php (5)
getAttribute(224-231)getId(63-66)Document(12-470)getCollection(85-88)setAttribute(244-261)src/Database/Validator/Authorization/Input.php (1)
Input(5-49)
🪛 PHPMD (2.15.0)
src/Database/Database.php
8872-8872: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (2)
src/Database/Database.php (2)
8894-9268: Complex nested relationship-filter conversion has existing test coverageThe review recommended adding targeted tests for depth-2 and depth-3 nested paths (like
project.employee.company.name), null short-circuit cases, and mixed relationship-with-non-relationship filters. However, these tests already exist intests/e2e/Adapter/Scopes/RelationshipTests.phpand comprehensively cover:
- Depth-2 paths:
employee.company.name,employee.department.name,project.employee.name- Depth-3 paths:
project.employee.company.name,project.employee.department.budget,project.employee.company.industry- Mixed filters: non-relationship attributes combined with nested relationship queries
- Empty-result cases: queries correctly returning zero matches when no documents satisfy the filters
The logic for
processNestedRelationshipPath()andconvertRelationshipFiltersToSubqueries()is appropriately tested across relationship types and nesting depths.Likely an incorrect or invalid review comment.
4295-4335: Remove this review comment—the SELECT-only constraint is intentional and well-tested.The new
getDocument()validation that rejects non-SELECT queries is not a breaking change. The codebase has been written to comply with this constraint: all internal calls pass SELECT queries only (e.g., lines 6140, 6169, 6283, 6326, 6407), and there is a dedicated test method (testGetDocumentOnlySelectQueriesat lines 1477–1510 in DocumentTests.php) that explicitly validates this behavior. No scan is needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Database.php (2)
4387-4394: Relationship population is disabled for collections without explicit nested selectionsIn both
getDocument()(lines 4387–4394) andfind()(lines 7868–7872), relationship population is gated by:if ( !$this->inBatchRelationshipPopulation && $this->resolveRelationships && !empty($relationships) && (empty($selects) || !empty($nestedSelections)) ) { // populateDocumentsRelationships(...) }However, before this check, both methods call
processRelationshipQueries(), which injects a$idsystem select whenever the collection has relationships:if (!empty($relationships)) { [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); }This means when a caller invokes
getDocument()orfind()without explicit select queries on a relationship-bearing collection,$selectsbecomes non-empty (containing the injected$id). The gate condition then evaluates to:
(FALSE || !empty($nestedSelections))when no nested selections were requested- Result: relationships are not populated unless the caller explicitly nested-selected them
This changes the historical behavior where relationships were auto-resolved by default. The gate should use a flag tracking user-specified selects (before system injection) rather than the post-injection
$selectsarray. Consider preserving the original$selectsor adding an$hasExplicitSelectsflag before callingprocessRelationshipQueries().Also applies to: 7868-7872
8801-8947: Nested-path relationship filtering (3+ levels) fails for virtual relationships due to skipRelationships preventing populationThe review correctly identifies a twoWayKey population issue but misattributes it. The actual problem:
Single-level many-to-many filtering works correctly.
convertRelationshipFiltersToSubqueries()at lines 9032–9034 callsfind()withoutskipRelationships()and without explicit SELECT queries, so the guard(empty($selects) || !empty($nestedSelections))at line 7868 is satisfied, andpopulateDocumentsRelationships()is invoked to populate the twoWayKey.Nested-path filtering breaks for all virtual relationships.
processNestedRelationshipPath()at lines 8869, 8881, and 8904 wraps all internalfind()calls inskipRelationships(), which sets$this->resolveRelationships = false. This causes the guard at line 7868 to fail, sopopulateDocumentsRelationships()is never invoked and the twoWayKey remains null for deep paths likeproject.employee.company.name.This affects:
- Many-to-many nested paths
- One-to-many relationships queried from parent side
- Many-to-one relationships queried from child side
Tests cover only single-level filters (e.g.,
assignedProjects.priority). Multi-level nested paths have no test coverage and will fail when attempting to read the twoWayKey.The fix requires either allowing relationship population in
processNestedRelationshipPath()or resolving parent IDs via direct junction collection queries instead of relying on the twoWayKey attribute.
♻️ Duplicate comments (1)
src/Database/Database.php (1)
8707-8783: Drop unused$idAddedfromprocessRelationshipQueries()to satisfy PHPMD and simplify intentPHPMD correctly reports
$idAddedas unused inprocessRelationshipQueries():if (!empty($relationships)) { [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); }Only the updated
$queriesare used; the boolean flag is ignored. You can keep behavior and fix the warning by discarding the second tuple element:- if (!empty($relationships)) { - [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); - } + if (!empty($relationships)) { + [$queries] = QueryContext::addSelect($queries, Query::select('$id', system: true)); + }This also aligns with the previously agreed guidance to add
$idonly when there are relationship attributes and to avoid unused locals in this helper.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (2)
src/Database/Validator/Queries/V2.php (1)
V2(23-714)src/Database/QueryContext.php (8)
QueryContext(8-136)add(68-76)addSelect(102-135)getCollections(32-35)skipAuth(83-94)addSkipAuth(78-81)getCollectionByAlias(45-63)getMainCollection(40-43)
🪛 PHPMD (2.15.0)
src/Database/Database.php
8779-8779: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (1)
src/Database/Database.php (1)
4305-4314: Context‑aware DocumentsValidator wiring across CRUD/find/count/sum looks consistentThe move to use
DocumentsValidator(Queries V2) withQueryContext—ingetDocument(),updateDocuments(),deleteDocuments(),find(),count(), andsum()—is wired consistently:
- Each method builds a
QueryContext, adds the main collection and any join collections, and then constructsDocumentsValidatorwith the same context and adapter limits (id type, max values, date range, max UID length, supportForAttributes).- Validation is run before query grouping / conversion, which matches the expected flow for guarding invalid queries early.
No issues here; the context propagation and validator usage look correct.
Also applies to: 5884-5893, 7525-7534, 7776-7784, 8030-8038, 8095-8103
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Database/Database.php (3)
8190-8207: Fix stale PHPStan suppression causing pipeline failure inencode()The
@phpstan-ignore-next-lineon theis_null($value)check is now reported as “No error to ignore” in the pipeline, so it’s actively breaking builds without providing value. The logic itself is fine; only the suppression is problematic.You can safely drop the suppression (and the accompanying comment, if you like) to clear the error without changing behavior:
Suggested change
- // Assign default only if no value provided - // False positive "Call to function is_null() with mixed will always evaluate to false" - // @phpstan-ignore-next-line - if (is_null($value) && !is_null($default)) { + // Assign default only if no value provided + if (is_null($value) && !is_null($default)) { // Skip applying defaults during updates to avoid resetting unspecified attributes if (!$applyDefaults) { continue; }
8240-8440: UseQueryExceptioninstead of generic\Exceptionfor unknown alias contexts indecode()/casting()Both
decode()andcasting()throw a generic\Exception('Invalid query: Unknown Alias context')whenQueryContext::getCollectionByAlias()returns an empty collection. Elsewhere (e.g.convertQuery()) the same condition is treated as aQueryException, which is more appropriate for user-facing query errors and keeps error handling consistent.Consider switching these throws to
QueryException:Suggested change in
decode()andcasting()- $collection = $context->getCollectionByAlias($alias); - if ($collection->isEmpty()) { - throw new \Exception('Invalid query: Unknown Alias context'); - } + $collection = $context->getCollectionByAlias($alias); + if ($collection->isEmpty()) { + throw new QueryException('Invalid query: Unknown Alias context'); + }This keeps all “bad query / bad alias” paths surfaced as query errors rather than generic 500s.
8801-9175: Many-to-many relationship filters fail to populate the two-way relationship key, causing all filters to return no matchesWhen filtering on many-to-many relationships (e.g.,
Query::equal('tags.name', ['foo'])), the code callsfind()on the related collection without explicit SELECT queries:if ($needsParentResolution) { $matchingDocs = $this->silent(fn () => $this->find( $relatedCollection, \array_merge($relatedQueries, [ Query::limit(PHP_INT_MAX), ]) )); }This path has two critical problems:
Missing relationship population:
find()internally callsprocessRelationshipQueries(), which auto-injects a system-only$idselect whenever relationships exist. This makes$selectsnon-empty, so the population condition(empty($selects) || !empty($nestedSelections))becomes false, andpopulateDocumentsRelationships()is never called.Unpopulated $twoWayKey: For many-to-many, the
$twoWayKeyattribute is a relationship that only gets populated viapopulateDocumentsRelationships(). Without it,$doc->getAttribute($twoWayKey)returns null for all matched documents, leaving$parentIdsempty. As a result, the method returns null, making the filter behave as "no matches" even when valid matches exist.The existing test passes because it manually pre-populates the relationship attribute on document creation (
'students' => ['student1']), masking the issue in real-world usage without manual pre-population.To fix this, ensure relationship population happens by either:
- Treating system-only selects as "no explicit selects" for relationship population, or
- Calling
find()without any selects to allow the normal population flow to triggerAlso add tests for many-to-many filtering without manual relationship pre-population to prevent regression.
♻️ Duplicate comments (1)
src/Database/Database.php (1)
8707-8783: Remove unused$idAddedinprocessRelationshipQueriesand keep$idauto-select relationship-onlyThe tail of
processRelationshipQueries()still captures$idAddedfromQueryContext::addSelect()but never uses it:if (!empty($relationships)) { [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); }PHPMD correctly flags
$idAddedas unused, and per the earlier learning we only need the updated$querieswhile keeping$idauto-selected when relationships exist.You can drop the unused variable by only capturing the first element:
Suggested change
- /** - * In order to populateDocumentRelationships we need $id - */ - if (!empty($relationships)) { - [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); - } + /** + * In order to populateDocumentsRelationships we need $id + */ + if (!empty($relationships)) { + [$queries] = QueryContext::addSelect($queries, Query::select('$id', system: true)); + }This matches the prior guidance for this method and clears the PHPMD warning.
Based on learnings, this is the intended pattern here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (2)
src/Database/Validator/Queries/V2.php (1)
V2(23-714)src/Database/QueryContext.php (3)
QueryContext(8-136)add(68-76)addSelect(102-135)
🪛 GitHub Actions: CodeQL
src/Database/Database.php
[error] 8203-8203: PHPStan error: No error to ignore is reported on line 8203.
🪛 PHPMD (2.15.0)
src/Database/Database.php
8779-8779: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Unit Test
Summary by CodeRabbit
New Features
Behavior Changes / Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.