Object(json) attribute support for postgres #741
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces comprehensive object (JSON) attribute support across the database layer. It adds abstract method declarations in the base Adapter class, implements support indicators across all adapters (with Postgres providing full JSONB query and GIN index capabilities), adds VAR_OBJECT and INDEX_OBJECT constants to the Database class, validates object attributes and indexes, integrates object type handling into query filtering and casting flows, and includes extensive end-to-end and unit tests covering object attribute lifecycle, indexes, defaults, and error cases. Changes
Sequence DiagramsequenceDiagram
actor User
participant Query as Query Handler
participant Database
participant Adapter as Adapter<br/>(Postgres)
participant SQL as SQL Engine
User->>Query: Create query with object attribute
Query->>Database: Validate attribute & check adapter support
Database->>Adapter: getSupportForObject()
Adapter-->>Database: true/false
alt Adapter supports objects
Database->>Adapter: Handle object query (getSQLCondition)
Adapter->>Adapter: handleObjectQueries()
Note over Adapter: Parse TYPE_EQUAL,<br/>TYPE_CONTAINS, etc.
Adapter-->>Database: JSONB condition + binds
Database->>SQL: Execute with JSONB operators
SQL-->>Database: Result rows
else Adapter does not support objects
Database-->>User: Error: unsupported
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Validator/Query/Filter.php (1)
209-219: notContains allowed here, but Postgres doesn’t implement it for objectsThis path allows CONTAINS/NOT_CONTAINS on non-array attributes when type is string/object/spatial. Postgres::handleObjectQueries supports CONTAINS but not NOT_CONTAINS (nor NOT_EQUAL), so NOT_CONTAINS on an object will throw at runtime.
Either: (a) implement NOT_CONTAINS (and NOT_EQUAL) in Postgres, or (b) reject NOT_CONTAINS for objects here.
Here’s option (a) preferred for parity:
- Postgres: add NOT_CONTAINS using
NOT (col @> :val::jsonb); add NOT_EQUAL usingNOT (col = :val::jsonb); keep multi‑value separator consistent with NOT queries (AND).If you prefer option (b), I can provide a compact guard here to block NOT_CONTAINS for object attributes.
🧹 Nitpick comments (2)
src/Database/Adapter/SQLite.php (1)
976-979: Capability flag implemented correctlySQLite advertises no object/JSON support, consistent with current storage-as-text behavior in create/update. No concerns.
If/when JSON1 usage is introduced, consider feature-detecting it and flipping this to true behind a config/feature flag.
src/Database/Adapter/Postgres.php (1)
2019-2027: Capability flag advertises JSONB supportLooks good. As a follow-up, consider first-class JSONB indexing for performance on
@>queries (GIN).I can draft a proposal to leverage
$indexAttributeTypesinsidecreateIndex()to emitUSING GIN ("col")for object attributes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/Postgres.php(5 hunks)src/Database/Adapter/SQL.php(1 hunks)src/Database/Adapter/SQLite.php(1 hunks)src/Database/Database.php(7 hunks)src/Database/Query.php(1 hunks)src/Database/Validator/Query/Filter.php(2 hunks)src/Database/Validator/Structure.php(1 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#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/AttributeTests.phptests/e2e/Adapter/Scopes/DocumentTests.php
🧬 Code graph analysis (10)
src/Database/Query.php (1)
src/Database/Database.php (1)
Database(37-8180)
src/Database/Adapter/SQLite.php (3)
src/Database/Adapter.php (1)
getSupportForObject(1064-1064)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/Postgres.php (1)
getSupportForObject(2024-2027)
src/Database/Adapter.php (3)
src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/Postgres.php (1)
getSupportForObject(2024-2027)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)
src/Database/Adapter/MariaDB.php (3)
src/Database/Adapter.php (1)
getSupportForObject(1064-1064)src/Database/Adapter/Postgres.php (1)
getSupportForObject(2024-2027)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)
src/Database/Validator/Query/Filter.php (2)
src/Database/Database.php (1)
Database(37-8180)src/Database/Query.php (1)
Query(8-1124)
src/Database/Adapter/SQL.php (1)
src/Database/Database.php (1)
Database(37-8180)
src/Database/Adapter/Postgres.php (5)
src/Database/Query.php (2)
Query(8-1124)isObjectAttribute(966-969)src/Database/Adapter.php (1)
getSupportForObject(1064-1064)src/Database/Database.php (1)
Database(37-8180)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)
src/Database/Validator/Structure.php (1)
src/Database/Database.php (1)
Database(37-8180)
src/Database/Database.php (4)
src/Database/Adapter.php (1)
getSupportForObject(1064-1064)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/Postgres.php (1)
getSupportForObject(2024-2027)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)
tests/e2e/Adapter/Scopes/DocumentTests.php (5)
src/Database/Adapter.php (1)
getSupportForObject(1064-1064)src/Database/Database.php (1)
Database(37-8180)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/Postgres.php (1)
getSupportForObject(2024-2027)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)
🪛 GitHub Actions: CodeQL
src/Database/Database.php
[error] 2057-2057: phpstan: Unreachable statement - code above always terminates. (Command: ./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G)
[error] 2065-2065: phpstan: Unreachable statement - code above always terminates. (Command: ./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G)
tests/e2e/Adapter/Scopes/DocumentTests.php
[error] 6571-6571: phpstan: Unreachable statement - code above always terminates. (in context of class Tests\E2E\Adapter\Base) (Command: ./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G)
[error] 7064-7064: phpstan: Unreachable statement - code above always terminates. (in context of class Tests\E2E\Adapter\Base) (Command: ./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/DocumentTests.php
6628-6628: Avoid unused local variables such as '$doc2'. (undefined)
(UnusedLocalVariable)
6839-6839: Avoid unused local variables such as '$doc7'. (undefined)
(UnusedLocalVariable)
6853-6853: Avoid unused local variables such as '$doc8'. (undefined)
(UnusedLocalVariable)
6944-6944: Avoid unused local variables such as '$doc11'. (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 (10)
src/Database/Database.php (7)
47-47: Add object type constant — looks good.
Public constant for object attributes is clear and scoped correctly.
1954-1964: Attribute validation for object type — good constraints.
Adapter gating + empty size + non-array are correct.
1980-1980: Error message update includes object type — OK.
Keeps guidance complete for unknown types.
2067-2067: Unknown-type message includes object — OK.
Matches earlier list.
2314-2325: Update-attribute rules for object type — good.
Same constraints as create path and adapter gating. Consistent.
7321-7325: Casting support for object — good fallback.
Decoding JSON string to array when the adapter doesn’t cast types is appropriate.
2052-2065: Fix unreachable code and incorrect array recursion for object/spatial types.The current code has an early return at line 2036 when
$defaultType === 'array', making thecase self::TYPE_OBJECTand spatial type cases (lines 2049–2062) unreachable. Additionally, for object and spatial types with array defaults, the code incorrectly recurses into each element, causing validation errors. The proposed diff moves the return inside a conditional that excludesTYPE_OBJECTand spatial types, allowing them to skip element-wise recursion and be validated directly in the switch statement.Apply the provided diff to resolve both the unreachable code and the incorrect validation logic.
Note: phpstan cannot be run in this environment, but the code inspection confirms the logic issue and the correctness of the proposed fix.
src/Database/Adapter/Postgres.php (3)
1646-1648: Early dispatch for object attributesRouting object queries here is good. With the above patch, NOT_* will also be handled cleanly; otherwise NOT_* will still throw. Please align both layers (validator and adapter) before merging.
1797-1800: Type mapping: object → JSONBCorrect choice for Postgres. No issues.
1811-1811: Error message updated to include objectGood catch adding TYPE_OBJECT to the exception text.
* updated not operator
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
2055-2070: Restore proper control flow invalidateDefaultTypes.
case self::TYPE_OBJECTcurrently hits avar_dumpand falls through todefault, so every object (and spatial) attribute default now throwsUnknown attribute type. Besides killing valid defaults, thevar_dumpdumps to stdout in production. Drop the debug call and add explicitbreaks so known types don’t cascade into thedefaultcase.Apply this diff:
- case self::TYPE_OBJECT: - // Object types expect arrays as default values - var_dump($defaultType); - if ($defaultType !== 'array') { - throw new DatabaseException('Default value for object type must be an array'); - } - // no break + case self::TYPE_OBJECT: + // Object types expect arrays as default values + if ($defaultType !== 'array') { + throw new DatabaseException('Default value for object type must be an array'); + } + break; case self::VAR_POINT: case self::VAR_LINESTRING: case self::VAR_POLYGON: // Spatial types expect arrays as default values if ($defaultType !== 'array') { throw new DatabaseException('Default value for spatial type ' . $type . ' must be an array'); } - // no break + break; default: throw new DatabaseException('Unknown attribute type: ' . $type . '. Must be one of ' . self::VAR_STRING . ', ' . self::VAR_INTEGER . ', ' . self::VAR_FLOAT . ', ' . self::VAR_BOOLEAN . ', ' . self::VAR_DATETIME . ', ' . self::VAR_RELATIONSHIP . ', ' . self::TYPE_OBJECT . ', ' . self::VAR_POINT . ', ' . self::VAR_LINESTRING . ', ' . self::VAR_POLYGON);
♻️ Duplicate comments (1)
src/Database/Adapter/Postgres.php (1)
1571-1624: JSONB query semantics: make EQUAL strict, drop auto-wrap, and guard types
- EQUAL uses @> for arrays/objects (containment), not equality.
- CONTAINS auto-wraps values; surprising and leaky.
- count($value) on non-arrays can warn at runtime.
Apply this refactor for predictable behavior and full parity (adds strict equality, NOT_* support, and input guarding):
- protected function handleObjectQueries(Query $query, array &$binds, string $attribute, string $alias, string $placeholder): string - { - switch ($query->getMethod()) { - case Query::TYPE_EQUAL: - case Query::TYPE_NOT_EQUAL: { - $isNot = $query->getMethod() === Query::TYPE_NOT_EQUAL; - $conditions = []; - foreach ($query->getValues() as $key => $value) { - $binds[":{$placeholder}_{$key}"] = json_encode($value); - if (is_array($value)) { - $fragment = "{$alias}.{$attribute} @> :{$placeholder}_{$key}::jsonb"; - $conditions[] = $isNot ? "NOT (" . $fragment . ")" : $fragment; - } else { - $fragment = "{$alias}.{$attribute} = :{$placeholder}_{$key}::jsonb"; - $conditions[] = $isNot ? "{$alias}.{$attribute} <> :{$placeholder}_{$key}::jsonb" : $fragment; - } - } - $separator = $isNot ? ' AND ' : ' OR '; - return empty($conditions) ? '' : '(' . implode($separator, $conditions) . ')'; - } - - case Query::TYPE_CONTAINS: - case Query::TYPE_NOT_CONTAINS: { - $isNot = $query->getMethod() === Query::TYPE_NOT_CONTAINS; - $conditions = []; - foreach ($query->getValues() as $key => $value) { - if (count($value) === 1) { - $jsonKey = array_key_first($value); - $jsonValue = $value[$jsonKey]; - // wrap to represent array; eg: key -> [value] - $value[$jsonKey] = [$jsonValue]; - } - $binds[":{$placeholder}_{$key}"] = json_encode($value); - $fragment = "{$alias}.{$attribute} @> :{$placeholder}_{$key}::jsonb"; - $conditions[] = $isNot ? "NOT (" . $fragment . ")" : $fragment; - } - $separator = $isNot ? ' AND ' : ' OR '; - return empty($conditions) ? '' : '(' . implode($separator, $conditions) . ')'; - } - - default: - throw new DatabaseException('Query method ' . $query->getMethod() . ' not supported for object attributes'); - } - } + protected function handleObjectQueries(Query $query, array &$binds, string $attribute, string $alias, string $placeholder): string + { + $conditions = []; + $method = $query->getMethod(); + $isNot = \in_array($method, [Query::TYPE_NOT_EQUAL, Query::TYPE_NOT_CONTAINS], true); + + foreach ($query->getValues() as $key => $value) { + // Expect JSON arrays/objects for object queries + if (!\is_array($value)) { + throw new DatabaseException('Object queries expect JSON objects/arrays'); + } + $binds[":{$placeholder}_{$key}"] = \json_encode($value); + $rhs = ":{$placeholder}_{$key}::jsonb"; + + $cond = match ($method) { + // Strict JSONB equality/inequality + Query::TYPE_EQUAL => "{$alias}.{$attribute} = {$rhs}", + Query::TYPE_NOT_EQUAL => "NOT ({$alias}.{$attribute} = {$rhs})", + // JSONB containment + Query::TYPE_CONTAINS => "{$alias}.{$attribute} @> {$rhs}", + Query::TYPE_NOT_CONTAINS => "NOT ({$alias}.{$attribute} @> {$rhs})", + default => throw new DatabaseException('Query method ' . $method . ' not supported for object attributes'), + }; + $conditions[] = $cond; + } + + $separator = $isNot ? ' AND ' : ' OR '; + return empty($conditions) ? '' : '(' . \implode($separator, $conditions) . ')'; + }
🧹 Nitpick comments (6)
src/Database/Adapter/Postgres.php (1)
870-887: Shared-tables + GIN/GIST: use per-tenant partial indexesSince _tenant can’t be a GIN/GIST column, add a WHERE clause to scope the index per tenant for planner usability.
$sql = "CREATE {$sqlType} {$key} ON {$this->getSQLTable($collection)}"; @@ - $sql .= " ({$attributes});"; + $whereTenant = ''; + if ($this->sharedTables && ($type === Database::INDEX_SPATIAL || $type === Database::INDEX_GIN)) { + $whereTenant = ' WHERE _tenant = ' . (int)$this->tenant; + } + $sql .= " ({$attributes}){$whereTenant};";tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (5)
30-30: Prefer assertTrue over assertEquals(true, …)Use assertTrue for boolean assertions.
- $this->assertEquals(true, $database->createAttribute($collectionId, 'meta', Database::TYPE_OBJECT, 0, false)); + $this->assertTrue($database->createAttribute($collectionId, 'meta', Database::TYPE_OBJECT, 0, false));- $this->assertEquals(true, $database->createAttribute($collectionId, 'data', Database::TYPE_OBJECT, 0, false)); + $this->assertTrue($database->createAttribute($collectionId, 'data', Database::TYPE_OBJECT, 0, false));- $this->assertEquals(true, $database->createAttribute($collectionId, 'metaDefaultEmpty', Database::TYPE_OBJECT, 0, false, [])); + $this->assertTrue($database->createAttribute($collectionId, 'metaDefaultEmpty', Database::TYPE_OBJECT, 0, false, []));- $this->assertEquals(true, $database->createAttribute($collectionId, 'settings', Database::TYPE_OBJECT, 0, false, $defaultSettings)); + $this->assertTrue($database->createAttribute($collectionId, 'settings', Database::TYPE_OBJECT, 0, false, $defaultSettings));- $this->assertEquals(true, $database->createAttribute($collectionId, 'profile', Database::TYPE_OBJECT, 0, true, null)); + $this->assertTrue($database->createAttribute($collectionId, 'profile', Database::TYPE_OBJECT, 0, true, null));- $this->assertEquals(true, $database->createAttribute($collectionId, 'profile2', Database::TYPE_OBJECT, 0, false, ['name' => 'anon'])); + $this->assertTrue($database->createAttribute($collectionId, 'profile2', Database::TYPE_OBJECT, 0, false, ['name' => 'anon']));- $this->assertEquals(true, $database->createAttribute($collectionId, 'misc', Database::TYPE_OBJECT, 0, false, null)); + $this->assertTrue($database->createAttribute($collectionId, 'misc', Database::TYPE_OBJECT, 0, false, null));Also applies to: 556-556, 848-848, 852-852, 855-855, 858-858, 861-861
80-80: Remove unused local variables flagged by PHPMDThese variables aren’t read; drop the assignments to quiet static analysis without changing behavior.
- $doc2 = $database->createDocument($collectionId, new Document([ + $database->createDocument($collectionId, new Document([- $doc7 = $database->createDocument($collectionId, new Document([ + $database->createDocument($collectionId, new Document([- $doc8 = $database->createDocument($collectionId, new Document([ + $database->createDocument($collectionId, new Document([- $doc11 = $database->createDocument($collectionId, new Document([ + $database->createDocument($collectionId, new Document([- $doc1 = $database->createDocument($collectionId, new Document([ + $database->createDocument($collectionId, new Document([- $doc2 = $database->createDocument($collectionId, new Document([ + $database->createDocument($collectionId, new Document([Also applies to: 313-313, 327-327, 430-430, 563-563, 576-576
446-446: Misleading commentThis isn’t a negative test; it expects a match. Tweak comment for clarity.
- // Test 29: Negative test - partial object match should still work (containment) + // Test 29: Partial object match (containment) should succeed
255-270: Duplicate of Test 15; consider making it a true partial-path checkThis block repeats the same nested object used in Test 15. If the goal is partial-path containment, reduce the structure or switch to contains() to exercise subset matching.
589-595: Test intent vs. query targetYou query data with ['env' => 'production'] but ‘env’ lives under 'config'. The 0-result assertion is fine, but consider clarifying the comment or querying ['config' => ['env' => 'production']] to demonstrate positive equality semantics separately.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/Postgres.php(7 hunks)src/Database/Database.php(14 hunks)src/Database/Validator/Index.php(4 hunks)tests/e2e/Adapter/Base.php(2 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(2 hunks)tests/e2e/Adapter/Scopes/ObjectAttributeTests.php(1 hunks)tests/unit/Validator/IndexTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/Scopes/DocumentTests.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#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.phptests/e2e/Adapter/Base.php
🧬 Code graph analysis (7)
src/Database/Validator/Index.php (1)
src/Database/Database.php (1)
Database(37-8211)
tests/unit/Validator/IndexTest.php (3)
src/Database/Document.php (2)
Document(12-470)getAttribute(224-231)src/Database/Database.php (1)
Database(37-8211)src/Database/Validator/Index.php (3)
Index(10-433)isValid(278-317)getDescription(70-73)
src/Database/Database.php (5)
src/Database/Adapter/Pool.php (1)
getSupportForObject(572-575)src/Database/Adapter/Postgres.php (1)
getSupportForObject(2027-2030)src/Database/Adapter.php (1)
getSupportForObject(1064-1064)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)
src/Database/Adapter/Postgres.php (7)
src/Database/Database.php (2)
Database(37-8211)count(6989-7038)src/Database/Adapter.php (2)
count(825-825)getSupportForObject(1064-1064)src/Database/Query.php (2)
Query(8-1124)isObjectAttribute(966-969)src/Database/Adapter/Pool.php (2)
count(273-276)getSupportForObject(572-575)src/Database/Adapter/SQL.php (1)
count(2534-2596)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (6)
src/Database/Adapter.php (1)
Adapter(16-1325)src/Database/Database.php (4)
Database(37-8211)createAttribute(1695-1750)count(6989-7038)createIndex(3271-3454)src/Database/Validator/Index.php (1)
Index(10-433)src/Database/Query.php (5)
Query(8-1124)equal(435-438)contains(507-510)notEqual(447-450)select(580-583)src/Database/Adapter/Pool.php (4)
getSupportForObject(572-575)createAttribute(168-171)count(273-276)createIndex(213-216)src/Database/Adapter/Postgres.php (3)
getSupportForObject(2027-2030)createAttribute(444-463)createIndex(839-897)
src/Database/Adapter/Pool.php (5)
src/Database/Adapter/Postgres.php (1)
getSupportForObject(2027-2030)src/Database/Adapter.php (1)
getSupportForObject(1064-1064)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)src/Database/Mirror.php (1)
delegate(88-103)
tests/e2e/Adapter/Base.php (1)
src/Database/Adapter.php (1)
Adapter(16-1325)
🪛 GitHub Actions: CodeQL
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php
[error] 841-841: PHPStan: Unreachable statement - code above always terminates. (tests/e2e/Adapter/Scopes/ObjectAttributeTests.php:841)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php
80-80: Avoid unused local variables such as '$doc2'. (undefined)
(UnusedLocalVariable)
313-313: Avoid unused local variables such as '$doc7'. (undefined)
(UnusedLocalVariable)
327-327: Avoid unused local variables such as '$doc8'. (undefined)
(UnusedLocalVariable)
430-430: Avoid unused local variables such as '$doc11'. (undefined)
(UnusedLocalVariable)
563-563: Avoid unused local variables such as '$doc1'. (undefined)
(UnusedLocalVariable)
576-576: Avoid unused local variables such as '$doc2'. (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). (10)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Mirror)
🔇 Additional comments (11)
src/Database/Adapter/Pool.php (1)
571-575: LGTM: delegator added for getSupportForObject()Matches existing delegation pattern. No issues.
src/Database/Adapter/Postgres.php (3)
1649-1651: LGTM: object-attribute query path integratedDelegates to handleObjectQueries when attribute type is object.
1800-1815: LGTM: TYPE_OBJECT mapped to JSONBConsistent with Postgres JSON storage.
2022-2031: LGTM: capability flag returns trueMatches adapter capabilities; Pool and interface aligned.
tests/unit/Validator/IndexTest.php (1)
241-326: LGTM: comprehensive GIN index validator coverageCovers support gating, single-attr constraint, type check, and order restriction.
src/Database/Validator/Index.php (3)
33-55: LGTM: plumbed objectIndexSupport through constructorDocblock updated; field assigned.
312-315: LGTM: GIN check included in validation chainOrder is appropriate; returns early on failure.
392-431: LGTM: dedicated GIN rulesRules match Database::createIndex() errors and new tests.
tests/e2e/Adapter/Base.php (1)
11-11: Verified: Trait gating properly implementedConfirmed that ObjectAttributeTests trait includes getSupportForObject() checks at multiple test entry points (lines 22, 548, 656, 839), each followed by markTestSkipped() calls. Tests will correctly skip on unsupporting adapters. Code changes approved.
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (2)
21-24: Good skip gating for unsupported adaptersAdapter capability checks are correct and prevent false negatives on non-JSON backends.
Also applies to: 547-551, 655-659
346-356: Query::notContains is available and properly wired for object attributes.Verification confirms that
Query::notContainsexists atsrc/Database/Query.php:519and is fully implemented for object attributes:
- Postgres adapter handles JSONB queries with explicit negation logic (wraps conditions with
NOT())- SQL adapter supports the method via
getLikeOperator()- Test 21 validates that
containsworks on the same object structure; Test 23b correctly expectsnotContainsto exclude matching documentsNo replacement or alternative is needed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (3)
153-156: Fix notEqual payload: remove extra array nesting.Query::notEqual expects a single value (object or scalar). The extra wrapping
[[...]]produces incorrect semantics.Apply:
- Query::notEqual('meta', [['age' => 26]]) + Query::notEqual('meta', ['age' => 26])
162-169: Fix notEqual payload: remove extra array nesting.Same issue—remove the outer array wrapper.
Apply:
- Query::notEqual('meta', [[ - 'user' => [ - 'info' => [ - 'country' => 'CA' - ] - ] - ]]) + Query::notEqual('meta', [ + 'user' => [ + 'info' => [ + 'country' => 'CA' + ] + ] + ])
478-482: Fix contains check for nested array element.To assert that
meta.matrixcontains[4,5,6]as an array element (not individual scalars), wrap it as an array-of-array:- Query::contains('meta', [['matrix' => [4, 5, 6]]]) + Query::contains('meta', [['matrix' => [[4, 5, 6]]]])
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (1)
589-593: Clarify comment: query doesn't match document structure.The comment "GIN index doesn't make equal queries work differently" is misleading. The query returns 0 results because it searches for
{'env': 'production'}at the top level ofdata, but the actual structure is nested underdata.config.env. Consider clarifying:- $this->assertCount(0, $results); // Note: GIN index doesn't make equal queries work differently + $this->assertCount(0, $results); // Query structure doesn't match document nesting (env is under config)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#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.php
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (5)
src/Database/Adapter.php (1)
Adapter(16-1325)src/Database/Database.php (5)
Database(37-8211)createAttribute(1695-1750)createDocument(4237-4331)updateDocument(4783-4984)createIndex(3271-3454)src/Database/Validator/Index.php (1)
Index(10-433)src/Database/Query.php (6)
Query(8-1124)equal(435-438)contains(507-510)notEqual(447-450)select(580-583)notContains(519-522)src/Database/Adapter/Postgres.php (5)
getSupportForObject(2027-2030)createAttribute(444-463)createDocument(956-1056)updateDocument(1070-1270)createIndex(839-897)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php
80-80: Avoid unused local variables such as '$doc2'. (undefined)
(UnusedLocalVariable)
313-313: Avoid unused local variables such as '$doc7'. (undefined)
(UnusedLocalVariable)
327-327: Avoid unused local variables such as '$doc8'. (undefined)
(UnusedLocalVariable)
430-430: Avoid unused local variables such as '$doc11'. (undefined)
(UnusedLocalVariable)
563-563: Avoid unused local variables such as '$doc1'. (undefined)
(UnusedLocalVariable)
576-576: Avoid unused local variables such as '$doc2'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (10)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (10)
22-24: LGTM: Clean adapter capability check.The skip logic correctly gates the test without the unreachable return that was flagged in other methods.
16-540: Excellent comprehensive test coverage.This test method thoroughly exercises object attribute functionality including CRUD operations, nested structures, special characters, mixed types, null/empty values, and edge cases. The test progression is logical and well-commented.
548-550: LGTM: Correct skip logic.The adapter support check is properly implemented without unreachable code.
609-644: Excellent error case coverage for GIN indexes.The negative tests properly validate all three GIN index constraints: object-type-only, single-attribute, and no explicit orders. Exception handling is thorough with appropriate assertions on exception type and message content.
656-658: LGTM: Clean skip logic.Adapter support check is properly implemented.
666-706: Correct type validation tests.The three tests properly validate that non-array values (string, integer, boolean) are rejected for object attributes with StructureException. The skip guard at line 656 ensures these only run on adapters with object support.
764-795: Good edge case: large structure test.Testing a 50-element nested structure validates that the implementation handles realistic large JSONB documents. The query verification on line 783 ensures retrieval works correctly within large structures.
839-841: LGTM: Unreachable return fixed.The previous issue with an unreachable return after
markTestSkippedhas been corrected. The skip logic is now clean.
862-874: Correct validation of required attribute.The test properly validates that omitting a required object attribute (
profile) throws StructureException. The exception handling and assertions are appropriate.
876-908: Thorough default value validation.The test correctly validates all default scenarios: empty array, nested object, explicit null, and required vs non-required behavior. The query test at lines 903-907 confirms defaults are properly stored and queryable.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
2599-2607: Pass the new “object support” flag to IndexValidator.
IndexValidatornow expects the adapter’s object-support flag. This call site still uses the old signature, soupdateAttribute()will trigger anArgumentCountErroras soon as it tries to validate indexes. Add the missing$this->adapter->getSupportForObject()argument here to keep attribute updates working.$validator = new IndexValidator( $attributes, $this->adapter->getMaxIndexLength(), $this->adapter->getInternalIndexesKeys(), $this->adapter->getSupportForIndexArray(), $this->adapter->getSupportForSpatialIndexNull(), $this->adapter->getSupportForSpatialIndexOrder(), - $this->adapter->getSupportForVectors(), + $this->adapter->getSupportForVectors(), + $this->adapter->getSupportForObject(), );
♻️ Duplicate comments (1)
src/Database/Adapter/Postgres.php (1)
1607-1618: Address the unresolved issues from previous review.The past review comment on lines 1594-1648 identified critical semantic issues that remain unaddressed:
EQUAL (line 1613): Uses
@>(containment) instead of=(strict equality). This meansQuery::equal('meta', ['status' => 'active'])will match documents wheremetais{"status": "active", "other": "field"}instead of requiring an exact match. This is a semantic mismatch with the query name "equal".CONTAINS auto-wrapping (lines 1633-1635): Still wraps scalar values in arrays automatically. The previous review recommended removing this auto-wrapping so callers explicitly pass the JSON shape they want to contain.
These issues affect query semantics and should be resolved before merge.
Also applies to: 1620-1643
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/Postgres.php(7 hunks)src/Database/Adapter/SQL.php(1 hunks)src/Database/Database.php(12 hunks)src/Database/Query.php(1 hunks)src/Database/Validator/Index.php(4 hunks)src/Database/Validator/Query/Filter.php(2 hunks)src/Database/Validator/Structure.php(1 hunks)tests/e2e/Adapter/Base.php(2 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/Database/Adapter/Pool.php
- src/Database/Adapter.php
- src/Database/Adapter/SQL.php
- src/Database/Validator/Structure.php
- src/Database/Query.php
- src/Database/Adapter/MariaDB.php
- tests/e2e/Adapter/Scopes/DocumentTests.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#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.php
🧬 Code graph analysis (4)
src/Database/Validator/Query/Filter.php (2)
src/Database/Database.php (1)
Database(37-8344)src/Database/Query.php (1)
Query(8-1177)
src/Database/Adapter/Postgres.php (6)
src/Database/Database.php (2)
Database(37-8344)count(7123-7172)src/Database/Adapter.php (2)
count(825-825)getSupportForObject(1071-1071)src/Database/Query.php (2)
Query(8-1177)isObjectAttribute(983-986)src/Database/Adapter/Pool.php (2)
count(273-276)getSupportForObject(577-580)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)
src/Database/Database.php (5)
src/Database/Adapter/Postgres.php (1)
getSupportForObject(2097-2100)src/Database/Adapter/Pool.php (1)
getSupportForObject(577-580)src/Database/Adapter.php (1)
getSupportForObject(1071-1071)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)
src/Database/Validator/Index.php (2)
src/Database/Document.php (2)
Document(12-470)getAttribute(224-231)src/Database/Database.php (1)
Database(37-8344)
⏰ 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/Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Unit Test
🔇 Additional comments (10)
tests/e2e/Adapter/Base.php (1)
11-11: LGTM! Clean addition of object attribute test coverage.The import and trait usage are correctly integrated into the Base test class. The import follows alphabetical ordering, and the trait usage is appropriately positioned.
Also applies to: 30-30
src/Database/Validator/Query/Filter.php (2)
150-158: LGTM! Clean validation for object type queries.The array-only requirement is now enforced consistently across validator and adapter layers.
232-236: LGTM! Error message accurately reflects object type support.The condition and message correctly allow contains/notContains on object attributes alongside strings and spatial types.
src/Database/Adapter/Postgres.php (4)
889-890: LGTM! GIN index support properly integrated.The index creation handles GIN consistently with other special index types (spatial, HNSW), includes appropriate tenant column handling for shared tables, and uses the correct
USING GINclause for JSONB indexes.Also applies to: 896-896, 909-909
1673-1675: LGTM! Routing logic is correct.The placement and pattern match the spatial query handling above. Once the issues in
handleObjectQueriesare resolved, this integration point will work correctly.
1858-1860: LGTM! SQL type mapping is correct.
JSONBis the appropriate PostgreSQL type for object attributes, and the error message accurately reflects the expanded type support.Also applies to: 1874-1874
2092-2100: LGTM! Capability method correctly implemented.Returns
trueto indicate Postgres supports object (JSONB) attributes, following the established pattern for adapter capability flags.src/Database/Validator/Index.php (3)
27-38: LGTM! Constructor parameter follows established pattern.The
objectIndexSupportflag is wired consistently with other adapter capability parameters and defaults safely tofalse.
411-413: LGTM! Validation flow correctly extended.The
checkGinIndexcall is properly integrated into the validation chain.
442-482: LGTM! GIN index validation is thorough and consistent.The implementation correctly mirrors the patterns from
checkSpatialIndexandcheckVectorIndex:
- Single attribute requirement
- Type checking (must be TYPE_OBJECT)
- Order restrictions
- Adapter support flag validation
All error messages are clear and actionable.
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/Validator/Index.php (1)
99-137: ThecheckGinIndexmethod is never called inisValid.The new
checkGinIndexmethod (lines 535-575) is defined but never invoked in the validation flow. TheisValidmethod calls all other check methods but is missing a call tocheckGinIndex, rendering GIN index validation completely non-functional.Add the missing call:
if (!$this->checkIdenticalIndexes($value)) { return false; } + if (!$this->checkGinIndex($value)) { + return false; + } return true; }src/Database/Database.php (1)
2487-2502: Bug: malformed supportedTypes list (string concatenation inside array).This turns two entries into one string ("relationship, object"), corrupting the list and error text. Fix by making TYPE_OBJECT its own element (conditioned on adapter support).
- $supportedTypes = [ + $supportedTypes = [ self::VAR_STRING, self::VAR_INTEGER, self::VAR_FLOAT, self::VAR_BOOLEAN, self::VAR_DATETIME, - self::VAR_RELATIONSHIP . ', ' . self::TYPE_OBJECT + self::VAR_RELATIONSHIP, ]; + if ($this->adapter->getSupportForObject()) { + $supportedTypes[] = self::TYPE_OBJECT; + } if ($this->adapter->getSupportForVectors()) { $supportedTypes[] = self::VAR_VECTOR; } if ($this->adapter->getSupportForSpatialAttributes()) { \array_push($supportedTypes, ...self::SPATIAL_TYPES); } throw new DatabaseException('Unknown attribute type: ' . $type . '. Must be one of ' . implode(', ', $supportedTypes));
♻️ Duplicate comments (2)
tests/unit/Validator/IndexTest.php (1)
323-323: Missing requiredindexesparameter in Index validator constructor.Same issue as line 277 - the
indexesparameter is missing from the constructor call.Apply the fix:
- $validatorNoSupport = new Index($collection->getAttribute('attributes'), 768, [], false, false, false, false, false); + $validatorNoSupport = new Index($collection->getAttribute('attributes'), $collection->getAttribute('indexes'), 768, [], false, false, false, false, true, true, true, false);src/Database/Validator/Structure.php (1)
361-368: Array-of-objects validation is bypassed bycontinue 2.The
continue 2at line 368 skips the generic array handling logic (lines 396-417), so attributes witharray: trueand type TYPE_OBJECT are not validated properly. Specifically:
- When
array: true, the value should be validated as a list of objects (each element must be an array)- The code should enforce that the value is a proper list using
array_is_list()- Currently, a single object or array-of-objects are treated identically
This is the same issue raised in the past review comment.
Apply this approach to validate both shapes:
case Database::TYPE_OBJECT: - // For JSONB/object types, just validate it's an array (associative or list) - if (!is_array($value)) { - $this->message = 'Attribute "'.$key.'" has invalid type. Value must be an array for object type'; - return false; - } - // No additional validators needed - JSONB accepts any valid array structure - continue 2; // Skip to next attribute + if ($array) { + // Expect a list of objects (each item must be an array) + if (!\is_array($value) || !\array_is_list($value)) { + $this->message = 'Attribute "'.$key.'" must be an array of objects'; + return false; + } + foreach ($value as $i => $child) { + if (!\is_array($child)) { + $this->message = 'Attribute "'.$key.'[\''.$i.'\']" must be an object'; + return false; + } + } + continue 2; + } + // Single JSON object (associative or list) represented as PHP array + if (!\is_array($value)) { + $this->message = 'Attribute "'.$key.'" must be an object'; + return false; + } + continue 2;
🧹 Nitpick comments (4)
src/Database/Database.php (4)
2018-2028: Object attribute validation: OK, but tighten boolean checks.Logic is correct. For clarity/readability, prefer direct boolean checks over empty():
- if (!empty($size)) { + if ($size !== 0) { throw new DatabaseException('Size must be empty for object attributes'); } - if (!empty($array)) { + if ($array) { throw new DatabaseException('Object attributes cannot be arrays'); } + // Optional: clear error if default is provided in a wrong type + if ($default !== null && !\is_array($default)) { + throw new DatabaseException('Default value for object type must be an array'); + }
2434-2444: Update-attribute object validation: OK, same optional boolean/readability tweaks.Mirror the create case; minor boolean style nit as above.
- if (!empty($size)) { + if ($size !== 0) { throw new DatabaseException('Size must be empty for object attributes'); } - if (!empty($array)) { + if ($array) { throw new DatabaseException('Object attributes cannot be arrays'); }
3531-3550: GIN index validation: add a lengths check.Orders are disallowed; lengths should be too. GIN ignores column length and allowing it can confuse users.
if (!empty($orders)) { throw new IndexException('GIN indexes do not support explicit orders. Remove the orders to create this index.'); } + if (!empty($lengths)) { + throw new IndexException('GIN indexes do not support lengths. Remove the lengths to create this index.'); + }
7564-7568: Guard JSON decode for object casting.Invalid JSON will become null. Preserve original on decode failure.
- if (is_string($node)) { - $node = json_decode($node, true); - } + if (is_string($node)) { + $decoded = json_decode($node, true); + if (json_last_error() === JSON_ERROR_NONE) { + $node = $decoded; + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/SQL.php(1 hunks)src/Database/Database.php(12 hunks)src/Database/Validator/Index.php(3 hunks)src/Database/Validator/Query/Filter.php(2 hunks)src/Database/Validator/Structure.php(1 hunks)tests/e2e/Adapter/Base.php(2 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(2 hunks)tests/unit/Validator/IndexTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Database/Adapter.php
- tests/e2e/Adapter/Scopes/DocumentTests.php
- tests/e2e/Adapter/Scopes/AttributeTests.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#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.php
🧬 Code graph analysis (8)
src/Database/Validator/Query/Filter.php (2)
src/Database/Database.php (1)
Database(37-8426)src/Database/Query.php (1)
Query(8-1177)
tests/unit/Validator/IndexTest.php (2)
src/Database/Database.php (1)
Database(37-8426)src/Database/Validator/Index.php (3)
Index(10-576)isValid(99-138)getDescription(74-77)
src/Database/Validator/Structure.php (1)
src/Database/Database.php (1)
Database(37-8426)
tests/e2e/Adapter/Base.php (1)
src/Database/Adapter.php (1)
Adapter(16-1408)
src/Database/Database.php (5)
src/Database/Adapter/Pool.php (1)
getSupportForObject(583-586)src/Database/Adapter.php (1)
getSupportForObject(1078-1078)src/Database/Adapter/Postgres.php (1)
getSupportForObject(2097-2100)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)
src/Database/Adapter/SQL.php (1)
src/Database/Database.php (1)
Database(37-8426)
src/Database/Validator/Index.php (2)
src/Database/Database.php (2)
Database(37-8426)count(7193-7244)src/Database/Adapter.php (1)
count(825-825)
src/Database/Adapter/Pool.php (5)
src/Database/Adapter.php (1)
getSupportForObject(1078-1078)src/Database/Adapter/Postgres.php (1)
getSupportForObject(2097-2100)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Mirror.php (1)
delegate(88-103)
🔇 Additional comments (14)
tests/e2e/Adapter/Base.php (1)
11-11: LGTM!The ObjectAttributeTests trait import and usage cleanly extends the test coverage to include object attribute scenarios.
Also applies to: 32-32
src/Database/Adapter/SQL.php (1)
1125-1132: LGTM!The TYPE_OBJECT case correctly accounts for JSONB/JSON external storage by adding only the 20-byte pointer contribution to row size, consistent with the existing pattern for other externally-stored types.
src/Database/Adapter/Pool.php (1)
583-586: LGTM!The getSupportForObject method correctly delegates to the underlying adapter using the established pattern, consistent with other getSupportFor* methods in the class.
src/Database/Validator/Query/Filter.php (2)
165-172: LGTM!The TYPE_OBJECT case correctly enforces that query values must be arrays (representing JSON objects) and bypasses further validation, which is appropriate for JSONB attributes that accept any valid array structure.
247-251: LGTM!The contains/notContains validation correctly includes TYPE_OBJECT as an allowed attribute type, with an appropriate error message update.
src/Database/Validator/Index.php (2)
31-31: LGTM!The
objectIndexSupportparameter is correctly added to the constructor signature and documentation, following the established pattern for other support flags.Also applies to: 46-46
535-575: LGTM with caveat!The
checkGinIndeximplementation correctly validates GIN index constraints:
- Adapter support check via
objectIndexSupportflag- Single attribute requirement
- No explicit orders allowed
- Attribute must be TYPE_OBJECT
However, this method is not called in
isValid(see separate comment on lines 99-137).src/Database/Database.php (7)
50-52: Constant addition looks good.TYPE_OBJECT name and placement align with existing conventions.
76-76: New index type constant is fine.INDEX_GIN added consistently with other index constants.
1462-1475: Validator wiring LGTM.Passing getSupportForObject() into IndexValidator is correct and consistent with other capability flags.
2135-2143: Default-type recursion change for object is correct.Allowing arrays for object defaults and skipping per-item recursion is appropriate.
3492-3497: Capability gate for GIN indexes is reasonable.Message is concise; gating via getSupportForObject() matches adapter contract.
3573-3576: Passing object-support into IndexValidator here also looks correct.
2073-2088: Review comment addresses only one of three identical code blocks; verify completeness of changes.The review correctly identifies that
TYPE_OBJECTshould be included in the error message when supported, but there are three separate occurrences of thissupportedTypespattern at lines 2087, 2180, and 2501. Additionally, line 2501 inupdateAttribute()already referencesTYPE_OBJECT, but via string concatenation (self::VAR_RELATIONSHIP . ', ' . self::TYPE_OBJECT), which is inconsistent with the pattern of other types.Verify:
- Whether all three methods need the same TYPE_OBJECT handling
- Whether line 2501's string concatenation approach is intentional or a pre-existing bug requiring separate correction
- Ensure consistency across all occurrences after implementing this fix
* updated index unit tests
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 (3)
src/Database/Database.php (3)
2118-2183: Add explicit default validation for TYPE_OBJECT.Non-array defaults should error with a clear message; currently they fall into “Unknown attribute type”.
@@ protected function validateDefaultTypes(string $type, mixed $default): void switch ($type) { case self::VAR_STRING: case self::VAR_INTEGER: case self::VAR_FLOAT: case self::VAR_BOOLEAN: if ($type !== $defaultType) { throw new DatabaseException('Default value ' . $default . ' does not match given type ' . $type); } break; case self::VAR_DATETIME: if ($defaultType !== self::VAR_STRING) { throw new DatabaseException('Default value ' . $default . ' does not match given type ' . $type); } break; + case self::TYPE_OBJECT: + if ($defaultType !== 'array') { + throw new DatabaseException('Default value for object type must be an array'); + } + break; case self::VAR_VECTOR: // When validating individual vector components (from recursion), they should be numeric if ($defaultType !== 'double' && $defaultType !== 'integer') { throw new DatabaseException('Vector components must be numeric values (float or integer)'); } break;
2488-2503: Fix supported types array concatenation (error message).Concatenating VAR_RELATIONSHIP and TYPE_OBJECT into one element breaks message formatting.
- self::VAR_DATETIME, - self::VAR_RELATIONSHIP . ', ' . self::TYPE_OBJECT + self::VAR_DATETIME, + self::VAR_RELATIONSHIP, + self::TYPE_OBJECT
1387-1397: Auto-wire JSON filter for object attributes to ensure consistent encode/decode behavior.Object attributes require the
jsonfilter for proper serialization/deserialization, but unlikeSPATIAL_TYPESandVAR_VECTOR, the code does not automatically apply it. This creates an inconsistency: developers must manually add thejsonfilter to object attributes, whereas spatial and vector types are automatically filtered increateCollection(),createAttribute(), andgetRequiredFilters().Apply the proposed changes to all three locations to automatically wire the
jsonfilter whenTYPE_OBJECTis used:@@ public function createCollection(... - foreach ($attributes as &$attribute) { + foreach ($attributes as &$attribute) { if (in_array($attribute['type'], Database::SPATIAL_TYPES) || $attribute['type'] === Database::VAR_VECTOR) { $existingFilters = $attribute['filters'] ?? []; if (!is_array($existingFilters)) { $existingFilters = [$existingFilters]; } $attribute['filters'] = array_values( array_unique(array_merge($existingFilters, [$attribute['type']])) ); } + if (($attribute['type'] ?? null) === Database::TYPE_OBJECT) { + $existingFilters = $attribute['filters'] ?? []; + if (!is_array($existingFilters)) { + $existingFilters = [$existingFilters]; + } + $attribute['filters'] = array_values(array_unique(array_merge($existingFilters, ['json']))); + } }@@ public function createAttribute(... - if ($type === Database::VAR_VECTOR) { + if ($type === Database::VAR_VECTOR) { $filters[] = $type; $filters = array_unique($filters); } + if ($type === Database::TYPE_OBJECT) { + $filters[] = 'json'; + $filters = array_unique($filters); + }@@ protected function getRequiredFilters(?string $type): array - return match ($type) { - self::VAR_DATETIME => ['datetime'], - default => [], - }; + return match ($type) { + self::VAR_DATETIME => ['datetime'], + self::TYPE_OBJECT => ['json'], + default => [], + };
♻️ Duplicate comments (3)
src/Database/Adapter/Postgres.php (1)
1594-1648: ** EQUAL and CONTAINS semantics still need fixes per prior review.**The past review comment on these lines identified critical issues with JSONB query semantics:
EQUAL (lines 1607-1618) still uses
@>(containment) instead of strict equality (=): This meansQuery::equal('meta', [['age' => 25]])matches any document where meta contains{"age": 25}, not just documents where meta equals that exact object. This is incorrect semantics for EQUAL.Auto-wrapping in CONTAINS (lines 1629-1635) remains: The past comment recommended removing this auto-wrapping logic to keep the adapter strict and predictable—callers should pass the exact JSON shape they want to contain.
Progress made: NOT_EQUAL and NOT_CONTAINS were added (good!), and the separator logic for NOT_* uses AND (correct).
Still needed: Apply the past review's suggested diff to fix EQUAL to use strict
=equality and remove the auto-wrapping in CONTAINS.tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (2)
153-169: ** Fix notEqual query payload shape.**Per the past review comment,
Query::notEqualexpects a single value, not an array-of-arrays. The current usage wraps the object in an extra array:Query::notEqual('meta', [['age' => 26]]) // current - double-wrappedShould be:
Query::notEqual('meta', ['age' => 26]) // single objectSame issue on lines 162-169 with the nested object. Apply the fix from the past review comment.
872-873: ** Fix misleading comment.**The comment states "Required with default" but the attribute is created with
required = false(fifth parameter). Update the comment to accurately reflect that the attribute is optional:- // 4) Required with default (should auto-populate) + // 4) Optional with default (should auto-populate when omitted) $this->assertEquals(true, $database->createAttribute($collectionId, 'profile2', Database::TYPE_OBJECT, 0, false, ['name' => 'anon']));
🧹 Nitpick comments (1)
src/Database/Database.php (1)
7564-7569: Object decode path: consider guarding invalid JSON.If json_decode returns null (invalid JSON), keep original value to avoid silent nulling.
- if (is_string($node)) { - $node = json_decode($node, true); - } + if (is_string($node)) { + $decoded = json_decode($node, true); + if (json_last_error() === JSON_ERROR_NONE) { + $node = $decoded; + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Adapter/Postgres.php(7 hunks)src/Database/Database.php(12 hunks)src/Database/Validator/Index.php(4 hunks)tests/e2e/Adapter/Scopes/ObjectAttributeTests.php(1 hunks)tests/unit/Validator/IndexTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/Validator/IndexTest.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#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.php
🧬 Code graph analysis (4)
src/Database/Validator/Index.php (1)
src/Database/Database.php (1)
Database(37-8427)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (7)
src/Database/Adapter.php (1)
Adapter(16-1408)src/Database/Database.php (5)
Database(37-8427)createAttribute(1754-1813)createDocument(4392-4489)find(6957-7097)createIndex(3435-3606)src/Database/Validator/Index.php (1)
Index(10-579)src/Database/Validator/Structure.php (1)
Structure(18-454)src/Database/Query.php (5)
Query(8-1177)equal(452-455)contains(524-527)notEqual(464-467)select(597-600)src/Database/Adapter/Postgres.php (4)
getSupportForObject(2097-2100)createAttribute(442-471)createDocument(979-1079)createIndex(863-920)src/Database/Adapter/Pool.php (5)
getSupportForObject(583-586)createAttribute(168-171)createDocument(228-231)find(263-266)createIndex(213-216)
src/Database/Adapter/Postgres.php (8)
src/Database/Database.php (2)
Database(37-8427)count(7194-7245)src/Database/Query.php (2)
Query(8-1177)isObjectAttribute(983-986)src/Database/Adapter/Pool.php (2)
count(273-276)getSupportForObject(583-586)src/Database/Adapter/Mongo.php (2)
count(2093-2180)getSupportForObject(2791-2794)src/Database/Adapter.php (2)
count(825-825)getSupportForObject(1078-1078)src/Database/Adapter/SQL.php (1)
count(2660-2722)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)
src/Database/Database.php (6)
src/Database/Adapter/Postgres.php (1)
getSupportForObject(2097-2100)src/Database/Adapter/Pool.php (1)
getSupportForObject(583-586)src/Database/Adapter/Mongo.php (1)
getSupportForObject(2791-2794)src/Database/Adapter.php (1)
getSupportForObject(1078-1078)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php
80-80: Avoid unused local variables such as '$doc2'. (undefined)
(UnusedLocalVariable)
313-313: Avoid unused local variables such as '$doc7'. (undefined)
(UnusedLocalVariable)
327-327: Avoid unused local variables such as '$doc8'. (undefined)
(UnusedLocalVariable)
430-430: Avoid unused local variables such as '$doc11'. (undefined)
(UnusedLocalVariable)
563-563: Avoid unused local variables such as '$doc1'. (undefined)
(UnusedLocalVariable)
576-576: Avoid unused local variables such as '$doc2'. (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 (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Unit Test
🔇 Additional comments (11)
src/Database/Adapter/Postgres.php (4)
858-860: LGTM: TYPE_OBJECT correctly maps to JSONB.The mapping of
Database::TYPE_OBJECTtoJSONBis appropriate for Postgres.
889-890: LGTM: Index_Object correctly mapped to GIN index.The Index_Object type is correctly mapped to use
USING GIN, which is the appropriate index type for JSONB columns in Postgres.Also applies to: 909-909
1673-1675: LGTM: Object query routing is correct.The routing of object attribute queries through
handleObjectQueriesintegrates properly with the existing query flow.
2092-2100: LGTM: Postgres adapter correctly reports object support.Returning
trueforgetSupportForObject()is correct for Postgres with JSONB support.src/Database/Validator/Index.php (3)
31-32: LGTM: Object index support flag properly integrated.The
supportForObjectIndexesparameter follows the established pattern for adapter capability flags and correctly defaults tofalse.Also applies to: 46-47
137-139: LGTM: Object index validation properly integrated into validation flow.The
checkObjectIndexescall is correctly placed in the validation chain.
538-578: LGTM: Object index validation logic is sound.The validation correctly enforces:
- Object indexes require adapter support
- Exactly one attribute allowed
- No explicit orders permitted
- Target attribute must be of TYPE_OBJECT
This follows the same pattern as spatial and vector index validation.
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (4)
16-540: LGTM: Comprehensive test coverage for object attributes.The
testObjectAttributemethod provides excellent coverage:
- CRUD operations with nested objects
- Query variations (equal, contains, notEqual, notContains, select)
- Edge cases (null, empty, deeply nested, mixed types, special characters)
- Complex structures (arrays of objects, nested arrays)
The test assertions are thorough and the document IDs are descriptive.
542-648: LGTM: GIN index validation is thorough.The
testObjectAttributeGinIndexmethod correctly validates:
- Successful creation and usage of object indexes
- Error handling for invalid configurations (non-object attribute, multiple attributes, explicit orders)
The negative test cases with proper exception assertions are particularly valuable.
650-847: LGTM: Invalid cases and edge cases well covered.The
testObjectAttributeInvalidCasesmethod provides strong validation coverage:
- Type checking (string, integer, boolean all correctly rejected)
- Non-matching queries return empty results as expected
- Large structures (50 keys) exercise scalability
- Key ordering and complex nested structures
The test balances negative cases with valid follow-up queries to ensure the adapter remains functional.
849-927: LGTM: Default value behavior properly tested.The
testObjectAttributeDefaultsmethod validates:
- Empty object defaults
- Nested object defaults
- Required attributes without defaults (fail correctly)
- Optional attributes with defaults (auto-populate)
- Explicit null defaults
The combination of positive tests (defaults work) and negative tests (required fails when missing) provides good coverage of the defaulting logic.
…ss the database adapter and validator classes
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Database.php (2)
2488-2503: Broken supportedTypes composition (concatenation inside array).This creates one combined element like "relationship, object" instead of two entries, degrading the error message and any consumer relying on this list.
Apply this fix:
@@ - $supportedTypes = [ - self::VAR_STRING, - self::VAR_INTEGER, - self::VAR_FLOAT, - self::VAR_BOOLEAN, - self::VAR_DATETIME, - self::VAR_RELATIONSHIP . ', ' . self::VAR_OBJECT - ]; + $supportedTypes = [ + self::VAR_STRING, + self::VAR_INTEGER, + self::VAR_FLOAT, + self::VAR_BOOLEAN, + self::VAR_DATETIME, + self::VAR_RELATIONSHIP, + self::VAR_OBJECT, + ];
2526-2536: Fix typo in setAttribute call.The method call at line 2527 uses incorrect casing. Apply this fix:
- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)This is the only occurrence of this typo in the codebase.
♻️ Duplicate comments (5)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (3)
153-156: Verify Query::notEqual payload shape.The previous review flagged this payload shape as potentially incorrect. The double-nested array
[['age' => 26]]may not match the expected semantics forQuery::notEqualon object attributes.Please verify that the payload shape matches the query semantics for JSONB containment checks.
162-169: Verify Query::notEqual payload shape for nested object.Similar to the previous comment, the previous review flagged this nested structure as potentially having incorrect array nesting.
Please verify that the payload shape matches the expected query semantics.
872-873: Fix misleading comment - attribute is not required.The comment states "Required with default" but line 873 creates the attribute with
required = false(the 5th parameter).Apply this diff:
- // 4) Required with default (should auto-populate) + // 4) Optional with default (should auto-populate when omitted) $this->assertEquals(true, $database->createAttribute($collectionId, 'profile2', Database::VAR_OBJECT, 0, false, ['name' => 'anon']));src/Database/Validator/Structure.php (1)
361-368: Array-of-objects validation is still bypassed (control-flow skip).The
continue 2at line 368 short-circuits before the generic array handler (lines 396–417) runs, so attributes declared witharray: true(array of objects) are never validated element-by-element. Single vs. array-of-objects semantics remain unenforced.src/Database/Adapter/Postgres.php (1)
1607-1618: EQUAL semantics:@>is containment, not strict equality.Line 1613 uses the containment operator (
@>), which returns true for superset matches. For example,{"a":1,"b":2} @> {"a":1}is true, but that's not strict equality.Apply this diff for strict equality:
case Query::TYPE_EQUAL: - case Query::TYPE_NOT_EQUAL: { - $isNot = $query->getMethod() === Query::TYPE_NOT_EQUAL; - $conditions = []; - foreach ($query->getValues() as $key => $value) { - $binds[":{$placeholder}_{$key}"] = json_encode($value); - $fragment = "{$alias}.{$attribute} @> :{$placeholder}_{$key}::jsonb"; - $conditions[] = $isNot ? "NOT (" . $fragment . ")" : $fragment; - } - $separator = $isNot ? ' AND ' : ' OR '; - return empty($conditions) ? '' : '(' . implode($separator, $conditions) . ')'; - } + $conditions = []; + foreach ($query->getValues() as $key => $value) { + $binds[":{$placeholder}_{$key}"] = json_encode($value); + $conditions[] = "{$alias}.{$attribute} = :{$placeholder}_{$key}::jsonb"; + } + return empty($conditions) ? '' : '(' . implode(' OR ', $conditions) . ')'; + + case Query::TYPE_NOT_EQUAL: + $conditions = []; + foreach ($query->getValues() as $key => $value) { + $binds[":{$placeholder}_{$key}"] = json_encode($value); + $conditions[] = "NOT ({$alias}.{$attribute} = :{$placeholder}_{$key}::jsonb)"; + } + return empty($conditions) ? '' : '(' . implode(' AND ', $conditions) . ')';
🧹 Nitpick comments (5)
src/Database/Database.php (3)
2136-2144: Default validation: skip recursion for object arrays — LGTM with a small enhancementLogic correctly treats object defaults as arrays. Consider adding an explicit case to error if a non‑array default is provided for object, for clearer messages.
Apply this focused enhancement:
@@ - if ($defaultType === 'array') { + if ($defaultType === 'array') { // Spatial types require the array itself if (!in_array($type, Database::SPATIAL_TYPES) && $type != Database::VAR_OBJECT) { foreach ($default as $value) { $this->validateDefaultTypes($type, $value); } } return; } switch ($type) { + case self::VAR_OBJECT: + // Object defaults must be arrays (associative or list) + throw new DatabaseException('Default value for object type must be an array');
2074-2089: Optional: include object in “Unknown attribute type” list when supported.Improves diagnostic accuracy for adapters that support objects.
Apply:
@@ - if ($this->adapter->getSupportForVectors()) { + if ($this->adapter->getSupportForVectors()) { $supportedTypes[] = self::VAR_VECTOR; } if ($this->adapter->getSupportForSpatialAttributes()) { \array_push($supportedTypes, ...self::SPATIAL_TYPES); } + if ($this->adapter->getSupportForObject()) { + $supportedTypes[] = self::VAR_OBJECT; + }
2167-2182: Optional: include object in default-type “Unknown attribute type” list.Same rationale for clearer errors during default validation.
@@ - if ($this->adapter->getSupportForVectors()) { + if ($this->adapter->getSupportForVectors()) { $supportedTypes[] = self::VAR_VECTOR; } if ($this->adapter->getSupportForSpatialAttributes()) { \array_push($supportedTypes, ...self::SPATIAL_TYPES); } + if ($this->adapter->getSupportForObject()) { + $supportedTypes[] = self::VAR_OBJECT; + }tests/unit/Validator/IndexTest.php (1)
323-323: Use named parameter for clarity.Line 323 passes 9 positional parameters, setting
supportForAttributes=false(9th param). The 12th parametersupportForObjectIndexesdefaults tofalse, which happens to be correct, but the code is fragile and obscures intent.Apply this diff for maintainability:
- $validatorNoSupport = new Index($collection->getAttribute('attributes'), $collection->getAttribute('indexes', []), 768, [], false, false, false, false, false); + $validatorNoSupport = new Index( + $collection->getAttribute('attributes'), + $collection->getAttribute('indexes', []), + 768, + supportForObjectIndexes: false + );src/Database/Adapter/Postgres.php (1)
1625-1636: Auto-wrapping in CONTAINS queries obscures intent and limits flexibility.Lines 1629–1634 automatically wrap scalar values in arrays (e.g.,
"react"becomes["react"]), which:
- Prevents exact JSON-shape queries.
- Forces array-element containment semantics even when the stored JSON may be a scalar.
- Makes the API less predictable.
Remove auto-wrapping and require callers to pass the exact JSON structure they want to query:
case Query::TYPE_CONTAINS: case Query::TYPE_NOT_CONTAINS: { $isNot = $query->getMethod() === Query::TYPE_NOT_CONTAINS; $conditions = []; foreach ($query->getValues() as $key => $value) { - if (count($value) === 1) { - $jsonKey = array_key_first($value); - $jsonValue = $value[$jsonKey]; - - // If scalar (e.g. "skills" => "typescript"), - // wrap it to express array containment: {"skills": ["typescript"]} - // If it's already an object/associative array (e.g. "config" => ["lang" => "en"]), - // keep as-is to express object containment. - if (!\is_array($jsonValue)) { - $value[$jsonKey] = [$jsonValue]; - } - } $binds[":{$placeholder}_{$key}"] = json_encode($value); $fragment = "{$alias}.{$attribute} @> :{$placeholder}_{$key}::jsonb"; $conditions[] = $isNot ? "NOT (" . $fragment . ")" : $fragment; } $separator = $isNot ? ' AND ' : ' OR '; return empty($conditions) ? '' : '(' . implode($separator, $conditions) . ')'; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Database/Adapter/Postgres.php(7 hunks)src/Database/Adapter/SQL.php(1 hunks)src/Database/Database.php(12 hunks)src/Database/Query.php(1 hunks)src/Database/Validator/Index.php(5 hunks)src/Database/Validator/IndexDependency.php(1 hunks)src/Database/Validator/Queries.php(1 hunks)src/Database/Validator/Query/Base.php(1 hunks)src/Database/Validator/Query/Filter.php(2 hunks)src/Database/Validator/Structure.php(1 hunks)tests/e2e/Adapter/Scopes/ObjectAttributeTests.php(1 hunks)tests/unit/Validator/IndexTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Database/Adapter/SQL.php
- src/Database/Query.php
- src/Database/Validator/Query/Filter.php
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 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/Structure.phptests/unit/Validator/IndexTest.phptests/e2e/Adapter/Scopes/ObjectAttributeTests.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/Structure.phptests/unit/Validator/IndexTest.phpsrc/Database/Adapter/Postgres.phptests/e2e/Adapter/Scopes/ObjectAttributeTests.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-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
🧬 Code graph analysis (6)
src/Database/Validator/Structure.php (1)
src/Database/Database.php (1)
Database(37-8427)
tests/unit/Validator/IndexTest.php (2)
src/Database/Database.php (1)
Database(37-8427)src/Database/Validator/Index.php (1)
Index(10-579)
src/Database/Adapter/Postgres.php (7)
src/Database/Database.php (1)
Database(37-8427)src/Database/Query.php (2)
Query(8-1177)isObjectAttribute(983-986)src/Database/Adapter/Mongo.php (1)
getSupportForObject(2791-2794)src/Database/Adapter/Pool.php (1)
getSupportForObject(583-586)src/Database/Adapter.php (1)
getSupportForObject(1078-1078)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (6)
src/Database/Adapter.php (1)
Adapter(16-1408)src/Database/Database.php (4)
Database(37-8427)getDocument(3658-3782)count(7194-7245)createIndex(3435-3606)src/Database/Validator/Index.php (1)
Index(10-579)src/Database/Validator/Structure.php (1)
Structure(18-454)src/Database/Query.php (6)
Query(8-1177)equal(452-455)contains(524-527)notEqual(464-467)select(597-600)notContains(536-539)src/Database/Adapter/Postgres.php (2)
getSupportForObject(2097-2100)createIndex(863-920)
src/Database/Database.php (6)
src/Database/Adapter/Postgres.php (1)
getSupportForObject(2097-2100)src/Database/Adapter/Mongo.php (1)
getSupportForObject(2791-2794)src/Database/Adapter/Pool.php (1)
getSupportForObject(583-586)src/Database/Adapter.php (1)
getSupportForObject(1078-1078)src/Database/Adapter/SQLite.php (1)
getSupportForObject(976-979)src/Database/Adapter/MariaDB.php (1)
getSupportForObject(1852-1855)
src/Database/Validator/Index.php (1)
src/Database/Database.php (1)
Database(37-8427)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php
80-80: Avoid unused local variables such as '$doc2'. (undefined)
(UnusedLocalVariable)
313-313: Avoid unused local variables such as '$doc7'. (undefined)
(UnusedLocalVariable)
327-327: Avoid unused local variables such as '$doc8'. (undefined)
(UnusedLocalVariable)
430-430: Avoid unused local variables such as '$doc11'. (undefined)
(UnusedLocalVariable)
563-563: Avoid unused local variables such as '$doc1'. (undefined)
(UnusedLocalVariable)
576-576: Avoid unused local variables such as '$doc2'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (17)
src/Database/Validator/IndexDependency.php (1)
83-83: LGTM!The constant rename from
self::TYPE_OBJECTtoself::VAR_OBJECTcorrectly aligns with the newDatabase::VAR_OBJECTconstant introduced for object-type support.src/Database/Validator/Index.php (4)
31-31: LGTM!The
$supportForObjectIndexesparameter addition follows the established pattern for adapter capability flags, with a safe default offalse.Also applies to: 46-46
67-67: LGTM!The constant rename to
self::VAR_OBJECTcorrectly aligns with the new object-type constant naming convention.
137-139: LGTM!The
checkObjectIndexesvalidation is properly integrated into the validation chain, following the established pattern of early-return on failure.
538-578: LGTM!The
checkObjectIndexesimplementation correctly validates object index constraints:
- Ensures adapter support via
$supportForObjectIndexesflag- Enforces single-attribute requirement
- Prohibits explicit orders
- Confirms attribute type is
VAR_OBJECTThe validation logic follows the established pattern from spatial and vector index checks. Based on learnings.
src/Database/Database.php (9)
50-52: Add VAR_OBJECT constant — LGTMConsistent with existing VAR_* types; no issues.
76-78: Add INDEX_OBJECT constant — LGTMNaming aligns with INDEX_* convention; resolves prior casing feedback.
1463-1476: Wire adapter object support into IndexValidator — LGTMPassing getSupportForObject() keeps validators capability‑aware.
2019-2029: Object attribute validation rules — LGTMChecks for adapter support, zero size, and non‑array are correct and mirror spatial handling.
2435-2446: updateAttribute: object validation — LGTMMirrors create-path checks; good consistency.
3493-3498: New INDEX_OBJECT branch — LGTMProperly guarded by adapter capability.
3532-3551: Object index constraints — LGTM
- Single attribute enforced
- Attribute existence/type verified
- Orders disallowed
Matches expected JSON/JSONB GIN semantics.
3564-3577: Validator wiring for object indexes — LGTMKeeps validation consistent across create/update code paths.
7565-7569: casting(): decode JSON string for object — LGTMNon‑casting adapters will now return arrays for object fields.
src/Database/Adapter/Postgres.php (3)
889-910: LGTM: Correct GIN index usage for JSONB.The
INDEX_OBJECTcase correctly usesUSING GIN, which is PostgreSQL's standard index method for JSONB columns and supports efficient containment queries.
1858-1859: LGTM: Correct SQL type for object attributes.Mapping
VAR_OBJECTtoJSONBis appropriate. JSONB is PostgreSQL's binary JSON format with indexing support.
1673-1675: LGTM: Correct integration and capability reporting.Lines 1673–1675 properly route object queries to
handleObjectQueries(). Lines 2092–2100 correctly report PostgreSQL's JSONB support.Also applies to: 2092-2100
| // Test 3: Query with equal on indexed JSONB column | ||
| $results = $database->find($collectionId, [ | ||
| Query::equal('data', [['env' => 'production']]) | ||
| ]); | ||
| $this->assertCount(0, $results); // Note: Object index doesn't make equal queries work differently |
There was a problem hiding this comment.
Fix query or update comment - query doesn't match document structure.
The query searches for top-level 'env' key in data, but the documents store it as data.config.env. This query returns 0 results because the path doesn't match, not because "Object index doesn't make equal queries work differently."
Apply this diff to fix the query to match the document structure:
- // Test 3: Query with equal on indexed JSONB column
+ // Test 3: Query with equal on nested path in indexed JSONB column
$results = $database->find($collectionId, [
- Query::equal('data', [['env' => 'production']])
+ Query::equal('data', [['config' => ['env' => 'production']]])
]);
- $this->assertCount(0, $results); // Note: Object index doesn't make equal queries work differently
+ $this->assertCount(1, $results);
+ $this->assertEquals('gin1', $results[0]->getId());🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/ObjectAttributeTests.php around lines 589 to 593,
the test queries the wrong path (it looks for top-level 'env' in data) so it
returns zero; update the Query to target the nested field (e.g.
Query::equal('data.config.env', 'production')) and adjust the expected assertion
count to match the document(s) (e.g. assertCount(1, $results)) so the query
matches the stored structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Database/Validator/ObjectValidator.php (1)
22-25: Consider adding explicit parentheses for clarity.While the operator precedence is correct (
&&binds tighter than||), explicit parentheses would make the intent clearer.Apply this diff:
- return empty($value) || is_array($value) && !array_is_list($value); + return empty($value) || (is_array($value) && !array_is_list($value));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Validator/Index.php(4 hunks)src/Database/Validator/ObjectValidator.php(1 hunks)src/Database/Validator/Structure.php(1 hunks)tests/unit/Validator/ObjectTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Validator/Structure.php
🧰 Additional context used
🧬 Code graph analysis (3)
src/Database/Validator/ObjectValidator.php (4)
src/Database/Validator/Index.php (4)
getDescription(74-77)isValid(99-141)isArray(86-89)getType(65-68)src/Database/Validator/Structure.php (4)
getDescription(194-197)isValid(208-242)isArray(433-436)getType(445-448)src/Database/Validator/Query/Base.php (3)
getDescription(25-28)isArray(37-40)getType(49-52)src/Database/Validator/Queries.php (4)
getDescription(44-47)isValid(53-148)isArray(157-160)getType(169-172)
tests/unit/Validator/ObjectTest.php (1)
src/Database/Validator/ObjectValidator.php (2)
ObjectValidator(7-42)isValid(22-25)
src/Database/Validator/Index.php (2)
src/Database/Database.php (2)
Database(37-8427)count(7194-7245)src/Database/Adapter.php (1)
count(825-825)
🔇 Additional comments (9)
src/Database/Validator/ObjectValidator.php (3)
9-15: LGTM!The description is clear and follows the established pattern used in other validators.
27-33: LGTM!Correctly returns
falseas objects are not arrays in this validation framework.
35-41: LGTM!Correctly returns
TYPE_OBJECTconstant, consistent with the validator's purpose.tests/unit/Validator/ObjectTest.php (3)
14-37: LGTM!These test cases correctly validate associative arrays, nested structures, null values, and mixed key types. The validation logic properly handles all these scenarios.
Also applies to: 42-46
49-60: LGTM!All invalid test cases correctly verify that list arrays, non-arrays, and numeric-only indexed arrays are rejected by the validator.
62-69: LGTM!Empty array handling is correct (allowed by
empty($value)check), and the string rejection is properly validated.src/Database/Validator/Index.php (3)
31-31: LGTM!The
$supportForObjectIndexesparameter follows the established pattern for feature flags, with a safe default value offalse.Also applies to: 46-46
137-139: LGTM!The object index validation is properly integrated into the validation chain, consistent with other index type checks.
538-578: LGTM!The
checkObjectIndexes()implementation is well-structured and consistent with other specialized index validators (vector, spatial). The validation logic correctly enforces:
- Single attribute constraint
- No explicit orders
- Attribute must be of type
VAR_OBJECT- Feature support check
The error messages are clear and actionable.
| } | ||
| } | ||
|
|
||
| public function testCreateDateTimeAttributeFailure(): void |
There was a problem hiding this comment.
removed this as it was creating datetime attribute without filters but now automatic filters to attributes are added
Summary by CodeRabbit
Release Notes
New Features
Tests