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: 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
# Conflicts: # src/Database/Adapter/SQL.php # src/Database/Validator/Queries.php # src/Database/Validator/Query/Filter.php
There was a problem hiding this comment.
Actionable comments posted: 4
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 (1)
8166-8213:sum()doesn't handle join queries likefind()andcount()do—missing context registration and auth validation.While
find()andcount()extract join queries viaQuery::getJoinQueries($queries), register joined collections toQueryContext, and validate authorization for all collections,sum()only registers the primary collection and ignores joins entirely. The adapter'ssum()signature doesn't even acceptQueryContext(unlikecount()), preventing proper join processing. Either implement full join support matchingfind()/count(), or reject join queries early with a clearQueryExceptionto avoid silent failures.src/Database/Adapter/Mongo.php (2)
1956-1988: Cursor pagination reads$cursor[...]keys without existence checks.
If the cursor payload is missing any ordered attribute (or has alias-qualified keys elsewhere), this will raise notices / break pagination unexpectedly.Defensive checks
for ($j = 0; $j < $i; $j++) { $originalPrev = $orderQueries[$j]->getAttribute(); $prevAttr = $this->filter($this->getInternalKeyForAttribute($originalPrev)); - $tmp = $cursor[$originalPrev]; + if (!array_key_exists($originalPrev, $cursor)) { + throw new DatabaseException("Missing cursor value for order attribute: {$originalPrev}"); + } + $tmp = $cursor[$originalPrev]; $andConditions[] = [ $prevAttr => $tmp ]; } - $tmp = $cursor[$originalAttribute]; + if (!array_key_exists($originalAttribute, $cursor)) { + throw new DatabaseException("Missing cursor value for order attribute: {$originalAttribute}"); + } + $tmp = $cursor[$originalAttribute];
2141-2227:count()likely incorrect for$tenant/other$...attributes + it silently returns 0 on MongoException.
- Unlike
find(),count()never runsreplaceInternalIdsKeys(...), so filters likeQuery::equal('$tenant', ...)won’t match stored_tenant.- Swallowing exceptions (
catch (MongoException) { return 0; }) will mask operational failures as “empty result”.Proposed fix
// Build filters from queries $filters = $this->buildFilters($filters); if ($this->sharedTables) { $filters['_tenant'] = $this->getTenantFilters($collection->getId()); } @@ + // Keep count() filter translation consistent with find() + $filters = $this->replaceInternalIdsKeys($filters, '$', '_', $this->operators); @@ $result = $this->client->aggregate($name, $pipeline, $options); @@ return 0; } catch (MongoException $e) { - return 0; + throw $this->processException($e); }src/Database/Adapter/SQL.php (1)
3011-3073: Bug: cursor pagination uses the current order alias for previous-order equality clauses.
Inside the$j < $iloop you build equality predicates as{$orderAlias}.prevAttr = :cursor_j. If different order queries use different aliases (main vs joined tables), pagination becomes wrong.Proposed fix
foreach ($orderQueries as $i => $order) { - $orderAlias = $order->getAlias(); + $orderAlias = $this->filter($order->getAlias()); @@ for ($j = 0; $j < $i; $j++) { $prevQuery = $orderQueries[$j]; + $prevAlias = $this->filter($prevQuery->getAlias()); $prevOriginal = $prevQuery->getAttribute(); $prevAttr = $this->filter($this->getInternalKeyForAttribute($prevOriginal)); @@ - $conditions[] = "{$this->quote($orderAlias)}.{$this->quote($prevAttr)} = {$bindName}"; + $conditions[] = "{$this->quote($prevAlias)}.{$this->quote($prevAttr)} = {$bindName}"; }
🤖 Fix all issues with AI agents
In `@src/Database/Adapter/Postgres.php`:
- Around line 1795-1799: The right alias in the Query::TYPE_RELATION_EQUAL case
is quoted without being sanitized; update the code to mirror the left side by
filtering the right alias before quoting (i.e., call
$this->filter($query->getRightAlias()) and then $this->quote(...) when building
$aliasRight) so getRightAlias() is sanitized prior to use in the returned
"{$alias}.{$attribute}={$aliasRight}.{$attributeRight}" expression.
In `@src/Database/Adapter/SQL.php`:
- Around line 2347-2385: getAttributeProjection is treating every element in the
passed array as a SELECT query; add a guard to skip non-select entries just like
getSQLConditions does. Inside getAttributeProjection (method name), before
processing each $select, check that $select->getType() === Query::TYPE_SELECT
(or equivalent TYPE_SELECT constant) and continue if not, so only SELECT queries
are used to build the projection; keep existing checks for '$collection' and
alias/as handling unchanged. Ensure you reference the same Query::TYPE_SELECT
symbol used elsewhere in the codebase to maintain consistency.
In `@src/Database/Database.php`:
- Around line 8320-8360: The decode() method (and the casting() block mentioned)
currently throws a generic \Exception('Invalid query: Unknown Alias context');
replace these throws with the existing QueryException type used by the query
flow so callers can catch query-specific errors consistently; update both the
throw in decode() and the similar throw in casting() (and any other occurrences
around lines ~8412-8459) to construct and throw QueryException with the same
message/context, and ensure the QueryException symbol is referenced/imported
correctly in this file so the new throws compile.
In `@src/Database/Validator/Queries/V2.php`:
- Around line 329-336: The switch branch handling Database::VAR_INTEGER uses an
undefined variable $attributeSchema; replace references to $attributeSchema with
the correct variable $attribute (used earlier) so signed and other flags are
read from $attribute; ensure you still reference $size for bit calculation and
instantiate Integer(false, $bits, $unsigned) as before (i.e., in the
Database::VAR_INTEGER case, read $signed = $attribute['signed'] ?? true and
compute $unsigned = !$signed && $bits < 64).
♻️ Duplicate comments (8)
src/Database/Query.php (1)
519-561: Validate new metadata fields to maintain consistent error handlingThe
parseQuery()method validatesmethod,attribute, andvaluestypes, but the new metadata fields (alias,aliasRight,attributeRight,collection,as) are taken as-is from the decoded array without type checking. If a serialized query contains a non-string value for one of these keys (e.g.,alias: 123oraliasRight: []), the constructor's strictstringparameter types will throw aTypeErrorinstead of aQueryException.Consider adding validation for consistency:
♻️ Suggested validation
// After line 528, add: foreach (['alias', 'aliasRight', 'attributeRight', 'collection', 'as'] as $field) { if (isset($query[$field]) && !\is_string($query[$field])) { throw new QueryException("Invalid query {$field}. Must be a string, got " . \gettype($query[$field])); } }src/Database/Validator/Queries/V2.php (2)
210-225: Handle missing attributes safely in schemaless modeWhen
$supportForAttributesisfalseand the attribute is not found in the schema,$attributeis an empty array. The code continues past line 213 but then tries to access$attribute['type']at line 219, which will cause an undefined index notice and incorrect behavior (the empty array comparison$attribute['type'] != Database::VAR_RELATIONSHIPwill likely throw a warning or return unexpected results).This was previously flagged in past reviews. Add an early return for schemaless mode when the attribute is unknown:
🔧 Proposed fix
$attribute = $this->schema[$collection->getId()][$attributeId] ?? []; - if (empty($attribute) && $this->supportForAttributes) { - throw new \Exception('Invalid query: Attribute not found in schema: '.$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'] ?? [])) {
534-538: Join-scope alias check is too strict for non-relation queriesThis block runs for every nested query under a join scope, but non-relation queries (filters, orders) typically have an empty
rightAlias. The check!in_array($query->getRightAlias(), $this->joinsAliasOrder)will fail for empty strings since''is not in$this->joinsAliasOrder, incorrectly rejecting valid filter queries inside joins.Restrict the alias check to relation queries only:
🔧 Proposed fix
if ($scope === 'joins') { - if (!in_array($query->getAlias(), $this->joinsAliasOrder) || !in_array($query->getRightAlias(), $this->joinsAliasOrder)){ + if ($query->getMethod() === Query::TYPE_RELATION_EQUAL && + (!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.'); } }src/Database/Database.php (2)
5591-5836: Post-write paths still skipDatabase::casting(...)beforedecode(...)in some branches.This looks like the same “decode without casting” gap previously discussed for
updateDocument(),updateDocuments(), and parts ofupsertDocumentsWithIncrease(). IfDatabase::casting()is required to normalize output types for some adapters, these returns will be inconsistent withgetDocument()/find()/createDocument(s).Also applies to: 5859-6064, 6624-6902
8787-8861: Remove unused$idAddedinprocessRelationshipQueries()(PHPMD) and keep$idinjection relationship-only.
$idAddedis assigned but never used. Also, per earlier discussion, the$idsystem select should only be injected when relationships exist (!empty($relationships)), which you already have.Proposed fix
if (!empty($relationships)) { - [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); + [$queries] = QueryContext::addSelect($queries, Query::select('$id', system: true)); }src/Database/Adapter/SQL.php (2)
3079-3095:skipAuth()keying still looks suspicious (filtered$collection).
IfQueryContextstores skip-auth keys using raw collection ids, filtering here can cause permissions to be applied unexpectedly.
3107-3114: RIGHT JOIN permission clause mixes quoted and unquoted alias usage ({$alias}._uid IS NULL).
This was already flagged earlier; it should use the same quoting helpers asgetSQLPermissionsCondition().Also applies to: 3269-3276
src/Database/Adapter/Mongo.php (1)
1939-1949: Guard against unsupported random ordering before callinggetOrderDirection().
This adapter reportsgetSupportForOrderRandom() === false, and this exact pattern was already flagged previously for Mongofind().
🧹 Nitpick comments (9)
src/Database/Validator/Queries/V2.php (1)
46-46: Consider resetting$this->vectorsat the start ofisValid()to prevent cross-call state leakageThe
$this->vectorscounter is incremented when a vector query is validated (line 376) but never reset. If the same validator instance is reused for multiple separateisValid()calls (not nested, but sequential), the counter accumulates across calls. A second validation call containing a single valid vector query would incorrectly fail because$this->vectors > 0.Per past discussion, you confirmed wanting this behavior for nested validations. However, for top-level calls, consider resetting at the start of
isValid():public function isValid($value, string $scope = ''): bool { + // Reset per-validation state for top-level calls + if ($scope === '') { + $this->vectors = 0; + } + try {Also applies to: 502-504
src/Database/Database.php (3)
4295-4324: Prefer callingQueryContext::addSelect(...)directly (avoid$context::...) and keep the$permissionsremoval comment accurate.
[$selects, $permissionsAdded] = $context::addSelect(...)works but reads like an instance method; usingQueryContext::addSelect(...)(or$context->addSelect(...)if you later make it non-static) is clearer. Also the comment “Or remove all queries added by system” doesn’t match the current behavior (only$permissionsis removed).Also applies to: 4411-4413
4986-5012: Handle SELECT aliases inapplySelectFiltersToDocuments()(don’t accidentally strip selected fields).If callers use
Query::select('field', as: 'alias'), the document key will be the alias, but this method only whitelistsgetAttribute(), so it can remove a legitimately selected field.Proposed fix
foreach ($selectQueries as $selectQuery) { - $attributesToKeep[$selectQuery->getAttribute()] = true; + $attributesToKeep[$selectQuery->getAs() ?: $selectQuery->getAttribute()] = true; }
8725-8751: Cache key hashing viaserialize($selects)is order-sensitive and object-serialization dependent.This will create different cache keys for semantically equivalent selects with different ordering, and it relies on PHP object serialization stability for
Query. Consider hashing a canonical representation (e.g., map to scalar arrays + sort) to reduce accidental cache fragmentation.src/Database/Adapter/Mongo.php (4)
708-735:renameAttribute()should validate no-op/ambiguous renames (old == new) before issuing$rename.
Right now it will still run an update across the full collection even if$old === $new(or if both map to the same internal key), which is wasted work and can cause surprising behavior.Proposed tweak
public function renameAttribute(string $collection, string $old, string $new): bool { $collection = $this->getNamespace() . '_' . $this->filter($collection); $from = $this->filter($this->getInternalKeyForAttribute($old)); $to = $this->filter($this->getInternalKeyForAttribute($new)); + if ($from === $to) { + return true; + } $options = $this->getTransactionOptions();
1126-1156: Projection input type mismatch risk:getDocument()passes$queriesintogetAttributeProjection()(which expects selects).
If$queriescan include non-SELECT query types, this can accidentally restrict the projection to filter attributes only.Safer `getAttributeProjection()` (also covers `find()` callers)
protected function getAttributeProjection(array $selects): array { $projection = []; if (empty($selects)) { return []; } foreach ($selects as $select) { + // Be defensive if callers pass mixed query arrays. + if (method_exists($select, 'getMethod') && $select->getMethod() !== Query::TYPE_SELECT) { + continue; + } if ($select->getAttribute() === '$collection') { continue; }
1876-1888:find()signature is compatible, but consider explicitly suppressing unused$joins/$vectors.
PHPMD is right: Mongo adapter can’t use them (no relationships/vectors), but the interface requires them.Minimal suppression
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 { + unset($joins, $vectors); $collection = $context->getMainCollection();
2574-2605: Projection builder should ignore non-select queries defensively.
Even if the “intended” contract is “selects only”, filtering byQuery::TYPE_SELECTmakes this safer (and avoids subtle bugs when a mixed query array is accidentally passed).src/Database/Adapter/SQL.php (1)
374-379: Consistency: quote_uidingetDocument()WHERE and don’t rely on raw._uid.
Not a security issue by itself, but it’s inconsistent with the rest of the file’s quoting and can bite if quoting rules change.Proposed tweak
- WHERE {$this->quote($alias)}._uid = :_uid + WHERE {$this->quote($alias)}.{$this->quote('_uid')} = :_uid
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
src/Database/Adapter/Mongo.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Database.phpsrc/Database/Query.phpsrc/Database/Validator/Queries/V2.phpsrc/Database/Validator/Structure.phptests/e2e/Adapter/Scopes/AttributeTests.phptests/e2e/Adapter/Scopes/CollectionTests.phptests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.phptests/e2e/Adapter/Scopes/Relationships/OneToManyTests.phptests/e2e/Adapter/Scopes/Relationships/OneToOneTests.phptests/e2e/Adapter/Scopes/SchemalessTests.phptests/unit/Validator/StructureTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/Adapter/Scopes/AttributeTests.php
- tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php
- tests/e2e/Adapter/Scopes/CollectionTests.php
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-07-01T11:31:37.438Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 613
File: src/Database/Adapter/Postgres.php:1254-1319
Timestamp: 2025-07-01T11:31:37.438Z
Learning: In PostgreSQL adapter methods like getUpsertStatement, complexity for database-specific SQL generation is acceptable when the main business logic is properly separated in the parent SQL adapter class, following the adapter pattern where each database adapter handles its own SQL syntax requirements.
Applied to files:
src/Database/Adapter/Postgres.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:
tests/unit/Validator/StructureTest.phpsrc/Database/Database.phptests/e2e/Adapter/Scopes/SchemalessTests.phptests/e2e/Adapter/Scopes/Relationships/OneToManyTests.phptests/e2e/Adapter/Scopes/Relationships/OneToOneTests.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Validator/Queries/V2.phpsrc/Database/Query.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.phptests/e2e/Adapter/Scopes/SchemalessTests.phptests/e2e/Adapter/Scopes/Relationships/OneToManyTests.phptests/e2e/Adapter/Scopes/Relationships/OneToOneTests.phpsrc/Database/Adapter/SQL.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.phptests/e2e/Adapter/Scopes/Relationships/OneToManyTests.phptests/e2e/Adapter/Scopes/Relationships/OneToOneTests.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/Structure.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/Adapter/Mongo.phpsrc/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
🧬 Code graph analysis (6)
tests/unit/Validator/StructureTest.php (3)
src/Database/Validator/Structure.php (1)
getDescription(197-200)src/Database/Validator/Operator.php (1)
getDescription(90-93)src/Database/Validator/Sequence.php (1)
getDescription(14-17)
src/Database/Database.php (1)
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)
tests/e2e/Adapter/Scopes/SchemalessTests.php (1)
src/Database/Query.php (2)
Query(8-1587)select(794-797)
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (2)
src/Database/Query.php (3)
Query(8-1587)select(794-797)limit(837-840)src/Database/Database.php (1)
find(7822-7974)
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (1)
src/Database/Query.php (2)
Query(8-1587)select(794-797)
src/Database/Adapter/SQL.php (3)
src/Database/QueryContext.php (2)
QueryContext(8-136)getMainCollection(40-43)src/Database/Adapter.php (4)
getAttributeProjection(1256-1256)quote(491-491)filter(1265-1274)getTenantQuery(1356-1356)src/Database/Query.php (4)
Query(8-1587)select(794-797)getAttribute(259-262)join(1009-1012)
🪛 PHPMD (2.15.0)
src/Database/Database.php
8859-8859: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
src/Database/Adapter/Mongo.php
1885-1885: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
1886-1886: Avoid unused parameters such as '$vectors'. (undefined)
(UnusedFormalParameter)
2141-2141: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
src/Database/Validator/Queries/V2.php
331-331: Avoid unused local variables such as '$attributeSchema'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (30)
tests/unit/Validator/StructureTest.php (1)
284-284: LGTM! Error message updates are consistent and informative.The updated expected error messages correctly reflect the new validator output, providing explicit 32-bit signed integer range bounds (-2,147,483,648 to 2,147,483,647). This aligns with the PR's goal of providing more descriptive validation feedback.
Also applies to: 462-462, 477-477, 543-543, 665-665
src/Database/Validator/Structure.php (1)
351-356: The hardcodedfalseparameter is intentional; signedness validation is delegated to the Range validator.The
Integer(false, $bits, $unsigned)call is correct. The Integer validator handles type checking only, while the Range validator (line 359) correctly uses the attribute's$signedvalue to enforce the min/max range constraints. For unsigned values, the Range validator sets$min = 0; for signed values, it sets$min = -$max. This design is also used identically inQueries/V2.phpand works correctly in all tests.src/Database/Query.php (2)
1003-1050: LGTM on join factory methodsThe new static factory methods for joins (
join,innerJoin,leftJoin,rightJoin,relationEqual) are well-structured and correctly pass the new metadata fields (alias,collection,attributeRight,aliasRight) to the constructor.
1059-1070: No external usages ofgetByTypewere detected. The visibility reduction toprotectedappears to be an intentional internal refactoring. No action required.Likely an incorrect or invalid review comment.
tests/e2e/Adapter/Scopes/SchemalessTests.php (2)
175-197: LGTM - Tests correctly updated for newQuery::select()signatureThe test updates correctly reflect the new
Query::select(string $attribute, ...)signature, replacing the previous array-based syntax with individual string arguments. This aligns with the API change inQuery.php.
862-886: LGTM - Internal attribute selection tests updatedThe tests for selecting internal attributes (
$id,$sequence,$collection,$createdAt,$updatedAt,$permissions) are correctly updated to use individualQuery::select()calls per field, matching the new API design.tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (8)
119-123: LGTM!The
Query::select('name')usage correctly aligns with the new string-based API signature.
150-153: LGTM!Splitting
Query::select(['*', 'albums.name'])into separateQuery::select('*')andQuery::select('albums.name')calls correctly implements the new per-field selection API.
162-165: LGTM!Consistent pattern applied to
getDocumentwith separate select calls for*andalbums.name.
588-591: LGTM!Two-way relationship test correctly updated to use separate
Query::selectcalls for*andaccounts.name.
600-603: LGTM!Consistent with the updated API pattern for
getDocumentwith relationship attribute selection.
924-927: LGTM!Single field selection
Query::select('name')correctly updated.
931-934: LGTM!Correctly using single
Query::select('*')for wildcard selection.
938-943: LGTM!Nested relationship selections (
*,cities.*,cities.mayor.*) correctly split into separateQuery::selectcalls, properly testing deep relationship field projections.tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (8)
171-173: LGTM!The
Query::select('name')usage correctly aligns with the new string-based API signature.
181-184: LGTM!Correctly split into separate
Query::select('*')andQuery::select('library.name')calls for the one-to-one relationship test.
193-197: LGTM!Three separate select calls for
*,library.name, and$idcorrectly demonstrate the new per-field selection API. The explicit$idselection ensures the document ID is always available.
204-206: LGTM!Single field selection for
namewith proper assertion thatlibraryis not included.
210-213: LGTM!Wildcard selection correctly returns
libraryas a string ID (not populated) when relationship fields aren't explicitly selected.
215-219: LGTM!Using
Query::select('library.*')correctly selects all fields from the relatedlibrarydocument while excluding the parent's direct attributes likename.
663-666: LGTM!Two-way one-to-one relationship test correctly updated with separate
Query::selectcalls for*andcity.name.
675-678: LGTM!Consistent pattern applied to
getDocumentin the two-way relationship context.src/Database/Adapter/Postgres.php (1)
1738-1748: LGTM on alias and attribute initialization.The left-side alias correctly goes through
filter()beforequote()for proper sanitization. ThesetAttributeRightcall appropriately normalizes the right attribute to its internal key.src/Database/Adapter/Mongo.php (3)
19-19: Good:QueryContextimport aligns Mongo adapter with context-driven API.
1791-1793: Nice:deleteDocuments()usesQuery::equal('$sequence', ...)and reusesbuildFilters().
2009-2058: Good: batch iteration +killCursorscleanup reduces cursor resource leak risk.src/Database/Adapter/SQL.php (4)
20-21: Good:QueryContextimport is consistent with the adapter API refactor.
1803-1808: Nice: JOIN query methods are mapped to SQL JOIN keywords viagetSQLOperator().
2305-2338:getTenantQuery(..., forceIsNull)needs careful validation with RIGHT JOIN semantics.
TheOR ... IS NULLclause can widen results significantly; please confirm this is only applied when it’s required for RIGHT JOIN correctness (and not for unrelated queries).
3228-3323:count()signature and flow look aligned with the JOIN-awarefind(), but recheck RIGHT JOIN + tenant widening.
Specifically validate combined behavior of:
($permissionsCondition OR alias._uid IS NULL)(once quoted), andgetTenantQuery(... forceIsNull: $rightJoins)
to ensure it doesn’t admit cross-tenant rows when joined side is NULL.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| case Query::TYPE_RELATION_EQUAL: | ||
| $attributeRight = $this->quote($this->filter($query->getAttributeRight())); | ||
| $aliasRight = $this->quote($query->getRightAlias()); | ||
|
|
||
| return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}"; |
There was a problem hiding this comment.
Missing filter() call on right alias.
The right alias is directly quoted without sanitization, unlike the left alias which is filtered at line 1746 before quoting. This inconsistency could lead to SQL injection or malformed queries if getRightAlias() returns unsanitized input.
🔒 Proposed fix
case Query::TYPE_RELATION_EQUAL:
$attributeRight = $this->quote($this->filter($query->getAttributeRight()));
- $aliasRight = $this->quote($query->getRightAlias());
+ $aliasRight = $this->quote($this->filter($query->getRightAlias()));
return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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}"; |
🤖 Prompt for AI Agents
In `@src/Database/Adapter/Postgres.php` around lines 1795 - 1799, The right alias
in the Query::TYPE_RELATION_EQUAL case is quoted without being sanitized; update
the code to mirror the left side by filtering the right alias before quoting
(i.e., call $this->filter($query->getRightAlias()) and then $this->quote(...)
when building $aliasRight) so getRightAlias() is sanitized prior to use in the
returned "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}" expression.
| protected function getAttributeProjection(array $selects): string | ||
| { | ||
| if (empty($selections) || \in_array('*', $selections)) { | ||
| return "{$this->quote($prefix)}.*"; | ||
|
|
||
| if (empty($selects)) { | ||
| return $this->quote(Query::DEFAULT_ALIAS).'.*'; | ||
| } | ||
|
|
||
| // Handle specific selections with spatial conversion where needed | ||
| $internalKeys = [ | ||
| '$id', | ||
| '$sequence', | ||
| '$permissions', | ||
| '$createdAt', | ||
| '$updatedAt', | ||
| ]; | ||
| $string = ''; | ||
| foreach ($selects as $select) { | ||
| if ($select->getAttribute() === '$collection') { | ||
| continue; | ||
| } | ||
|
|
||
| $selections = \array_diff($selections, [...$internalKeys, '$collection']); | ||
| $alias = $select->getAlias(); | ||
| $alias = $this->filter($alias); | ||
|
|
||
| $attribute = $this->getInternalKeyForAttribute($select->getAttribute()); | ||
|
|
||
| foreach ($internalKeys as $internalKey) { | ||
| $selections[] = $this->getInternalKeyForAttribute($internalKey); | ||
| } | ||
|
|
||
| $projections = []; | ||
| foreach ($selections as $selection) { | ||
| $filteredSelection = $this->filter($selection); | ||
| $quotedSelection = $this->quote($filteredSelection); | ||
| $projections[] = "{$this->quote($prefix)}.{$quotedSelection}"; | ||
| if ($attribute !== '*') { | ||
| $attribute = $this->filter($attribute); | ||
| $attribute = $this->quote($attribute); | ||
| } | ||
|
|
||
| $as = $select->getAs(); | ||
|
|
||
| if (!empty($as)) { | ||
| $as = ' as '.$this->quote($this->filter($as)); | ||
| } | ||
|
|
||
| if (!empty($string)) { | ||
| $string .= ', '; | ||
| } | ||
|
|
||
| $string .= "{$this->quote($alias)}.{$attribute}{$as}"; | ||
| } | ||
|
|
||
| return \implode(',', $projections); | ||
| return $string; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the structure and find the Query class
fd -t f "\.php$" | head -20Repository: utopia-php/database
Length of output: 579
🏁 Script executed:
# Find the Query class definition
rg "class Query" --type php -A 5Repository: utopia-php/database
Length of output: 1341
🏁 Script executed:
# Check for TYPE_SELECT constant
rg "TYPE_SELECT" --type phpRepository: utopia-php/database
Length of output: 2462
🏁 Script executed:
# Look at the select parameter type hint - need to find what type $selects contains
rg "getAttributeProjection" --type php -B 5 -A 15Repository: utopia-php/database
Length of output: 9828
🏁 Script executed:
# Find the getMethod() method definition
rg "function getMethod\|public function getMethod" --type php -A 3Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look at where getAttributeProjection is called and see the full context
rg "getAttributeProjection" --type php -B 20 | head -100Repository: utopia-php/database
Length of output: 5559
🏁 Script executed:
# Check how selects/queries parameters are built before being passed to getAttributeProjection
rg "getAttributeProjection\(\$" --type php -B 30 | head -150Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look at the find method in SQL.php that calls getAttributeProjection
sed -n '2200,2400p' src/Database/Adapter/SQL.phpRepository: utopia-php/database
Length of output: 4950
🏁 Script executed:
# Check Query class to understand all query types and getMethod
rg "public const TYPE_" src/Database/Query.php | head -20Repository: utopia-php/database
Length of output: 1014
🏁 Script executed:
# Look at the Query class structure more carefully
sed -n '1,150p' src/Database/Query.phpRepository: utopia-php/database
Length of output: 4872
🏁 Script executed:
# Check Mongo adapter's getAttributeProjection implementation
sed -n '1620,1680p' src/Database/Adapter/Mongo.phpRepository: utopia-php/database
Length of output: 2159
🏁 Script executed:
# Check if there are any tests showing mixed arrays being passed to these methods
rg "getAttributeProjection" --type php -A 5 tests/Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look for where find() and getDocument() methods get their queries parameter to see if mixed arrays are realistic
sed -n '2100,2250p' src/Database/Adapter/SQL.phpRepository: utopia-php/database
Length of output: 4348
🏁 Script executed:
# Check the find method more carefully to see how it processes queries before calling getAttributeProjection
rg "protected function find" src/Database/Adapter/SQL.php -A 100 | head -150Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look for where selects are extracted from queries
rg "\$selects\s*=" src/Database/Adapter/SQL.php -B 3 -A 3Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Let's look at the full find method to understand the flow
rg "public function find" src/Database/Adapter/SQL.php -A 150 | head -200Repository: utopia-php/database
Length of output: 5991
🏁 Script executed:
# Look for where the queries parameter is used before getAttributeProjection
sed -n '1900,2100p' src/Database/Adapter/SQL.phpRepository: utopia-php/database
Length of output: 8251
🏁 Script executed:
# Check how queries are processed - look for array_filter or similar operations on $queries
rg "array_filter|array_map" src/Database/Adapter/SQL.php -B 2 -A 2Repository: utopia-php/database
Length of output: 3017
🏁 Script executed:
# Let's search how the queries parameter flows through find()
rg "function find\(" src/Database/Adapter/SQL.php -A 200 | head -250Repository: utopia-php/database
Length of output: 7527
🏁 Script executed:
# Find all places that call getAttributeProjection
rg "getAttributeProjection\(" src/ --type php -B 5Repository: utopia-php/database
Length of output: 2853
🏁 Script executed:
# Check the getDocument method signature and how it's called
rg "public function getDocument" src/Database/Adapter/SQL.php -A 30Repository: utopia-php/database
Length of output: 1941
Add TYPE_SELECT guard to getAttributeProjection().
The method receives a generic array $queries parameter in getDocument() without pre-filtering by query type. If a caller passes a mixed array containing both SELECT and non-SELECT queries (filters, pagination, ordering), non-SELECT queries will be incorrectly processed as projection attributes, resulting in malformed SQL.
The pattern of defensive filtering is already established in the codebase—getSQLConditions() explicitly filters SELECT queries out of mixed arrays. Apply the same defensive check here.
Suggested guard
foreach ($selects as $select) {
+ if ($select->getMethod() !== Query::TYPE_SELECT) {
+ continue;
+ }
if ($select->getAttribute() === '$collection') {
continue;
}🤖 Prompt for AI Agents
In `@src/Database/Adapter/SQL.php` around lines 2347 - 2385,
getAttributeProjection is treating every element in the passed array as a SELECT
query; add a guard to skip non-select entries just like getSQLConditions does.
Inside getAttributeProjection (method name), before processing each $select,
check that $select->getType() === Query::TYPE_SELECT (or equivalent TYPE_SELECT
constant) and continue if not, so only SELECT queries are used to build the
projection; keep existing checks for '$collection' and alias/as handling
unchanged. Ensure you reference the same Query::TYPE_SELECT symbol used
elsewhere in the codebase to maintain consistency.
| public function decode(QueryContext $context, Document $document, array $selects = []): Document | ||
| { | ||
| $attributes = \array_filter( | ||
| $collection->getAttribute('attributes', []), | ||
| fn ($attribute) => $attribute['type'] !== self::VAR_RELATIONSHIP | ||
| ); | ||
|
|
||
| $relationships = \array_filter( | ||
| $collection->getAttribute('attributes', []), | ||
| fn ($attribute) => $attribute['type'] === self::VAR_RELATIONSHIP | ||
| ); | ||
|
|
||
| $filteredValue = []; | ||
| $internals = []; | ||
| $schema = []; | ||
|
|
||
| foreach ($relationships as $relationship) { | ||
| $key = $relationship['$id'] ?? ''; | ||
| foreach (Database::INTERNAL_ATTRIBUTES as $attribute) { | ||
| $internals[$attribute['$id']] = $attribute; | ||
| } | ||
|
|
||
| if ( | ||
| \array_key_exists($key, (array)$document) | ||
| || \array_key_exists($this->adapter->filter($key), (array)$document) | ||
| ) { | ||
| $value = $document->getAttribute($key); | ||
| $value ??= $document->getAttribute($this->adapter->filter($key)); | ||
| $document->removeAttribute($this->adapter->filter($key)); | ||
| $document->setAttribute($key, $value); | ||
| foreach ($context->getCollections() as $collection) { | ||
| foreach ($collection->getAttribute('attributes', []) as $attribute) { | ||
| $key = $attribute->getAttribute('key', $attribute->getAttribute('$id')); | ||
| $key = $this->adapter->filter($key); | ||
| $schema[$collection->getId()][$key] = $attribute->getArrayCopy(); | ||
| } | ||
| } | ||
|
|
||
| foreach ($this->getInternalAttributes() as $attribute) { | ||
| $attributes[] = $attribute; | ||
| } | ||
| foreach ($document as $key => $value) { | ||
| $alias = Query::DEFAULT_ALIAS; | ||
| $attributeKey = ''; | ||
|
|
||
| foreach ($attributes as $attribute) { | ||
| $key = $attribute['$id'] ?? ''; | ||
| $type = $attribute['type'] ?? ''; | ||
| $array = $attribute['array'] ?? false; | ||
| $filters = $attribute['filters'] ?? []; | ||
| $value = $document->getAttribute($key); | ||
| foreach ($selects as $select) { | ||
| if ($select->getAs() === $key) { | ||
| $attributeKey = $key; | ||
| $key = $select->getAttribute(); | ||
| $alias = $select->getAlias(); | ||
| break; | ||
| } | ||
|
|
||
| if ($key === '$permissions') { | ||
| continue; | ||
| if ($select->getAttribute() == $key || | ||
| $this->adapter->filter($select->getAttribute()) == $key) { | ||
| $alias = $select->getAlias(); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (\is_null($value)) { | ||
| $value = $document->getAttribute($this->adapter->filter($key)); | ||
| $collection = $context->getCollectionByAlias($alias); | ||
| if ($collection->isEmpty()) { | ||
| throw new \Exception('Invalid query: Unknown Alias context'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Throw QueryException (not generic \Exception) for unknown alias context in decode()/casting().
Right now decode()/casting() throw \Exception('Invalid query: Unknown Alias context'), which is harder for callers to handle consistently (and bypasses the existing QueryException flow used elsewhere).
Proposed fix
- if ($collection->isEmpty()) {
- throw new \Exception('Invalid query: Unknown Alias context');
- }
+ if ($collection->isEmpty()) {
+ throw new QueryException("Unknown alias context '{$alias}'");
+ }Also applies to: 8412-8459
🤖 Prompt for AI Agents
In `@src/Database/Database.php` around lines 8320 - 8360, The decode() method (and
the casting() block mentioned) currently throws a generic \Exception('Invalid
query: Unknown Alias context'); replace these throws with the existing
QueryException type used by the query flow so callers can catch query-specific
errors consistently; update both the throw in decode() and the similar throw in
casting() (and any other occurrences around lines ~8412-8459) to construct and
throw QueryException with the same message/context, and ensure the
QueryException symbol is referenced/imported correctly in this file so the new
throws compile.
# Conflicts: # composer.lock # src/Database/Adapter/Mongo.php # src/Database/Database.php # src/Database/Query.php # tests/unit/Validator/QueriesTest.php
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Database/Query.php (1)
1224-1306:groupByType()assigns the wrong fallback variables foroffsetandcursor.
$offsetand$cursorcurrently fall back to$limit, which can silently corrupt pagination.Proposed fix
case Query::TYPE_OFFSET: @@ - $offset = $values[0] ?? $limit; + $offset = $values[0] ?? $offset; break; @@ case Query::TYPE_CURSOR_AFTER: case Query::TYPE_CURSOR_BEFORE: @@ - $cursor = $values[0] ?? $limit; + $cursor = $values[0] ?? $cursor; $cursorDirection = $method === Query::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE; break;src/Database/Adapter/Mongo.php (2)
1128-1159: Projection bug:getDocument()passes mixed$queriesintogetAttributeProjection()
getAttributeProjection()assumes a list of SELECT queries, butgetDocument()passes$queries(potentially mixed: filters/orders/etc.). If any non-select query is present, projection can unintentionally exclude fields and return incomplete documents.Fix either by (a) passing only selects into
getAttributeProjection(), or (b) makinggetAttributeProjection()ignore non-TYPE_SELECTentries.Suggested guard (option b)
protected function getAttributeProjection(array $selects): array { $projection = []; if (empty($selects)) { return []; } foreach ($selects as $select) { + if ($select->getMethod() !== Query::TYPE_SELECT) { + continue; + } if ($select->getAttribute() === '$collection') { continue; } // ... } return $projection; }
2042-2074: Inconsistent result normalization between first batch and getMoreFirst batch does
new Document($this->convertStdClassToArray($record)), but getMore path doesnew Document($record)(no deep conversion). This will produce inconsistent shapes/types across pages.Also: unlike
getDocument(),find()is not applyingcastingAfter($collection, $doc), so type casting may silently regress.Unify both paths (convert + cast in both).
Proposed fix
- $doc = new Document($this->convertStdClassToArray($record)); + $doc = new Document($this->convertStdClassToArray($record)); if ($removeSequence) { $doc->removeAttribute('$sequence'); } + $doc = $this->castingAfter($collection, $doc); $found[] = $doc; @@ - $doc = new Document($record); + $doc = new Document($this->convertStdClassToArray($record)); if ($removeSequence) { $doc->removeAttribute('$sequence'); } + $doc = $this->castingAfter($collection, $doc); $found[] = $doc;
🤖 Fix all issues with AI agents
In `@src/Database/Adapter/SQL.php`:
- Around line 374-379: In getDocument() (where the SQL SELECT is built) the
WHERE clause mixes a quoted alias with an unquoted column
(`{$this->quote($alias)}._uid`); change the column to be quoted as well so
identifier quoting is consistent — replace `{$this->quote($alias)}._uid` with
`{$this->quote($alias)}.{$this->quote('_uid')}` (keep the surrounding use of
getAttributeProjection, getSQLTable, quote($alias) and getTenantQuery as-is).
In `@src/Database/Database.php`:
- Around line 7858-7867: The comment in find() contradicts the code: either
update the comment to state that when no unique order on $id or $sequence is
found the code appends Query::orderAsc() to $orders, or change the logic so the
default order is only appended when not in a join context (e.g., add/check an
$isJoin or similar flag before pushing Query::orderAsc()); locate the
$uniqueOrderBy variable, the foreach over $orders, and the Query::orderAsc()
append and implement one of these fixes to make behavior and comment consistent.
- Around line 8383-8388: The early-return in the casting method is inverted:
change the condition in Database::casting(QueryContext $context, Document
$document, array $selects = []) so the method returns immediately when the
adapter reports it DOES support casting (adapter->getSupportForCasting() ===
true), and only performs manual casting when getSupportForCasting() is false
(e.g., Mongo). Locate the casting method and flip the boolean check on
$this->adapter->getSupportForCasting(), preserving the existing return
$document; behavior and ensuring manual casting logic runs for adapters that do
not support casting.
♻️ Duplicate comments (11)
src/Database/Database.php (4)
5794-5798: Post-write paths still skipcasting()beforedecode()(type normalization inconsistency).
This appears to repeat the earlier review concern:updateDocument()andupdateDocuments()decode without casting, andupsertDocumentsWithIncrease()decodes only in one branch.Also applies to: 6024-6030, 6847-6851
5950-5957: Bulk update/delete: either auto-add required system selects or document the requirement.
These runtime checks (“Missing required attribute … in select query”) are easy to trip if callers use any explicit selects. Consider auto-injecting$permissionsand$sequencesimilarly to other system-select logic.Also applies to: 7632-7640
8327-8330: ThrowQueryException(not generic\\Exception) for unknown alias context indecode()/casting().
Keeps error handling consistent with the rest of the query pipeline.Also applies to: 8426-8429
8869-8875: Fix unused$idAddedand avoid auto-adding$idwhen selects are empty (same projection-mode risk).
Also matches the prior guidance: only inject$idfor relationship-bearing collections, and don’t capture the unused flag. Based on learnings, keep$idinjection relationship-only and remove$idAdded.Proposed fix
- if (!empty($relationships)) { - [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); - } + if (!empty($relationships) && !empty($queries)) { // only when in projection mode + [$queries] = QueryContext::addSelect($queries, Query::select('$id', system: true)); + }src/Database/Query.php (2)
79-127:Query::TYPESis now out of sync withisMethod()(join/relation types missing).
If any validators/consumers rely onQuery::TYPESfor “allowed methods”, they’ll reject join/relation queries even thoughparseQuery()accepts them viaisMethod().Proposed fix
public const TYPES = [ @@ self::TYPE_ELEM_MATCH, - self::TYPE_REGEX + self::TYPE_REGEX, + self::TYPE_RELATION_EQUAL, + self::TYPE_INNER_JOIN, + self::TYPE_LEFT_JOIN, + self::TYPE_RIGHT_JOIN, ];Also applies to: 407-461
525-568:parseQuery()should validate join metadata + nested elements to avoid leakingTypeError.
Right nowattributeRight/alias/aliasRight/as/collectioncan be non-strings and nestedvalues[]can be non-arrays, which will throw aTypeError(or argument type error) instead of aQueryException.Proposed fix
public static function parseQuery(array $query): self { $method = $query['method'] ?? ''; $attribute = $query['attribute'] ?? ''; $attributeRight = $query['attributeRight'] ?? ''; $values = $query['values'] ?? []; $alias = $query['alias'] ?? ''; $aliasRight = $query['aliasRight'] ?? ''; $as = $query['as'] ?? ''; $collection = $query['collection'] ?? ''; if (!\is_string($method)) { throw new QueryException('Invalid query method. Must be a string, got ' . \gettype($method)); } @@ if (!\is_array($values)) { throw new QueryException('Invalid query values. Must be an array, got ' . \gettype($values)); } + + foreach ([ + 'attributeRight' => $attributeRight, + 'alias' => $alias, + 'aliasRight' => $aliasRight, + 'as' => $as, + 'collection' => $collection, + ] as $key => $value) { + if (!\is_string($value)) { + throw new QueryException("Invalid query {$key}. Must be a string, got " . \gettype($value)); + } + } - if (\in_array($method, self::LOGICAL_TYPES) || \in_array($method, self::JOINS_TYPES)) { + if (\in_array($method, self::LOGICAL_TYPES, true) || \in_array($method, self::JOINS_TYPES, true)) { foreach ($values as $index => $value) { + if (!\is_array($value)) { + throw new QueryException("Invalid query values[{$index}]. Must be an array, got " . \gettype($value)); + } $values[$index] = self::parseQuery($value); } }src/Database/Adapter/Postgres.php (1)
1745-1755: FiltergetRightAlias()before quoting inTYPE_RELATION_EQUAL(injection/malformed identifier risk).
Left alias is filtered, but right alias isn’t.Proposed fix
case Query::TYPE_RELATION_EQUAL: $attributeRight = $this->quote($this->filter($query->getAttributeRight())); - $aliasRight = $this->quote($query->getRightAlias()); + $aliasRight = $this->quote($this->filter($query->getRightAlias())); return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}";Also applies to: 1802-1807
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
4279-4283: Decode input likely should be the encoded$result, not the original$document(still).
This matches the earlier review note; if the intent is to testdecodeindependently, an inline comment would help.Proposed change
$context = new QueryContext(); $context->add($collection); - $result = $database->decode($context, $document); + $result = $database->decode($context, $result);src/Database/Adapter/Mongo.php (1)
1910-2024: Guard againstORDER_RANDOMbefore callinggetOrderDirection()
This code still calls$order->getOrderDirection()for every order. If anorderRandom()query reaches Mongo, this can throw and take down the request path; either explicitly reject it here (clear “not supported”) or ensure it’s filtered earlier.src/Database/Adapter/SQL.php (2)
2357-2395:getAttributeProjection()must ignore non-SELECT queries (especially becausegetDocument()passes$queries)Without a
TYPE_SELECTguard, any non-select query in$queriescan be rendered into the SELECT list, producing invalid SQL or wrong projections.Proposed fix
foreach ($selects as $select) { + if ($select->getMethod() !== Query::TYPE_SELECT) { + continue; + } if ($select->getAttribute() === '$collection') { continue; } // ... }
3091-3124: RIGHT JOIN permission clause uses unquoted{$alias}._uid IS NULL+ verifyskipAuth()keying
- The
OR {$alias}._uid IS NULLbranch should use the same quoting helper asgetSQLPermissionsCondition().- Also,
skipAuth()is called with filtered collection IDs; please confirmQueryContext::addSkipAuth()uses the same (filtered vs raw) keys, otherwise skip-auth may silently fail.Also applies to: 3253-3286
🧹 Nitpick comments (5)
src/Database/Database.php (1)
8739-8765: Replacemd5(serialize($selects))with a normalized string using stable Query fields.Object serialization includes internal implementation details (like
$onArray,$isObjectAttribute) that aren't part of the public API. If the Query class evolves, the serialization hash changes even when the logical selection hasn't, causing unnecessary cache misses. Instead, build the hash from stable getter methods—getAlias(),getAttribute(),getAs(),getMethod(),isSystem()—to create a deterministic key tied only to the relevant selection parameters.tests/e2e/Adapter/Scopes/DocumentTests.php (2)
3197-3214:Query::select('director')addition is correct for the new per-field select API.
Optional: consider also asserting the selected field is present (e.g.,director) since you already assertnameis absent.
3792-3930: Find-select tests are updated correctly for per-field selects + internal-field visibility.
This gives solid coverage across user fields and system fields ($id,$sequence,$createdAt,$updatedAt,$permissions).
Optional: the repeated “internal keys not present” assertions could be extracted into a tiny helper to reduce duplication/noise.src/Database/Adapter.php (1)
829-841: Avoid the “mega-signature” by introducing a single query DTO (optional)
find()now has many parallel arrays ($selects/$filters/$joins/$vectors/$orderQueries), which is hard to evolve and easy to call incorrectly. Consider aQueryPlan/FindParamsvalue object (or moving more of these intoQueryContext) so future additions don’t keep widening the interface.src/Database/Adapter/Mongo.php (1)
2704-2737: Projection builder should reusegetInternalKeyForAttribute()and ignore non-selectsRight now this re-implements the internal attribute mapping via an inline
match, and (as above) it will happily treat non-select queries as projection entries.
- Prefer
$this->getInternalKeyForAttribute($attribute)for consistency.- Add a
TYPE_SELECTguard to make it safe when callers pass mixed query arrays.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Database/Adapter.phpsrc/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/Pool.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Database.phpsrc/Database/Query.phpsrc/Database/Validator/Queries/V2.phptests/e2e/Adapter/Scopes/DocumentTests.phptests/e2e/Adapter/Scopes/ObjectAttributeTests.phptests/e2e/Adapter/Scopes/SchemalessTests.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Database/Adapter/MariaDB.php
- src/Database/Validator/Queries/V2.php
🧰 Additional context used
🧠 Learnings (7)
📚 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/ObjectAttributeTests.phpsrc/Database/Adapter.phptests/e2e/Adapter/Scopes/SchemalessTests.phptests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Adapter/SQL.phpsrc/Database/Query.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:
tests/e2e/Adapter/Scopes/ObjectAttributeTests.phpsrc/Database/Adapter/Postgres.phptests/e2e/Adapter/Scopes/SchemalessTests.phptests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Adapter/SQL.phpsrc/Database/Query.phpsrc/Database/Database.php
📚 Learning: 2025-07-01T11:31:37.438Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 613
File: src/Database/Adapter/Postgres.php:1254-1319
Timestamp: 2025-07-01T11:31:37.438Z
Learning: In PostgreSQL adapter methods like getUpsertStatement, complexity for database-specific SQL generation is acceptable when the main business logic is properly separated in the parent SQL adapter class, following the adapter pattern where each database adapter handles its own SQL syntax requirements.
Applied to files:
src/Database/Adapter/Postgres.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-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-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/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 (6)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (1)
src/Database/Query.php (2)
Query(8-1614)select(800-803)
src/Database/Adapter/Postgres.php (4)
src/Database/Query.php (5)
getAttributeRight(296-299)getAttribute(264-267)getAlias(286-289)Query(8-1614)getRightAlias(291-294)src/Database/Adapter/SQL.php (1)
getInternalKeyForAttribute(2397-2409)src/Database/Adapter.php (1)
quote(491-491)src/Database/Adapter/MariaDB.php (1)
quote(1921-1924)
src/Database/Adapter.php (2)
src/Database/Adapter/Mongo.php (2)
find(1910-2099)getAttributeProjection(2704-2737)src/Database/Adapter/SQL.php (2)
find(2994-3224)getAttributeProjection(2357-2395)
tests/e2e/Adapter/Scopes/SchemalessTests.php (1)
src/Database/Query.php (3)
Query(8-1614)select(800-803)getAttribute(264-267)
src/Database/Adapter/Mongo.php (4)
src/Database/QueryContext.php (3)
QueryContext(8-136)getMainCollection(40-43)skipAuth(83-94)src/Database/Adapter/Postgres.php (1)
renameAttribute(533-548)src/Database/Adapter/SQL.php (4)
renameAttribute(310-327)getInternalKeyForAttribute(2397-2409)find(2994-3224)count(3238-3333)src/Database/Document.php (3)
removeAttribute(287-293)getId(63-66)Document(12-470)
src/Database/Database.php (4)
src/Database/Validator/Queries/V2.php (1)
V2(23-719)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/Query.php (10)
Query(8-1614)getSelectQueries(1082-1087)select(800-803)join(1015-1018)getCollection(306-309)groupByType(1224-1306)limit(843-846)offset(854-857)isNested(1313-1320)isJoin(1325-1328)src/Database/Adapter.php (3)
count(865-870)filter(1272-1281)find(829-841)
🪛 PHPMD (2.15.0)
src/Database/Adapter/Mongo.php
1919-1919: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
1920-1920: Avoid unused parameters such as '$vectors'. (undefined)
(UnusedFormalParameter)
2174-2174: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
src/Database/Database.php
8873-8873: 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). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (16)
src/Database/Database.php (3)
34-34: Validator alias swap looks fine, but ensure “V2” is the intended class for this file’s query-validation contract.
No issues in this hunk by itself.
4959-4983:applySelectFiltersToDocuments()looks good for post-fetch projection.
Nice to see internal attributes preserved and wildcard handled early.
4266-4274: This review comment is incorrect. TheQueryContext::addSelect()method does not flip empty selects into projection mode.When
$selectsis empty, theforeachloop at line 108 does not execute (no SELECT queries to iterate), so$foundremainsfalse. The conditionif ($found && $merge)at line 125 evaluates to false, causing the method to return the original empty array unchanged withfalsefor the second value. No$permissionsselect is added when the array is empty.The current code at lines 4292-4295 is safe and the proposed guard (
if (!empty($selects))) is unnecessary.Likely an incorrect or invalid review comment.
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (1)
219-239: Per-fieldQuery::select()updates look correct and keep intent readable.Also applies to: 532-577, 836-866
tests/e2e/Adapter/Scopes/SchemalessTests.php (1)
176-201: Per-fieldQuery::select()updates are consistent with the new selection API.Also applies to: 919-944
src/Database/Adapter/Pool.php (3)
271-285: LGTM!The updated
findmethod signature properly adopts the context-first approach withQueryContext, and the delegation viafunc_get_args()correctly forwards all parameters to the underlying adapter.
292-299: LGTM!The updated
countmethod signature correctly adopts the context-first approach withQueryContextand separates filters from joins for cleaner query building.
526-529: LGTM!The simplified signature for
getAttributeProjectionremoving the$prefixparameter aligns with the QueryContext-based alias resolution approach.tests/e2e/Adapter/Scopes/DocumentTests.php (6)
24-24: Import looks good; keep it consistent with other test files.
QueryContextis now referenced later intestEncodeDecode, so this inclusion is justified.
1441-1477: Select-projection assertions match the new “explicit selects only” behavior.
Good coverage for “internal fields excluded unless explicitly selected” (and$collectionstill present).
1481-1514: Nice guard test: reject non-select queries in getDocument “select” context.
This is useful to prevent silent misuse of the API; the assertions on message + exception type are sufficient.
1999-2004: Good addition:notSearchbehavior exercised alongsidesearch.
The expected count (4 out of 6) aligns with the seeded dataset in this test.
5020-5034: Bulk delete select lists updated cleanly (spread +array_map(Query::select)pattern).
This reads well and keeps the test intent clear while adapting toQuery::select(string $field).Also applies to: 5231-5246, 5254-5274
5509-5512:Query::select('*')is explicitly supported as a special-case "all fields" selector.The codebase confirms
'*'has defined semantics: when used withQuery::select('*'), it returns all fields including internal fields (mapped in adapters as_uid,_id,_tenant,_createdAt,_updatedAt,_permissions). This is intentionally implemented in both Mongo and SQL adapters and is widely used throughout tests. No risk of confusion or footgun here.src/Database/Adapter.php (1)
1260-1264:getAttributeProjection(array $selects)signature change looks consistentGood direction aligning projection on “select queries” rather than generic selections.
src/Database/Adapter/Mongo.php (1)
720-737:renameAttribute()correctly maps internal attributes before$renameUsing
getInternalKeyForAttribute()for$old/$newavoids mismatches for$id/$sequence/....
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| abstract public function count( | ||
| QueryContext $context, | ||
| ?int $max, | ||
| array $filters, | ||
| array $joins, | ||
| ): int; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check PHP version requirements in composer.json
if [ -f composer.json ]; then
cat composer.json | jq '.require.php // .config.platform.php // empty'
fiRepository: utopia-php/database
Length of output: 70
🏁 Script executed:
# Find the exact location and content of the method
cat -n src/Database/Adapter.php | sed -n '860,875p'Repository: utopia-php/database
Length of output: 514
🏁 Script executed:
# Search for all implementations of count() method to check override compatibility
rg -B 2 -A 5 'function count\(' --type phpRepository: utopia-php/database
Length of output: 2421
Fix override compatibility violation in Mongo.php adapter
The abstract method requires array $filters and array $joins as non-optional parameters, but the Mongo.php adapter adds default values (= []) to both. This violates the Liskov Substitution Principle—callers of the abstract contract cannot safely use the concrete implementation with fewer arguments.
Ensure all concrete adapters match the abstract signature exactly: remove the = [] defaults from Mongo.php's count() method, or add them to all implementations consistently if the abstract method contract changes to make these parameters optional.
The trailing comma after $joins is valid in PHP 8.1+ (project requirement) and can remain.
| public function count(QueryContext $context, ?int $max, array $filters = [], array $joins = []): int | ||
| { |
There was a problem hiding this comment.
count() signature likely incompatible with abstract method (defaults on required params)
Abstract Adapter::count() requires $filters/$joins (no defaults). Here they’re optional (= []), which can trigger a fatal “must be compatible” error.
Either remove defaults here, or make the abstract signature optional and update all implementations to match.
Proposed fix (minimal)
-public function count(QueryContext $context, ?int $max, array $filters = [], array $joins = []): int
+public function count(QueryContext $context, ?int $max, array $filters, array $joins): int📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function count(QueryContext $context, ?int $max, array $filters = [], array $joins = []): int | |
| { | |
| public function count(QueryContext $context, ?int $max, array $filters, array $joins): int | |
| { |
🧰 Tools
🪛 PHPMD (2.15.0)
2174-2174: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
| $sql = " | ||
| SELECT {$this->getAttributeProjection($selections, $alias)} | ||
| SELECT {$this->getAttributeProjection($queries)} | ||
| FROM {$this->getSQLTable($name)} AS {$this->quote($alias)} | ||
| WHERE {$this->quote($alias)}.{$this->quote('_uid')} = :_uid | ||
| WHERE {$this->quote($alias)}._uid = :_uid | ||
| {$this->getTenantQuery($collection, $alias)} | ||
| "; |
There was a problem hiding this comment.
Quote _uid consistently in getDocument() WHERE clause
This mixes quoted alias with unquoted column ({$this->quote($alias)}._uid). It’s low risk, but it breaks consistency with the rest of the adapter and could bite if identifier quoting rules change.
Proposed fix
- WHERE {$this->quote($alias)}._uid = :_uid
+ WHERE {$this->quote($alias)}.{$this->quote('_uid')} = :_uid 🤖 Prompt for AI Agents
In `@src/Database/Adapter/SQL.php` around lines 374 - 379, In getDocument() (where
the SQL SELECT is built) the WHERE clause mixes a quoted alias with an unquoted
column (`{$this->quote($alias)}._uid`); change the column to be quoted as well
so identifier quoting is consistent — replace `{$this->quote($alias)}._uid` with
`{$this->quote($alias)}.{$this->quote('_uid')}` (keep the surrounding use of
getAttributeProjection, getSQLTable, quote($alias) and getTenantQuery as-is).
| // Add equality conditions for previous attributes | ||
| for ($j = 0; $j < $i; $j++) { | ||
| $prevOriginal = $orderAttributes[$j]; | ||
| $prevQuery = $orderQueries[$j]; | ||
| $prevOriginal = $prevQuery->getAttribute(); | ||
| $prevAttr = $this->filter($this->getInternalKeyForAttribute($prevOriginal)); | ||
|
|
||
| $bindName = ":cursor_{$j}"; | ||
| $binds[$bindName] = $cursor[$prevOriginal]; | ||
|
|
||
| $conditions[] = "{$this->quote($alias)}.{$this->quote($prevAttr)} = {$bindName}"; | ||
| $conditions[] = "{$this->quote($orderAlias)}.{$this->quote($prevAttr)} = {$bindName}"; | ||
| } |
There was a problem hiding this comment.
Cursor pagination bug when ordering spans multiple aliases
Inside the cursorWhere builder, previous-order equality conditions use the current $orderAlias:
$conditions[] = "{$this->quote($orderAlias)}.... = :cursor_{$j}";…but prevQuery can have a different alias (especially with joins). This yields incorrect pagination predicates.
Use $prevQuery->getAlias() for the $j conditions.
Proposed fix
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($orderAlias)}.{$this->quote($prevAttr)} = {$bindName}";
+ $conditions[] = "{$this->quote($prevAlias)}.{$this->quote($prevAttr)} = {$bindName}";
}| $uniqueOrderBy = false; | ||
| foreach ($orderAttributes as $order) { | ||
| if ($order === '$id' || $order === '$sequence') { | ||
| foreach ($orders as $order) { | ||
| if ($order->getAttribute() === '$id' || $order->getAttribute() === '$sequence') { | ||
| $uniqueOrderBy = true; | ||
| } | ||
| } | ||
|
|
||
| if ($uniqueOrderBy === false) { | ||
| $orderAttributes[] = '$sequence'; | ||
| $orders[] = Query::orderAsc(); // In joins we should not add a default order, we should validate when using a cursor we should have a unique order | ||
| } |
There was a problem hiding this comment.
find(): comment contradicts behavior (default order is still added).
Either remove/update the comment or adjust logic; currently it says “we should not add a default order” but then appends Query::orderAsc().
🤖 Prompt for AI Agents
In `@src/Database/Database.php` around lines 7858 - 7867, The comment in find()
contradicts the code: either update the comment to state that when no unique
order on $id or $sequence is found the code appends Query::orderAsc() to
$orders, or change the logic so the default order is only appended when not in a
join context (e.g., add/check an $isJoin or similar flag before pushing
Query::orderAsc()); locate the $uniqueOrderBy variable, the foreach over
$orders, and the Query::orderAsc() append and implement one of these fixes to
make behavior and comment consistent.
| public function casting(QueryContext $context, Document $document, array $selects = []): Document | ||
| { | ||
| if (!$this->adapter->getSupportForCasting()) { | ||
| return $document; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the getSupportForCasting() method definition and its documentation
rg -n "function getSupportForCasting\(\)" src/Database/ -A5 -B2Repository: utopia-php/database
Length of output: 1788
🏁 Script executed:
#!/bin/bash
# Check all adapter implementations to see what they return
rg -n "getSupportForCasting" src/Database/Adapter/ -B3 -A2Repository: utopia-php/database
Length of output: 1105
🏁 Script executed:
#!/bin/bash
# Look at the full casting() method implementation to understand its purpose
rg -n "public function casting\(" src/Database/Database.php -A50Repository: utopia-php/database
Length of output: 2109
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about what getSupportForCasting means
rg -n "SupportForCasting" src/Database/ -B3 -A3Repository: utopia-php/database
Length of output: 2022
The casting() early-return condition is inverted.
The method currently skips casting work when getSupportForCasting() is false (like Mongo), but it should skip when it's true (like SQL). SQL adapters return true and handle type casting natively in their drivers, so the Database class should not duplicate that work. Mongo returns false precisely because it doesn't support automatic type casting at the adapter level, making it the case where Database needs to perform manual casting.
Fix
- if (!$this->adapter->getSupportForCasting()) {
+ if ($this->adapter->getSupportForCasting()) {
return $document;
}🤖 Prompt for AI Agents
In `@src/Database/Database.php` around lines 8383 - 8388, The early-return in the
casting method is inverted: change the condition in
Database::casting(QueryContext $context, Document $document, array $selects =
[]) so the method returns immediately when the adapter reports it DOES support
casting (adapter->getSupportForCasting() === true), and only performs manual
casting when getSupportForCasting() is false (e.g., Mongo). Locate the casting
method and flip the boolean check on $this->adapter->getSupportForCasting(),
preserving the existing return $document; behavior and ensuring manual casting
logic runs for adapters that do not support casting.
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/Query.php (1)
468-494: Fix instance method calls to staticisSpatialQuerymethod.The code has two instances where
isSpatialQuery()is called as an instance method but it's now a static method requiring a$methodparameter. These will fail at runtime:
Line 131 (
src/Database/Validator/Query/Filter.php): Change$query->isSpatialQuery()toQuery::isSpatialQuery($method)(the$methodparameter is already available).Line 488 (
src/Database/Validator/Query/Filter.php): Change$value->isSpatialQuery()toQuery::isSpatialQuery($value->getMethod())(where$valueis a Query instance).
♻️ Duplicate comments (1)
src/Database/Query.php (1)
529-550: New metadata fields lack type validation.While
$methodand$attributeare validated (lines 536-546), the new fields ($attributeRight,$alias,$aliasRight,$as,$collection) are not type-checked. If malformed JSON provides a non-string value (e.g.,"alias": []), the constructor will throw aTypeErrorinstead of aQueryException, making error handling inconsistent.♻️ Suggested validation
$stringFields = ['attributeRight' => $attributeRight, 'alias' => $alias, 'aliasRight' => $aliasRight, 'as' => $as, 'collection' => $collection]; foreach ($stringFields as $name => $value) { if (!\is_string($value)) { throw new QueryException("Invalid query {$name}. Must be a string, got " . \gettype($value)); } }
🧹 Nitpick comments (5)
src/Database/Query.php (5)
191-192: Missing getter for$systemproperty.The
$systemproperty is set via the constructor andselect()factory (line 802), but there's nogetSystem(): boolgetter. Other metadata properties like$alias,$collection,$asall have corresponding getters. For consistency, consider adding:public function isSystem(): bool { return $this->system; }
347-375: UseQueryExceptioninstead of generic\Exception.
getCursorDirection()(line 357) andgetOrderDirection()(line 374) throw generic\Exception. For consistency with the rest of this class (e.g.,parse(),parseQuery()), useQueryException:♻️ Suggested fix
- 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');
1015-1029:join()is redundant withinnerJoin().
join()andinnerJoin()are identical implementations. Consider either:
- Removing
join()to avoid confusion, or- Documenting that
join()is an alias forinnerJoin()If kept as an alias, add a docblock note:
/** * Alias for innerJoin() * `@see` innerJoin() */
1177-1189: Method naming inconsistency:getCursorQueriesreturns singleQuery|null.
getCursorQueriesreturns?Query(a single query or null), but the plural "Queries" suffix suggests it returns an array like the otherget*Queriesmethods. Consider renaming togetCursorQueryfor clarity:♻️ Suggested rename
- public static function getCursorQueries(array $queries): ?Query + public static function getCursorQuery(array $queries): ?Query
1325-1333: Minor API inconsistency betweenisJoin()andisFilter().
isJoin()is an instance method (likeisNested()), whileisFilter()is static. This creates an inconsistent API:
$query->isJoin()Query::isFilter($method)Consider making
isFilteran instance method or adding a staticisJoinType(string $method)variant for symmetry. Not blocking, but worth considering for API consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Query.phptests/e2e/Adapter/Base.php
🧰 Additional context used
🧠 Learnings (2)
📚 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/Base.phpsrc/Database/Query.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/Query.php
⏰ 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/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Unit Test
🔇 Additional comments (4)
tests/e2e/Adapter/Base.php (1)
12-12: LGTM!The new
JoinsTeststrait is correctly imported and integrated into the base e2e test class. The import is properly placed in alphabetical order, and the trait usage follows the existing pattern in the class. This cleanly enables the new join-related tests to execute across all adapter implementations.Also applies to: 27-27
src/Database/Query.php (3)
147-182: Logical typesTYPE_ANDandTYPE_ORinFILTER_TYPESmay cause unexpected behavior.
TYPE_ANDandTYPE_OR(lines 178-179) are logical container types, not filter predicates. Including them inFILTER_TYPESmeansgetFilterQueries()will return nested logical queries alongside regular filters, which may not be the expected behavior for callers expecting only leaf filter predicates.If this is intentional (to support recursive filter traversal), consider documenting this behavior. Otherwise, these should be in
LOGICAL_TYPESonly.
451-459: LGTM!Join and relation-equal types are now correctly included in
isMethod(), addressing the serialization/parsing concern from previous reviews.
600-632: LGTM!Serialization now properly includes the new metadata fields and handles join child queries correctly. The conditional inclusion of aliases (only when non-default) keeps payloads minimal.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary by CodeRabbit
New Features
Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.