Sync 3.x #778
Sync 3.x #778
Conversation
Added hashing for the key in postgres if more than 63
Add operator support for relationship arrays
Update limit message
# Conflicts: # src/Database/Database.php
📝 WalkthroughWalkthroughAdds EXISTS and NOT_EXISTS query types and end-to-end support: query constructors and validation, MongoDB Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QueryAPI as Query API
participant Validator
participant DatabaseCore as Database Core
participant Adapter
participant DB as Data Store
Client->>QueryAPI: Query::exists(['a','b'])
QueryAPI-->>Client: Query(TYPE_EXISTS)
Client->>Validator: validate(query)
Validator->>Validator: Map TYPE_EXISTS → METHOD_TYPE_FILTER
Validator->>Validator: Validate attributes (skip values)
Validator-->>Client: Valid
Client->>DatabaseCore: find(query)
DatabaseCore->>DatabaseCore: Preprocess (relationships/operators)
DatabaseCore->>Adapter: buildFilter(query)
alt Adapter is Mongo
Adapter->>Adapter: Map TYPE_EXISTS → `$exists`
Adapter->>Adapter: Build OR: {a:{$exists:true}} OR {b:{$exists:true}}
Adapter-->>DatabaseCore: Filter
else Adapter is SQL/Postgres
Adapter-->>DatabaseCore: Throw DatabaseException (exists unsupported)
end
DatabaseCore->>DB: execute(filter)
DB-->>DatabaseCore: results
DatabaseCore-->>Client: results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
# Conflicts: # src/Database/Adapter/Postgres.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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Validator/Query/Filter.php (1)
80-124: EXISTS/NOT_EXISTS validation ignores attributes invaluesand may reject valid queriesThe new handling for
Query::TYPE_EXISTS/Query::TYPE_NOT_EXISTSinFilterdoesn’t quite match the intended semantics:
- In
isValidAttributeAndValues(), the special-case block:// exists and notExists queries don't require values, just attribute validation if (in_array($method, [Query::TYPE_EXISTS, Query::TYPE_NOT_EXISTS])) { // Validate attribute (handles encrypted attributes, schemaless mode, etc.) return $this->isValidAttribute($attribute); }only validates
$attribute, not the attributes passed via$values. Forexists/notExistsqueries, the attributes to check live inQuery::getValues(), while$attributeis often empty.
- With schemas enabled (
$supportForAttributes === true), this can cause:
- Legitimate
exists/notExistsqueries to be rejected because$attributeis''and not in the schema.- Attributes listed in
$valuesto bypass schema/encryption checks entirely.At the same time,
isValid()now requires at least one “value” for EXISTS/NOT_EXISTS and routes them throughisValidAttributeAndValues(), so this path is exercised for real queries.A more accurate implementation is:
- Treat the “values” for EXISTS/NOT_EXISTS as attribute names.
- For those methods, skip all value-type validation and only run
isValidAttribute()on each attribute name in$values.- Let
isValid()continue to enforce “at least one value” as it does now.🔧 Proposed fix for EXISTS/NOT_EXISTS validation
@@ - protected function isValidAttributeAndValues(string $attribute, array $values, string $method): bool - { - if (!$this->isValidAttribute($attribute)) { - return false; - } - - $originalAttribute = $attribute; - // isset check if for special symbols "." in the attribute name - if (\str_contains($attribute, '.') && !isset($this->schema[$attribute])) { - // For relationships, just validate the top level. - // Utopia will validate each nested level during the recursive calls. - $attribute = \explode('.', $attribute)[0]; - } - - // exists and notExists queries don't require values, just attribute validation - if (in_array($method, [Query::TYPE_EXISTS, Query::TYPE_NOT_EXISTS])) { - // Validate attribute (handles encrypted attributes, schemaless mode, etc.) - return $this->isValidAttribute($attribute); - } + protected function isValidAttributeAndValues(string $attribute, array $values, string $method): bool + { + // EXISTS / NOT_EXISTS: `values` are attribute names; we only need attribute validation, + // not value-type validation. + if (\in_array($method, [Query::TYPE_EXISTS, Query::TYPE_NOT_EXISTS], true)) { + foreach ($values as $attr) { + if (!\is_string($attr)) { + $this->message = \ucfirst($method) . ' queries accept only attribute names.'; + return false; + } + + if (!$this->isValidAttribute($attr)) { + // `isValidAttribute` sets $this->message appropriately + return false; + } + } + + return true; + } + + if (!$this->isValidAttribute($attribute)) { + return false; + } + + $originalAttribute = $attribute; + // isset check if for special symbols "." in the attribute name + if (\str_contains($attribute, '.') && !isset($this->schema[$attribute])) { + // For relationships, just validate the top level. + // Utopia will validate each nested level during the recursive calls. + $attribute = \explode('.', $attribute)[0]; + } @@ - if ( - $array && - !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]) - ) { + if ( + $array && + !\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, + ], + true + ) + ) { $this->message = 'Cannot query '. $method .' on attribute "' . $attribute . '" because it is an array.'; return false; } @@ - case Query::TYPE_CONTAINS: - case Query::TYPE_NOT_CONTAINS: - case Query::TYPE_EXISTS: - case Query::TYPE_NOT_EXISTS: + case Query::TYPE_CONTAINS: + case Query::TYPE_NOT_CONTAINS: + case Query::TYPE_EXISTS: + case Query::TYPE_NOT_EXISTS: if ($this->isEmpty($value->getValues())) { $this->message = \ucfirst($method) . ' queries require at least one value.'; return false; } return $this->isValidAttributeAndValues($attribute, $value->getValues(), $method);This keeps schemaless behavior intact (
$supportForAttributes === falsestill bypasses schema lookups) while ensuring:
- Schemaful adapters validate each attribute used in EXISTS/NOT_EXISTS.
- Mongo’s new
$exists-based implementation receives a well-formed list of attribute names.Also applies to: 243-263, 311-323
src/Database/Validator/Operator.php (1)
305-326: Guard against empty$valuesbefore accessing index 0 inTYPE_ARRAY_REMOVEIn the
TYPE_ARRAY_REMOVEbranch you do:$toValidate = \is_array($values[0]) ? $values[0] : $values; foreach ($toValidate as $item) { ... } ... if (empty($values)) { $this->message = "Cannot apply {$method} operator: requires a value to remove"; return false; }If an operator is constructed with an empty
valuesarray (manually or via malformed input), accessing$values[0]will trigger an “undefined offset 0” notice before theempty($values)guard.Reordering the emptiness check avoids this runtime warning:
🔧 Suggested fix for `TYPE_ARRAY_REMOVE`
- case DatabaseOperator::TYPE_ARRAY_REMOVE: - if ($type === Database::VAR_RELATIONSHIP) { + case DatabaseOperator::TYPE_ARRAY_REMOVE: + if (empty($values)) { + $this->message = "Cannot apply {$method} operator: requires a value to remove"; + return false; + } + + if ($type === Database::VAR_RELATIONSHIP) { if (!$this->isRelationshipArray($attribute)) { $this->message = "Cannot apply {$method} operator to single-value relationship '{$operator->getAttribute()}'"; return false; } $toValidate = \is_array($values[0]) ? $values[0] : $values; foreach ($toValidate as $item) { if (!$this->isValidRelationshipValue($item)) { $this->message = "Cannot apply {$method} operator: relationship values must be document IDs (strings) or Document objects"; return false; } } - } elseif (!$isArray) { + } elseif (!$isArray) { $this->message = "Cannot apply {$method} operator to non-array field '{$operator->getAttribute()}'"; return false; } - - if (empty($values)) { - $this->message = "Cannot apply {$method} operator: requires a value to remove"; - return false; - }
🤖 Fix all issues with AI agents
In @src/Database/Database.php:
- Around line 6464-6519: The array_filter() calls in applyRelationshipOperator
are currently dropping all falsy values (losing valid IDs like "0" or ""), so
update the construction of $valueIds and $toRemoveIds to only remove nulls:
replace the two array_filter(...) uses that wrap array_map(...) with
array_filter(..., fn($v) => $v !== null) so they preserve empty-string/"0" IDs;
keep the existing mapping logic that converts Document instances to getId() and
strings as-is and retain the existing checks that use $itemId !== null.
In @tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php:
- Around line 2680-2789: The test creates articles but assigns them to unused
locals ($article1, $article2, $article3); simplify by removing these unused
variables in testOneToManyRelationshipWithArrayOperators and call
$database->createDocument('article', new Document(...)) directly (keep the same
'$id' values 'article1','article2','article3' and permissions/title payloads) so
behavior is unchanged but static analysis warnings are resolved.
🧹 Nitpick comments (4)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (1)
2138-2172: Remove unused local variable assignments.The
$book1,$book2,$book3, and$book4variables are assigned but never used. Since you only need the documents to exist in the database, you can callcreateDocument()without storing the return value.🔧 Suggested fix
// Create some books - $book1 = $database->createDocument('book', new Document([ + $database->createDocument('book', new Document([ '$id' => 'book1', '$permissions' => [ Permission::read(Role::any()), Permission::update(Role::any()), ], 'title' => 'Book 1', ])); - $book2 = $database->createDocument('book', new Document([ + $database->createDocument('book', new Document([ '$id' => 'book2', ... ])); - $book3 = $database->createDocument('book', new Document([ + $database->createDocument('book', new Document([ '$id' => 'book3', ... ])); - $book4 = $database->createDocument('book', new Document([ + $database->createDocument('book', new Document([ '$id' => 'book4', ... ]));src/Database/Database.php (2)
3003-3017: More descriptive limit errors are helpful; consider avoiding duplicate adapter callsThe new
LimitExceptionmessages on Line 3006 and Line 3013 provide much better diagnostics (current vs max and remediation hints), which is great.As a minor refinement, you could cache
getCountOfAttributes($collection)andgetAttributeWidth($collection)in local variables within this method to avoid calling them twice (once in the condition and once in the exception message). Same for the correspondinggetLimitFor*()calls. This is purely a micro-optimisation/readability tweak, not a functional issue.
5654-5677: Redundant Operator check inside relationship comparison loopOn Line 5674–Line 5677 you re-check
Operator::isOperator($value)inside the relationship-specific branch, but there is already an earlier pass over the document (Line 5625–Line 5631) that sets$shouldUpdate = truewhenever any attribute value is anOperator.This inner check is therefore redundant and slightly increases cognitive load. You could safely drop it and rely on the initial Operator scan.
Optional clean-up diff
- case Database::RELATION_ONE_TO_MANY: - case Database::RELATION_MANY_TO_ONE: - case Database::RELATION_MANY_TO_MANY: - if ( + case Database::RELATION_ONE_TO_MANY: + case Database::RELATION_MANY_TO_ONE: + case Database::RELATION_MANY_TO_MANY: + if ( ($relationType === Database::RELATION_MANY_TO_ONE && $side === Database::RELATION_SIDE_PARENT) || ($relationType === Database::RELATION_ONE_TO_MANY && $side === Database::RELATION_SIDE_CHILD) ) { @@ - if ((\is_null($value) !== \is_null($oldValue)) + if ((\is_null($value) !== \is_null($oldValue)) || (\is_string($value) && $value !== $oldValue) || ($value instanceof Document && $value->getId() !== $oldValue) ) { $shouldUpdate = true; } break; } - - if (Operator::isOperator($value)) { - $shouldUpdate = true; - break; - }src/Database/Query.php (1)
1187-1208: Alignexists/notExistshelpers and clarify attribute/value semantics
exists()andnotExists()both encode the checked attributes intovalueswith an empty$attribute, but their signatures differ (exists(array $attributes)vsnotExists(string|int|float|bool|array $attribute)), andnotExistsallows non-string scalar types for “attribute” values.For API clarity and type-safety, consider:
- Restricting
notExiststostring|array<string>(or justarray<string>), mirroringexists.- Documenting explicitly that for these methods,
valuesholds attribute names andattributeis intentionally empty.This keeps the public
QueryAPI predictable and reduces accidental misuse.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 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/Operator.phpsrc/Database/Validator/Queries.phpsrc/Database/Validator/Query/Filter.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.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/e2e/Adapter/Scopes/CollectionTests.phptests/e2e/Adapter/Scopes/AttributeTests.phptests/e2e/Adapter/Scopes/Relationships/OneToOneTests.phptests/e2e/Adapter/Scopes/Relationships/OneToManyTests.phpsrc/Database/Adapter/SQL.phptests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.phptests/e2e/Adapter/Scopes/SchemalessTests.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/Relationships/OneToOneTests.phpsrc/Database/Validator/Operator.phptests/e2e/Adapter/Scopes/Relationships/OneToManyTests.phpsrc/Database/Database.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Adapter/Mongo.phptests/e2e/Adapter/Scopes/SchemalessTests.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.phptests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.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/Adapter/SQL.php
📚 Learning: 2025-10-03T01:50:11.943Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/AttributeTests.php:1329-1334
Timestamp: 2025-10-03T01:50:11.943Z
Learning: MongoDB has a 1024kb (1,048,576 bytes) limit for index entries. The MongoDB adapter's getMaxIndexLength() method should return this limit rather than 0.
Applied to files:
src/Database/Adapter/Postgres.php
🧬 Code graph analysis (12)
tests/e2e/Adapter/Scopes/AttributeTests.php (1)
src/Database/Database.php (1)
Database(39-9432)
src/Database/Query.php (6)
src/Database/Adapter/Mongo.php (1)
exists(344-362)src/Database/Adapter/SQL.php (1)
exists(179-221)src/Database/Database.php (1)
exists(1533-1538)src/Database/Adapter.php (1)
exists(523-523)src/Database/Adapter/Pool.php (1)
exists(145-148)src/Database/Adapter/SQLite.php (1)
exists(76-106)
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (1)
src/Database/Operator.php (1)
arrayAppend(441-444)
src/Database/Validator/Operator.php (1)
src/Database/Database.php (1)
Database(39-9432)
src/Database/Adapter/Mongo.php (1)
src/Database/Query.php (1)
Query(8-1209)
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (2)
src/Database/Document.php (1)
getId(63-66)src/Database/Operator.php (2)
arrayAppend(441-444)arrayRemove(475-478)
src/Database/Adapter/SQL.php (1)
src/Database/Query.php (1)
Query(8-1209)
src/Database/Adapter/Postgres.php (3)
src/Database/Adapter/SQL.php (1)
getSQLTable(1872-1875)src/Database/Adapter.php (4)
getNamespace(131-134)getDatabase(184-187)filter(1270-1279)quote(491-491)src/Database/Adapter/MariaDB.php (1)
quote(1907-1910)
src/Database/Validator/Query/Filter.php (2)
src/Database/Query.php (1)
Query(8-1209)src/Database/Validator/Query/Order.php (1)
isValidAttribute(30-55)
src/Database/Database.php (2)
src/Database/Adapter.php (4)
getCountOfAttributes(1181-1181)getLimitForAttributes(888-888)getDocumentSizeLimit(1211-1211)getAttributeWidth(1222-1222)src/Database/Operator.php (2)
Operator(14-698)isOperator(664-667)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (3)
src/Database/Database.php (1)
Database(39-9432)src/Database/Document.php (1)
getId(63-66)src/Database/Operator.php (3)
arrayAppend(441-444)arrayRemove(475-478)arrayPrepend(452-455)
tests/e2e/Adapter/Scopes/SchemalessTests.php (4)
tests/e2e/Adapter/Schemaless/MongoDBTest.php (1)
getDatabase(33-69)src/Database/Adapter/Mongo.php (3)
getSupportForAttributes(2656-2659)delete(389-394)exists(344-362)src/Database/Query.php (5)
Query(8-1209)exists(1194-1197)and(811-814)or(802-805)notExists(1205-1208)src/Database/Document.php (2)
getId(63-66)getAttributes(194-212)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php
2721-2721: Avoid unused local variables such as '$article1'. (undefined)
(UnusedLocalVariable)
2730-2730: Avoid unused local variables such as '$article2'. (undefined)
(UnusedLocalVariable)
2739-2739: Avoid unused local variables such as '$article3'. (undefined)
(UnusedLocalVariable)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php
2138-2138: Avoid unused local variables such as '$book1'. (undefined)
(UnusedLocalVariable)
2147-2147: Avoid unused local variables such as '$book2'. (undefined)
(UnusedLocalVariable)
2156-2156: Avoid unused local variables such as '$book3'. (undefined)
(UnusedLocalVariable)
2165-2165: Avoid unused local variables such as '$book4'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (19)
tests/e2e/Adapter/Scopes/CollectionTests.php (1)
1585-1680: LGTM!The test comprehensively validates collection creation with a long UUID-like ID (36 characters). It properly:
- Guards against adapters without attribute support
- Tests attributes and indexes creation
- Validates document CRUD operations
- Cleans up resources
This is a valuable addition for testing identifier length handling.
tests/e2e/Adapter/Scopes/AttributeTests.php (2)
962-963: Improved test resilience by using substring checks.The change from exact message equality to substring checks makes the tests more maintainable. The assertions still validate the essential error information ("Column limit reached" and guidance to "Remove some attributes") while being resilient to minor message wording changes.
Also applies to: 971-973
1042-1045: Improved test resilience by using substring checks.The change from exact message equality to multiple substring checks is well-executed. The assertions validate all critical parts of the width limit error:
- The primary error ("Row width limit reached")
- The limit information ("bytes but the maximum is 65535 bytes")
- The remediation guidance ("Reduce the size of existing attributes or remove some attributes to free up space")
This approach maintains test coverage while being more maintainable.
Also applies to: 1052-1055
tests/e2e/Adapter/Scopes/SchemalessTests.php (2)
1158-1273: LGTM! Comprehensive exists query test coverage.This test thoroughly validates EXISTS semantics in schemaless mode:
- Basic existence checks (including null values)
- Non-existent attribute handling
- Multiple attributes with OR semantics
- Multiple queries with AND semantics
- Complex nested logical operators
The test correctly validates that documents with null values match exists queries (lines 1199-1204), which is the expected behavior. All assertions and expected counts are accurate.
1275-1379: LGTM! Comprehensive notExists query test coverage.This test thoroughly validates NOT_EXISTS semantics in schemaless mode:
- Basic non-existence checks
- Correct handling of null values (not matching notExists)
- Non-existent attribute handling (matching all documents)
- Multiple attributes with OR semantics
- Combination with exists queries
- Complex nested logical operators
The test correctly validates that documents with null values do NOT match notExists queries (line 1316), as null indicates the field exists. The comment on line 1367 helpfully explains the OR logic. All assertions are accurate.
src/Database/Adapter/SQL.php (1)
1770-1805: Explicitly rejecting EXISTS/NOT_EXISTS in SQL adapter is consistent and clearAdding a dedicated case for
Query::TYPE_EXISTS/Query::TYPE_NOT_EXISTSthat throws aDatabaseExceptionmatches the existing pattern for unsupported vector queries and avoids the ambiguous “Unknown method” path. This keeps the adapter contract explicit for callers relying on feature detection or error messages.src/Database/Validator/Queries.php (1)
80-128: Correctly classifying EXISTS/NOT_EXISTS as filter methodsMapping
Query::TYPE_VECTOR_EUCLIDEAN,Query::TYPE_EXISTS, andQuery::TYPE_NOT_EXISTStoBase::METHOD_TYPE_FILTERis consistent with the rest of the API and ensures these queries are validated by the filter validator. No further changes needed here.src/Database/Adapter/Mongo.php (1)
28-46: Mongo$existsintegration for EXISTS/NOT_EXISTS looks correct
- Adding
'$exists'to$operatorsensuresreplaceInternalIdsKeys()doesn’t corrupt the operator name while still rewriting attribute keys like$id→_uid.- Mapping
Query::TYPE_EXISTS/Query::TYPE_NOT_EXISTSto boolean values and handling$operator === '$exists'by building an$orof{ attr: { $exists: true|false } }clauses aligns with the documented “attributes list” semantics for these queries.getQueryOperator()returning'$exists'for both types keeps the switch compact and consistent with other operators.Once the Filter validator is updated to treat
getValues()as the list of attribute names for EXISTS/NOT_EXISTS, this adapter-side implementation is coherent end-to-end.Would you confirm that the intended semantics for
Query::exists([...])/Query::notExists([...])are “match documents where any of the listed attributes satisfy the existence condition” (OR), and not “all attributes must satisfy it” (AND)? The current$or-based implementation encodes the former.Also applies to: 2371-2385, 2440-2443, 2461-2485
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (1)
2097-2252: Well-structured test for array operators on many-to-many relationships.The test comprehensively covers:
arrayAppendwith single and multiple itemsarrayRemovewith single and multiple itemsarrayPrependoperation- Proper cleanup of test collections
The note about order not being guaranteed for many-to-many relationships (line 2238) is helpful context.
src/Database/Adapter/Postgres.php (5)
33-33: Good addition of PostgreSQL identifier limit constant.PostgreSQL truncates identifiers longer than 63 characters (NAMEDATALEN - 1). Making this a constant improves maintainability.
2761-2783: Solid implementation for handling long identifiers.The approach is reasonable:
- Return as-is if within limit
- Use MD5 hash with preserved suffix for readability
- Fall back to hash-only if suffix is too long
Minor observation: MD5 produces 32 characters, so the final
substr($hash, 0, 63)at line 2782 will always return the full 32-character hash (no truncation needed). This is fine but could be simplified to justreturn $hash;.
248-266: Index naming now consistently uses shortened keys.The changes apply
getShortKey()uniformly to all index names (_uid,_created,_updated,_tenant_id), which is correct for PostgreSQL's identifier limit.
2785-2791: Verify compatibility for existing databases with long table names during adapter updates.The PostgreSQL adapter now applies
getShortKey()to table names viagetSQLTable()to comply with PostgreSQL's 63-character identifier limit. While this shortening is consistently applied to both table names and index names increateCollection(), existing databases with table names created before this hashing was in place will not be found by the adapter if they exceed 63 characters.For upgrades from versions where table names were stored unshortened:
- Tables created with names longer than 63 characters will have different hash values when accessed
- Existing indexes will reference the old (unshortened) table names and become orphaned
- A migration strategy is needed to rename existing tables to use the new hashing scheme, or conditional logic to support both old and new table name formats during a transition period
Confirm whether this adapter is only for new installations or if upgrade/migration guidance is needed for existing deployments.
970-984: Consider backward compatibility if index hashing logic was recently introduced.The
renameIndexmethod usesgetShortKeyto generate the index name, which applies MD5 hashing for names exceeding 63 characters. This is consistent with howcreateIndexgenerates index names. However, if this hashing logic was recently added and existing indexes in production databases were created with PostgreSQL's default truncation behavior (before hashing was implemented), therenameIndexoperation would fail—the calculated hashed name won't match the actual truncated index name in the database.Verify whether:
- The hashing logic in
getShortKeywas always present or is a recent addition- Any existing production indexes need migration to match the new naming scheme
- A migration path is needed for existing indexes, or if all deployments are fresh
src/Database/Database.php (1)
6066-6103: Pre-processing relationship array operators before relationship logic looks correctThe new block on Line 6087–Line 6103 that detects
Operatorvalues for relationship attributes and, for array operations, normalizes them to a flat array of related IDs viaapplyRelationshipOperator()is a good approach:
- It lets you keep all the existing relationship-type branching (
ONE_TO_MANY/MANY_TO_MANYetc.) unchanged, operating on plain ID arrays.- It correctly derives
$existingIdsfrom$oldValue, handling bothDocumentinstances and raw IDs.- It is used both by
updateDocument()andupdateDocuments()viaupdateDocumentRelationships(), so array operators behave consistently for single and bulk updates.This does assume that
Operator::isArrayOperation()is only true for multi-valued relationships (which the updatedOperatorvalidator now enforces), otherwise scalar relationships would treat array operators as appending/removing against an empty set. Given the validator’s new relationship-aware checks, that assumption looks safe.src/Database/Query.php (1)
29-30: EXISTS/NOT_EXISTS query types are wired consistently into the core APIThe new
TYPE_EXISTS/TYPE_NOT_EXISTSconstants are properly registered inTYPESandisMethod, so they’ll parse, validate, and group just like other filter methods. No functional issues spotted here.Also applies to: 70-116, 256-305
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (1)
2601-2675: Good coverage: one-to-one relationship correctly rejects array operatorsThis test cleanly exercises the failure path for applying
arrayAppendon a single-value one-to-one relationship, with proper feature guards and cleanup, and asserts against the new “single-value relationship” error message. Looks solid.tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (1)
2791-2865: Child side of one-to-many correctly rejects array operatorsThis test mirrors the one-to-one case on the child side of a one-to-many relationship, ensuring
arrayAppendon the single-valueparentrelationship throws aStructureexception with the expected “single-value relationship” message. Good targeted coverage of the negative path.src/Database/Validator/Operator.php (1)
39-82: Relationship-aware array operator validation is well factored and consistentThe new helpers and validations substantially tighten behavior around relationship attributes:
isValidRelationshipValue()cleanly centralizes the allowed value types for relationship operators (string IDs orDocumentinstances).isRelationshipArray()correctly derives “many” vs single sides fromrelationType/side, so operators likearrayAppend,arrayRemove,arrayIntersect,arrayDiff, andarrayFiltercan:
- Allow usage only on multi-valued relationship attributes,
- Produce clear, consistent error messages for single-value relationships (
"single-value relationship ..."),- Enforce relationship value typing instead of treating them as opaque arrays.
Combined with the new tests, this looks like a solid and maintainable way to guard misuse of array operators on relationship fields.
Also applies to: 210-223, 239-247, 252-283, 305-366, 373-401
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Adapter/Postgres.php (1)
248-298: Index name generation is inconsistent between createCollection and index operation methods for non-shared tables.For non-shared tables,
createCollection()generates index names without tenant ({namespace}_{id}_uid), butcreateIndex(),deleteIndex(), andrenameIndex()always include tenant in the key construction ({namespace}_{tenant}_{collection}_{id}). This causes a mismatch: indexes created for non-shared tables cannot be found, deleted, or renamed by subsequent operations since they search for names containing{tenant}(which would be null for non-shared collections).Update
createIndex(),deleteIndex(), andrenameIndex()to conditionally include tenant only when$this->sharedTablesis true, matching the pattern increateCollection():if ($this->sharedTables) { $keyName = $this->getShortKey("{$this->getNamespace()}_{$this->tenant}_{$collection}_{$id}"); } else { $keyName = $this->getShortKey("{$this->getNamespace()}_{$collection}_{$id}"); }src/Database/Adapter/Mongo.php (1)
2375-2444: Fix MongoDB EXISTS/NOT_EXISTS to normalize internal attribute namesThe
$existsoperator handler builds filters usinggetValues()directly without applying the same internal attribute normalization that other operators use at the start ofbuildFilter(). WhilereplaceInternalIdsKeys()runs afterward with a simplestr_replace('$','_'), this produces incorrect mappings for internal pseudo-attributes:
$id→_id(should map to_uid)$sequence→_sequence(should map to_id)$createdAt→_createdAt(works by coincidence)Since internal attributes are supported in other query types via the same normalization block, they should be normalized consistently here as well. Apply
getInternalKeyForAttribute()to each attribute fromgetValues()before building the filter, or validate upfront to reject internal attributes if they're not intended to be supported:Suggested fix
} elseif ($operator === '$exists') { foreach ($query->getValues() as $attribute) { - $filter['$or'][] = [$attribute => [$operator => $value]]; + $normalized = $this->filter($this->getInternalKeyForAttribute($attribute)); + $filter['$or'][] = [$normalized => [$operator => $value]]; }
🤖 Fix all issues with AI agents
In @src/Database/Database.php:
- Around line 6464-6519: The extraction of IDs in applyRelationshipOperator()
uses array_filter() without a callback which removes falsy-but-valid IDs like
"0" and ""—change both occurrences (the $valueIds creation and the $toRemoveIds
creation) to filter only nulls by passing a callback that keeps values !== null
so all non-null string IDs are preserved while still dropping nulls; update the
array_filter calls that wrap the array_map results accordingly (references:
applyRelationshipOperator, $valueIds, $toRemoveIds).
🧹 Nitpick comments (4)
src/Database/Adapter/Postgres.php (2)
33-33: Add documentation for the PostgreSQL identifier limit.Consider adding a brief comment explaining that this constant represents PostgreSQL's maximum identifier length (NAMEDATALEN - 1).
📝 Suggested documentation
+ /** + * PostgreSQL's maximum identifier length (NAMEDATALEN - 1 = 63 characters) + * @see https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS + */ public const MAX_IDENTIFIER_NAME = 63;
2760-2788: Consider documenting collision handling strategy.The
getShortKey()implementation uses MD5 hashing to shorten identifiers. While MD5 collisions are extremely rare in practice, they're theoretically possible. Since PostgreSQL will error on duplicate index/table names, collisions would be caught at runtime, but debugging could be difficult.Consider adding:
- A comment documenting the collision risk and detection strategy
- Error handling context when PostgreSQL rejects a duplicate name
📝 Suggested documentation
/** * Ensure index key length stays within PostgreSQL's 63 character limit. + * + * Uses MD5 hashing for long identifiers. While MD5 collisions are extremely + * rare (probability ~1 in 2^128), if they occur, PostgreSQL will error on + * duplicate names. The suffix preservation aids debugging when possible. * * @param string $key * @return string */ protected function getShortKey(string $key): stringsrc/Database/Database.php (2)
2996-3017: More informative limit exceptions; consider caching computed valuesThe richer
LimitExceptionmessages are helpful for users/admins diagnosing schema limits, and the control flow is unchanged. Minor nit: bothgetCountOfAttributes()andgetAttributeWidth()are called twice (in the condition and again when building the message). If these hit the adapter/database, consider storing the results in local variables to avoid redundant calls, even if it only occurs on the exceptional path.
5674-5677: Operator-based relationship updates: behavior looks correct; rely on validation for scalar sidesThe new branches that (a) short-circuit relationship comparison when
$valueis anOperatorand (b) translate array operators into concrete ID arrays viaapplyRelationshipOperator()before running the existing relationship logic are consistent with the existing flow: operators now trigger$shouldUpdate = truewithout tripping the "array or list" validations, and the actual relationship payload seen by the DB is still a plain list of IDs.One design assumption this introduces is that array operators are only ever used on relationship attributes that are semantically array-valued (e.g. parent side of one-to-many, many-to-many). For scalar relationship sides (e.g. many-to-one parent, one-to-many child),
updateDocumentRelationships()will still see anOperatorand, ifisArrayOperation()returned true there, would try to treat a scalar relationship as an array. That should already be prevented by the operator/relationship validator layer; if not, you may want to either (a) reject array operators here for scalar sides up-front, or (b) extend validation accordingly so invalid combinations never reach this code.Also applies to: 6087-6103
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Adapter/Mongo.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Database.php
🧰 Additional context used
🧠 Learnings (5)
📚 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/Database.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/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.phpsrc/Database/Adapter/SQL.php
📚 Learning: 2025-10-03T01:50:11.943Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/AttributeTests.php:1329-1334
Timestamp: 2025-10-03T01:50:11.943Z
Learning: MongoDB has a 1024kb (1,048,576 bytes) limit for index entries. The MongoDB adapter's getMaxIndexLength() method should return this limit rather than 0.
Applied to files:
src/Database/Adapter/Postgres.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/Adapter/SQL.php
🧬 Code graph analysis (2)
src/Database/Adapter/SQL.php (1)
src/Database/Query.php (1)
Query(8-1209)
src/Database/Adapter/Mongo.php (3)
src/Database/Query.php (2)
Query(8-1209)getValues(185-188)src/Database/Operator.php (1)
getValues(155-158)src/Database/Adapter.php (1)
filter(1270-1279)
⏰ 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 (6)
src/Database/Adapter/Postgres.php (3)
2790-2796: LGTM! Consistent table name shortening.The
getSQLTable()method correctly applies identifier shortening to the full table name and produces properly qualified table references. The approach is consistent with how index names are shortened elsewhere in the code.
908-916: LGTM! Consistent index name shortening in createIndex().The index creation correctly applies
getShortKey()to generate shortened index names that respect PostgreSQL's 63-character limit. The key construction pattern matches what's used increateCollection().
945-985: LGTM! Consistent identifier shortening across all index operations.Both
deleteIndex()andrenameIndex()correctly applygetShortKey()using the same key construction pattern ascreateIndex()andcreateCollection(). This ensures that:
- Indexes can be found when deleting or renaming
- The same shortened names are generated consistently
- Operations won't fail due to name mismatches
src/Database/Adapter/SQL.php (1)
1770-1805: Explicitly rejecting EXISTS/NOT_EXISTS in SQL adapter looks correctAdding
TYPE_EXISTS/TYPE_NOT_EXISTScases that throwDatabaseException('Exists queries are not supported by this database')keeps behavior explicit and consistent with vector query handling. No further changes needed here.src/Database/Adapter/Mongo.php (2)
28-46: Including$existsin the operator whitelist is appropriateAdding
'$exists'to the$operatorsarray ensuresreplaceInternalIdsKeys()does not mangle$existsinto_existswhen rewriting internal keys, which is required for the new existence filters to work correctly.
2459-2485: Mapping EXISTS/NOT_EXISTS to Mongo’s$existsoperator is correctExtending
getQueryOperator()so that bothQuery::TYPE_EXISTSandQuery::TYPE_NOT_EXISTSmap to'$exists'aligns with the value selection inbuildFilter()(usingtruevsfalse) and with Mongo’s API. With the above normalization fix in place, this should give predictable behavior for existence checks.
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.