Object attribute query and index #785
Conversation
📝 WalkthroughWalkthroughThis PR adds support for nested object attribute paths using dot notation (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Validator
participant Adapter
participant Engine
Client->>Database: createIndex(nested.path)
Database->>Validator: checkValidIndex(nested.path)
Validator->>Validator: Extract base attribute from nested.path
Validator->>Validator: Verify base attribute is OBJECT type
Validator-->>Database: Valid
Database->>Adapter: createIndex with processed path
Adapter->>Adapter: Convert nested.path to engine expression (JSONB/dot notation)
Adapter->>Engine: CREATE INDEX on expression
Engine-->>Adapter: Index created
Adapter-->>Database: Success
Database-->>Client: Index created
sequenceDiagram
participant Client
participant Database
participant QueryConverter
participant Validator
participant Adapter
participant Engine
Client->>Database: convertQuery(nested.path = value)
Database->>QueryConverter: Detect nested path in query attribute
QueryConverter->>QueryConverter: Extract base attribute
QueryConverter->>QueryConverter: Resolve base attribute type (OBJECT)
QueryConverter-->>Database: Force attribute type to OBJECT
Database->>Validator: Validate nested path as string attribute
Validator-->>Database: Valid
Database->>Adapter: Generate SQL/filter for nested.path
Adapter->>Adapter: Convert to JSONB expression or dot notation
Adapter->>Engine: Execute query with nested path expression
Engine-->>Adapter: Results
Adapter-->>Database: Documents
Database-->>Client: Filtered results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Validator/Query/Filter.php (1)
86-189: Bug: dotted-object path values skip validation due tocontinue 2.When
$isDottedOnObjectis true you create aTextvalidator butcontinue 2;bypasses the actual$validator->isValid($value)check. This can let non-strings through and blow up later in adapters (e.g., string ops for LIKE/regex).Proposed fix
@@ case Database::VAR_OBJECT: // For dotted attributes on objects, validate as string (path queries) if ($isDottedOnObject) { $validator = new Text(0, 0); - continue 2; + break; } - // object containment queries on the base object attribute - elseif (\in_array($method, [Query::TYPE_EQUAL, Query::TYPE_NOT_EQUAL, Query::TYPE_CONTAINS, Query::TYPE_NOT_CONTAINS], true) + // object containment queries on the base object attribute + if (\in_array($method, [Query::TYPE_EQUAL, Query::TYPE_NOT_EQUAL, Query::TYPE_CONTAINS, Query::TYPE_NOT_CONTAINS], true) && !$this->isValidObjectQueryValues($value)) { $this->message = 'Invalid object query structure for attribute "' . $attribute . '"'; return false; } continue 2;
🤖 Fix all issues with AI agents
In `@src/Database/Validator/Index.php`:
- Around line 768-776: Both helper methods lack return type declarations causing
PHPStan failures; add explicit return types: change isDottedAttribute(string
$attribute) to return bool and change getBaseAttributeFromDottedAttribute(string
$attribute) to return string (i.e., add ": bool" and ": string" to the
respective method signatures), keeping the current logic intact.
- Around line 169-180: The current loop in Index.php incorrectly iterates over
all index attributes and enforces object-type on non-dotted attributes; change
the loop to iterate only over $dottedAttributes (the result of array_filter) so
the object-type check runs only for dotted attributes, derive $baseAttribute via
getBaseAttributeFromDottedAttribute($attribute) as already intended, and keep
the existing error message/return false when the base attribute exists but is
not Database::VAR_OBJECT.
In `@tests/e2e/Adapter/Scopes/ObjectAttributeTests.php`:
- Around line 1180-1190: Remove the redundant if-check around the object-index
test: the outer test already skipped when getSupportForObjectIndexes() is false,
so delete the conditional "if
($database->getAdapter()->getSupportForObjectIndexes()) {" and its matching
closing brace, leaving the try/catch that creates the index
'idx_profile_nested_object' with Database::INDEX_OBJECT and the subsequent
assertions intact; make sure the block still catches Exception $e and asserts
InstanceOf IndexException and that $exceptionThrown is asserted true.
🧹 Nitpick comments (4)
src/Database/Adapter/Postgres.php (1)
1751-1772: Good nested-path handling; consider aliasingSEARCH/NOT_SEARCHfor consistency.
filterObjectPath()+ skippinghandleObjectQueries()for nested paths makes sense. One gotcha:TYPE_SEARCH/TYPE_NOT_SEARCHusesregexp_replace({$attribute}, ...)without the table alias, which can become ambiguous in joined queries; using{$alias}.{$attribute}would be safer for both simple and nested paths.Proposed tweak
@@ case Query::TYPE_SEARCH: $binds[":{$placeholder}_0"] = $this->getFulltextValue($query->getValue()); - return "to_tsvector(regexp_replace({$attribute}, '[^\w]+',' ','g')) @@ websearch_to_tsquery(:{$placeholder}_0)"; + return "to_tsvector(regexp_replace({$alias}.{$attribute}, '[^\w]+',' ','g')) @@ websearch_to_tsquery(:{$placeholder}_0)"; case Query::TYPE_NOT_SEARCH: $binds[":{$placeholder}_0"] = $this->getFulltextValue($query->getValue()); - return "NOT (to_tsvector(regexp_replace({$attribute}, '[^\w]+',' ','g')) @@ websearch_to_tsquery(:{$placeholder}_0))"; + return "NOT (to_tsvector(regexp_replace({$alias}.{$attribute}, '[^\w]+',' ','g')) @@ websearch_to_tsquery(:{$placeholder}_0))";tests/unit/Validator/IndexTest.php (1)
328-439: Test coverage is solid; small readability nits.
$validNestedObjectIndexis assertedfalse→ rename to$invalidNestedObjectIndex.- Consider using named args for the
new Index(...)constructor here (positional booleans are brittle).Minimal rename
@@ - $validNestedObjectIndex = new Document([ + $invalidNestedObjectIndex = new Document([ '$id' => ID::custom('idx_nested_object'), 'type' => Database::INDEX_OBJECT, 'attributes' => ['data.key.nestedKey'], 'lengths' => [], 'orders' => [], ]); - $this->assertFalse($validator->isValid($validNestedObjectIndex)); + $this->assertFalse($validator->isValid($invalidNestedObjectIndex));src/Database/Database.php (2)
8147-8162: Minor perf/clarity: compute the base attribute once for nested-object query inference.Right now
explode()happens inside the loop; you can compute$baseAttributeonce and reuse$queryAttributeconsistently.
3638-3715: Make base-attribute matching more robust for dot-path indexes (key fallback + case-insensitive compare).
validateAttribute()treats attribute IDs case-insensitively, butcreateIndex()matches$baseAttragainstkeywith a case-sensitive comparison. That can leave$indexAttributesWithTypesincomplete (and skip length/array adjustments) if callers pass a different case, or if an attribute lackskey.Proposed tweak (more defensive matching)
- $baseAttr = $attr; - if (\str_contains($attr, '.')) { - $baseAttr = \explode('.', $attr, 2)[0] ?? $attr; - } + $baseAttr = \str_contains($attr, '.') + ? \explode('.', $attr, 2)[0] + : $attr; + $baseAttrLc = \strtolower($baseAttr); foreach ($collectionAttributes as $collectionAttribute) { - if ($collectionAttribute->getAttribute('key') === $baseAttr) { + $collectionAttrKey = (string)$collectionAttribute->getAttribute('key', $collectionAttribute->getId()); + if (\strtolower($collectionAttrKey) === $baseAttrLc) { $attributeType = $collectionAttribute->getAttribute('type'); $indexAttributesWithTypes[$attr] = $attributeType;#!/bin/bash set -euo pipefail # Ensure Adapter::createIndex signature/arity matches this call (and all implementations are updated). rg -nP '\bfunction\s+createIndex\s*\(' rg -nP '->createIndex\s*\('
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Database/Adapter/Mongo.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Database.phpsrc/Database/Validator/Index.phpsrc/Database/Validator/Query/Filter.phptests/e2e/Adapter/Scopes/ObjectAttributeTests.phptests/e2e/Adapter/Scopes/SchemalessTests.phptests/unit/Validator/IndexTest.php
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/unit/Validator/IndexTest.phpsrc/Database/Adapter/Mongo.phptests/e2e/Adapter/Scopes/SchemalessTests.phptests/e2e/Adapter/Scopes/ObjectAttributeTests.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Adapter/Mongo.phptests/e2e/Adapter/Scopes/SchemalessTests.php
📚 Learning: 2025-10-03T01:50:11.943Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/AttributeTests.php:1329-1334
Timestamp: 2025-10-03T01:50:11.943Z
Learning: MongoDB has a 1024kb (1,048,576 bytes) limit for index entries. The MongoDB adapter's getMaxIndexLength() method should return this limit rather than 0.
Applied to files:
src/Database/Adapter/Mongo.php
📚 Learning: 2025-11-25T10:46:37.666Z
Learnt from: fogelito
Repo: utopia-php/database PR: 763
File: src/Database/Database.php:8671-8751
Timestamp: 2025-11-25T10:46:37.666Z
Learning: In Utopia\Database\Database::processRelationshipQueries, ensure the system select for '$id' is added only when the collection has relationship attributes (i.e., !empty($relationships)), and avoid capturing the unused '$idAdded' from Query::addSelect to satisfy static analysis.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
Repo: utopia-php/database PR: 733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
tests/e2e/Adapter/Scopes/ObjectAttributeTests.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-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/Query/Filter.php
🧬 Code graph analysis (6)
src/Database/Validator/Index.php (1)
src/Database/Database.php (1)
Database(37-8812)
src/Database/Adapter/Mongo.php (2)
src/Database/Adapter.php (1)
filter(1255-1264)src/Database/Adapter/SQL.php (1)
getInternalKeyForAttribute(2383-2395)
tests/e2e/Adapter/Scopes/SchemalessTests.php (1)
src/Database/Adapter/Mongo.php (3)
getSupportForAttributes(2792-2795)createCollection(408-572)createIndex(907-1034)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (6)
src/Database/Database.php (2)
Database(37-8812)getDatabase(914-917)src/Database/Adapter.php (2)
getDatabase(162-165)getSupportForObjectIndexes(1087-1087)src/Database/Adapter/Postgres.php (1)
getSupportForObjectIndexes(2241-2244)src/Database/Adapter/MySQL.php (1)
getSupportForObjectIndexes(256-259)src/Database/Adapter/SQLite.php (1)
getSupportForObjectIndexes(1021-1024)src/Database/Adapter/MariaDB.php (1)
getSupportForObjectIndexes(2148-2151)
src/Database/Adapter/Postgres.php (3)
src/Database/Adapter.php (2)
filter(1255-1264)quote(469-469)src/Database/Query.php (2)
isObjectAttribute(999-1002)getAttribute(182-185)src/Database/Document.php (1)
getAttribute(224-231)
src/Database/Validator/Query/Filter.php (2)
src/Database/Database.php (1)
Database(37-8812)src/Database/Query.php (1)
Query(8-1237)
🪛 GitHub Actions: CodeQL
src/Database/Validator/Index.php
[error] 768-768: isDottedAttribute() has no return type specified. (PHPStan: missing return type)
[error] 773-773: getBaseAttributeFromDottedAttribute() has no return type specified. (PHPStan: missing return type)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php
[error] 1181-1181: If condition is always true. (PHPStan: always-true conditional)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
🔇 Additional comments (7)
src/Database/Adapter/Postgres.php (1)
884-899: No action needed. The code correctly handles nested path indexing. The$indexAttributesWithTypesparameter passed fromDatabase.php::createIndex()is keyed by the full dotted attribute path, so theisset($indexAttributeTypes[$attr])check inPostgres.phpworks as intended.src/Database/Validator/Index.php (3)
251-266: LGTM!The validation logic correctly allows dotted attributes when object indexes are supported and the base attribute exists.
401-403: LGTM!Correctly resolves dotted attributes to their base for index length validation.
749-755: LGTM!Correctly enforces that
INDEX_OBJECTcan only be created on top-level object attributes, preventing invalid GIN indexes on nested paths.tests/e2e/Adapter/Scopes/SchemalessTests.php (1)
1869-1958: LGTM!Comprehensive test coverage for dot-notation indexing in schemaless adapters:
- Correctly gates test for schemaless-only execution
- Validates both KEY and UNIQUE index creation on nested paths
- Verifies unique constraint enforcement with expected
DuplicateException- Tests dot-notation query functionality
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (2)
8-8: LGTM!Import correctly added for
DuplicateExceptionused in the new tests.
1205-1333: LGTM!Excellent test coverage for querying nested object attributes:
- Tests multiple query operators (
equal,startsWith,endsWith,contains,notEndsWith)- Validates logical combinations (
AND,OR)- Properly seeds test data with varied nested structures
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| protected function convertObjectPathToJSONB(string $path): string | ||
| { | ||
| $parts = \explode('.', $path); | ||
|
|
||
| // validating the parts so that there is no injection | ||
| foreach ($parts as $part) { | ||
| if (!preg_match('/^[a-zA-Z0-9_]+$/', $part)) { | ||
| throw new DatabaseException('Invalid JSON key '.$part); | ||
| } | ||
| } | ||
|
|
||
| if (\count($parts) === 1) { | ||
| $baseColumn = $this->filter($parts[0]); | ||
| return "\"{$baseColumn}\""; | ||
| } | ||
|
|
||
| $baseColumn = $this->filter($parts[0]); | ||
| $baseColumnQuoted = "\"{$baseColumn}\""; | ||
| $nestedPath = \array_slice($parts, 1); | ||
|
|
||
| // (data->'key'->>'nestedKey')::text | ||
| $chain = $baseColumnQuoted; | ||
| $lastKey = \array_pop($nestedPath); | ||
|
|
||
| foreach ($nestedPath as $key) { | ||
| $chain .= "->'" . $key . "'"; | ||
| } | ||
|
|
||
| return "(({$chain}->>'" . $lastKey . "')::text)"; | ||
| } | ||
|
|
||
| protected function filterObjectPath(string $path): string | ||
| { | ||
| $parts = \explode('.', $path); | ||
|
|
||
| // validating the parts so that there is no injection | ||
| foreach ($parts as $part) { | ||
| if (!preg_match('/^[a-zA-Z0-9_]+$/', $part)) { | ||
| throw new DatabaseException('Invalid JSON key '.$part); | ||
| } | ||
| } | ||
|
|
||
| if (\count($parts) === 1) { | ||
| $baseColumn = $this->filter($parts[0]); | ||
| return $this->quote($baseColumn); | ||
| } | ||
|
|
||
| $baseColumn = $this->filter($parts[0]); | ||
| $baseColumnQuoted = "\"{$baseColumn}\""; | ||
| $nestedPath = \array_slice($parts, 1); | ||
|
|
||
| // (data->'key'->>'nestedKey')::text | ||
| $chain = $baseColumnQuoted; | ||
| $lastKey = \array_pop($nestedPath); | ||
|
|
||
| foreach ($nestedPath as $key) { | ||
| $chain .= "->'" . $key . "'"; | ||
| } | ||
|
|
||
| return "{$chain}->>'{$lastKey}'"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me locate and examine the Postgres.php file
git ls-files | grep -E "(Adapter|Postgres)" | head -20Repository: utopia-php/database
Length of output: 753
🏁 Script executed:
# Check the structure of the database module
git ls-files src/Database/Adapter/ | head -20Repository: utopia-php/database
Length of output: 282
🏁 Script executed:
# Let me read the Postgres.php file around the mentioned lines
wc -l src/Database/Adapter/Postgres.phpRepository: utopia-php/database
Length of output: 101
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '2841,2901p' src/Database/Adapter/Postgres.phpRepository: utopia-php/database
Length of output: 1919
🏁 Script executed:
# Look at the filter method in the base Adapter class
grep -n "public function filter\|protected function filter" src/Database/Adapter.php | head -5Repository: utopia-php/database
Length of output: 117
🏁 Script executed:
# Let's read the filter method implementation
sed -n '1,100p' src/Database/Adapter.php | grep -A 20 "function filter"Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Find the filter method in Adapter.php
grep -n "public function filter" src/Database/Adapter.phpRepository: utopia-php/database
Length of output: 117
🏁 Script executed:
# Read the filter method implementation
sed -n '1255,1280p' src/Database/Adapter.phpRepository: utopia-php/database
Length of output: 617
🏁 Script executed:
# Let me also check if there's a quote() method
grep -n "public function quote\|protected function quote" src/Database/Adapter.phpRepository: utopia-php/database
Length of output: 129
🏁 Script executed:
# Find the quote() method implementation in Postgres.php
grep -n "protected function quote" src/Database/Adapter/Postgres.phpRepository: utopia-php/database
Length of output: 120
🏁 Script executed:
# Read the quote implementation
sed -n '2565,2580p' src/Database/Adapter/Postgres.phpRepository: utopia-php/database
Length of output: 908
🏁 Script executed:
# Search for usage of convertObjectPathToJSONB and filterObjectPath
rg "convertObjectPathToJSONB|filterObjectPath" --type php -B 2 -A 2Repository: utopia-php/database
Length of output: 1716
🏁 Script executed:
# Read the quote method in Postgres
sed -n '2211,2225p' src/Database/Adapter/Postgres.phpRepository: utopia-php/database
Length of output: 335
🏁 Script executed:
# Check if there are any tests for these methods with hyphens or special characters
rg "convertObjectPathToJSONB|filterObjectPath" tests/ -B 3 -A 3Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Let's also verify the escaping in the methods - check if manually adding quotes is a concern
# Look for any tests using hyphens in attribute keys with nested objects
rg "\-" tests/e2e/Adapter/Scopes/DocumentTests.php | head -20Repository: utopia-php/database
Length of output: 2097
🏁 Script executed:
# Let's check if there are any test cases with hyphens in object attributes
rg "test.*[Oo]bject.*path|test.*nested.*object" tests/e2e/Adapter/Scopes/ -lRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look at DocumentTests for any hyphenated attributes
rg "(\-|hyphen)" tests/e2e/Adapter/Scopes/DocumentTests.php | head -20Repository: utopia-php/database
Length of output: 2097
🏁 Script executed:
# Check how filter() is actually used and what attributes are tested with it
rg "filter\(" src/Database/Adapter/Postgres.php -B 2 -A 2 | head -40Repository: utopia-php/database
Length of output: 1393
🏁 Script executed:
# Let's search for any attribute creation with hyphens in tests
rg "createAttribute.*[\-]" tests/e2e/Adapter/ -B 1 -A 1Repository: utopia-php/database
Length of output: 7139
🏁 Script executed:
# Look for tests that use hyphenated attribute names with nested objects
rg "with-dash|as-5dasdasdas" tests/e2e/Adapter/Scopes/DocumentTests.php -B 3 -A 3Repository: utopia-php/database
Length of output: 8200
🏁 Script executed:
# Check if there are tests that query nested objects with hyphens
rg "Query\.|getDocument|find" tests/e2e/Adapter/Scopes/DocumentTests.php | rg "with-dash" -B 2 -A 2Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Let's search more broadly for any nested object queries in DocumentTests
rg "getDocument.*with-dash|Query.*with-dash" tests/e2e/Adapter/Scopes/DocumentTests.php -B 2 -A 2Repository: utopia-php/database
Length of output: 619
🏁 Script executed:
# Let's find if there are any object-type attributes used with hyphens
rg "VAR_OBJECT" tests/e2e/Adapter/Scopes/DocumentTests.php -B 1 -A 1 | head -40Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Let's verify the exact behavior of the regex pattern and what filter() allows
# Check if attributes with hyphens can actually be used in nested paths
rg "getDocument.*\.|find.*\." tests/e2e/Adapter/Scopes/DocumentTests.php | grep -v "^Binary" | head -20Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Search for any nested object attribute tests
rg "VAR_OBJECT.*createAttribute|createAttribute.*VAR_OBJECT" tests/e2e/Adapter/Scopes/ -B 1 -A 1Repository: utopia-php/database
Length of output: 5166
🏁 Script executed:
# Let's also verify the regex escaping - check if the character class needs escaping
python3 << 'EOF'
import re
# Current regex in convertObjectPathToJSONB and filterObjectPath
current_regex = '/^[a-zA-Z0-9_]+$/'
# Proposed regex with hyphen
proposed_regex = '/^[a-zA-Z0-9_\-]+$/'
# Test patterns
test_values = ['data', 'with-dash', 'as-5', 'normal_key', 'with_underscore', '123-456']
# Test with current pattern (no hyphen)
current_pattern = re.compile(r'^[a-zA-Z0-9_]+$')
print("Current regex (no hyphen):")
for val in test_values:
matches = bool(current_pattern.match(val))
print(f" {val}: {matches}")
# Test with proposed pattern (with hyphen)
proposed_pattern = re.compile(r'^[a-zA-Z0-9_\-]+$')
print("\nProposed regex (with hyphen escape):")
for val in test_values:
matches = bool(proposed_pattern.match(val))
print(f" {val}: {matches}")
# Test simpler form without escape
simple_pattern = re.compile(r'^[a-zA-Z0-9_-]+$')
print("\nAlternative (hyphen at end):")
for val in test_values:
matches = bool(simple_pattern.match(val))
print(f" {val}: {matches}")
EOFRepository: utopia-php/database
Length of output: 149
🏁 Script executed:
# Check if convertObjectPathToJSONB is actually used with attributes containing hyphens
# by looking at how attributes are validated when stored
rg "convertObjectPathToJSONB|filterObjectPath" src/Database/Adapter/Postgres.php -B 5 -A 5 | grep -A 10 "convertObjectPathToJSONB"Repository: utopia-php/database
Length of output: 943
🏁 Script executed:
# Check if there are any tests using nested object paths (like 'meta.key' or 'data.key')
rg "meta\.|data\.|profile\.|settings\." tests/e2e/Adapter/Scopes/ObjectAttributeTests.php -B 2 -A 2 | head -60Repository: utopia-php/database
Length of output: 2320
🏁 Script executed:
# Look for Query operations on nested object attributes
rg "Query::(equal|lessThan|greaterThan)" tests/e2e/Adapter/Scopes/ObjectAttributeTests.php -B 2 -A 2Repository: utopia-php/database
Length of output: 7909
🏁 Script executed:
# Check what happens when attributes with hyphens exist - can they be accessed as nested paths?
rg "\.with-dash|with-dash\." tests/e2e/Adapter/Scopes/ -B 2 -A 2Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Let me check if the two methods are ever actually called with paths that contain hyphens
# by looking at the actual SQL construction
python3 << 'EOF'
# Verify the regex escaping differences
import re
# In PHP regex, within character class:
# \- is hyphen (escaped, but not needed at end)
# - at start or end doesn't need escaping
# - in middle needs escaping or must be at start/end
# Current pattern in code
current = r'^[a-zA-Z0-9_]+$'
# Proposed with escaping
proposed_escaped = r'^[a-zA-Z0-9_\-]+$'
# Alternative - hyphen at end (doesn't need escaping)
proposed_end = r'^[a-zA-Z0-9_-]+$'
test_cases = ['normal', 'with-dash', 'test_123', '-start', 'end-', 'mid-dle']
current_re = re.compile(current)
proposed_esc_re = re.compile(proposed_escaped)
proposed_end_re = re.compile(proposed_end)
print("Testing regex patterns:")
print(f"Current: {current}")
print(f"Proposed (escaped): {proposed_escaped}")
print(f"Alternative (end): {proposed_end}")
print()
for test in test_cases:
c = bool(current_re.match(test))
p = bool(proposed_esc_re.match(test))
a = bool(proposed_end_re.match(test))
print(f"{test:15} | Current: {c!s:5} | Proposed: {p!s:5} | Alternative: {a!s:5}")
EOFRepository: utopia-php/database
Length of output: 630
Allow hyphens in nested object path parts to align with Adapter::filter().
The convertObjectPathToJSONB() and filterObjectPath() methods validate path parts using /^[a-zA-Z0-9_]+$/, which rejects hyphens. However, Adapter::filter() explicitly allows hyphens via /[^A-Za-z0-9_\-]/. This creates an inconsistency: attributes with hyphens can be created (which passes filter()), but nested object queries on those attributes will fail. The second change to use $this->quote() instead of manual quoting improves consistency with the codebase patterns.
Proposed fix
@@
- if (!preg_match('/^[a-zA-Z0-9_]+$/', $part)) {
- throw new DatabaseException('Invalid JSON key '.$part);
+ if (!preg_match('/^[a-zA-Z0-9_\-]+$/', $part)) {
+ throw new DatabaseException('Invalid JSON key ' . $part);
}
@@
- $baseColumnQuoted = "\"{$baseColumn}\"";
+ $baseColumnQuoted = $this->quote($baseColumn);
@@
- if (!preg_match('/^[a-zA-Z0-9_]+$/', $part)) {
- throw new DatabaseException('Invalid JSON key '.$part);
+ if (!preg_match('/^[a-zA-Z0-9_\-]+$/', $part)) {
+ throw new DatabaseException('Invalid JSON key ' . $part);
}
@@
- $baseColumnQuoted = "\"{$baseColumn}\"";
+ $baseColumnQuoted = $this->quote($baseColumn);| if ($this->supportForObjectIndexes) { | ||
| $dottedAttributes = array_filter($index->getAttribute('attributes'), fn ($attr) => $this->isDottedAttribute($attr)); | ||
| if (\count($dottedAttributes)) { | ||
| foreach ($index->getAttribute('attributes') as $attribute) { | ||
| $baseAttribute = isset($this->attributes[\strtolower($attribute)]) ? $attribute : $this->getBaseAttributeFromDottedAttribute($attribute); | ||
| if (isset($this->attributes[\strtolower($baseAttribute)]) && $this->attributes[\strtolower($baseAttribute)]->getAttribute('type') != Database::VAR_OBJECT) { | ||
| $this->message = 'Index attribute "' . $attribute . '" is only supported on object attributes'; | ||
| return false; | ||
| }; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential logic bug: non-object attributes incorrectly rejected in mixed indexes.
When an index contains both regular and dotted attributes, the loop at line 172 iterates over ALL attributes. For a non-dotted attribute like name (STRING type), line 173 sets $baseAttribute = 'name', and line 174 then fails because it's not VAR_OBJECT.
This would reject valid indexes like ['name', 'profile.user.email'] where name is a string and profile is an object.
The check should only apply to dotted attributes, not all attributes.
🐛 Proposed fix
if ($this->supportForObjectIndexes) {
$dottedAttributes = array_filter($index->getAttribute('attributes'), fn ($attr) => $this->isDottedAttribute($attr));
if (\count($dottedAttributes)) {
- foreach ($index->getAttribute('attributes') as $attribute) {
- $baseAttribute = isset($this->attributes[\strtolower($attribute)]) ? $attribute : $this->getBaseAttributeFromDottedAttribute($attribute);
+ foreach ($dottedAttributes as $attribute) {
+ $baseAttribute = $this->getBaseAttributeFromDottedAttribute($attribute);
if (isset($this->attributes[\strtolower($baseAttribute)]) && $this->attributes[\strtolower($baseAttribute)]->getAttribute('type') != Database::VAR_OBJECT) {
$this->message = 'Index attribute "' . $attribute . '" is only supported on object attributes';
return false;
};
}
}
}🤖 Prompt for AI Agents
In `@src/Database/Validator/Index.php` around lines 169 - 180, The current loop in
Index.php incorrectly iterates over all index attributes and enforces
object-type on non-dotted attributes; change the loop to iterate only over
$dottedAttributes (the result of array_filter) so the object-type check runs
only for dotted attributes, derive $baseAttribute via
getBaseAttributeFromDottedAttribute($attribute) as already intended, and keep
the existing error message/return false when the base attribute exists but is
not Database::VAR_OBJECT.
| private function isDottedAttribute(string $attribute) | ||
| { | ||
| return \str_contains($attribute, '.'); | ||
| } | ||
|
|
||
| private function getBaseAttributeFromDottedAttribute(string $attribute) | ||
| { | ||
| return $this->isDottedAttribute($attribute) ? \explode('.', $attribute, 2)[0] ?? '' : $attribute; | ||
| } |
There was a problem hiding this comment.
Add return type declarations to fix pipeline failures.
Both helper methods are missing return type declarations as flagged by PHPStan.
🐛 Proposed fix
-private function isDottedAttribute(string $attribute)
+private function isDottedAttribute(string $attribute): bool
{
return \str_contains($attribute, '.');
}
-private function getBaseAttributeFromDottedAttribute(string $attribute)
+private function getBaseAttributeFromDottedAttribute(string $attribute): string
{
return $this->isDottedAttribute($attribute) ? \explode('.', $attribute, 2)[0] ?? '' : $attribute;
}🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 768-768: isDottedAttribute() has no return type specified. (PHPStan: missing return type)
[error] 773-773: getBaseAttributeFromDottedAttribute() has no return type specified. (PHPStan: missing return type)
🤖 Prompt for AI Agents
In `@src/Database/Validator/Index.php` around lines 768 - 776, Both helper methods
lack return type declarations causing PHPStan failures; add explicit return
types: change isDottedAttribute(string $attribute) to return bool and change
getBaseAttributeFromDottedAttribute(string $attribute) to return string (i.e.,
add ": bool" and ": string" to the respective method signatures), keeping the
current logic intact.
| // 3) INDEX_OBJECT must NOT be allowed on nested paths | ||
| if ($database->getAdapter()->getSupportForObjectIndexes()) { | ||
| $exceptionThrown = false; | ||
| try { | ||
| $database->createIndex($collectionId, 'idx_profile_nested_object', Database::INDEX_OBJECT, ['profile.user.email']); | ||
| } catch (Exception $e) { | ||
| $exceptionThrown = true; | ||
| $this->assertInstanceOf(IndexException::class, $e); | ||
| } | ||
| $this->assertTrue($exceptionThrown, 'Expected Index exception for object index on nested path'); | ||
| } |
There was a problem hiding this comment.
Remove redundant conditional (always true).
The condition at line 1181 is always true because line 1131 already skips the test when getSupportForObjectIndexes() returns false. This is flagged by PHPStan.
♻️ Proposed fix
// 3) INDEX_OBJECT must NOT be allowed on nested paths
- if ($database->getAdapter()->getSupportForObjectIndexes()) {
- $exceptionThrown = false;
- try {
- $database->createIndex($collectionId, 'idx_profile_nested_object', Database::INDEX_OBJECT, ['profile.user.email']);
- } catch (Exception $e) {
- $exceptionThrown = true;
- $this->assertInstanceOf(IndexException::class, $e);
- }
- $this->assertTrue($exceptionThrown, 'Expected Index exception for object index on nested path');
+ $exceptionThrown = false;
+ try {
+ $database->createIndex($collectionId, 'idx_profile_nested_object', Database::INDEX_OBJECT, ['profile.user.email']);
+ } catch (Exception $e) {
+ $exceptionThrown = true;
+ $this->assertInstanceOf(IndexException::class, $e);
}
+ $this->assertTrue($exceptionThrown, 'Expected Index exception for object index on nested path');📝 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.
| // 3) INDEX_OBJECT must NOT be allowed on nested paths | |
| if ($database->getAdapter()->getSupportForObjectIndexes()) { | |
| $exceptionThrown = false; | |
| try { | |
| $database->createIndex($collectionId, 'idx_profile_nested_object', Database::INDEX_OBJECT, ['profile.user.email']); | |
| } catch (Exception $e) { | |
| $exceptionThrown = true; | |
| $this->assertInstanceOf(IndexException::class, $e); | |
| } | |
| $this->assertTrue($exceptionThrown, 'Expected Index exception for object index on nested path'); | |
| } | |
| // 3) INDEX_OBJECT must NOT be allowed on nested paths | |
| $exceptionThrown = false; | |
| try { | |
| $database->createIndex($collectionId, 'idx_profile_nested_object', Database::INDEX_OBJECT, ['profile.user.email']); | |
| } catch (Exception $e) { | |
| $exceptionThrown = true; | |
| $this->assertInstanceOf(IndexException::class, $e); | |
| } | |
| $this->assertTrue($exceptionThrown, 'Expected Index exception for object index on nested path'); |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 1181-1181: If condition is always true. (PHPStan: always-true conditional)
🤖 Prompt for AI Agents
In `@tests/e2e/Adapter/Scopes/ObjectAttributeTests.php` around lines 1180 - 1190,
Remove the redundant if-check around the object-index test: the outer test
already skipped when getSupportForObjectIndexes() is false, so delete the
conditional "if ($database->getAdapter()->getSupportForObjectIndexes()) {" and
its matching closing brace, leaving the try/catch that creates the index
'idx_profile_nested_object' with Database::INDEX_OBJECT and the subsequent
assertions intact; make sure the block still catches Exception $e and asserts
InstanceOf IndexException and that $exceptionThrown is asserted true.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.