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: 3
🤖 Fix all issues with AI agents
In `@src/Database/Validator/Queries/V2.php`:
- Around line 704-709: The array attribute validation currently throws an
exception for methods not in [Query::TYPE_CONTAINS, Query::TYPE_NOT_CONTAINS,
Query::TYPE_IS_NULL, Query::TYPE_IS_NOT_NULL]; update that allowed-method list
in the block that checks "$array && !in_array($method, ...)" to also include
Query::TYPE_EXISTS and Query::TYPE_NOT_EXISTS so array attributes permit
EXISTS/NOT_EXISTS queries, preserving parity with the Filter.php validator.
- Line 5: Remove the unused import Utopia\CLI\Console from the top of V2.php; it
is only referenced in commented-out debug code (around the block that references
Console on lines ~407-409), so delete the use statement (use Utopia\CLI\Console)
to satisfy the linter and avoid dead code while leaving the commented debug
lines intact if you want to keep them.
In `@tests/unit/Validator/QueryTest.php`:
- Around line 375-387: The comment says this complex multi-key nested structure
is disallowed but the test uses assertTrue; change the expectation to match the
comment by replacing assertTrue(...) with assertFalse(...) for the
validator->isValid([ Query::contains('meta', [...]) ]) assertion so the test
asserts rejection of that structure.
♻️ Duplicate comments (5)
src/Database/Query.php (1)
532-570: Validate new metadata fields inparseQuery()for consistent error handling.The new fields (
alias,aliasRight,attributeRight,as,collection) extracted at lines 532-537 are passed directly to the constructor without type validation. If a serialized query contains a non-string value (e.g.,"alias": []), the constructor's strictstringparameter types will throw aTypeErrorinstead of aQueryException.For consistency with the existing validation pattern (lines 539-553), add type checks:
if (!is_string($alias)) { throw new QueryException('Invalid query alias. Must be a string, got ' . \gettype($alias)); } // Similar for aliasRight, attributeRight, as, collectionThis keeps
parse()/parseQuery()failures predictable and avoids leaking raw type errors from malformed JSON.src/Database/Validator/Queries/V2.php (1)
198-202: Join-scope alias validation may reject valid non-relation queries.This check runs for all queries when
$scope === 'joins', butgetRightAlias()returns'main'(the default) for non-relation queries like filters or orders. Since'main'is in$joinsAliasOrder, this should work, but the check feels overly broad.Consider narrowing to only
TYPE_RELATION_EQUALqueries where both aliases are semantically relevant:- if ($scope === 'joins') { - if (!in_array($query->getAlias(), $this->joinsAliasOrder) || !in_array($query->getRightAlias(), $this->joinsAliasOrder)) { + if ($scope === 'joins' && $query->getMethod() === Query::TYPE_RELATION_EQUAL) { + if (!in_array($query->getAlias(), $this->joinsAliasOrder, true) || !in_array($query->getRightAlias(), $this->joinsAliasOrder, true)) {This prevents false positives if future changes alter default alias behavior.
src/Database/Validator/Queries.php (1)
1-177: Delete this file or fix the pipeline failure.The entire class is commented out, which:
- Triggers the linter error (
blank_line_after_opening_tag- the<?phptag should be followed by a blank line, but instead there's a comment)- Leaves a "zombie" file with no functional code
Since the class has been superseded by the V2 validator, delete this file entirely. If the code must be retained for reference, version control already preserves history.
src/Database/Validator/Query/Filter.php (1)
1-509: Delete this file instead of keeping commented-out code.The entire
Filtervalidator class is commented out. This triggers the linter error and provides no value—version control preserves history if you need to reference the old implementation.tests/unit/Validator/QueriesTest.php (1)
1-119: Delete this test file if tests have been migrated to the new validator.The entire test class is commented out. If test coverage has been migrated to
DocumentsQueriesTest.phpor similar files using the newV2validator, delete this file rather than leaving commented-out code. This also resolves the linter error.
🧹 Nitpick comments (4)
src/Database/Query.php (3)
47-77: Join types not added toTYPESarray - intentional or oversight?The new constants
TYPE_INNER_JOIN,TYPE_LEFT_JOIN,TYPE_RIGHT_JOIN, andTYPE_RELATION_EQUALare added toisMethod()(lines 454-457) but are not included in the publicTYPESarray (lines 79-127).If joins are intended to be first-class query methods that consumers can enumerate via
Query::TYPES, add them:self::TYPE_OR, 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, ];If joins are internal-only and should not appear in the public API enumeration, document this decision.
184-184: Minor: Inconsistent self-reference style inFILTER_TYPES.All other entries use
self::TYPE_*, but this line usesQuery::TYPE_ELEM_MATCH. For consistency:- Query::TYPE_ELEM_MATCH, + self::TYPE_ELEM_MATCH,
350-378: Consider usingQueryExceptioninstead of generic\Exception.
getCursorDirection()andgetOrderDirection()throw\Exceptionwhen called on an invalid query type. For consistency with the rest of the Query class (which usesQueryExceptionfor parsing errors), consider:- throw new \Exception('Invalid method: Get cursor direction on "'.$this->method.'" Query'); + throw new QueryException('Invalid method: Get cursor direction on "'.$this->method.'" Query');This provides a consistent exception hierarchy for callers to catch.
tests/unit/Validator/QueryTest.php (1)
93-98: Inconsistent attribute definition formeta.The
metaattribute is missing fields (size,required,signed,filters) that all other attributes define. While this may work forVAR_OBJECT, the inconsistency could cause unexpected behavior if validators or other code paths expect these fields.Consider adding the missing fields for consistency
[ '$id' => 'meta', 'key' => 'meta', 'type' => Database::VAR_OBJECT, 'array' => false, + 'size' => 0, + 'required' => false, + 'signed' => false, + 'filters' => [], ]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Database/Query.phpsrc/Database/Validator/Queries.phpsrc/Database/Validator/Queries/V2.phpsrc/Database/Validator/Query/Filter.phptests/unit/Validator/QueriesTest.phptests/unit/Validator/QueryTest.php
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Validator/Queries/V2.php
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Validator/Queries/V2.phptests/unit/Validator/QueriesTest.phptests/unit/Validator/QueryTest.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/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/Validator/Queries/V2.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Validator/Queries/V2.php
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
Repo: utopia-php/database PR: 733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
src/Database/Validator/Queries/V2.php
🧬 Code graph analysis (1)
src/Database/Validator/Queries/V2.php (7)
src/Database/Document.php (3)
Document(12-470)getId(63-66)getArrayCopy(423-458)src/Database/Query.php (9)
Query(8-1617)__construct(210-245)getAttribute(267-270)parse(506-519)isNested(1316-1323)getMethod(259-262)getFilterQueries(1198-1201)isSpatialQuery(471-488)isVectorQuery(494-497)src/Database/QueryContext.php (4)
QueryContext(8-136)__construct(25-27)getCollections(32-35)getCollectionByAlias(45-63)src/Database/Validator/Alias.php (5)
Alias(7-70)getDescription(15-18)isArray(54-57)getType(66-69)isValid(26-45)src/Database/Validator/AsQuery.php (6)
AsQuery(7-84)__construct(14-18)getDescription(24-27)isArray(68-71)getType(80-83)isValid(35-59)src/Database/Validator/Query/Cursor.php (1)
Cursor(10-56)src/Database/Validator/Sequence.php (1)
Sequence(9-60)
🪛 GitHub Actions: Linter
src/Database/Validator/Query/Filter.php
[error] 1-1: Pint: PSR-12 issue detected - blank_line_after_opening_tag.
src/Database/Validator/Queries.php
[error] 1-1: Pint: PSR-12 issue detected - blank_line_after_opening_tag in file.
src/Database/Validator/Queries/V2.php
[error] 1-1: Pint: PSR-12 issue detected - statement_indentation.
tests/unit/Validator/QueriesTest.php
[error] 1-1: Pint: PSR-12 issue detected - blank_line_after_opening_tag.
⏰ 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 (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Postgres)
🔇 Additional comments (12)
src/Database/Query.php (3)
803-806: Well-designedselect()factory with alias support.The updated
select()method properly acceptsalias,as, andsystemparameters, enabling both user-facing and internal select queries. Thesystemflag is useful for internal queries that shouldn't be user-modifiable.
1012-1059: Join factory methods are well-structured.The
join(),innerJoin(),leftJoin(),rightJoin(), andrelationEqual()factories provide a clean API for constructing join queries with proper aliasing and collection references.
1081-1210: Useful query extraction helpers.The new static methods (
getSelectQueries,getJoinQueries,getLimitQueries,getLimitQuery,getOffsetQueries,getOffsetQuery,getOrderQueries,getCursorQueries,getFilterQueries,getVectorQueries) provide a clean API for extracting specific query types from a query array. These properly clone queries to avoid mutation issues.src/Database/Validator/Queries/V2.php (4)
96-108: System attributes$idand$sequencetyped asVAR_STRINGinstead ofVAR_ID.The synthetic attributes added in the constructor use
Database::VAR_STRINGfor$idand$sequence:$attributes[] = new Document([ '$id' => '$id', 'key' => '$id', 'type' => Database::VAR_STRING, // Should this be VAR_ID? 'array' => false, ]);In
validateValues(),Database::VAR_IDtriggers theSequencevalidator (line 585). If$idand$sequenceare typed asVAR_STRING, they'll use theTextvalidator instead.Verify this is intentional. If
$id/$sequenceshould useSequencevalidation:$attributes[] = new Document([ '$id' => '$id', 'key' => '$id', - 'type' => Database::VAR_STRING, + 'type' => Database::VAR_ID, 'array' => false, ]);
437-488: Well-structured attribute existence validation.The
validateAttributeExist()method properly:
- Allows internal attributes (
$permissions,$collection)- Validates alias context exists
- Handles dot-notation for nested/relationship attributes
- Returns early for schemaless mode (line 469-471)
- Prevents queries on encrypted attributes
- Restricts ordering by nested attributes
715-738: Fulltext index validation correctly handles bothSEARCHandNOT_SEARCH.The method now validates both
TYPE_SEARCHandTYPE_NOT_SEARCHrequire a fulltext index on the queried attribute, addressing the previous review concern.
745-762: Clean recursive relation existence check.The
isRelationExist()method correctly traverses nested queries to ensure joins contain at least oneTYPE_RELATION_EQUALquery referencing the joined collection's alias.tests/unit/Validator/QueryTest.php (5)
10-15: LGTM!The imports and property declaration are properly updated to use the new
QueryContextandV2validator (aliased asDocumentsValidator), maintaining clear naming while adopting the new validation architecture.
101-117: LGTM!Clean context setup pattern: attributes are properly converted to
Documentobjects, wrapped in a collection, and added to theQueryContext. This provides a solid foundation for context-aware validation testing.
150-153: LGTM!The
Query::select()usage correctly reflects the new per-field selection API, with separate calls for each attribute instead of a single multi-field call.
129-129: LGTM!Validator instantiation is correctly updated to use
DocumentsValidatorwith theQueryContextparameter. The pattern is consistent across all test methods.
268-273: Method namegetCursorQueries(plural) is misleading: it returns a singleQueryobject.The return type is
?Query, notarray<Query>. The implementation filters cursor queries but intentionally returns only the first result viareturn $queries[0]. While similar methods likegetFilterQueries()andgetVectorQueries()returnarray<Query>, this method's plural naming contradicts its singular return type, which is confusing for API users.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/unit/Validator/QueriesTest.php`:
- Around line 3-120: Remove the entirely commented-out test file
Tests\Unit\Validator\QueriesTest.php: the file contains a fully commented class
QueriesTest (methods setUp, tearDown, testEmptyQueries, testInvalidMethod,
testInvalidValue, testValid and references to Queries, Cursor, Filter, Limit,
Offset, Order, Query) and should be deleted from the repository; delete the file
and commit the removal (no code changes required inside other files).
♻️ Duplicate comments (5)
src/Database/Validator/Queries/V2.php (3)
5-5: Remove unusedConsoleimport.The
Utopia\CLI\Consoleimport is unused in the active code. The only references are in commented-out debug code (lines 407-409). Remove this import to satisfy linters and reduce dead code.-use Utopia\CLI\Console;
198-202: Join-scope alias check is overly broad and will reject valid non-relation queries.This block runs for every query when
$scope === 'joins', but non-relation queries (filters, orders) typically have an emptygetRightAlias(). This causesin_array('', $this->joinsAliasOrder)to fail, incorrectly rejecting valid filter queries inside joins.Restrict this check to relation queries only:
- if ($scope === 'joins') { - if (!in_array($query->getAlias(), $this->joinsAliasOrder) || !in_array($query->getRightAlias(), $this->joinsAliasOrder)) { + 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.'); } }
704-709: MissingTYPE_EXISTSandTYPE_NOT_EXISTSin array attribute allowed methods.The array attribute validation is missing
TYPE_EXISTSandTYPE_NOT_EXISTSfrom the allowed methods list. The originalFilter.phpvalidator included them (line 268). Add them to maintain parity:if ( $array && - !in_array($method, [Query::TYPE_CONTAINS, Query::TYPE_NOT_CONTAINS, Query::TYPE_IS_NULL, Query::TYPE_IS_NOT_NULL]) + !in_array($method, [Query::TYPE_CONTAINS, Query::TYPE_NOT_CONTAINS, Query::TYPE_IS_NULL, Query::TYPE_IS_NOT_NULL, Query::TYPE_EXISTS, Query::TYPE_NOT_EXISTS]) ) {src/Database/Validator/Query/Filter.php (1)
3-510: Remove commented-out code entirely.The entire
Filtervalidator class (507 lines) remains commented out. If this validator has been superseded by V2, delete the file entirely rather than keeping commented code:
- Creates confusion about what code is active
- Makes the codebase harder to maintain
- Can be recovered from version control if needed
src/Database/Validator/Queries.php (1)
3-178: Breaking change:Queriesclass is now unavailable.The
Utopia\Database\Validator\Queriesclass is entirely commented out. Any external code instantiating this class will encounter a fatal error. Either:
- Remove the file outright and document this as a major-version breaking change, or
- Provide a deprecated stub that throws on construction or proxies to V2
Leaving commented-out code is the worst option for API consumers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Validator/Queries.phpsrc/Database/Validator/Queries/V2.phpsrc/Database/Validator/Query/Filter.phptests/unit/Validator/QueriesTest.php
🧰 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/unit/Validator/QueriesTest.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-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Validator/Queries/V2.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-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Validator/Queries/V2.php
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
Repo: utopia-php/database PR: 733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
src/Database/Validator/Queries/V2.php
🧬 Code graph analysis (1)
src/Database/Validator/Queries/V2.php (8)
src/Database/Query.php (3)
Query(8-1617)parse(506-519)isNested(1316-1323)src/Database/QueryContext.php (3)
QueryContext(8-136)getCollections(32-35)getCollectionByAlias(45-63)src/Database/Validator/Alias.php (5)
Alias(7-70)getDescription(15-18)isArray(54-57)getType(66-69)isValid(26-45)src/Database/Validator/AsQuery.php (5)
AsQuery(7-84)getDescription(24-27)isArray(68-71)getType(80-83)isValid(35-59)src/Database/Validator/Query/Cursor.php (1)
Cursor(10-51)src/Database/Validator/Query/Limit.php (1)
Limit(9-58)src/Database/Validator/Query/Offset.php (2)
Offset(9-54)isValid(25-53)src/Database/Validator/Sequence.php (1)
Sequence(9-60)
🔇 Additional comments (6)
src/Database/Validator/Queries/V2.php (6)
88-128: Schema construction looks correct; system attributes properly added.The constructor correctly clones collections to avoid mutating the original context, and appends all required system attributes (
$id,$sequence,$createdAt,$updatedAt) to each collection's schema. The schema indexing by collection ID and attribute key is appropriate for the lookup patterns used throughout the validator.
166-416: TheisValidmethod structure is well-organized with comprehensive query type coverage.The switch-case structure provides clear handling for each query type with appropriate validation calls. The nested query recursion via
$this->isValid()(instead of the previousself::isValid()) correctly maintains instance state. Error handling with the try-catch block and message propagation is appropriate.
437-488: Attribute existence validation handles schemaless mode correctly.The early return at line 470 for schemaless mode (when
!$this->supportForAttributes) is consistent with the validation pattern invalidateValues(). The nested attribute handling via dot notation and the relationship type check are properly implemented.
526-710: Value validation logic is comprehensive with proper type handling.The
validateValuesmethod correctly handles all attribute types including spatial, vector, object, and relationship types. The vector query limit (single vector per request) at lines 639-643 is a sensible constraint. The relationship validation at lines 670-692 properly restricts queries on virtual relationship attributes.
715-738: Fulltext index validation correctly handles both SEARCH and NOT_SEARCH.The guard at line 717 now properly includes both
TYPE_SEARCHandTYPE_NOT_SEARCH, ensuring consistent fulltext index requirements for both query types.
778-806: Object query validation is well-implemented.The
isValidObjectQueryValuesmethod correctly prevents ambiguous mixed-key arrays (both integer and string keys at the same level) while allowing valid object query structures. The recursive validation ensures nested structures are also validated.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Adapter/Mongo.php (1)
1128-1159: Fix projection:getAttributeProjection()must ignore non-SELECT queries (and getDocument shouldn’t pass mixed$queries).
Right nowgetDocument()passes$queriesintogetAttributeProjection(), which can include filters/orders; that can unintentionally restrict returned fields.Proposed fix
--- a/src/Database/Adapter/Mongo.php +++ b/src/Database/Adapter/Mongo.php @@ - $projections = $this->getAttributeProjection($queries); + $selects = array_values(array_filter( + $queries, + static fn (Query $q) => $q->getMethod() === Query::TYPE_SELECT + )); + $projections = $this->getAttributeProjection($selects); @@ protected function getAttributeProjection(array $selects): array { $projection = []; @@ foreach ($selects as $select) { + if ($select->getMethod() !== Query::TYPE_SELECT) { + continue; + } if ($select->getAttribute() === '$collection') { continue; }Also applies to: 2704-2737
♻️ Duplicate comments (5)
src/Database/Validator/Queries/V2.php (1)
197-201: Join-scope alias check is overly strict for non-relation queries.This block runs for all nested queries when
$scope === 'joins', but non-relation queries (filters, orders) typically have an emptyrightAlias. Thein_array($query->getRightAlias(), $this->joinsAliasOrder)check will fail for these valid queries since an empty string is not in the alias order.Restrict this check to relation queries only:
- if ($scope === 'joins') { - if (!in_array($query->getAlias(), $this->joinsAliasOrder) || !in_array($query->getRightAlias(), $this->joinsAliasOrder)) { + 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.'); } }src/Database/Adapter/Mongo.php (1)
1973-1983: Guard againstorderRandombefore callinggetOrderDirection()(still unsafe).
This matches a previously raised concern: Mongo reportsgetSupportForOrderRandom() === false, andgetOrderDirection()can throw for random-order queries.Proposed defensive guard
--- a/src/Database/Adapter/Mongo.php +++ b/src/Database/Adapter/Mongo.php @@ foreach ($orderQueries as $i => $order) { + if ($order->getMethod() === Query::TYPE_ORDER_RANDOM) { + throw new DatabaseException('Random ordering is not supported by the Mongo adapter.'); + } $attribute = $order->getAttribute(); $originalAttribute = $attribute; @@ $direction = $order->getOrderDirection();src/Database/Adapter/SQL.php (3)
374-379: Quote_uidconsistently in getDocument() WHERE clause.
Currently mixes quoted alias with unquoted column.Proposed fix
--- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ - WHERE {$this->quote($alias)}._uid = :_uid + WHERE {$this->quote($alias)}.{$this->quote('_uid')} = :_uid
2357-2395: Add TYPE_SELECT guard to getAttributeProjection() (projection currently treats all queries as selects).
This is especially important sincegetDocument()passes$queries(mixed types) into this method.Proposed fix
--- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ foreach ($selects as $select) { + if ($select->getMethod() !== Query::TYPE_SELECT) { + continue; + } if ($select->getAttribute() === '$collection') { continue; }
3117-3124: RIGHT JOIN permission NULL-branch should use consistent quoting.
Currently uses{$alias}._uid IS NULLwhile other branches quote identifiers.Proposed fix
--- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ $permissionsCondition = $this->getSQLPermissionsCondition($name, $roles, $alias, $forPermission); if ($rightJoins) { - $permissionsCondition = "($permissionsCondition OR {$alias}._uid IS NULL)"; + $permissionsCondition = sprintf( + '(%s OR %s.%s IS NULL)', + $permissionsCondition, + $this->quote($alias), + $this->quote('_uid'), + ); } @@ $permissionsCondition = $this->getSQLPermissionsCondition($name, $roles, $alias); if ($rightJoins) { - $permissionsCondition = "($permissionsCondition OR {$alias}._uid IS NULL)"; + $permissionsCondition = sprintf( + '(%s OR %s.%s IS NULL)', + $permissionsCondition, + $this->quote($alias), + $this->quote('_uid'), + ); }Also applies to: 3279-3286
🧹 Nitpick comments (5)
src/Database/Validator/Queries/V2.php (1)
665-687: UseDatabase::VAR_RELATIONSHIPconstant instead of string literal.Line 665 uses the string literal
'relationship'while line 557 correctly usesDatabase::VAR_RELATIONSHIP. This inconsistency could lead to bugs if the constant value ever changes.- if ($attribute['type'] === 'relationship') { + if ($attribute['type'] === Database::VAR_RELATIONSHIP) {tests/unit/Validator/QueryTest.php (1)
93-98: Consider adding required fields to the 'meta' attribute definition.The 'meta' attribute definition is missing fields like
size,required,signed, andfiltersthat other attributes have. While this may work, it's inconsistent with the other attribute definitions in the test setup.[ '$id' => 'meta', 'key' => 'meta', 'type' => Database::VAR_OBJECT, 'array' => false, + 'size' => 0, + 'required' => false, + 'signed' => false, + 'filters' => [], ]src/Database/Adapter/Mongo.php (2)
1936-1940: Don’tfilter()the collection id used as the QueryContext skipAuth key.
If QueryContext stores skip-auth by raw collection id, filtering here can cause skip-auth to be missed and permissions to be enforced unexpectedly.Proposed fix
--- a/src/Database/Adapter/Mongo.php +++ b/src/Database/Adapter/Mongo.php @@ - $skipAuth = $context->skipAuth($this->filter($collection->getId()), $forPermission, $this->authorization); + $skipAuth = $context->skipAuth($collection->getId(), $forPermission, $this->authorization); @@ - $skipAuth = $context->skipAuth($this->filter($collection->getId()), $permission, $this->authorization); + $skipAuth = $context->skipAuth($collection->getId(), $permission, $this->authorization);Also applies to: 2176-2196
2174-2260: count(): avoid swallowing Mongo exceptions (returning 0 hides outages/bugs).
Returning0onMongoExceptioncan turn “DB failure” into “no results”, which is hard to detect and can break callers.Proposed fix
--- a/src/Database/Adapter/Mongo.php +++ b/src/Database/Adapter/Mongo.php @@ - } catch (MongoException $e) { - return 0; + } catch (MongoException $e) { + throw $this->processException($e); }src/Database/Adapter/SQL.php (1)
3021-3034: VerifyorderRandomhandling: guard before callinggetOrderDirection().
Right nowgetOrderDirection()is called unconditionally; if order-random is represented as a distinct query method, this can still throw.Proposed fix
--- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ foreach ($orderQueries as $i => $order) { $orderAlias = $order->getAlias(); $attribute = $order->getAttribute(); $originalAttribute = $attribute; + + if ($order->getMethod() === Query::TYPE_ORDER_RANDOM) { + $orders[] = $this->getRandomOrder(); + continue; + } @@ $direction = $order->getOrderDirection(); - - // Handle random ordering specially - if ($direction === Database::ORDER_RANDOM) { - $orders[] = $this->getRandomOrder(); - continue; - }#!/bin/bash # Verify how order-random is represented and whether getOrderDirection can be called safely. rg -n "function getOrderDirection" -n src/Database/Query.php -A40 rg -n "TYPE_ORDER_RANDOM|ORDER_RANDOM" src/Database/Query.php
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Adapter/Mongo.phpsrc/Database/Adapter/Pool.phpsrc/Database/Adapter/SQL.phpsrc/Database/Validator/Queries/V2.phptests/unit/Validator/QueryTest.php
🧰 Additional context used
🧠 Learnings (7)
📚 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.phptests/unit/Validator/QueryTest.php
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Adapter/SQL.phptests/unit/Validator/QueryTest.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/Validator/Queries/V2.phpsrc/Database/Adapter/SQL.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-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Adapter/Mongo.php
📚 Learning: 2025-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.phpsrc/Database/Adapter/SQL.php
📚 Learning: 2025-08-14T06:35:30.429Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 661
File: tests/e2e/Adapter/Scopes/SpatialTests.php:180-186
Timestamp: 2025-08-14T06:35:30.429Z
Learning: Query::distance method in Utopia\Database\Query expects an array of values parameter, where each value is a [geometry, distance] pair. So the correct format is Query::distance('attribute', [[[lat, lng], distance]]) where the outer array contains the values and each value is [geometry, distance].
Applied to files:
src/Database/Validator/Queries/V2.php
🧬 Code graph analysis (4)
src/Database/Adapter/SQL.php (3)
src/Database/QueryContext.php (3)
QueryContext(8-136)getMainCollection(40-43)skipAuth(83-94)src/Database/Query.php (4)
Query(8-1617)select(803-806)join(1018-1021)getCollection(309-312)src/Database/Validator/Authorization.php (1)
getRoles(105-108)
tests/unit/Validator/QueryTest.php (2)
src/Database/QueryContext.php (2)
QueryContext(8-136)add(68-76)src/Database/Validator/Queries/V2.php (2)
V2(23-809)getDescription(135-138)
src/Database/Adapter/Mongo.php (2)
src/Database/QueryContext.php (3)
QueryContext(8-136)getMainCollection(40-43)skipAuth(83-94)src/Database/Adapter/SQL.php (3)
getInternalKeyForAttribute(2397-2409)getAttributeProjection(2357-2395)find(2994-3224)
src/Database/Adapter/Pool.php (4)
src/Database/Database.php (3)
Database(38-9499)find(7793-7945)count(8043-8122)src/Database/QueryContext.php (1)
QueryContext(8-136)src/Database/Adapter/Mongo.php (3)
find(1910-2099)count(2174-2260)getAttributeProjection(2704-2737)src/Database/Adapter.php (3)
find(829-841)count(865-870)getAttributeProjection(1263-1263)
🪛 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)
⏰ 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). (11)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Mirror)
🔇 Additional comments (13)
src/Database/Validator/Queries/V2.php (3)
56-128: LGTM! Constructor properly builds per-collection schema with system attributes.The constructor correctly:
- Clones collections from the context to avoid mutating the original
- Adds required system attributes (
$id,$sequence,$createdAt,$updatedAt)- Builds an indexed schema for efficient attribute lookups
780-808: LGTM! Object query validation prevents ambiguous nested structures.The
isValidObjectQueryValuesmethod correctly identifies and rejects arrays that mix integer and string keys (e.g.,['a' => [1, 'b' => [212]]]), which would be ambiguous for object queries.
717-740: LGTM! Fulltext index validation now covers both SEARCH and NOT_SEARCH.The guard correctly checks for both
TYPE_SEARCHandTYPE_NOT_SEARCH, ensuring consistent index requirements for negated search queries.tests/unit/Validator/QueryTest.php (3)
10-11: LGTM! Test file updated for V2 validator with QueryContext.The imports and property type correctly reflect the migration to the new context-based validation approach.
Also applies to: 15-15
346-388: LGTM! Object attribute validation tests cover key scenarios.The new
testObjectAttributemethod correctly tests:
- Valid shape with separate top-level keys (lines 354-362)
- Invalid mixed integer/string keys in same array (lines 365-373)
- Valid complex nested structure with consistent key types (lines 376-387)
The assertions align with the comments describing expected behavior.
258-274: Test correctly verifiesgetCursorQueriesreturns the first cursor type.The
getCursorQueriesmethod filters the input queries usinggetByType, which preserves the original array order by iterating sequentially. SincecursorBeforeappears beforecursorAfterin the test's queries array, it is correctly returned as the first element, matching the assertion that expectsTYPE_CURSOR_BEFORE.src/Database/Adapter/Pool.php (2)
241-269: Signatures look aligned with the new Adapter contract; delegation remains correct.
find()/count()now match the context-first shape and correctly forward all args to the pooled adapter.
496-499:getAttributeProjection()delegation is fine.
Pool stays a pure pass-through wrapper with the updated signature.src/Database/Adapter/Mongo.php (3)
720-737: renameAttribute(): internal-key mapping + transaction options usage looks good.
1825-1828: deleteDocuments(): switching to$sequence-based filter construction is consistent with the new query model.
2044-2051: Normalize getMore batch documents the same way as firstBatch (stdClass handling).
First batch usesconvertStdClassToArray(...), later batches don’t—this can yield inconsistent document shapes across pages.Proposed fix
--- a/src/Database/Adapter/Mongo.php +++ b/src/Database/Adapter/Mongo.php @@ - $doc = new Document($record); + $doc = new Document($this->convertStdClassToArray($record)); if ($removeSequence) { $doc->removeAttribute('$sequence'); }Also applies to: 2067-2074
⛔ Skipped due to learnings
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.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.src/Database/Adapter/SQL.php (2)
1770-1813: JOIN operator mapping via getSQLOperator(): good direction.
Centralizing join keyword generation makes join assembly cleaner.
2315-2348:getTenantQuery(..., forceIsNull: bool)is a reasonable hook for RIGHT JOIN semantics.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
# Conflicts: # src/Database/Validator/Query/Filter.php
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 (2)
4179-4203: Relationship population can be skipped when only system selects are present.
processRelationshipQueries()adds$id, and you later add$permissions. That makesempty($selects)false even when the caller passed no selects, so relationship population is skipped unless nested selections exist. This effectively disables default relationship resolution for collections with relationships.Consider tracking explicit user selects before system additions (and use that in the guard), and apply the same logic in
find().🔧 Suggested fix (apply similarly in find())
- $selects = Query::getSelectQueries($queries); + $selects = Query::getSelectQueries($queries); + $hasExplicitSelects = !empty($selects); @@ - if (!$this->inBatchRelationshipPopulation && $this->resolveRelationships && !empty($relationships) && (empty($selects) || !empty($nestedSelections))) { + if ( + !$this->inBatchRelationshipPopulation + && $this->resolveRelationships + && !empty($relationships) + && (!$hasExplicitSelects || !empty($nestedSelections)) + ) {Also applies to: 4266-4271
4865-4889: Select aliases (as) aren’t honored in the post-filtering path.
applySelectFiltersToDocuments()keeps attributes bygetAttribute()only. If a select usesas, the alias field is dropped and the original key is retained, which diverges from normal select behavior (especially in relationship batch population).🔧 Suggested fix
- $attributesToKeep = []; + $attributesToKeep = []; + $aliasMap = []; - foreach ($selectQueries as $selectQuery) { - $attributesToKeep[$selectQuery->getAttribute()] = true; - } + foreach ($selectQueries as $selectQuery) { + $attr = $selectQuery->getAttribute(); + $as = $selectQuery->getAs() ?? $attr; + $attributesToKeep[$as] = true; + $aliasMap[$attr] = $as; + } @@ - foreach ($documents as $doc) { + foreach ($documents as $doc) { + foreach ($aliasMap as $attr => $as) { + if ($as !== $attr && $doc->offsetExists($attr)) { + $doc->setAttribute($as, $doc->getAttribute($attr)); + $doc->removeAttribute($attr); + } + } $allKeys = \array_keys($doc->getArrayCopy()); foreach ($allKeys as $attrKey) {src/Database/Adapter/Mongo.php (1)
2042-2073: Ensure consistent stdClass-to-array conversion across cursor batches.The first batch uses
convertStdClassToArray(), butgetMorebatches don’t, producing mixed shapes across the result set. Apply the same conversion for all batches.🐛 Proposed fix
- $doc = new Document($record); + $doc = new Document($this->convertStdClassToArray($record));
♻️ Duplicate comments (7)
src/Database/Validator/Queries/V2.php (1)
187-191: VerifygetRightAlias()return value for non-relation queries in join scope.This check runs for all queries within join scope, but non-relation queries (filters, orders) may not have a meaningful
rightAlias. IfgetRightAlias()returns an empty string for such queries,in_array('', $this->joinsAliasOrder)will fail and incorrectly reject valid queries.The previous review suggested narrowing this check to
TYPE_RELATION_EQUALqueries only. If that wasn't applied, consider:if ($scope === 'joins') { - if (!in_array($query->getAlias(), $this->joinsAliasOrder) || !in_array($query->getRightAlias(), $this->joinsAliasOrder)) { + // Only validate alias references for relation queries that actually use rightAlias + 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.'); } }#!/bin/bash # Check what getRightAlias returns for non-relation queries ast-grep --pattern $'class Query { $$$ getRightAlias($$$) { $$$ } $$$ }'src/Database/Database.php (1)
8777-8782: Drop unused$idAddedfromprocessRelationshipQueries().
$idAddedis never used; PHPMD already flags it. This can be removed by only capturing the updated$queriesfromQueryContext::addSelect(...). Based on learnings, this should stay relationship-only and avoid the unused local.🔧 Suggested fix
- 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)); + }src/Database/Adapter/Mongo.php (1)
1973-1980: Still missing guard forTYPE_ORDER_RANDOMbeforegetOrderDirection().This matches the prior review note; if random order can reach this adapter, it will still throw.
src/Database/Adapter/SQL.php (4)
374-378: Quote_uidconsistently ingetDocument()WHERE.
The column is currently unquoted while the alias is quoted; keep identifier quoting consistent to avoid edge cases with reserved words or quoting changes.♻️ Suggested fix
- WHERE {$this->quote($alias)}._uid = :_uid + WHERE {$this->quote($alias)}.{$this->quote('_uid')} = :_uid
2370-2405: Guard projection builder against non‑SELECT queries.
getAttributeProjection()is used with$queriesingetDocument(). If the array is mixed, non‑SELECT queries can generate invalid projection SQL. Add a method check to skip non‑SELECT entries.♻️ Suggested fix
foreach ($selects as $select) { + if ($select->getMethod() !== Query::TYPE_SELECT) { + continue; + } if ($select->getAttribute() === '$collection') { continue; }
3106-3111: EnsureskipAuth()uses the same key format asQueryContext::addSkipAuth().
Filtering the collection id beforeskipAuth()may prevent matches if the context stores raw ids.Also applies to: 3268-3273
3132-3135: Quote alias/column in right‑join NULL permission clause.
TheOR {$alias}._uid IS NULLbranch mixes unquoted identifiers with quoted ones.♻️ Suggested fix
- if ($rightJoins) { - $permissionsCondition = "($permissionsCondition OR {$alias}._uid IS NULL)"; - } + if ($rightJoins) { + $permissionsCondition = sprintf( + '(%s OR %s.%s IS NULL)', + $permissionsCondition, + $this->quote($alias), + $this->quote('_uid'), + ); + }Also applies to: 3294-3297
🧹 Nitpick comments (2)
src/Database/Validator/Queries/V2.php (2)
77-79: Minor typo in comment.The comment has a grammatical error.
/** - * Since $context includes Documents , clone if original data is changes. + * Since $context includes Documents, clone if original data is changed. */
659-681: UseDatabase::VAR_RELATIONSHIPconstant instead of string literal.Line 659 uses the string
'relationship'while line 547 and line 604 useDatabase::VAR_RELATIONSHIP. This inconsistency could cause subtle bugs if the constant value ever changes.♻️ Suggested fix
- if ($attribute['type'] === 'relationship') { + if ($attribute['type'] === Database::VAR_RELATIONSHIP) {
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Query.php (1)
1264-1280: Copy-paste bugs: fallback values use$limitinstead of correct variables.Lines 1270 and 1279 incorrectly fall back to
$limitinstead of$offsetand$cursorrespectively. While the null-coalescing operator makes these fallbacks rarely triggered (values should always exist for valid queries), the code is misleading and could cause subtle bugs if a malformed query slips through.Proposed fix
case Query::TYPE_OFFSET: // Keep the 1st offset encountered and ignore the rest if ($offset !== null) { break; } - $offset = $values[0] ?? $limit; + $offset = $values[0] ?? null; break; case Query::TYPE_CURSOR_AFTER: case Query::TYPE_CURSOR_BEFORE: // Keep the 1st cursor encountered and ignore the rest if ($cursor !== null) { break; } - $cursor = $values[0] ?? $limit; + $cursor = $values[0] ?? null; $cursorDirection = $method === Query::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE; break;
🤖 Fix all issues with AI agents
In `@tests/unit/Validator/QueryTest.php`:
- Around line 268-275: Replace the unsafe use of reset() and the wrong
assertion: instead of $cursor = reset($queries) (which can be false), assign
$cursor = $queries[0] after asserting the array has two elements; and change the
incorrect assertNotEquals on $queries[1]->getMethod() to
assertEquals($queries[1]->getMethod(), Query::TYPE_CURSOR_AFTER) so the second
query is asserted to be the cursor-after type; keep references to
Query::getCursorQueries, $cursor, getMethod, Query::TYPE_CURSOR_BEFORE and
Query::TYPE_CURSOR_AFTER to locate the lines to change.
🧹 Nitpick comments (1)
src/Database/Query.php (1)
350-378: UseQueryExceptioninstead of generic\Exceptionfor consistency.The
getCursorDirection()andgetOrderDirection()methods throw\Exception, but the rest of the class usesQueryExceptionfor error handling. This inconsistency could make error handling more difficult for consumers.Suggested fix
public function getCursorDirection(): string { if ($this->method === self::TYPE_CURSOR_AFTER) { return Database::CURSOR_AFTER; } if ($this->method === self::TYPE_CURSOR_BEFORE) { return Database::CURSOR_BEFORE; } - throw new \Exception('Invalid method: Get cursor direction on "'.$this->method.'" Query'); + throw new QueryException('Invalid method: Get cursor direction on "'.$this->method.'" Query'); } public function getOrderDirection(): string { if ($this->method === self::TYPE_ORDER_ASC) { return Database::ORDER_ASC; } if ($this->method === self::TYPE_ORDER_DESC) { return Database::ORDER_DESC; } if ($this->method === self::TYPE_ORDER_RANDOM) { return Database::ORDER_RANDOM; } - throw new \Exception('Invalid method: Get order direction on "'.$this->method.'" Query'); + throw new QueryException('Invalid method: Get order direction on "'.$this->method.'" Query'); }
tests/unit/Validator/QueryTest.php
Outdated
| $queries = Query::getCursorQueries($queries); | ||
| $cursor = reset($queries); | ||
|
|
||
| $this->assertEquals(count($queries), 2); | ||
| $this->assertInstanceOf(Query::class, $queries[0]); | ||
| $this->assertEquals($cursor->getMethod(), Query::TYPE_CURSOR_BEFORE); | ||
| $this->assertNotEquals($queries[1]->getMethod(), Query::TYPE_CURSOR_AFTER); | ||
| } |
There was a problem hiding this comment.
**Fix static analysis error and incorrect assertion logic.**Two issues need to be addressed:
-
Pipeline failure:
reset()returns the value of the first array element, orfalseif the array is empty. Static analysis detects that$cursorcould befalse, causing thegetMethod()call on line 273 to fail. -
Logic error: Line 274 uses
assertNotEqualsto check that$queries[1]->getMethod()is NOT equal toTYPE_CURSOR_AFTER. However,$queries[1]should be thecursorAfterquery (based on input order), so its method SHOULD equalTYPE_CURSOR_AFTER. This assertion will fail at runtime.
Proposed fix
$queries = Query::getCursorQueries($queries);
- $cursor = reset($queries);
$this->assertEquals(count($queries), 2);
$this->assertInstanceOf(Query::class, $queries[0]);
- $this->assertEquals($cursor->getMethod(), Query::TYPE_CURSOR_BEFORE);
- $this->assertNotEquals($queries[1]->getMethod(), Query::TYPE_CURSOR_AFTER);
+ $this->assertEquals($queries[0]->getMethod(), Query::TYPE_CURSOR_BEFORE);
+ $this->assertEquals($queries[1]->getMethod(), Query::TYPE_CURSOR_AFTER);🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 273-273: Cannot call method getMethod() on Utopia\Database\Query|false.
🤖 Prompt for AI Agents
In `@tests/unit/Validator/QueryTest.php` around lines 268 - 275, Replace the
unsafe use of reset() and the wrong assertion: instead of $cursor =
reset($queries) (which can be false), assign $cursor = $queries[0] after
asserting the array has two elements; and change the incorrect assertNotEquals
on $queries[1]->getMethod() to assertEquals($queries[1]->getMethod(),
Query::TYPE_CURSOR_AFTER) so the second query is asserted to be the cursor-after
type; keep references to Query::getCursorQueries, $cursor, getMethod,
Query::TYPE_CURSOR_BEFORE and Query::TYPE_CURSOR_AFTER to locate the lines to
change.
# Conflicts: # src/Database/Query.php # tests/unit/Validator/QueryTest.php
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/Query.php (1)
1069-1080: Breaking change:getByTypevisibility reduced to protected.Changing
getByTypefrompublictoprotectedis a breaking API change. External consumers and tests that call this method directly will fail. The pipeline failures confirm this affects the test suite.If this is intentional, consider:
- Providing public alternatives (which you've done with
getCursorQueries,getSelectQueries, etc.)- Documenting this breaking change in release notes
The test file needs to be updated to use the new public methods instead.
tests/unit/Validator/QueryTest.php (1)
258-324: Pipeline failure: Calling protected methodQuery::getByType().Lines 266 and 288 call
Query::getByType()which is nowprotected, causing the pipeline to fail. The test is meant to verify the behavior of filtering queries by type, but must use the public API.Since this test specifically validates the clone vs reference behavior of
getByType, you could:
- Remove the direct
getByTypecalls and rely on the specialized public methods- Keep one test using
getCursorQueriesto verify the same behaviorProposed fix using public getCursorQueries method
public function testQueryGetByType(): void { $queries = [ Query::equal('key', ['value']), Query::cursorBefore(new Document([])), Query::cursorAfter(new Document([])), ]; - $queries1 = Query::getByType($queries, [Query::TYPE_CURSOR_AFTER, Query::TYPE_CURSOR_BEFORE]); + // Test with clone (default behavior) + $queries1 = Query::getCursorQueries($queries); $this->assertCount(2, $queries1); foreach ($queries1 as $query) { $this->assertEquals(true, in_array($query->getMethod(), [Query::TYPE_CURSOR_AFTER, Query::TYPE_CURSOR_BEFORE])); } $cursor = reset($queries1); $this->assertInstanceOf(Query::class, $cursor); $cursor->setValue(new Document(['$id' => 'hello1'])); $query1 = $queries[1]; $this->assertEquals(Query::TYPE_CURSOR_BEFORE, $query1->getMethod()); $this->assertInstanceOf(Document::class, $query1->getValue()); $this->assertTrue($query1->getValue()->isEmpty()); // Cursor Document is not updated /** * Using reference $queries2 => $queries */ - $queries2 = Query::getByType($queries, [Query::TYPE_CURSOR_AFTER, Query::TYPE_CURSOR_BEFORE], false); + $queries2 = Query::getCursorQueries($queries, false); $cursor = reset($queries2); $this->assertInstanceOf(Query::class, $cursor);
🧹 Nitpick comments (2)
src/Database/Query.php (2)
350-378: Direction helpers could throw unclear exceptions.
getCursorDirection()andgetOrderDirection()throw generic\Exceptionwith a message, but the rest of the class usesQueryException. Consider usingQueryExceptionfor consistency, or at minimum add@throwsdocblocks.Suggested improvement for exception consistency
+ /** + * `@throws` QueryException + */ public function getCursorDirection(): string { if ($this->method === self::TYPE_CURSOR_AFTER) { return Database::CURSOR_AFTER; } if ($this->method === self::TYPE_CURSOR_BEFORE) { return Database::CURSOR_BEFORE; } - throw new \Exception('Invalid method: Get cursor direction on "'.$this->method.'" Query'); + throw new QueryException('Invalid method: Get cursor direction on "'.$this->method.'" Query'); } + /** + * `@throws` QueryException + */ public function getOrderDirection(): string { // ... similar change for the throw statement }
532-570: New metadata fields extracted but not type-validated.The new fields (
alias,aliasRight,as,collection,attributeRight) are extracted from the decoded array but not validated to be strings before passing to the constructor. If malformed JSON provides a non-string value (e.g.,"alias": []), it will cause aTypeErrorinstead of aQueryException, which is inconsistent with the error handling formethod,attribute, andvalues.Add type validation for new metadata fields
$alias = $query['alias'] ?? ''; $aliasRight = $query['aliasRight'] ?? ''; $as = $query['as'] ?? ''; $collection = $query['collection'] ?? ''; + foreach (['alias' => $alias, 'aliasRight' => $aliasRight, 'as' => $as, 'collection' => $collection, 'attributeRight' => $attributeRight] as $key => $value) { + if (!\is_string($value)) { + throw new QueryException("Invalid query {$key}. Must be a string, got " . \gettype($value)); + } + } + if (!\is_string($method)) {
Summary by CodeRabbit
New Features
Refactor
Tests
Breaking Changes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.