Object(json) attribute support for postgres #741
Conversation
WalkthroughAdds first-class object (JSON/JSONB) attribute support: new Database type and INDEX_GIN, adapter capability flags and Pool delegation, Postgres JSONB SQL/query handling and GIN index DDL, validation updates (attributes, queries, indexes), width accounting, and new e2e/unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Q as Query
participant DB as Database
participant V as Validator
participant A as Adapter(Postgres)
Note over Q,DB: Query targets attribute typed TYPE_OBJECT
Q->>DB: isObjectAttribute()
DB->>V: validate attribute/index (object & GIN checks)
V-->>DB: validation result
DB->>A: getSQLCondition(...) for object attribute
alt object condition path
A->>A: handleObjectQueries() → JSONB operators (=, !=, @>, <@)
A-->>DB: SQL fragment (may include USING GIN in DDL)
end
DB->>DB: execute SQL / return results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/Database/Database.php (5)
⏰ 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)
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.
src/Database/Database.php
Outdated
| // 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 |
There was a problem hiding this comment.
Remove debug output and stop falling through to the vector case.
var_dump($defaultType) will leak debug output in production, and the missing break; means any valid object default falls into the vector branch and immediately throws (“Vector components must be numeric”). As written, every non-array default for type: object is unusable. Please delete the debug call and add a break; after the array check so object defaults validate correctly.
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
+ break;
case self::VAR_VECTOR:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 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 | |
| // Object types expect arrays as default values | |
| if ($defaultType !== 'array') { | |
| throw new DatabaseException('Default value for object type must be an array'); | |
| } | |
| break; |
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 2153 to 2158, remove the stray debug
var_dump($defaultType) and add a break after the array-type check so object-type
default handling does not fall through to the vector branch; specifically delete
the var_dump call and insert a break; immediately after confirming $defaultType
=== 'array' so valid object defaults are accepted and do not trigger the vector
error.
1 participant
Summary by CodeRabbit
New Features
Behavior / Validation
Tests