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
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.
# 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.
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.
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.
Summary by CodeRabbit
New Features
Changes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.