Sync 3.x #783
Sync 3.x #783
Conversation
…hing with existing collections
* support for object contains , not contains, equals, not equals
…indexed and associative arrays
…Validator classes
…ttributes, unique indexes, and fulltext indexes
…allow mixed lists and improve depth handling
Co-authored-by: Jake Barnby <jakeb994@gmail.com>
…es and update related logic
…nal session scenarios
# Conflicts: # composer.lock # src/Database/Adapter.php # src/Database/Adapter/MariaDB.php # src/Database/Adapter/Mongo.php # src/Database/Adapter/SQLite.php # src/Database/Database.php
📝 WalkthroughWalkthroughThis pull request extends the database adapter architecture by introducing regex query support, trigram indexing, and enhanced object attribute handling across multiple adapters. New capability detection methods are added to Adapter implementations, Query types are expanded, and validation logic is strengthened to support the new features. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
abnegate
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/Database/Query.php (1)
261-312:TYPE_REGEXis missing from theisMethod()match statement.The new
TYPE_REGEXconstant is added to theTYPESarray but not included in theisMethod()match statement. This will causeQuery::isMethod('regex')to returnfalse, potentially breaking parsing and validation of regex queries.🐛 Proposed fix
self::TYPE_AND, self::TYPE_ELEM_MATCH, + self::TYPE_REGEX, self::TYPE_SELECT, self::TYPE_VECTOR_DOT,src/Database/Database.php (1)
651-677: Fix VAR_OBJECT decode filter return-type contract (it can return non-array objects).The decode closure’s docblock says
@return array|null, but it can return non-string Mongo values (likely objects). This can confuse static analysis and callers relying on the docblock.Proposed diff
- /** - * `@param` mixed $value - * `@return` array|null - */ + /** + * `@param` mixed $value + * `@return` mixed Decoded object payload. May be array (JSON) or a native object for MongoDB. + */ function (mixed $value) { if (is_null($value)) { - return; + return null; } // can be non string in case of mongodb as it stores the value as object if (!is_string($value)) { return $value; } $decoded = json_decode($value, true); return is_array($decoded) ? $decoded : $value; }src/Database/Adapter/Postgres.php (2)
154-168: Makepg_trgminstallation failures actionable (privileges/extension availability)
CREATE EXTENSIONoften requires elevated privileges (and some hosted Postgres setups restrict it). If this fails, it’ll currently throw a raw PDOException.Proposed improvement (wrap with a clearer DatabaseException)
// Enable extensions - $this->getPDO()->prepare('CREATE EXTENSION IF NOT EXISTS postgis')->execute(); - $this->getPDO()->prepare('CREATE EXTENSION IF NOT EXISTS vector')->execute(); - $this->getPDO()->prepare('CREATE EXTENSION IF NOT EXISTS pg_trgm')->execute(); + try { + $this->getPDO()->prepare('CREATE EXTENSION IF NOT EXISTS postgis')->execute(); + $this->getPDO()->prepare('CREATE EXTENSION IF NOT EXISTS vector')->execute(); + $this->getPDO()->prepare('CREATE EXTENSION IF NOT EXISTS pg_trgm')->execute(); + } catch (PDOException $e) { + throw new DatabaseException( + 'Failed to enable required Postgres extensions (postgis, vector, pg_trgm). ' . + 'Ensure the extensions are available and the DB user has permission to CREATE EXTENSION.', + $e->getCode(), + $e + ); + }
879-933: Trigram index SQL generation is fragile (ORDER tokens can leak into GIN clause)For
Database::INDEX_TRIGRAM,$orderis currently not forced off, and the GIN clause is built by re-parsing the imploded$attributesstring. If callers pass orders, you can end up generating invalid SQL like"title" DESC gin_trgm_ops.Also, the “Unknown index type” exception message appears to omit
Database::INDEX_TRIGRAM(minor, but confusing during debugging).Proposed fix (build trigram SQL from the array + disallow order)
- foreach ($attributes as $i => $attr) { - $order = empty($orders[$i]) || Database::INDEX_FULLTEXT === $type ? '' : $orders[$i]; + foreach ($attributes as $i => $attr) { + $order = empty($orders[$i]) || \in_array($type, [Database::INDEX_FULLTEXT, Database::INDEX_OBJECT, Database::INDEX_TRIGRAM], true) + ? '' + : $orders[$i]; $attr = match ($attr) { '$id' => '_uid', '$createdAt' => '_createdAt', '$updatedAt' => '_updatedAt', default => $this->filter($attr), }; $attributes[$i] = "\"{$attr}\" {$order}"; } @@ - default => throw new DatabaseException('Unknown index type: ' . $type . '. Must be one of ' . Database::INDEX_KEY . ', ' . Database::INDEX_UNIQUE . ', ' . Database::INDEX_FULLTEXT . ', ' . Database::INDEX_SPATIAL . ', ' . Database::INDEX_OBJECT . ', ' . Database::INDEX_HNSW_EUCLIDEAN . ', ' . Database::INDEX_HNSW_COSINE . ', ' . Database::INDEX_HNSW_DOT), + default => throw new DatabaseException('Unknown index type: ' . $type . '. Must be one of ' . Database::INDEX_KEY . ', ' . Database::INDEX_UNIQUE . ', ' . Database::INDEX_FULLTEXT . ', ' . Database::INDEX_SPATIAL . ', ' . Database::INDEX_OBJECT . ', ' . Database::INDEX_TRIGRAM . ', ' . Database::INDEX_HNSW_EUCLIDEAN . ', ' . Database::INDEX_HNSW_COSINE . ', ' . Database::INDEX_HNSW_DOT), }; @@ - Database::INDEX_TRIGRAM => - " USING GIN (" . implode(', ', array_map( - fn ($attr) => "$attr gin_trgm_ops", - array_map(fn ($attr) => trim($attr), explode(',', $attributes)) - )) . ")", + Database::INDEX_TRIGRAM => + " USING GIN (" . implode(', ', array_map( + fn (string $attr) => trim($attr) . " gin_trgm_ops", + $attributes + )) . ")", default => " ({$attributes})", };src/Database/Adapter/Mongo.php (1)
2012-2031: Inconsistent stdClass conversion between first batch and subsequent batches.Line 2014 applies
convertStdClassToArrayto the first batch results, but line 2031 does not apply it togetMorebatch results. This could cause documents from different batches to have different structures (stdClass vs array).🐛 Proposed fix
foreach ($moreResults as $result) { $record = $this->replaceChars('_', '$', (array)$result); - $found[] = new Document($record); + $found[] = new Document($this->convertStdClassToArray($record)); }
🤖 Fix all issues with AI agents
In `@bin/tasks/operators.php`:
- Line 150: The code calls $authorization->addRole(Role::any()->toString()) but
$authorization is undefined; fix by retrieving the authorization object from the
DB adapter before use (e.g., assign $authorization =
$dbAdapter->getAuthorization() or the appropriate method on your adapter),
ensure $dbAdapter (or the adapter variable in scope) is available, and add a
null check or error handling before calling addRole(Role::any()->toString()).
In `@src/Database/Adapter/Mongo.php`:
- Around line 1354-1356: In the Database::VAR_OBJECT case, json_decode($node)
can silently return null on invalid JSON; after calling json_decode($node) check
json_last_error() (and use json_last_error_msg()) and handle errors the same way
as the array branch above (i.e., throw or return the same exception/error with
the JSON error message and original $node context) so invalid JSON is detected
and reported consistently.
- Around line 47-48: The operators array contains a duplicate '$exists' entry;
locate the operators array definition (the list that includes '$elemMatch' and
'$exists') and remove the redundant '$exists' occurrence so each operator
appears only once, ensuring the array retains the original order otherwise.
- Around line 2371-2378: The elemMatch branch for Query::TYPE_ELEM_MATCH uses
$query->getAttribute() directly, bypassing the special attribute name
conversions performed by buildFilter(), which can produce invalid Mongo field
names (e.g. '$permissions'); update the branch in the Mongo adapter so you first
map the attribute through the same conversion logic used in buildFilter()
(handle '$id' -> '_uid', '$sequence' -> '_id', '$createdAt' -> '_createdAt',
'$updatedAt' -> '_updatedAt', etc.), then use that converted attribute as the
key for the '$elemMatch' array and call $this->buildFilters($query->getValues(),
$separator) for the nested filters.
- Around line 2536-2542: The code uses $logicalOperator = $isNot ? '$and' :
'$or' which causes equality (TYPE_EQUAL) to combine flattened object leaf
conditions with '$or' instead of requiring all properties to match; change this
to always use '$and' for these flattened-equality branches (i.e., set
$logicalOperator = '$and') and keep the existing merge/assignment logic on
$filter, $conditions so both equality and not-equality cases use '$and' (refer
to $logicalOperator, $isNot, $filter, $conditions in Mongo.php).
In `@src/Database/Validator/Index.php`:
- Around line 601-616: The method checkKeyUniqueFulltextSupport() is named to
include FULLTEXT but only checks KEY and UNIQUE; either rename it to
checkKeyUniqueSupport() and update all call sites (including where
checkValidIndex() invokes it) to reflect the new name, or add the missing
FULLTEXT branch: check for if ($type === Database::INDEX_FULLTEXT &&
$this->supportForFulltextIndexes === false) set $this->message = 'Fulltext index
is not supported' and return false, ensuring consistency with the KEY and UNIQUE
checks and with checkValidIndex()'s behavior.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 7154-7205: Remove the unused $foundOther variable: delete the
declaration "$foundOther = false;" and the assignment "$foundOther = true;"
inside the first foreach over $results; keep the surrounding logic that checks
if $text === 'other' and performs the preg_match/fail assertions (so behavior is
unchanged but the unused flag is removed). This change touches the foreach
($results as $doc) block and the $foundOther symbol in DocumentTests.php.
- Around line 7259-7465: The test mutates adapter timeout state with
$database->setTimeout(...) but only calls $database->clearTimeout() on the
normal path; wrap the portion that sets the timeout and runs queries inside a
try/finally so clearTimeout() always runs: after computing $supportsTimeout and
calling $database->setTimeout(2000) (when $supportsTimeout is true) ensure you
enter a try block that contains the foreach($redosPatterns...) loop and the
safePattern check, and in the finally block call $database->clearTimeout()
(guarded by the same $supportsTimeout check) and perform collection cleanup;
this guarantees cleanup even if assertions or exceptions occur in
testRegexRedos().
In `@tests/e2e/Adapter/Scopes/SchemalessTests.php`:
- Around line 1690-1698: There is a duplicate assignment to $database using
static::getDatabase(); remove the repeated line so $database is only assigned
once before the conditional that checks
$database->getAdapter()->getSupportForAttributes(), preserving the subsequent if
block and the call to $this->expectNotToPerformAssertions() unchanged.
In `@tests/unit/Validator/StructureTest.php`:
- Around line 284-285: The test uses a hardcoded "2,147,483,647" which makes the
out-of-32-bit-range path platform-dependent; instead, compute the 32-bit bounds
in the test (e.g. $max32 = 2147483647; $min32 = -2147483648) and build the
expected messages from them, and for the out-of-range cases use ($max32 + 1) or
($min32 - 1) to ensure deterministic behavior; update the assertions that
reference the message (the ones comparing $validator->getDescription()) to use
these computed values rather than a hardcoded 2,147,483,647 (also apply the same
change to the other affected assertions mentioned).
🧹 Nitpick comments (16)
src/Database/Validator/Query/Filter.php (2)
149-155: Potential issue with 64-bit unsigned integer validation.The logic at line 153 sets
$unsigned = !$signed && $bits < 64, which means 64-bit unsigned integers will be validated as signed. The comment mentions this is because "PHP doesn't support true 64-bit unsigned", but this could cause valid large unsigned values to fail validation. Consider documenting this limitation more explicitly or handling the edge case.📝 Suggested documentation improvement
$bits = $size >= 8 ? 64 : 32; - // For 64-bit unsigned, use signed since PHP doesn't support true 64-bit unsigned + // For 64-bit unsigned, fallback to signed validation since PHP integers are signed + // and cannot represent values > PHP_INT_MAX. Values between PHP_INT_MAX and UINT64_MAX + // will fail validation even if valid in the database. $unsigned = !$signed && $bits < 64;
176-182: Consider adding validation for other query methods on object attributes.Currently, object attribute validation only allows
equal,notEqual,contains, andnotContainsmethods. Other methods likelessThan,greaterThan, etc. will silently pass through viacontinue 2without explicit rejection. Consider whether these should be explicitly disallowed for object types.📝 Suggested improvement
case Database::VAR_OBJECT: 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; } + // Only allow specific query methods on object attributes + if (!\in_array($method, [Query::TYPE_EQUAL, Query::TYPE_NOT_EQUAL, Query::TYPE_CONTAINS, Query::TYPE_NOT_CONTAINS, Query::TYPE_IS_NULL, Query::TYPE_IS_NOT_NULL], true)) { + $this->message = 'Query method "' . $method . '" is not supported for object attribute "' . $attribute . '"'; + return false; + } continue 2;tests/unit/Validator/QueriesTest.php (1)
105-117: Clarify the test case expectation with a descriptive comment.The comment says "disallowed complex multi-key nested structure" but the assertion is
assertTrue. This appears intentional (multiple string keys at the same level like'name'and'ex'are allowed), but the comment is misleading.📝 Fix the misleading comment
- // Object attribute query: disallowed complex multi-key nested structure + // Object attribute query: allowed complex multi-key nested structure (all string keys at each level) $this->assertTrue( $validator->isValid([ Query::contains('meta', [tests/unit/Validator/IndexTest.php (1)
527-528: Consider using named parameters for better readability.The
Indexconstructor call has many positional boolean arguments, making it difficult to understand what eachfalsemeans. WhilesupportForTrigramIndexesuses a named parameter, the preceding booleans are positional.📝 Suggested improvement for readability
- $validator = new Index($collection->getAttribute('attributes'), $collection->getAttribute('indexes', []), 768, [], false, false, false, false, false, false, false, false, supportForTrigramIndexes: true); + $validator = new Index( + attributes: $collection->getAttribute('attributes'), + indexes: $collection->getAttribute('indexes', []), + maxLength: 768, + reservedIndexKeys: [], + supportForFulltextIndex: false, + supportForAttributes: false, + supportForSpatialIndex: false, + supportForSpatialIndexNull: false, + supportForObjectIndexes: false, + supportForSpatialIndexOrder: false, + supportForArrayIndex: false, + supportForIdenticalIndexes: false, + supportForTrigramIndexes: true + );src/Database/Validator/Index.php (2)
166-230: Duplicate validation: spatial index support and order checks.Lines 191-200 check
supportForSpatialIndexesandsupportForSpatialIndexOrder, butcheckSpatialIndexes()(lines 449-481) performs the same checks. This results in redundant validation that could become inconsistent if one is modified without the other.Consider consolidating spatial validation in one place—either here for early exit or in
checkSpatialIndexes()for all spatial-specific validation.
449-452: Duplicate spatial support check.This check duplicates the validation already performed in
checkValidIndex()at lines 191-195. While harmless, it adds redundancy. Consider removing one of the checks to maintain a single source of truth.src/Database/Database.php (1)
8573-8608: isCompatibleObjectValue(): handle “direct associative array” inputs + remove no-op loop.As written, this will return
falseif$valuesis itself an associative array (common if something bypasses helper constructors and sets Query values directly). Also, the innerforeach ($value as $nestedValue)currently doesn’t enforce anything (it’s effectively a no-op).A small tweak makes it more robust without changing intent:
Proposed diff
private function isCompatibleObjectValue(array $values): bool { if (empty($values)) { return false; } + + // If caller passed a single associative array directly (not wrapped as a list), + // treat it as a single value. + if (!\array_is_list($values)) { + $values = [$values]; + } foreach ($values as $value) { if (!\is_array($value)) { return false; } // Check associative array (hashmap) or nested structure if (empty($value)) { continue; } // simple indexed array => not an object - if (\array_keys($value) === \range(0, \count($value) - 1)) { + if (\array_is_list($value)) { return false; } - - foreach ($value as $nestedValue) { - if (\is_array($nestedValue)) { - continue; - } - } } return true; }Also applies to: 8646-8651
src/Database/Adapter/Mongo.php (2)
1277-1284: Nested arrays within schemaless documents may contain unconverted stdClass objects.The loop only checks top-level values. If a schemaless document has an array attribute containing stdClass objects, they won't be converted. Consider using
convertStdClassToArraywhich handles nested structures:♻️ Suggested improvement
if (!$this->getSupportForAttributes()) { foreach ($document->getArrayCopy() as $key => $value) { - // mongodb results out a stdclass for objects - if (is_object($value) && get_class($value) === stdClass::class) { - $document->setAttribute($key, $this->convertStdClassToArray($value)); - } + // mongodb results out stdclass for objects - convert recursively + $converted = $this->convertStdClassToArray($value); + if ($converted !== $value) { + $document->setAttribute($key, $converted); + } } }
1288-1302: Minor style inconsistency in recursive calls.Line 1291 uses first-class callable syntax while line 1296 uses a closure. Consider using consistent style:
♻️ Consistent style suggestion
private function convertStdClassToArray(mixed $value): mixed { if (is_object($value) && get_class($value) === stdClass::class) { return array_map($this->convertStdClassToArray(...), get_object_vars($value)); } if (is_array($value)) { - return array_map( - fn ($v) => $this->convertStdClassToArray($v), - $value - ); + return array_map($this->convertStdClassToArray(...), $value); } return $value; }tests/e2e/Adapter/Scopes/DocumentTests.php (3)
6711-6750: Harden$verifyRegexQueryagainst invalid patterns / non-string values (avoid noisy warnings)Right now
preg_match($phpPattern, $value)will emit warnings (and behave inconsistently) if$valueisnullor if$phpPatternis invalid; tests may become flaky depending on PHP error handling configuration. Consider failing fast with a clearer assertion whenpreg_matchreturnsfalse.
6752-7051: Remove unused$pattern = '/.../'locals intestFindRegexSeveral
$pattern = '/.../'assignments are never used (the test uses raw patterns like'^Captain'viaQuery::regex()andverifyRegexQuery()), which adds noise and may trigger static analysis in some configs.Proposed cleanup (remove unused locals)
- $pattern = '/^Captain/'; $documents = $database->find('moviesRegex', [ Query::regex('name', '^Captain'), ]); ... - $pattern = '/Frozen/'; $documents = $database->find('moviesRegex', [ Query::regex('name', 'Frozen'), ]); ... - $pattern = '/Marvel$/'; $documents = $database->find('moviesRegex', [ Query::regex('name', 'Marvel$'), ]); ... - $pattern = '/.*Work.*/'; $documents = $database->find('moviesRegex', [ Query::regex('name', '.*Work.*'), ]); ... - $pattern = '/.*Buck.*/'; $documents = $database->find('moviesRegex', [ Query::regex('director', '.*Buck.*'), ]); ... - $pattern = '/.*:.*/'; $documents = $database->find('moviesRegex', [ Query::regex('name', '.*:.*'), // Match movies with colon ]);
7005-7015: Tight wall-clock assertion may be flaky in CI (ReDoS check)
$this->assertLessThan(1.0, $elapsed, ...)can fail under DB/CI load even when behavior is correct. Since you already have a dedicatedtestRegexRedos()with adapter timeouts, consider either removing this timing assertion here, or loosening it / gating it on$database->getAdapter()->getSupportForTimeouts().tests/e2e/Adapter/Scopes/SchemalessTests.php (3)
754-763: Inconsistent database retrieval method.This test uses
static::getDatabase()while other tests in this trait use$this->getDatabase(). For consistency, consider using the same approach throughout the trait.- $database = static::getDatabase(); + $database = $this->getDatabase();
1455-1479: Unused local variables from document creation.The static analysis correctly identifies that
$doc1,$doc2, and$doc3are assigned but never used. Since the tests only need the documents to exist in the database for subsequent queries, you can either:
- Remove the variable assignments, or
- Suppress the warnings if the explicit assignments improve readability
♻️ Suggested fix
- $doc1 = $database->createDocument($collectionId, new Document([ + $database->createDocument($collectionId, new Document([ '$id' => 'order1', '$permissions' => [Permission::read(Role::any())], 'items' => [ ['sku' => 'ABC', 'qty' => 5, 'price' => 10.50], ['sku' => 'XYZ', 'qty' => 2, 'price' => 20.00], ] ])); - $doc2 = $database->createDocument($collectionId, new Document([ + $database->createDocument($collectionId, new Document([ '$id' => 'order2', ... ])); - $doc3 = $database->createDocument($collectionId, new Document([ + $database->createDocument($collectionId, new Document([ '$id' => 'order3', ... ]));
1608-1624: Same unused variable issue in testElemMatchComplex.
$doc1and$doc2are assigned but never used.♻️ Suggested fix
- $doc1 = $database->createDocument($collectionId, new Document([ + $database->createDocument($collectionId, new Document([ '$id' => 'store1', ... ])); - $doc2 = $database->createDocument($collectionId, new Document([ + $database->createDocument($collectionId, new Document([ '$id' => 'store2', ... ]));tests/e2e/Adapter/Scopes/IndexTests.php (1)
667-676: Inconsistent database retrieval method in trigram tests.Both trigram test methods use
static::getDatabase()instead of$this->getDatabase(). While this works, it's inconsistent with the pattern used in other tests in this trait (e.g.,testCreateDeleteIndex,testIndexValidation).- $database = static::getDatabase(); + $database = $this->getDatabase();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
bin/tasks/operators.phpsrc/Database/Adapter.phpsrc/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/MySQL.phpsrc/Database/Adapter/Pool.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Adapter/SQLite.phpsrc/Database/Database.phpsrc/Database/Query.phpsrc/Database/Validator/Index.phpsrc/Database/Validator/Queries.phpsrc/Database/Validator/Query/Filter.phpsrc/Database/Validator/Structure.phptests/e2e/Adapter/Scopes/AttributeTests.phptests/e2e/Adapter/Scopes/DocumentTests.phptests/e2e/Adapter/Scopes/IndexTests.phptests/e2e/Adapter/Scopes/ObjectAttributeTests.phptests/e2e/Adapter/Scopes/SchemalessTests.phptests/unit/Validator/IndexTest.phptests/unit/Validator/QueriesTest.phptests/unit/Validator/StructureTest.php
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Learnt from: 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-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/SchemalessTests.phptests/e2e/Adapter/Scopes/ObjectAttributeTests.phptests/e2e/Adapter/Scopes/AttributeTests.phptests/unit/Validator/StructureTest.phpsrc/Database/Validator/Structure.phptests/e2e/Adapter/Scopes/IndexTests.phptests/unit/Validator/QueriesTest.phptests/unit/Validator/IndexTest.phptests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Validator/Query/Filter.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/SchemalessTests.phptests/e2e/Adapter/Scopes/AttributeTests.phptests/e2e/Adapter/Scopes/IndexTests.phptests/e2e/Adapter/Scopes/DocumentTests.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:
tests/e2e/Adapter/Scopes/SchemalessTests.phptests/e2e/Adapter/Scopes/ObjectAttributeTests.phpsrc/Database/Adapter/Mongo.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/Structure.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:
tests/e2e/Adapter/Scopes/IndexTests.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.php
📚 Learning: 2025-08-14T06:35:30.429Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 661
File: tests/e2e/Adapter/Scopes/SpatialTests.php:180-186
Timestamp: 2025-08-14T06:35:30.429Z
Learning: Query::distance method in Utopia\Database\Query expects an array of values parameter, where each value is a [geometry, distance] pair. So the correct format is Query::distance('attribute', [[[lat, lng], distance]]) where the outer array contains the values and each value is [geometry, distance].
Applied to files:
src/Database/Validator/Query/Filter.php
📚 Learning: 2025-10-16T08:48:36.715Z
Learnt from: fogelito
Repo: utopia-php/database PR: 739
File: src/Database/Adapter/Postgres.php:154-158
Timestamp: 2025-10-16T08:48:36.715Z
Learning: For the utopia-php/database repository, no migration scripts are needed for the collation change from utf8_ci to utf8_ci_ai in the Postgres adapter because there is no existing production data.
Applied to files:
src/Database/Adapter/Postgres.php
🧬 Code graph analysis (11)
src/Database/Adapter/SQL.php (2)
src/Database/Query.php (1)
Query(8-1237)src/Database/Adapter/Postgres.php (1)
getRegexOperator(2148-2151)
bin/tasks/operators.php (1)
src/Database/Validator/Authorization.php (1)
addRole(87-90)
tests/e2e/Adapter/Scopes/SchemalessTests.php (4)
src/Database/Adapter.php (3)
getSupportForAttributes(945-945)getSupportForObject(1102-1102)createAttribute(581-581)src/Database/Adapter/SQLite.php (1)
getSupportForObject(1011-1014)tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (1)
createAttribute(31-40)src/Database/Document.php (1)
getId(63-66)
src/Database/Adapter/MySQL.php (7)
src/Database/Adapter/SQL.php (1)
getPDO(2161-2164)src/Database/Adapter/Mongo.php (1)
getSupportForObjectIndexes(2962-2965)src/Database/Adapter.php (1)
getSupportForObjectIndexes(1109-1109)src/Database/Adapter/MariaDB.php (1)
getSupportForObjectIndexes(2153-2156)src/Database/Adapter/Pool.php (1)
getSupportForObjectIndexes(615-618)src/Database/Adapter/Postgres.php (1)
getSupportForObjectIndexes(2232-2235)src/Database/Adapter/SQLite.php (1)
getSupportForObjectIndexes(1021-1024)
tests/unit/Validator/StructureTest.php (3)
src/Database/Validator/Structure.php (1)
getDescription(197-200)src/Database/Validator/Operator.php (1)
getDescription(90-93)src/Database/Validator/Sequence.php (1)
getDescription(14-17)
src/Database/Adapter/MariaDB.php (6)
src/Database/Adapter/Mongo.php (4)
getSupportForObjectIndexes(2962-2965)getSupportForTrigramIndex(3399-3402)getSupportForPCRERegex(2887-2890)getSupportForPOSIXRegex(2897-2900)src/Database/Adapter.php (4)
getSupportForObjectIndexes(1109-1109)getSupportForTrigramIndex(1487-1487)getSupportForPCRERegex(1495-1495)getSupportForPOSIXRegex(1503-1503)src/Database/Adapter/MySQL.php (1)
getSupportForObjectIndexes(261-264)src/Database/Adapter/Pool.php (4)
getSupportForObjectIndexes(615-618)getSupportForTrigramIndex(385-388)getSupportForPCRERegex(375-378)getSupportForPOSIXRegex(380-383)src/Database/Adapter/Postgres.php (4)
getSupportForObjectIndexes(2232-2235)getSupportForTrigramIndex(2132-2135)getSupportForPCRERegex(2122-2125)getSupportForPOSIXRegex(2127-2130)src/Database/Adapter/SQLite.php (3)
getSupportForObjectIndexes(1021-1024)getSupportForPCRERegex(1901-1904)getSupportForPOSIXRegex(1912-1915)
src/Database/Adapter.php (6)
src/Database/Adapter/Mongo.php (4)
getSupportForObjectIndexes(2962-2965)getSupportForTrigramIndex(3399-3402)getSupportForPCRERegex(2887-2890)getSupportForPOSIXRegex(2897-2900)src/Database/Adapter/MariaDB.php (4)
getSupportForObjectIndexes(2153-2156)getSupportForTrigramIndex(2254-2257)getSupportForPCRERegex(2259-2262)getSupportForPOSIXRegex(2264-2267)src/Database/Adapter/MySQL.php (1)
getSupportForObjectIndexes(261-264)src/Database/Adapter/Pool.php (4)
getSupportForObjectIndexes(615-618)getSupportForTrigramIndex(385-388)getSupportForPCRERegex(375-378)getSupportForPOSIXRegex(380-383)src/Database/Adapter/Postgres.php (4)
getSupportForObjectIndexes(2232-2235)getSupportForTrigramIndex(2132-2135)getSupportForPCRERegex(2122-2125)getSupportForPOSIXRegex(2127-2130)src/Database/Adapter/SQLite.php (3)
getSupportForObjectIndexes(1021-1024)getSupportForPCRERegex(1901-1904)getSupportForPOSIXRegex(1912-1915)
src/Database/Adapter/Pool.php (7)
src/Database/Adapter/Mongo.php (4)
getSupportForPCRERegex(2887-2890)getSupportForPOSIXRegex(2897-2900)getSupportForTrigramIndex(3399-3402)getSupportForObjectIndexes(2962-2965)src/Database/Adapter.php (4)
getSupportForPCRERegex(1495-1495)getSupportForPOSIXRegex(1503-1503)getSupportForTrigramIndex(1487-1487)getSupportForObjectIndexes(1109-1109)src/Database/Adapter/MariaDB.php (4)
getSupportForPCRERegex(2259-2262)getSupportForPOSIXRegex(2264-2267)getSupportForTrigramIndex(2254-2257)getSupportForObjectIndexes(2153-2156)src/Database/Adapter/Postgres.php (4)
getSupportForPCRERegex(2122-2125)getSupportForPOSIXRegex(2127-2130)getSupportForTrigramIndex(2132-2135)getSupportForObjectIndexes(2232-2235)src/Database/Adapter/SQLite.php (3)
getSupportForPCRERegex(1901-1904)getSupportForPOSIXRegex(1912-1915)getSupportForObjectIndexes(1021-1024)src/Database/Mirror.php (1)
delegate(88-103)src/Database/Adapter/MySQL.php (1)
getSupportForObjectIndexes(261-264)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
src/Database/Adapter/MySQL.php (1)
setTimeout(24-46)
src/Database/Database.php (8)
src/Database/Adapter/MariaDB.php (4)
getSupportForObjectIndexes(2153-2156)getSupportForTrigramIndex(2254-2257)getSupportForSpatialAttributes(2138-2141)getSupportForObject(2143-2146)src/Database/Adapter/MySQL.php (1)
getSupportForObjectIndexes(261-264)src/Database/Adapter/Pool.php (7)
getSupportForObjectIndexes(615-618)getSupportForTrigramIndex(385-388)getSupportForSpatialAttributes(465-468)getSupportForIndex(345-348)getSupportForUniqueIndex(360-363)getSupportForFulltextIndex(365-368)getSupportForObject(610-613)src/Database/Adapter/Postgres.php (4)
getSupportForObjectIndexes(2232-2235)getSupportForTrigramIndex(2132-2135)getSupportForSpatialAttributes(2212-2215)getSupportForObject(2222-2225)src/Database/Adapter/SQLite.php (4)
getSupportForObjectIndexes(1021-1024)getSupportForSpatialAttributes(1006-1009)getSupportForFulltextIndex(911-914)getSupportForObject(1011-1014)src/Database/Adapter/SQL.php (4)
getSupportForSpatialAttributes(1556-1559)getSupportForIndex(938-941)getSupportForUniqueIndex(958-961)getSupportForFulltextIndex(968-971)src/Database/Query.php (2)
getValues(190-193)setAttributeType(976-979)src/Database/Operator.php (1)
getValues(155-158)
src/Database/Adapter/Postgres.php (4)
src/Database/Adapter/SQL.php (2)
getPDO(2161-2164)getRegexOperator(2294-2297)src/Database/Adapter/MariaDB.php (4)
getSupportForPCRERegex(2259-2262)getSupportForPOSIXRegex(2264-2267)getSupportForTrigramIndex(2254-2257)getSupportForObjectIndexes(2153-2156)src/Database/Adapter/SQLite.php (3)
getSupportForPCRERegex(1901-1904)getSupportForPOSIXRegex(1912-1915)getSupportForObjectIndexes(1021-1024)src/Database/Adapter/MySQL.php (1)
getSupportForObjectIndexes(261-264)
🪛 PHPMD (2.15.0)
bin/tasks/operators.php
150-150: Avoid unused local variables such as '$authorization'. (undefined)
(UnusedLocalVariable)
tests/e2e/Adapter/Scopes/SchemalessTests.php
1455-1455: Avoid unused local variables such as '$doc1'. (undefined)
(UnusedLocalVariable)
1464-1464: Avoid unused local variables such as '$doc2'. (undefined)
(UnusedLocalVariable)
1473-1473: Avoid unused local variables such as '$doc3'. (undefined)
(UnusedLocalVariable)
1608-1608: Avoid unused local variables such as '$doc1'. (undefined)
(UnusedLocalVariable)
1617-1617: Avoid unused local variables such as '$doc2'. (undefined)
(UnusedLocalVariable)
tests/e2e/Adapter/Scopes/DocumentTests.php
7162-7162: Avoid unused local variables such as '$foundOther'. (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 (43)
tests/e2e/Adapter/Scopes/AttributeTests.php (1)
1407-1426: LGTM! Updated test expectations correctly reflect the enhanced validation messages.The error message updates accurately describe the unsigned 32-bit integer constraints (0 to 4,294,967,295). The test cases appropriately verify that both float values and negative values are rejected for unsigned integer attributes.
src/Database/Validator/Queries.php (1)
110-110: LGTM!The new query types
TYPE_ELEM_MATCHandTYPE_REGEXare correctly mapped toBase::METHOD_TYPE_FILTER, which is consistent with other filter-based query operations in the match statement.Also applies to: 126-126
src/Database/Validator/Structure.php (1)
351-359: LGTM!The enhanced integer validation correctly handles PHP's limitation with 64-bit unsigned integers. The logic appropriately falls back to signed validation for 64-bit types while relying on the
Rangevalidator to enforce non-negative bounds, which is the correct approach given PHP's integer semantics.src/Database/Adapter.php (2)
1104-1109: LGTM!The new abstract method follows the established capability-check pattern and is consistently implemented across adapters (MariaDB, MySQL, SQLite return
false; Postgres returnstrue; Mongo returnsfalseas per the relevant code snippets).
1482-1514: LGTM!The new regex and trigram capability methods are well-designed:
- Clear documentation distinguishing PCRE (
\bfor word boundaries) from POSIX (\yfor word boundaries)- The composite
getSupportForRegex()method provides a convenient unified check- Implementations across adapters are consistent: MariaDB/Mongo support PCRE, Postgres supports POSIX, SQLite supports neither
src/Database/Adapter/Pool.php (2)
375-388: LGTM!The delegate implementations for the new regex and trigram capability methods correctly follow the established pattern, properly forwarding calls to the underlying adapter in the pool.
615-618: LGTM!The delegate implementation for
getSupportForObjectIndexes()is correctly placed and follows the same delegation pattern as other capability methods.src/Database/Query.php (2)
1194-1236: LGTM!The
regex()andelemMatch()factory methods follow the established patterns in the class. Theregex()method correctly wraps the pattern in an array, andelemMatch()properly accepts an array of Query objects for nested conditions.
137-137: No action needed. The$isObjectAttributeproperty has a public getter method (isObjectAttribute()at line 999) and is actively used in both the Postgres and Mongo adapters. The property is properly implemented and not unused.src/Database/Adapter/SQL.php (2)
1796-1797: LGTM!The
TYPE_REGEXcase correctly delegates togetRegexOperator(), allowing adapter-specific implementations. PostgreSQL overrides this to return~as shown in the relevant snippets.
2291-2297: SQLite adapter does not override getRegexOperator() and lacks REGEXP user-defined function registration.The SQLite adapter inherits
getRegexOperator()from the base SQL adapter, which returns'REGEXP'. However, SQLite's own documentation in the adapter (lines 1897, 1908) explicitly states that "SQLite does not have native REGEXP support - it requires compile-time option or user-defined function." ThegetSupportForPCRERegex()andgetSupportForPOSIXRegex()methods both returnfalse.While regex query tests skip SQLite (via
getSupportForRegex()check), there is no evidence of:
- An override of
getRegexOperator()in the SQLite adapter- A registered user-defined function for
REGEXPsupportThe adapter should either override
getRegexOperator()or ensure REGEXP is properly registered to prevent potential errors if regex queries reach the query builder.src/Database/Validator/Query/Filter.php (2)
305-347: LGTM!The
isValidObjectQueryValues()method correctly validates object query structures by recursively checking for mixed integer/string keys at each nesting level. This prevents ambiguous nested structures while allowing valid patterns like arrays of objects or multiple top-level paths.
460-484: LGTM!The
elemMatchvalidation logic is well-structured:
- Correctly rejects elemMatch in schema mode (attribute-backed adapters)
- Validates the attribute exists
- Ensures nested queries are filter queries with at least one filter
- Appropriately allows elemMatch in schemaless mode
tests/unit/Validator/QueriesTest.php (2)
60-67: LGTM!Adding the
metaobject attribute to the test schema enables comprehensive testing of object-type query validation.
96-103: Test case correctly validates mixed key rejection.The test verifies that
['a' => [1, 'b' => [212]]]is rejected because it mixes integer key0(for value1) with string key'b'at the same nesting level. This aligns with theisValidObjectQueryValues()implementation.tests/unit/Validator/IndexTest.php (1)
480-598: LGTM!The trigram index validation tests provide comprehensive coverage including:
- Valid single and multi-attribute string indexes
- Invalid non-string and mixed-type attributes
- Invalid orders and lengths
- Behavior when trigram support is disabled
src/Database/Validator/Index.php (3)
32-56: LGTM!Constructor parameters are well-documented and follow the existing pattern with sensible defaults (key, unique, fulltext default to
truefor backward compatibility).
111-159: LGTM!The validation flow properly chains all index checks, with the new
checkValidIndex()at the beginning to fail fast on unsupported index types.
564-595: LGTM!The trigram index validation properly:
- Early-exits for non-trigram types
- Checks adapter support
- Validates all attributes are strings
- Forbids orders and lengths (matching vector index behavior)
src/Database/Adapter/MySQL.php (2)
163-166: LGTM!Correctly maps MySQL error code 3699 (
ER_REGEXP_TIME_OUT) toTimeoutException, consistent with the existing timeout handling for error 3024.
261-264: LGTM!Correctly implements the abstract
getSupportForObjectIndexes()method, returningfalseto indicate MySQL doesn't support object indexes. This aligns with the other adapters (MariaDB, SQLite, Mongo returnfalse; Postgres returnstrue).src/Database/Database.php (2)
82-91: Expose trigram index type looks good. Addingpublic const INDEX_TRIGRAM = 'trigram';is a clean/public surface addition for downstream callers.
1677-1696: No issues found with IndexValidator constructor ordering across call sites.All three instantiations (lines 1678, 2921, 4127) pass parameters in the correct order matching the
IndexValidator::__construct()signature. The adapter capability flags (parameters 11-17) are identically ordered and all corresponding adapter methods (getSupportForObjectIndexes(),getSupportForTrigramIndex(), etc.) are properly defined inAdapter.php. The differences between call sites (parameters 1-2) are contextual and appropriate to their usage contexts.src/Database/Adapter/SQLite.php (1)
1016-1024: LGTM: capability methods added and consistently returnfalsefor SQLiteAlso applies to: 1894-1915
src/Database/Adapter/MariaDB.php (1)
2153-2157: Regex capability flags are correct for all supported MariaDB versionsMariaDB 10.5 (documented minimum per README) and 10.11 (tested per docker-compose.yml) both use PCRE exclusively. MariaDB has shipped only PCRE for REGEXP since version 10.0.5; there is no POSIX variant in any supported version. The flags
getSupportForPCRERegex(): trueandgetSupportForPOSIXRegex(): falseare accurate and require no changes.src/Database/Adapter/Postgres.php (1)
2122-2151: Regex capability semantics: PostgreSQL~operator enforces POSIX-only behavior at runtimeThe adapter correctly declares
getSupportForPCRERegex(): falseandgetSupportForPOSIXRegex(): true, matching PostgreSQL's POSIX ERE (Spencer engine) regex flavor for the~operator. However, the query layer does not validate regex patterns before passing them to PostgreSQL—PCRE-only constructs (lookahead/lookbehind, Unicode escapes\p{...}, atomic groups, etc.) will be rejected only at query execution time.Consider adding application-level validation or documentation to guide users toward POSIX-compatible patterns (
\yfor word boundaries instead of\b, etc.), as the test suite demonstrates. Patterns are safely parameterized but unsanitized, relying on PostgreSQL to reject invalid syntax.src/Database/Adapter/Mongo.php (6)
1263-1268: LGTM!The VAR_OBJECT case correctly handles stdClass conversion for object attributes in the casting flow.
2607-2613: LGTM!The operator mappings for
TYPE_REGEXandTYPE_ELEM_MATCHare correctly added to support the new query types.
2882-2900: LGTM!MongoDB uses PCRE for regex operations, so
getSupportForPCRERegex() = trueandgetSupportForPOSIXRegex() = falseare correct.
2952-2965: LGTM!MongoDB natively supports embedded documents (objects), and the capability flags accurately reflect this. Object indexes returning false is appropriate as MongoDB handles this differently than SQL databases.
3398-3402: LGTM!MongoDB doesn't support trigram indexes natively (unlike PostgreSQL's pg_trgm), so returning
falseis correct.
2564-2564: No issue. The project's minimum PHP version is 8.1 (as specified in composer.json), which is exactly whenarray_is_list()was introduced. The function usage is fully compatible with the project's requirements.tests/e2e/Adapter/Scopes/DocumentTests.php (1)
5-19: Imports look correct and are used (PDOException,TimeoutException).tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (3)
18-40: Well-structured helper method for conditional attribute creation.The helper appropriately short-circuits for schemaless adapters (
getSupportForAttributes() === false) and includes an assertion to verify successful creation. This centralizes attribute creation logic and aligns with the learned pattern that schemaless adapters should not expect attribute-related exceptions.
583-585: Guard condition updated to use capability-specific check.The change from
getSupportForObject()togetSupportForObjectIndexes()is more precise for this test, as it specifically tests object index functionality rather than just object attribute support.
691-694: Appropriate compound guard condition for schema-based tests.The combined check for both
getSupportForObject()andgetSupportForAttributes()correctly identifies adapters that support schema-defined object attributes, excluding schemaless adapters from tests that expect structural validation exceptions.tests/e2e/Adapter/Scopes/SchemalessTests.php (3)
1443-1589: Comprehensive elemMatch test coverage.The
testElemMatchmethod provides thorough coverage of elemMatch query functionality including:
- Basic equality with comparison operators
- Multiple conditions within elemMatch
- Numeric and string comparisons
- Price/float conditions
- NotEqual operator combinations
The test assertions are well-structured and verify expected document IDs.
1596-1686: Good coverage of complex elemMatch scenarios.The
testElemMatchComplexmethod extends coverage with:
- Boolean field conditions
- Between operator
- Nested OR/AND groupings within elemMatch
This complements the basic test well.
1782-1866: Thorough nested object attribute query tests.The test method covers important scenarios for schemaless adapters:
- Mongo-style dotted path queries (
profile.location.city)- Nested numeric field comparisons
existson nested paths- Combined filters on nested paths
containswith nested structureselemMatchwith nested object conditionsThis provides good coverage for the new nested query capabilities.
tests/e2e/Adapter/Scopes/IndexTests.php (4)
165-183: Validator instantiation updated with expanded capability parameters.The Index validator now receives additional capability flags:
getSupportForObject()getSupportForTrigramIndex()getSupportForSpatialAttributes()getSupportForIndex()getSupportForUniqueIndex()getSupportForFulltextIndex()This aligns with the broader adapter capability expansion in this PR.
284-303: Improved error message handling for fulltext index support.The conditional logic now properly distinguishes between:
- Adapters that don't support fulltext indexes at all
- Adapters that don't support multiple fulltext indexes
- Adapters with attribute validation
This prevents test failures on adapters that lack fulltext support entirely.
667-714: Well-structured trigram index test.The test properly:
- Skips when trigram support is unavailable
- Creates and verifies trigram indexes
- Tests deletion of trigram indexes
- Uses
try/finallyfor cleanup
716-787: Comprehensive trigram index validation test.Good coverage of edge cases:
- Non-string attribute rejection
- Multiple string attributes support
- Mixed attribute type rejection
- Orders parameter rejection
- Lengths parameter rejection
Each case properly asserts the expected error message.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if ($query->getMethod() === Query::TYPE_ELEM_MATCH) { | ||
| $filters[$separator][] = [ | ||
| $query->getAttribute() => [ | ||
| '$elemMatch' => $this->buildFilters($query->getValues(), $separator) | ||
| ] | ||
| ]; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check context around the TYPE_ELEM_MATCH handling
sed -n '2350,2400p' src/Database/Adapter/Mongo.php | cat -nRepository: utopia-php/database
Length of output: 1931
🏁 Script executed:
#!/bin/bash
# Check how other query types handle attributes
rg -n "Query::TYPE_" src/Database/Adapter/Mongo.php | grep -E "if|case" | head -20Repository: utopia-php/database
Length of output: 1192
🏁 Script executed:
#!/bin/bash
# Look for filter() usage and attribute handling patterns
rg -n "getAttribute\(\)|filter\(|getInternalKeyForAttribute" src/Database/Adapter/Mongo.php -A 2 -B 1 | head -100Repository: utopia-php/database
Length of output: 4133
🏁 Script executed:
#!/bin/bash
# Get the full buildFilter method implementation
sed -n '2047,2650p' src/Database/Adapter/Mongo.php | cat -nRepository: utopia-php/database
Length of output: 26272
🏁 Script executed:
#!/bin/bash
# Check how getInternalKeyForAttribute is used with getAttribute results
rg -n "getInternalKeyForAttribute\(" src/Database/Adapter/Mongo.php -B 2 -A 2 | head -80Repository: utopia-php/database
Length of output: 1837
🏁 Script executed:
#!/bin/bash
# Search for elemMatch usage in tests
rg -r "ELEM_MATCH|elemMatch" tests/ -A 3 -B 1 | head -150Repository: utopia-php/database
Length of output: 2165
🏁 Script executed:
#!/bin/bash
# Search for elemMatch in the main codebase
rg "ELEM_MATCH|elemMatch" src/ -A 2 -B 1 | head -100Repository: utopia-php/database
Length of output: 4841
🏁 Script executed:
#!/bin/bash
# Check if any Query objects are created with special attributes and elemMatch
rg "TYPE_ELEM_MATCH" . --include="*.php" -B 5 -A 5 | head -200Repository: utopia-php/database
Length of output: 504
🏁 Script executed:
#!/bin/bash
# Search for elemMatch test usage more carefully
find tests -name "*.php" -exec grep -l "elemMatch\|ELEM_MATCH" {} \;Repository: utopia-php/database
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Check validator - does it validate the attribute name for elemMatch?
sed -n '1,200p' src/Database/Validator/Query/Filter.php | cat -nRepository: utopia-php/database
Length of output: 9097
🏁 Script executed:
#!/bin/bash
# Look at the full elemMatch validator logic
rg -n "TYPE_ELEM_MATCH" src/Database/Validator/Query/Filter.php -A 15 -B 2Repository: utopia-php/database
Length of output: 950
🏁 Script executed:
#!/bin/bash
# Check the schemaless tests to see actual elemMatch usage
sed -n '1,100p' tests/e2e/Adapter/Scopes/SchemalessTests.php | cat -nRepository: utopia-php/database
Length of output: 5458
🏁 Script executed:
#!/bin/bash
# Search for elemMatch usage in SchemalessTests
rg -n "elemMatch" tests/e2e/Adapter/Scopes/SchemalessTests.php -B 3 -A 10Repository: utopia-php/database
Length of output: 9431
🏁 Script executed:
#!/bin/bash
# Check if special attributes like $id are ever used with elemMatch
rg -n "elemMatch.*\\\$id\|\\\$sequence\|\\\$createdAt\|\\\$updatedAt" tests/ -iRepository: utopia-php/database
Length of output: 45
Apply attribute name filtering to the $elemMatch field name for consistency.
The $elemMatch handling uses $query->getAttribute() directly (line 327) without applying the special attribute conversions that are applied in buildFilter() for other query types. This creates an inconsistency: nested queries inside elemMatch are converted via the recursive buildFilters()->buildFilter() chain, but the array field name itself bypasses conversion.
For example, if someone used Query::elemMatch('$permissions', [...]), the generated filter would incorrectly use the literal field name '$permissions' instead of the converted '_permissions'. Since MongoDB field names cannot start with $, this would fail.
Apply the same special attribute conversions before using the attribute name in the $elemMatch structure:
if ($query->getMethod() === Query::TYPE_ELEM_MATCH) {
$attribute = $query->getAttribute();
// Apply the same conversions as buildFilter
if ($attribute === '$id') {
$attribute = '_uid';
} elseif ($attribute === '$sequence') {
$attribute = '_id';
} elseif ($attribute === '$createdAt') {
$attribute = '_createdAt';
} elseif ($attribute === '$updatedAt') {
$attribute = '_updatedAt';
}
$filters[$separator][] = [
$attribute => [
'$elemMatch' => $this->buildFilters($query->getValues(), $separator)
]
];
continue;
}🤖 Prompt for AI Agents
In `@src/Database/Adapter/Mongo.php` around lines 2371 - 2378, The elemMatch
branch for Query::TYPE_ELEM_MATCH uses $query->getAttribute() directly,
bypassing the special attribute name conversions performed by buildFilter(),
which can produce invalid Mongo field names (e.g. '$permissions'); update the
branch in the Mongo adapter so you first map the attribute through the same
conversion logic used in buildFilter() (handle '$id' -> '_uid', '$sequence' ->
'_id', '$createdAt' -> '_createdAt', '$updatedAt' -> '_updatedAt', etc.), then
use that converted attribute as the key for the '$elemMatch' array and call
$this->buildFilters($query->getValues(), $separator) for the nested filters.
|
|
||
| $logicalOperator = $isNot ? '$and' : '$or'; | ||
| if (count($conditions) && isset($filter[$logicalOperator])) { | ||
| $filter[$logicalOperator] = array_merge($filter[$logicalOperator], $conditions); | ||
| } else { | ||
| $filter[$logicalOperator] = $conditions; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full context of the handleObjectFilters method
head -n 2550 src/Database/Adapter/Mongo.php | tail -n 100Repository: utopia-php/database
Length of output: 4454
🏁 Script executed:
# Search for tests involving TYPE_EQUAL with objects
rg -n "TYPE_EQUAL" tests/ -A 5 -B 5 | head -100Repository: utopia-php/database
Length of output: 3443
🏁 Script executed:
# Search for handleObjectFilters tests and object equality scenarios
rg -n "handleObjectFilters|object.*equal|metadata.*equal" tests/ -i -A 3 -B 3Repository: utopia-php/database
Length of output: 744
🏁 Script executed:
# Let me see more context around that test case
sed -n '1064,1100p' tests/e2e/Adapter/Scopes/ObjectAttributeTests.phpRepository: utopia-php/database
Length of output: 1382
🏁 Script executed:
# Search for tests with Query::equal on objects with multiple properties
rg -n "Query::equal.*metadata.*\[" tests/e2e/Adapter/Scopes/ObjectAttributeTests.php -A 5 | head -80Repository: utopia-php/database
Length of output: 779
🏁 Script executed:
# Let's search for any test with multiple keys in object equality
rg -n "Query::equal.*metadata" tests/e2e/Adapter/Scopes/ObjectAttributeTests.php -B 2 -A 15 | grep -A 15 "multiple\|Multi" | head -80Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Let me look at the ObjectAttributeTests file more carefully to understand the test scenarios
grep -n "public function test" tests/e2e/Adapter/Scopes/ObjectAttributeTests.php | head -20Repository: utopia-php/database
Length of output: 352
🏁 Script executed:
# Let's look at the testObjectAttribute test method more closely
sed -n '42,200p' tests/e2e/Adapter/Scopes/ObjectAttributeTests.phpRepository: utopia-php/database
Length of output: 6050
🏁 Script executed:
# Let's search for tests with multiple properties being queried at the same level
sed -n '150,250p' tests/e2e/Adapter/Scopes/ObjectAttributeTests.phpRepository: utopia-php/database
Length of output: 4268
🏁 Script executed:
# Search for tests that pass multiple properties in one Query::equal call
rg -n "Query::equal.*meta.*\[.*'[a-z]+'\s*=>" tests/e2e/Adapter/Scopes/ObjectAttributeTests.php -A 10 | head -100Repository: utopia-php/database
Length of output: 4259
🏁 Script executed:
# Let me check if there's any test with multiple top-level properties in a single equal query
rg -n "Query::equal.*meta.*\[\[" tests/e2e/Adapter/Scopes/ObjectAttributeTests.php -A 8 | grep -E "'=>|^\d+" | head -60Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Search for tests where Query::equal has multiple keys at the same level
rg "Query::equal\('meta'" tests/e2e/Adapter/Scopes/ObjectAttributeTests.php -A 6 | grep -B 5 -E "'[a-z]+'\s*=>\s*[^,}]*,\s*'[a-z]+'\s*=>" | head -80Repository: utopia-php/database
Length of output: 631
🏁 Script executed:
# Let me look at the test with 'config' having multiple properties
rg -n "config.*theme.*language|key_25" tests/e2e/Adapter/Scopes/ObjectAttributeTests.php -B 5 -A 10Repository: utopia-php/database
Length of output: 2382
🏁 Script executed:
# Let me understand the Query structure better
rg -n "class Query" src/ -A 30 | head -60Repository: utopia-php/database
Length of output: 2340
🏁 Script executed:
# Let me check what flattenWithDotNotation does
rg -n "flattenWithDotNotation" src/Database/Adapter/Mongo.php -A 20 | head -80Repository: utopia-php/database
Length of output: 2301
🏁 Script executed:
# Let me check if there's validation that limits object queries to one value
rg -n "notEqual.*meta\|object.*equal\|QueryException" src/Database/ -B 3 -A 3 | head -100Repository: utopia-php/database
Length of output: 45
Logical operator for object equality queries should use $and instead of $or.
When flattening nested object properties for equality queries, multiple leaf conditions are created (e.g., config.theme and config.language). Using $or to combine these conditions means "match any property" rather than "match all properties". For equality semantics, all specified properties should match simultaneously, requiring $and.
The current asymmetry is problematic: TYPE_NOT_EQUAL correctly uses $and (all conditions must fail), while TYPE_EQUAL incorrectly uses $or (any condition can pass). Both should use $and to ensure all properties match.
🤖 Prompt for AI Agents
In `@src/Database/Adapter/Mongo.php` around lines 2536 - 2542, The code uses
$logicalOperator = $isNot ? '$and' : '$or' which causes equality (TYPE_EQUAL) to
combine flattened object leaf conditions with '$or' instead of requiring all
properties to match; change this to always use '$and' for these
flattened-equality branches (i.e., set $logicalOperator = '$and') and keep the
existing merge/assignment logic on $filter, $conditions so both equality and
not-equality cases use '$and' (refer to $logicalOperator, $isNot, $filter,
$conditions in Mongo.php).
| public function checkKeyUniqueFulltextSupport(Document $index): bool | ||
| { | ||
| $type = $index->getAttribute('type'); | ||
|
|
||
| if ($type === Database::INDEX_KEY && $this->supportForKeyIndexes === false) { | ||
| $this->message = 'Key index is not supported'; | ||
| return false; | ||
| } | ||
|
|
||
| if ($type === Database::INDEX_UNIQUE && $this->supportForUniqueIndexes === false) { | ||
| $this->message = 'Unique index is not supported'; | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Method name implies fulltext support check, but fulltext validation is missing.
checkKeyUniqueFulltextSupport() name suggests it validates KEY, UNIQUE, and FULLTEXT indexes, but it only checks KEY and UNIQUE. The FULLTEXT check exists in checkValidIndex() but not here.
Either rename the method to checkKeyUniqueSupport() or add the missing fulltext check for consistency:
Suggested fix
public function checkKeyUniqueFulltextSupport(Document $index): bool
{
$type = $index->getAttribute('type');
if ($type === Database::INDEX_KEY && $this->supportForKeyIndexes === false) {
$this->message = 'Key index is not supported';
return false;
}
if ($type === Database::INDEX_UNIQUE && $this->supportForUniqueIndexes === false) {
$this->message = 'Unique index is not supported';
return false;
}
+ if ($type === Database::INDEX_FULLTEXT && $this->supportForFulltextIndexes === false) {
+ $this->message = 'Fulltext index is not supported';
+ return false;
+ }
+
return true;
}🤖 Prompt for AI Agents
In `@src/Database/Validator/Index.php` around lines 601 - 616, The method
checkKeyUniqueFulltextSupport() is named to include FULLTEXT but only checks KEY
and UNIQUE; either rename it to checkKeyUniqueSupport() and update all call
sites (including where checkValidIndex() invokes it) to reflect the new name, or
add the missing FULLTEXT branch: check for if ($type ===
Database::INDEX_FULLTEXT && $this->supportForFulltextIndexes === false) set
$this->message = 'Fulltext index is not supported' and return false, ensuring
consistency with the KEY and UNIQUE checks and with checkValidIndex()'s
behavior.
| foreach ($allInjectionPatterns as $pattern) { | ||
| try { | ||
| $results = $database->find($collectionName, [ | ||
| Query::regex('text', $pattern), | ||
| ]); | ||
|
|
||
| // Critical check: if injection succeeded, we might get the "other" document | ||
| // which should NOT match a pattern starting with "target" | ||
| $foundOther = false; | ||
| foreach ($results as $doc) { | ||
| $text = $doc->getAttribute('text'); | ||
| if ($text === 'other') { | ||
| $foundOther = true; | ||
|
|
||
| // Verify that "other" doesn't actually match the pattern as a regex | ||
| $matches = @preg_match('/' . str_replace('/', '\/', $pattern) . '/', $text); | ||
| if ($matches === 0 || $matches === false) { | ||
| // "other" doesn't match the pattern but was returned | ||
| // This indicates potential injection vulnerability | ||
| $this->fail( | ||
| "Potential injection detected: Pattern '{$pattern}' returned document 'other' " . | ||
| "which doesn't match the pattern. This suggests SQL/MongoDB injection may have succeeded." | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Additional verification: check that all returned documents actually match the pattern | ||
| foreach ($results as $doc) { | ||
| $text = $doc->getAttribute('text'); | ||
| $matches = @preg_match('/' . str_replace('/', '\/', $pattern) . '/', $text); | ||
|
|
||
| // If pattern is invalid, skip validation | ||
| if ($matches === false) { | ||
| continue; | ||
| } | ||
|
|
||
| // If document doesn't match but was returned, it's suspicious | ||
| if ($matches === 0) { | ||
| $this->fail( | ||
| "Potential injection: Document '{$text}' was returned for pattern '{$pattern}' " . | ||
| "but doesn't match the regex pattern." | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| } catch (\Exception $e) { | ||
| // Exceptions are acceptable - they indicate the injection was blocked or caused an error | ||
| // This is actually good - it means the system rejected the malicious pattern | ||
| $this->assertInstanceOf(\Exception::class, $e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix PHPMD issue: remove unused $foundOther
PHPMD is right: $foundOther is never read.
Proposed fix
- $foundOther = false;
foreach ($results as $doc) {
$text = $doc->getAttribute('text');
if ($text === 'other') {
- $foundOther = true;
-
// Verify that "other" doesn't actually match the pattern as a regex
$matches = `@preg_match`('/' . str_replace('/', '\/', $pattern) . '/', $text);
if ($matches === 0 || $matches === false) {
// "other" doesn't match the pattern but was returned
// This indicates potential injection vulnerability
$this->fail(
"Potential injection detected: Pattern '{$pattern}' returned document 'other' " .
"which doesn't match the pattern. This suggests SQL/MongoDB injection may have succeeded."
);
}
}
}🧰 Tools
🪛 PHPMD (2.15.0)
7162-7162: Avoid unused local variables such as '$foundOther'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@tests/e2e/Adapter/Scopes/DocumentTests.php` around lines 7154 - 7205, Remove
the unused $foundOther variable: delete the declaration "$foundOther = false;"
and the assignment "$foundOther = true;" inside the first foreach over $results;
keep the surrounding logic that checks if $text === 'other' and performs the
preg_match/fail assertions (so behavior is unchanged but the unused flag is
removed). This change touches the foreach ($results as $doc) block and the
$foundOther symbol in DocumentTests.php.
| $collectionName = 'redosTimeoutTest'; | ||
| $database->createCollection($collectionName, permissions: [ | ||
| Permission::create(Role::any()), | ||
| Permission::read(Role::any()), | ||
| Permission::update(Role::any()), | ||
| Permission::delete(Role::any()), | ||
| ]); | ||
|
|
||
| if ($database->getAdapter()->getSupportForAttributes()) { | ||
| $this->assertEquals(true, $database->createAttribute($collectionName, 'text', Database::VAR_STRING, 1000, true)); | ||
| } | ||
|
|
||
| // Create documents with strings designed to trigger ReDoS | ||
| // These strings have many 'a's but end with 'c' instead of 'b' | ||
| // This causes catastrophic backtracking with patterns like (a+)+b | ||
| $redosStrings = []; | ||
| for ($i = 15; $i <= 35; $i += 5) { | ||
| $redosStrings[] = str_repeat('a', $i) . 'c'; | ||
| } | ||
|
|
||
| // Also add some normal strings | ||
| $normalStrings = [ | ||
| 'normal text', | ||
| 'another string', | ||
| 'test123', | ||
| 'valid data', | ||
| ]; | ||
|
|
||
| $documents = []; | ||
| foreach ($redosStrings as $text) { | ||
| $documents[] = new Document([ | ||
| '$permissions' => [ | ||
| Permission::read(Role::any()), | ||
| Permission::create(Role::any()), | ||
| Permission::update(Role::any()), | ||
| Permission::delete(Role::any()), | ||
| ], | ||
| 'text' => $text, | ||
| ]); | ||
| } | ||
|
|
||
| foreach ($normalStrings as $text) { | ||
| $documents[] = new Document([ | ||
| '$permissions' => [ | ||
| Permission::read(Role::any()), | ||
| Permission::create(Role::any()), | ||
| Permission::update(Role::any()), | ||
| Permission::delete(Role::any()), | ||
| ], | ||
| 'text' => $text, | ||
| ]); | ||
| } | ||
|
|
||
| $database->createDocuments($collectionName, $documents); | ||
|
|
||
| // ReDoS patterns that cause exponential backtracking | ||
| $redosPatterns = [ | ||
| '(a+)+b', // Classic ReDoS: nested quantifiers | ||
| '(a|a)*b', // Alternation with quantifier | ||
| '(a+)+$', // Anchored pattern | ||
| '(a*)*b', // Nested star quantifiers | ||
| '(a+)+b+', // Multiple nested quantifiers | ||
| '(.+)+b', // Generic nested quantifiers | ||
| '(.*)+b', // Generic nested quantifiers | ||
| ]; | ||
|
|
||
| $supportsTimeout = $database->getAdapter()->getSupportForTimeouts(); | ||
|
|
||
| if ($supportsTimeout) { | ||
| $database->setTimeout(2000); | ||
| } | ||
|
|
||
| foreach ($redosPatterns as $pattern) { | ||
| $startTime = microtime(true); | ||
|
|
||
| try { | ||
| $results = $database->find($collectionName, [ | ||
| Query::regex('text', $pattern), | ||
| ]); | ||
| $elapsed = microtime(true) - $startTime; | ||
| // If timeout is supported, the query should either: | ||
| // 1. Complete quickly (< 3 seconds) if ReDoS is mitigated | ||
| // 2. Throw TimeoutException if it takes too long | ||
| if ($supportsTimeout) { | ||
| // If we got here without timeout, it should have completed quickly | ||
| $this->assertLessThan( | ||
| 3.0, | ||
| $elapsed, | ||
| "Regex pattern '{$pattern}' should complete quickly or timeout. Took {$elapsed}s" | ||
| ); | ||
| } else { | ||
| // Without timeout support, we just check it doesn't hang forever | ||
| // Set a reasonable upper bound (15 seconds) for systems without timeout | ||
| $this->assertLessThan( | ||
| 15.0, | ||
| $elapsed, | ||
| "Regex pattern '{$pattern}' should not cause excessive delay. Took {$elapsed}s" | ||
| ); | ||
| } | ||
|
|
||
| // Verify results: none of our ReDoS strings should match these patterns | ||
| // (they all end with 'c', not 'b') | ||
| foreach ($results as $doc) { | ||
| $text = $doc->getAttribute('text'); | ||
| // If it matched, verify it's actually a valid match | ||
| $matches = @preg_match('/' . str_replace('/', '\/', $pattern) . '/', $text); | ||
| if ($matches !== false) { | ||
| $this->assertEquals( | ||
| 1, | ||
| $matches, | ||
| "Document with text '{$text}' should actually match pattern '{$pattern}'" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| } catch (TimeoutException $e) { | ||
| // Timeout is expected for ReDoS patterns if not properly mitigated | ||
| $elapsed = microtime(true) - $startTime; | ||
| $this->assertInstanceOf( | ||
| TimeoutException::class, | ||
| $e, | ||
| "Regex pattern '{$pattern}' should timeout if it causes ReDoS. Elapsed: {$elapsed}s" | ||
| ); | ||
|
|
||
| // Timeout should happen within reasonable time (not immediately, but not too late) | ||
| // Fast timeouts are actually good - they mean the system is protecting itself quickly | ||
| $this->assertGreaterThan( | ||
| 0.05, | ||
| $elapsed, | ||
| "Timeout should occur after some minimal processing time" | ||
| ); | ||
|
|
||
| // Timeout should happen before the timeout limit (with some buffer) | ||
| if ($supportsTimeout) { | ||
| $this->assertLessThan( | ||
| 5.0, | ||
| $elapsed, | ||
| "Timeout should occur within reasonable time (before 5 seconds)" | ||
| ); | ||
| } | ||
|
|
||
| } catch (\Exception $e) { | ||
| // Check if this is a query interruption/timeout from MySQL (error 1317) | ||
| // MySQL sometimes throws "Query execution was interrupted" instead of TimeoutException | ||
| $message = $e->getMessage(); | ||
| $isQueryInterrupted = false; | ||
|
|
||
| // Check message for interruption keywords | ||
| if (strpos($message, 'Query execution was interrupted') !== false || | ||
| strpos($message, 'interrupted') !== false) { | ||
| $isQueryInterrupted = true; | ||
| } | ||
|
|
||
| // Check if it's a PDOException with error code 1317 | ||
| if ($e instanceof PDOException) { | ||
| $errorInfo = $e->errorInfo ?? []; | ||
| // Error 1317 is "Query execution was interrupted" | ||
| if (isset($errorInfo[1]) && $errorInfo[1] === 1317) { | ||
| $isQueryInterrupted = true; | ||
| } | ||
| // Also check SQLSTATE 70100 | ||
| if ($e->getCode() === '70100') { | ||
| $isQueryInterrupted = true; | ||
| } | ||
| } | ||
|
|
||
| if ($isQueryInterrupted) { | ||
| // This is effectively a timeout - MySQL interrupted the query | ||
| $elapsed = microtime(true) - $startTime; | ||
| $this->assertGreaterThan( | ||
| 0.05, | ||
| $elapsed, | ||
| "Query interruption should occur after some minimal processing time" | ||
| ); | ||
| // This is acceptable - the query was interrupted due to timeout | ||
| continue; | ||
| } | ||
|
|
||
| // Other exceptions are unexpected | ||
| $this->fail("Unexpected exception for pattern '{$pattern}': " . get_class($e) . " - " . $e->getMessage()); | ||
| } | ||
| } | ||
|
|
||
| // Test with a pattern that should match quickly (not ReDoS) | ||
| $safePattern = 'normal'; | ||
| $startTime = microtime(true); | ||
| $results = $database->find($collectionName, [ | ||
| Query::regex('text', $safePattern), | ||
| ]); | ||
| $elapsed = microtime(true) - $startTime; | ||
|
|
||
| // Safe patterns should complete very quickly | ||
| $this->assertLessThan(1.0, $elapsed, 'Safe regex pattern should complete quickly'); | ||
| $this->assertGreaterThan(0, count($results), 'Safe pattern should match some documents'); | ||
|
|
||
| // Verify safe pattern results are correct | ||
| foreach ($results as $doc) { | ||
| $text = $doc->getAttribute('text'); | ||
| $this->assertStringContainsString('normal', $text, "Document '{$text}' should contain 'normal'"); | ||
| } | ||
|
|
||
| // Cleanup | ||
| if ($supportsTimeout) { | ||
| $database->clearTimeout(); | ||
| } | ||
| $database->deleteCollection($collectionName); | ||
| } |
There was a problem hiding this comment.
Ensure setTimeout() is always cleared (avoid cascading failures across tests)
testRegexRedos() mutates adapter-level timeout state, but clearTimeout() is only called at the end of the happy path. Any assertion failure / unexpected exception will skip cleanup and may break subsequent tests (especially since PHPUnit will continue running remaining tests).
Proposed fix (use try/finally for cleanup)
public function testRegexRedos(): void
{
/** `@var` Database $database */
$database = static::getDatabase();
@@
- $collectionName = 'redosTimeoutTest';
- $database->createCollection($collectionName, permissions: [
+ $collectionName = 'redosTimeoutTest';
+ $supportsTimeout = $database->getAdapter()->getSupportForTimeouts();
+
+ $database->createCollection($collectionName, permissions: [
Permission::create(Role::any()),
Permission::read(Role::any()),
Permission::update(Role::any()),
Permission::delete(Role::any()),
]);
- if ($database->getAdapter()->getSupportForAttributes()) {
- $this->assertEquals(true, $database->createAttribute($collectionName, 'text', Database::VAR_STRING, 1000, true));
- }
+ try {
+ if ($database->getAdapter()->getSupportForAttributes()) {
+ $this->assertEquals(true, $database->createAttribute($collectionName, 'text', Database::VAR_STRING, 1000, true));
+ }
@@
- $supportsTimeout = $database->getAdapter()->getSupportForTimeouts();
-
- if ($supportsTimeout) {
- $database->setTimeout(2000);
- }
+ if ($supportsTimeout) {
+ $database->setTimeout(2000);
+ }
@@
- // Cleanup
- if ($supportsTimeout) {
- $database->clearTimeout();
- }
- $database->deleteCollection($collectionName);
+ } finally {
+ if ($supportsTimeout) {
+ try {
+ $database->clearTimeout();
+ } catch (\Throwable) {
+ // ignore cleanup failures to avoid hiding the original assertion failure
+ }
+ }
+ try {
+ $database->deleteCollection($collectionName);
+ } catch (\Throwable) {
+ // ignore cleanup failures to avoid hiding the original assertion failure
+ }
+ }
}📝 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.
| $collectionName = 'redosTimeoutTest'; | |
| $database->createCollection($collectionName, permissions: [ | |
| Permission::create(Role::any()), | |
| Permission::read(Role::any()), | |
| Permission::update(Role::any()), | |
| Permission::delete(Role::any()), | |
| ]); | |
| if ($database->getAdapter()->getSupportForAttributes()) { | |
| $this->assertEquals(true, $database->createAttribute($collectionName, 'text', Database::VAR_STRING, 1000, true)); | |
| } | |
| // Create documents with strings designed to trigger ReDoS | |
| // These strings have many 'a's but end with 'c' instead of 'b' | |
| // This causes catastrophic backtracking with patterns like (a+)+b | |
| $redosStrings = []; | |
| for ($i = 15; $i <= 35; $i += 5) { | |
| $redosStrings[] = str_repeat('a', $i) . 'c'; | |
| } | |
| // Also add some normal strings | |
| $normalStrings = [ | |
| 'normal text', | |
| 'another string', | |
| 'test123', | |
| 'valid data', | |
| ]; | |
| $documents = []; | |
| foreach ($redosStrings as $text) { | |
| $documents[] = new Document([ | |
| '$permissions' => [ | |
| Permission::read(Role::any()), | |
| Permission::create(Role::any()), | |
| Permission::update(Role::any()), | |
| Permission::delete(Role::any()), | |
| ], | |
| 'text' => $text, | |
| ]); | |
| } | |
| foreach ($normalStrings as $text) { | |
| $documents[] = new Document([ | |
| '$permissions' => [ | |
| Permission::read(Role::any()), | |
| Permission::create(Role::any()), | |
| Permission::update(Role::any()), | |
| Permission::delete(Role::any()), | |
| ], | |
| 'text' => $text, | |
| ]); | |
| } | |
| $database->createDocuments($collectionName, $documents); | |
| // ReDoS patterns that cause exponential backtracking | |
| $redosPatterns = [ | |
| '(a+)+b', // Classic ReDoS: nested quantifiers | |
| '(a|a)*b', // Alternation with quantifier | |
| '(a+)+$', // Anchored pattern | |
| '(a*)*b', // Nested star quantifiers | |
| '(a+)+b+', // Multiple nested quantifiers | |
| '(.+)+b', // Generic nested quantifiers | |
| '(.*)+b', // Generic nested quantifiers | |
| ]; | |
| $supportsTimeout = $database->getAdapter()->getSupportForTimeouts(); | |
| if ($supportsTimeout) { | |
| $database->setTimeout(2000); | |
| } | |
| foreach ($redosPatterns as $pattern) { | |
| $startTime = microtime(true); | |
| try { | |
| $results = $database->find($collectionName, [ | |
| Query::regex('text', $pattern), | |
| ]); | |
| $elapsed = microtime(true) - $startTime; | |
| // If timeout is supported, the query should either: | |
| // 1. Complete quickly (< 3 seconds) if ReDoS is mitigated | |
| // 2. Throw TimeoutException if it takes too long | |
| if ($supportsTimeout) { | |
| // If we got here without timeout, it should have completed quickly | |
| $this->assertLessThan( | |
| 3.0, | |
| $elapsed, | |
| "Regex pattern '{$pattern}' should complete quickly or timeout. Took {$elapsed}s" | |
| ); | |
| } else { | |
| // Without timeout support, we just check it doesn't hang forever | |
| // Set a reasonable upper bound (15 seconds) for systems without timeout | |
| $this->assertLessThan( | |
| 15.0, | |
| $elapsed, | |
| "Regex pattern '{$pattern}' should not cause excessive delay. Took {$elapsed}s" | |
| ); | |
| } | |
| // Verify results: none of our ReDoS strings should match these patterns | |
| // (they all end with 'c', not 'b') | |
| foreach ($results as $doc) { | |
| $text = $doc->getAttribute('text'); | |
| // If it matched, verify it's actually a valid match | |
| $matches = @preg_match('/' . str_replace('/', '\/', $pattern) . '/', $text); | |
| if ($matches !== false) { | |
| $this->assertEquals( | |
| 1, | |
| $matches, | |
| "Document with text '{$text}' should actually match pattern '{$pattern}'" | |
| ); | |
| } | |
| } | |
| } catch (TimeoutException $e) { | |
| // Timeout is expected for ReDoS patterns if not properly mitigated | |
| $elapsed = microtime(true) - $startTime; | |
| $this->assertInstanceOf( | |
| TimeoutException::class, | |
| $e, | |
| "Regex pattern '{$pattern}' should timeout if it causes ReDoS. Elapsed: {$elapsed}s" | |
| ); | |
| // Timeout should happen within reasonable time (not immediately, but not too late) | |
| // Fast timeouts are actually good - they mean the system is protecting itself quickly | |
| $this->assertGreaterThan( | |
| 0.05, | |
| $elapsed, | |
| "Timeout should occur after some minimal processing time" | |
| ); | |
| // Timeout should happen before the timeout limit (with some buffer) | |
| if ($supportsTimeout) { | |
| $this->assertLessThan( | |
| 5.0, | |
| $elapsed, | |
| "Timeout should occur within reasonable time (before 5 seconds)" | |
| ); | |
| } | |
| } catch (\Exception $e) { | |
| // Check if this is a query interruption/timeout from MySQL (error 1317) | |
| // MySQL sometimes throws "Query execution was interrupted" instead of TimeoutException | |
| $message = $e->getMessage(); | |
| $isQueryInterrupted = false; | |
| // Check message for interruption keywords | |
| if (strpos($message, 'Query execution was interrupted') !== false || | |
| strpos($message, 'interrupted') !== false) { | |
| $isQueryInterrupted = true; | |
| } | |
| // Check if it's a PDOException with error code 1317 | |
| if ($e instanceof PDOException) { | |
| $errorInfo = $e->errorInfo ?? []; | |
| // Error 1317 is "Query execution was interrupted" | |
| if (isset($errorInfo[1]) && $errorInfo[1] === 1317) { | |
| $isQueryInterrupted = true; | |
| } | |
| // Also check SQLSTATE 70100 | |
| if ($e->getCode() === '70100') { | |
| $isQueryInterrupted = true; | |
| } | |
| } | |
| if ($isQueryInterrupted) { | |
| // This is effectively a timeout - MySQL interrupted the query | |
| $elapsed = microtime(true) - $startTime; | |
| $this->assertGreaterThan( | |
| 0.05, | |
| $elapsed, | |
| "Query interruption should occur after some minimal processing time" | |
| ); | |
| // This is acceptable - the query was interrupted due to timeout | |
| continue; | |
| } | |
| // Other exceptions are unexpected | |
| $this->fail("Unexpected exception for pattern '{$pattern}': " . get_class($e) . " - " . $e->getMessage()); | |
| } | |
| } | |
| // Test with a pattern that should match quickly (not ReDoS) | |
| $safePattern = 'normal'; | |
| $startTime = microtime(true); | |
| $results = $database->find($collectionName, [ | |
| Query::regex('text', $safePattern), | |
| ]); | |
| $elapsed = microtime(true) - $startTime; | |
| // Safe patterns should complete very quickly | |
| $this->assertLessThan(1.0, $elapsed, 'Safe regex pattern should complete quickly'); | |
| $this->assertGreaterThan(0, count($results), 'Safe pattern should match some documents'); | |
| // Verify safe pattern results are correct | |
| foreach ($results as $doc) { | |
| $text = $doc->getAttribute('text'); | |
| $this->assertStringContainsString('normal', $text, "Document '{$text}' should contain 'normal'"); | |
| } | |
| // Cleanup | |
| if ($supportsTimeout) { | |
| $database->clearTimeout(); | |
| } | |
| $database->deleteCollection($collectionName); | |
| } | |
| public function testRegexRedos(): void | |
| { | |
| /** `@var` Database $database */ | |
| $database = static::getDatabase(); | |
| $collectionName = 'redosTimeoutTest'; | |
| $supportsTimeout = $database->getAdapter()->getSupportForTimeouts(); | |
| $database->createCollection($collectionName, permissions: [ | |
| Permission::create(Role::any()), | |
| Permission::read(Role::any()), | |
| Permission::update(Role::any()), | |
| Permission::delete(Role::any()), | |
| ]); | |
| try { | |
| if ($database->getAdapter()->getSupportForAttributes()) { | |
| $this->assertEquals(true, $database->createAttribute($collectionName, 'text', Database::VAR_STRING, 1000, true)); | |
| } | |
| // Create documents with strings designed to trigger ReDoS | |
| // These strings have many 'a's but end with 'c' instead of 'b' | |
| // This causes catastrophic backtracking with patterns like (a+)+b | |
| $redosStrings = []; | |
| for ($i = 15; $i <= 35; $i += 5) { | |
| $redosStrings[] = str_repeat('a', $i) . 'c'; | |
| } | |
| // Also add some normal strings | |
| $normalStrings = [ | |
| 'normal text', | |
| 'another string', | |
| 'test123', | |
| 'valid data', | |
| ]; | |
| $documents = []; | |
| foreach ($redosStrings as $text) { | |
| $documents[] = new Document([ | |
| '$permissions' => [ | |
| Permission::read(Role::any()), | |
| Permission::create(Role::any()), | |
| Permission::update(Role::any()), | |
| Permission::delete(Role::any()), | |
| ], | |
| 'text' => $text, | |
| ]); | |
| } | |
| foreach ($normalStrings as $text) { | |
| $documents[] = new Document([ | |
| '$permissions' => [ | |
| Permission::read(Role::any()), | |
| Permission::create(Role::any()), | |
| Permission::update(Role::any()), | |
| Permission::delete(Role::any()), | |
| ], | |
| 'text' => $text, | |
| ]); | |
| } | |
| $database->createDocuments($collectionName, $documents); | |
| // ReDoS patterns that cause exponential backtracking | |
| $redosPatterns = [ | |
| '(a+)+b', // Classic ReDoS: nested quantifiers | |
| '(a|a)*b', // Alternation with quantifier | |
| '(a+)+$', // Anchored pattern | |
| '(a*)*b', // Nested star quantifiers | |
| '(a+)+b+', // Multiple nested quantifiers | |
| '(.+)+b', // Generic nested quantifiers | |
| '(.*)+b', // Generic nested quantifiers | |
| ]; | |
| if ($supportsTimeout) { | |
| $database->setTimeout(2000); | |
| } | |
| foreach ($redosPatterns as $pattern) { | |
| $startTime = microtime(true); | |
| try { | |
| $results = $database->find($collectionName, [ | |
| Query::regex('text', $pattern), | |
| ]); | |
| $elapsed = microtime(true) - $startTime; | |
| // If timeout is supported, the query should either: | |
| // 1. Complete quickly (< 3 seconds) if ReDoS is mitigated | |
| // 2. Throw TimeoutException if it takes too long | |
| if ($supportsTimeout) { | |
| // If we got here without timeout, it should have completed quickly | |
| $this->assertLessThan( | |
| 3.0, | |
| $elapsed, | |
| "Regex pattern '{$pattern}' should complete quickly or timeout. Took {$elapsed}s" | |
| ); | |
| } else { | |
| // Without timeout support, we just check it doesn't hang forever | |
| // Set a reasonable upper bound (15 seconds) for systems without timeout | |
| $this->assertLessThan( | |
| 15.0, | |
| $elapsed, | |
| "Regex pattern '{$pattern}' should not cause excessive delay. Took {$elapsed}s" | |
| ); | |
| } | |
| // Verify results: none of our ReDoS strings should match these patterns | |
| // (they all end with 'c', not 'b') | |
| foreach ($results as $doc) { | |
| $text = $doc->getAttribute('text'); | |
| // If it matched, verify it's actually a valid match | |
| $matches = `@preg_match`('/' . str_replace('/', '\/', $pattern) . '/', $text); | |
| if ($matches !== false) { | |
| $this->assertEquals( | |
| 1, | |
| $matches, | |
| "Document with text '{$text}' should actually match pattern '{$pattern}'" | |
| ); | |
| } | |
| } | |
| } catch (TimeoutException $e) { | |
| // Timeout is expected for ReDoS patterns if not properly mitigated | |
| $elapsed = microtime(true) - $startTime; | |
| $this->assertInstanceOf( | |
| TimeoutException::class, | |
| $e, | |
| "Regex pattern '{$pattern}' should timeout if it causes ReDoS. Elapsed: {$elapsed}s" | |
| ); | |
| // Timeout should happen within reasonable time (not immediately, but not too late) | |
| // Fast timeouts are actually good - they mean the system is protecting itself quickly | |
| $this->assertGreaterThan( | |
| 0.05, | |
| $elapsed, | |
| "Timeout should occur after some minimal processing time" | |
| ); | |
| // Timeout should happen before the timeout limit (with some buffer) | |
| if ($supportsTimeout) { | |
| $this->assertLessThan( | |
| 5.0, | |
| $elapsed, | |
| "Timeout should occur within reasonable time (before 5 seconds)" | |
| ); | |
| } | |
| } catch (\Exception $e) { | |
| // Check if this is a query interruption/timeout from MySQL (error 1317) | |
| // MySQL sometimes throws "Query execution was interrupted" instead of TimeoutException | |
| $message = $e->getMessage(); | |
| $isQueryInterrupted = false; | |
| // Check message for interruption keywords | |
| if (strpos($message, 'Query execution was interrupted') !== false || | |
| strpos($message, 'interrupted') !== false) { | |
| $isQueryInterrupted = true; | |
| } | |
| // Check if it's a PDOException with error code 1317 | |
| if ($e instanceof PDOException) { | |
| $errorInfo = $e->errorInfo ?? []; | |
| // Error 1317 is "Query execution was interrupted" | |
| if (isset($errorInfo[1]) && $errorInfo[1] === 1317) { | |
| $isQueryInterrupted = true; | |
| } | |
| // Also check SQLSTATE 70100 | |
| if ($e->getCode() === '70100') { | |
| $isQueryInterrupted = true; | |
| } | |
| } | |
| if ($isQueryInterrupted) { | |
| // This is effectively a timeout - MySQL interrupted the query | |
| $elapsed = microtime(true) - $startTime; | |
| $this->assertGreaterThan( | |
| 0.05, | |
| $elapsed, | |
| "Query interruption should occur after some minimal processing time" | |
| ); | |
| // This is acceptable - the query was interrupted due to timeout | |
| continue; | |
| } | |
| // Other exceptions are unexpected | |
| $this->fail("Unexpected exception for pattern '{$pattern}': " . get_class($e) . " - " . $e->getMessage()); | |
| } | |
| } | |
| // Test with a pattern that should match quickly (not ReDoS) | |
| $safePattern = 'normal'; | |
| $startTime = microtime(true); | |
| $results = $database->find($collectionName, [ | |
| Query::regex('text', $safePattern), | |
| ]); | |
| $elapsed = microtime(true) - $startTime; | |
| // Safe patterns should complete very quickly | |
| $this->assertLessThan(1.0, $elapsed, 'Safe regex pattern should complete quickly'); | |
| $this->assertGreaterThan(0, count($results), 'Safe pattern should match some documents'); | |
| // Verify safe pattern results are correct | |
| foreach ($results as $doc) { | |
| $text = $doc->getAttribute('text'); | |
| $this->assertStringContainsString('normal', $text, "Document '{$text}' should contain 'normal'"); | |
| } | |
| } finally { | |
| if ($supportsTimeout) { | |
| try { | |
| $database->clearTimeout(); | |
| } catch (\Throwable) { | |
| // ignore cleanup failures to avoid hiding the original assertion failure | |
| } | |
| } | |
| try { | |
| $database->deleteCollection($collectionName); | |
| } catch (\Throwable) { | |
| // ignore cleanup failures to avoid hiding the original assertion failure | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@tests/e2e/Adapter/Scopes/DocumentTests.php` around lines 7259 - 7465, The
test mutates adapter timeout state with $database->setTimeout(...) but only
calls $database->clearTimeout() on the normal path; wrap the portion that sets
the timeout and runs queries inside a try/finally so clearTimeout() always runs:
after computing $supportsTimeout and calling $database->setTimeout(2000) (when
$supportsTimeout is true) ensure you enter a try block that contains the
foreach($redosPatterns...) loop and the safePattern check, and in the finally
block call $database->clearTimeout() (guarded by the same $supportsTimeout
check) and perform collection cleanup; this guarantees cleanup even if
assertions or exceptions occur in testRegexRedos().
| /** @var Database $database */ | ||
| $database = static::getDatabase(); | ||
|
|
||
| /** @var Database $database */ | ||
| $database = static::getDatabase(); | ||
| if ($database->getAdapter()->getSupportForAttributes()) { | ||
| $this->expectNotToPerformAssertions(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Duplicate database variable assignment.
The database variable is assigned twice consecutively. Remove the duplicate.
public function testSchemalessNestedObjectAttributeQueries(): void
{
/** `@var` Database $database */
$database = static::getDatabase();
- /** `@var` Database $database */
- $database = static::getDatabase();
if ($database->getAdapter()->getSupportForAttributes()) {
$this->expectNotToPerformAssertions();
return;
}📝 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.
| /** @var Database $database */ | |
| $database = static::getDatabase(); | |
| /** @var Database $database */ | |
| $database = static::getDatabase(); | |
| if ($database->getAdapter()->getSupportForAttributes()) { | |
| $this->expectNotToPerformAssertions(); | |
| return; | |
| } | |
| /** `@var` Database $database */ | |
| $database = static::getDatabase(); | |
| if ($database->getAdapter()->getSupportForAttributes()) { | |
| $this->expectNotToPerformAssertions(); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@tests/e2e/Adapter/Scopes/SchemalessTests.php` around lines 1690 - 1698, There
is a duplicate assignment to $database using static::getDatabase(); remove the
repeated line so $database is only assigned once before the conditional that
checks $database->getAdapter()->getSupportForAttributes(), preserving the
subsequent if block and the call to $this->expectNotToPerformAssertions()
unchanged.
| $this->assertEquals('Invalid document structure: Attribute "rating" has invalid type. Value must be a valid signed 32-bit integer between -2,147,483,648 and 2,147,483,647', $validator->getDescription()); | ||
| } |
There was a problem hiding this comment.
Potential test portability issue: PHP_INT_MAX is platform-dependent
If 32-bit PHP is (ever) in scope, PHP_INT_MAX may equal 2,147,483,647 and this test would stop exercising the “out of 32-bit range” path.
Proposed tweak (more deterministic max-range input)
- 'rating' => PHP_INT_MAX,
+ 'rating' => 2147483648,Also applies to: 462-463, 477-478, 543-544, 665-666
🤖 Prompt for AI Agents
In `@tests/unit/Validator/StructureTest.php` around lines 284 - 285, The test uses
a hardcoded "2,147,483,647" which makes the out-of-32-bit-range path
platform-dependent; instead, compute the 32-bit bounds in the test (e.g. $max32
= 2147483647; $min32 = -2147483648) and build the expected messages from them,
and for the out-of-range cases use ($max32 + 1) or ($min32 - 1) to ensure
deterministic behavior; update the assertions that reference the message (the
ones comparing $validator->getDescription()) to use these computed values rather
than a hardcoded 2,147,483,647 (also apply the same change to the other affected
assertions mentioned).
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.