Add operators base #713
Add operators base #713
Conversation
WalkthroughAdds first-class Operator support: new Operator model, exception, and validator; integrates operator extraction/validation and refetch logic into Database update/upsert flows; implements operator SQL translation/binding across adapters (SQL/MariaDB/MySQL/Postgres/SQLite); adds CLI benchmark task, wrapper, tests, and a debug script. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
participant DB
rect rgba(230,245,255,0.9)
Note over Client,Database: Update/upsert request containing Operator values
end
Client->>Database: updateDocument(collection, payload)
Database->>Database: Operator::extractOperators(payload)
Database->>Database: OperatorValidator::isValid(operators)
Database->>Adapter: updateDocument(..., regularAttrs, operators)
Adapter->>Adapter: build SQL (regular SET + getOperatorSQL fragments)
Adapter->>Adapter: bindOperatorParams(stmt, operator...)
Adapter->>DB: execute UPDATE/UPSERT with operator fragments & params
DB-->>Adapter: result
Adapter-->>Database: affected rows / ids
alt operators present
Database->>Database: refetchDocuments(updated ids)
end
Database-->>Client: return updated document(s)
sequenceDiagram
participant CLI
participant Task as "bin/tasks/operators.php"
participant Bench
participant Display
participant Cleanup
CLI->>Task: invoke "operators" task
Task->>Bench: setupTestEnvironment(adapter)
Bench-->>Task: env ready
loop per operator/test
Task->>Bench: runAllBenchmarks(iterations)
Bench->>Bench: benchmarkOperator(operator vs traditional)
Bench-->>Task: result
end
Task->>Display: displayResults(results)
Task->>Cleanup: cleanup(env)
Cleanup-->>Task: done
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
# Conflicts: # src/Database/Adapter/Postgres.php # src/Database/Database.php # tests/e2e/Adapter/Scopes/DocumentTests.php
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
5158-5175: Operators leave stale data inupdateDocumentscallbacksWhen
$updatescontains operators, each$docin$batchstill holds theOperatorobjects afteradapter->updateDocuments(...). Becausedecode()now skips operator instances, we never swap them out for the freshly computed scalar values. As a result,onNextconsumers (and any downstream event hooks) see staleOperatorplaceholders instead of the post-update data. To stay consistent with the single-document path—which re-fetches the row after applying operators—we need to reload each document (or otherwise resolve the operator to its new value) before we decode and hand it to callbacks.Apply this diff so operator-based bulk updates return the actual persisted values:
$this->adapter->updateDocuments( $collection, $updates, $batch ); }); foreach ($batch as $index => $doc) { $doc->removeAttribute('$skipPermissionsUpdate'); + if (!empty($operators)) { + $doc = Authorization::skip(fn () => $this->silent( + fn () => $this->getDocument($collection->getId(), $doc->getId()) + )); + } $this->purgeCachedDocument($collection->getId(), $doc->getId()); $doc = $this->decode($collection, $doc); try { $onNext && $onNext($doc, $old[$index]);
🧹 Nitpick comments (2)
src/Database/Adapter/SQL.php (1)
2501-2558: Document or validate consistent operator usage across batch.The upsert logic uses operators from the last document in the batch (line 2509) to build the update clause that applies to all upserted rows. If documents have different operators on different attributes, the behavior will be incorrect. Consider adding validation similar to the sequence check in
createDocuments(line 2359-2361) or documenting this requirement.Example validation to add after line 2510:
// Validate consistent operators across batch if ($firstOperators === null) { $firstOperators = array_keys($operators); } elseif (array_keys($operators) !== $firstOperators) { throw new DatabaseException('All documents in upsert batch must have operators on the same attributes'); }Initialize
$firstOperators = nullbefore the loop at line 2503.src/Database/Validator/Operator.php (1)
145-146: Remove unreachable fallback logic.The ternary fallback
$operatorValue != 0 ? ... : $currentValueat lines 145-146 is unreachable because division and modulo by zero are already prevented at line 125.Apply this diff to simplify:
- DatabaseOperator::TYPE_DIVIDE => $operatorValue != 0 ? $currentValue / $operatorValue : $currentValue, - DatabaseOperator::TYPE_MODULO => $operatorValue != 0 ? $currentValue % $operatorValue : $currentValue, + DatabaseOperator::TYPE_DIVIDE => $currentValue / $operatorValue, + DatabaseOperator::TYPE_MODULO => $currentValue % $operatorValue,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Database/Adapter/SQL.php(9 hunks)src/Database/Database.php(19 hunks)src/Database/Validator/Operator.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Database/Adapter/SQL.php (6)
src/Database/Operator.php (6)
Operator(14-698)isOperator(664-667)getMethod(139-142)getValues(155-158)replace(498-501)extractOperators(675-696)src/Database/Adapter.php (4)
filter(1196-1205)quote(467-467)Adapter(16-1318)execute(1293-1293)src/Database/Adapter/MySQL.php (2)
getOperatorSQL(278-306)processException(148-165)src/Database/Adapter/SQLite.php (3)
getOperatorSQL(1437-1721)bindOperatorParams(1383-1416)processException(1273-1296)src/Database/Adapter/MariaDB.php (5)
getOperatorSQL(1901-2059)quote(1887-1890)getPDOType(1699-1707)processException(1819-1885)getUpsertStatement(1206-1279)src/Database/Adapter/Postgres.php (8)
getOperatorSQL(2295-2497)quote(1974-1977)bindOperatorParams(2508-2543)Postgres(21-2544)getPDOType(1797-1806)execute(86-106)processException(1925-1968)getUpsertStatement(1299-1353)
src/Database/Validator/Operator.php (2)
src/Database/Database.php (2)
Database(38-8236)__construct(406-567)src/Database/Operator.php (7)
Operator(14-698)__construct(120-125)getAttribute(147-150)getMethod(139-142)isMethod(226-251)getValues(155-158)getValue(164-167)
src/Database/Database.php (7)
src/Database/Validator/Operator.php (3)
Operator(10-363)isValid(60-85)getDescription(46-49)src/Database/Operator.php (4)
Operator(14-698)extractOperators(675-696)setAttribute(188-193)getAttribute(147-150)src/Database/Document.php (5)
getArrayCopy(423-458)setAttribute(244-261)getId(63-66)Document(12-470)getAttribute(224-231)src/Database/Adapter/SQLite.php (1)
updateDocument(646-881)src/Database/Adapter/MariaDB.php (2)
updateDocument(950-1193)getMinDateTime(1730-1733)src/Database/Adapter/Postgres.php (2)
updateDocument(1066-1287)getMinDateTime(1867-1870)src/Database/Adapter.php (4)
updateDocument(725-725)getDocument(691-691)getIdAttributeType(890-890)getMinDateTime(883-883)
🪛 PHPMD (2.15.0)
src/Database/Database.php
5113-5113: Avoid unused local variables such as '$attribute'. (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). (9)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
🔇 Additional comments (8)
src/Database/Adapter/SQL.php (5)
16-16: LGTM!The import of the Operator class is necessary for the operator functionality added in this PR.
480-515: Operator extraction and SQL generation logic is sound.The two-pass approach (extract operators, then build SQL) maintains operators in the
$attributesarray, which allows the comma logic to work correctly. The defensivertrimat line 511 ensures no trailing comma, though it should be unnecessary if the logic is correct.
546-560: Spatial binding fix properly implemented.The code now correctly uses
in_arrayinstead ofissetfor spatial attribute detection and converts arrays to WKT format before binding, addressing the past review comment. The PostgreSQL boolean preservation logic is also handled correctly.
1602-1613: LGTM!The signature extension to include the
$operatorsparameter with a default empty array maintains backward compatibility while enabling operator support in upsert operations.
1930-2079: Binding logic correctly matches SQL generation.Parameter binding for all operator types is consistent with the SQL fragments generated in
getOperatorSQL. PDO types are appropriate for each value type.src/Database/Validator/Operator.php (3)
264-290: LGTM! Concat validation correctly addressed.The concat operator validation now properly restricts usage to non-array string fields and validates string values, addressing the previous review feedback.
291-335: LGTM! String, boolean, and date operator validations are correct.The validation logic for replace, toggle, and date operators properly checks attribute types, value requirements, and provides clear error messages.
347-362: LGTM! Type metadata methods are correct.The
isArray()andgetType()methods correctly indicate this validator accepts a single Operator object.
| case DatabaseOperator::TYPE_ARRAY_FILTER: | ||
| if (!$isArray) { | ||
| $this->message = "Cannot use {$method} operator on non-array attribute '{$operator->getAttribute()}'"; | ||
| return false; | ||
| } | ||
|
|
||
| if (\count($values) < 1 || \count($values) > 2) { | ||
| $this->message = "Array filter operator requires 1 or 2 values: condition and optional comparison value"; | ||
| return false; | ||
| } | ||
|
|
||
| if (!\is_string($values[0])) { | ||
| $this->message = "Array filter condition must be a string"; | ||
| return false; | ||
| } | ||
|
|
||
| break; |
There was a problem hiding this comment.
Consider validating filter condition operators.
While the code checks that the filter condition is a string, it doesn't verify it's a valid comparison operator (e.g., '=', '!=', '>', '<', 'contains'). Invalid conditions would fail during SQL translation.
If the set of valid conditions is well-defined, add validation to catch errors earlier.
🤖 Prompt for AI Agents
In src/Database/Validator/Operator.php around lines 247 to 263, the code ensures
the array filter condition is a string but doesn't verify it's a permitted
filter operator; add a whitelist check after the is_string check that validates
$values[0] against the allowed operators (e.g. '=', '!=', '>', '<', '>=', '<=',
'contains', 'not_contains', 'in', 'not_in' — adjust per your domain), and if it
is not in the list set $this->message to something like "Invalid array filter
condition '{$values[0]}'" and return false so invalid conditions fail early.
There was a problem hiding this comment.
It actually needs to validate that it's a valid operation string like equal, notEqual etc. Pretty sure that's already handled elsewhere, but the logic needs moved here.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for filter operator validation or constants
rg -nP --type=php -C5 'equal|notEqual|lessThan|greaterThan' src/Database/Validator/Query/ src/Database/Query/
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Search for Query method validation or operator types
rg -nP --type=php -C3 '\bMETHOD_\w+|getMethod\(\)|isMethod\(' src/Database/Query/
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Look for any existing filter/query operator validation
ast-grep --pattern 'class Query {
$$$
}'
Length of output: 63642
🏁 Script executed:
#!/bin/bash
# Search for array filter implementation in adapters to see which conditions are supported
rg -nP --type=php -C10 'TYPE_ARRAY_FILTER|ARRAY_FILTER' src/Database/Adapter/
Length of output: 12557
🏁 Script executed:
#!/bin/bash
# Look for any validation of filter conditions in the codebase
rg -nP --type=php -C5 'filter.*condition|filterCondition' src/Database/
Length of output: 1853
@abnegate, you're absolutely right! I found the validation logic in src/Database/Adapter/SQL.php (lines 2117-2123). It validates against a whitelist of permitted filter conditions but it's currently happening at the adapter layer. This should be moved to the validator for earlier detection.
Here's the code to add the validation in the TYPE_ARRAY_FILTER case:
case DatabaseOperator::TYPE_ARRAY_FILTER:
if (!$isArray) {
$this->message = "Cannot use {$method} operator on non-array attribute '{$operator->getAttribute()}'";
return false;
}
if (\count($values) < 1 || \count($values) > 2) {
$this->message = "Array filter operator requires 1 or 2 values: condition and optional comparison value";
return false;
}
if (!\is_string($values[0])) {
$this->message = "Array filter condition must be a string";
return false;
}
// Validate filter condition against whitelist
$validConditions = [
'equal', 'notEqual', 'equals', 'notEquals', // Comparison
'greaterThan', 'greaterThanOrEqual', 'lessThan', 'lessThanOrEqual', // Numeric
'isNull', 'isNotNull', 'null', 'notNull' // Null checks
];
if (!\in_array($values[0], $validConditions, true)) {
$this->message = "Invalid array filter condition '{$values[0]}'. Must be one of: " . \implode(', ', $validConditions);
return false;
}
break;This will catch invalid filter conditions early during validation rather than waiting until SQL generation in the adapter.
# Conflicts: # src/Database/Database.php
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Database.php (1)
5242-5317: Operators remain in batch documents after updateWe re-add operator objects to each
$batchdocument before calling$this->adapter->updateDocuments(), but we never replace them with the computed scalar values afterwards. As a result the documents we keep using still holdOperatorinstances:Query::cursorAfter($last)can no longer derive the ordered attributes (it now sees anOperatorobject instead of a scalar), and the$onNextcallback is given documents whose updated fields are still operators rather than the final values. This breaks pagination for multi-batch updates and makes the callback output wrong whenever operators are involved. Please refresh each document (similar to the single-document path) or otherwise materialize the operator results before the documents are reused or returned.src/Database/Adapter/Postgres.php (1)
1326-1380: Upsert ignores Operator updates and leaves $operators unused. Implement operator-aware upserts and bind in attribute order.Currently, operator values are lost in ON CONFLICT updates and PHPMD flags $operators as unused. Integrate getOperatorSQL and bindOperatorParams, starting bindIndex at count($bindValues) to align placeholders.
- protected function getUpsertStatement( + protected function getUpsertStatement( string $tableName, string $columns, array $batchKeys, array $attributes, array $bindValues, string $attribute = '', - array $operators = [], + array $operators = [], ): mixed { $getUpdateClause = function (string $attribute, bool $increment = false): string { $attribute = $this->quote($this->filter($attribute)); @@ - if (!empty($attribute)) { + if (!empty($attribute)) { // Increment specific column by its new value in place $updateColumns = [ $getUpdateClause($attribute, increment: true), $getUpdateClause('_updatedAt'), ]; } else { - // Update all columns - $updateColumns = []; - foreach (array_keys($attributes) as $attr) { - /** - * @var string $attr - */ - $updateColumns[] = $getUpdateClause($this->filter($attr)); - } + // Update all columns, handling operators + $updateColumns = []; + $bindIndex = \count($bindValues); + foreach (\array_keys($attributes) as $attr) { + $filteredAttr = $this->filter($attr); + if (isset($operators[$attr])) { + $opSql = $this->getOperatorSQL($filteredAttr, $operators[$attr], $bindIndex); + if ($opSql !== null) { + $updateColumns[] = $opSql; + } + } else { + $updateColumns[] = $getUpdateClause($filteredAttr); + } + } } @@ - foreach ($bindValues as $key => $binding) { + foreach ($bindValues as $key => $binding) { $stmt->bindValue($key, $binding, $this->getPDOType($binding)); } + // Bind operator parameters in attribute order to preserve bindIndex alignment + if (!empty($operators)) { + $opBindIndex = \count($bindValues); + foreach (\array_keys($attributes) as $attr) { + if (isset($operators[$attr])) { + $this->bindOperatorParams($stmt, $operators[$attr], $opBindIndex); + } + } + } return $stmt; }
♻️ Duplicate comments (6)
src/Database/Adapter/SQL.php (3)
1900-1903: Fix NULL handling in ARRAY_REMOVE when value is not found.When
JSON_SEARCHdoesn't find a match, it returns NULL. The current implementation chains this NULL directly toJSON_REMOVE, which returns NULL if the path is NULL. This means attempting to remove a non-existent value from the array will set the entire column to NULL instead of leaving it unchanged.Apply this diff to preserve the original column value:
case Operator::TYPE_ARRAY_REMOVE: $bindKey = "op_{$bindIndex}"; $bindIndex++; - return "{$quotedColumn} = JSON_REMOVE({$quotedColumn}, JSON_UNQUOTE(JSON_SEARCH({$quotedColumn}, 'one', :$bindKey)))"; + return "{$quotedColumn} = IFNULL(JSON_REMOVE({$quotedColumn}, JSON_UNQUOTE(JSON_SEARCH({$quotedColumn}, 'one', :$bindKey))), {$quotedColumn})";
1939-1956: Fix ARRAY_FILTER condition mismatch between SQL and binding.Line 1948 checks for
'equal'(without 's'), butbindOperatorParamsat line 2097 defaults the condition to'equals'(with 's'). This mismatch causes the ELSE branch (line 1954) to return TRUE, effectively disabling filtering when no explicit condition is provided.The fix should be applied in
bindOperatorParamsat line 2097 (see next comment for the binding method).
2096-2109: Fix ARRAY_FILTER default condition to match SQL case statement.Line 2097 defaults the condition to
'equals', but the SQL generated ingetOperatorSQLat line 1948 expects'equal'. Change the default here to match.Apply this diff:
case Operator::TYPE_ARRAY_FILTER: - $condition = $values[0] ?? 'equals'; + $condition = $values[0] ?? 'equal'; $value = $values[1] ?? null;src/Database/Validator/Operator.php (2)
124-128: Use strict zero check to avoid type-coercion bugs.Prevent "0-like" strings from bypassing divide/modulo-by-zero guard.
- if (($method === DatabaseOperator::TYPE_DIVIDE || $method === DatabaseOperator::TYPE_MODULO) && $values[0] == 0) { + if (($method === DatabaseOperator::TYPE_DIVIDE || $method === DatabaseOperator::TYPE_MODULO) && $values[0] === 0) {
238-246: Validate intersect/diff operator values are non-empty arrays.Prevents runtime errors in adapters when values are missing or not arrays.
case DatabaseOperator::TYPE_ARRAY_INTERSECT: case DatabaseOperator::TYPE_ARRAY_DIFF: if (!$isArray) { $this->message = "Cannot use {$method} operator on non-array attribute '{$operator->getAttribute()}'"; return false; } + if (empty($values) || !\is_array($values[0])) { + $this->message = "{$method} operator requires a non-empty array value"; + return false; + } + break;src/Database/Adapter/MariaDB.php (1)
1268-1277: Bind operators in attribute order to keep bindIndex alignment deterministic.Iterating $operators can produce a different order than attribute iteration used to build SQL, causing op_n/key_n mismatches. Bind in the same attribute order.
- // Bind operator parameters - foreach ($operators as $attr => $operator) { - $this->bindOperatorParams($stmt, $operator, $bindIndex); - } + // Bind operator parameters in the same order used to build SQL + foreach (array_keys($attributes) as $attr) { + if (isset($operators[$attr])) { + $this->bindOperatorParams($stmt, $operators[$attr], $bindIndex); + } + }
🧹 Nitpick comments (3)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
9-9: Verify if this import is needed.The
DateTimeimport fromUtopia\Databasedoesn't appear to be used in this test file. The code uses native PHP's\DateTimeclass instead. If this import isn't needed, consider removing it to avoid confusion.If this import is unused, apply this diff:
-use Utopia\Database\DateTime;src/Database/Validator/Operator.php (2)
247-263: Whitelist arrayFilter conditions to fail fast on invalid operators.Optional but recommended: validate $values[0] against allowed conditions to avoid SQL errors later.
- if (!\is_string($values[0])) { + if (!\is_string($values[0])) { $this->message = "Cannot apply {$method} operator: condition must be a string"; return false; } + $allowed = ['equals','notEquals','greaterThan','lessThan','null','notNull']; + if (!\in_array($values[0], $allowed, true)) { + $this->message = "Cannot apply {$method} operator: invalid condition '{$values[0]}'"; + return false; + }
136-159: Power overflow may occur before bounds check.Computing $currentValue ** $operatorValue can overflow/convert to float, weakening the check. Consider pre-validating exponent/base or using safe bounds math.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Database/Adapter/MariaDB.php(9 hunks)src/Database/Adapter/MySQL.php(2 hunks)src/Database/Adapter/Postgres.php(7 hunks)src/Database/Adapter/SQL.php(9 hunks)src/Database/Database.php(15 hunks)src/Database/Validator/Operator.php(1 hunks)src/Database/Validator/Query/Filter.php(1 hunks)tests/e2e/Adapter/Base.php(2 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/Adapter/Base.php
- src/Database/Validator/Query/Filter.php
- src/Database/Adapter/MySQL.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.php
📚 Learning: 2025-10-16T09:37:33.512Z
Learnt from: fogelito
PR: utopia-php/database#733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.512Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
src/Database/Adapter/MariaDB.php
🧬 Code graph analysis (6)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
src/Database/DateTime.php (1)
DateTime(7-86)
src/Database/Validator/Operator.php (2)
src/Database/Database.php (2)
Database(38-8368)__construct(419-617)src/Database/Operator.php (7)
Operator(14-698)__construct(120-125)getAttribute(147-150)getMethod(139-142)isMethod(226-251)getValues(155-158)getValue(164-167)
src/Database/Adapter/MariaDB.php (2)
src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)increment(409-416)getMethod(139-142)getValues(155-158)src/Database/Adapter/SQL.php (3)
getOperatorSQL(1790-1960)bindOperatorParams(1970-2111)getPDOType(2129-2129)
src/Database/Database.php (3)
src/Database/Validator/Operator.php (3)
Operator(10-363)isValid(60-85)getDescription(46-49)src/Database/Operator.php (4)
Operator(14-698)extractOperators(675-696)setAttribute(188-193)getAttribute(147-150)src/Database/Adapter/Postgres.php (1)
updateDocument(1093-1314)
src/Database/Adapter/SQL.php (4)
src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)getMethod(139-142)getValues(155-158)extractOperators(675-696)src/Database/Adapter/Postgres.php (6)
getOperatorSQL(2368-2570)quote(2047-2050)bindOperatorParams(2581-2616)Postgres(21-2617)getPDOType(1860-1869)getUpsertStatement(1326-1380)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)src/Database/Adapter/MariaDB.php (4)
getOperatorSQL(1901-2059)quote(1887-1890)getPDOType(1699-1707)getUpsertStatement(1206-1279)
src/Database/Adapter/Postgres.php (4)
src/Database/Operator.php (4)
Operator(14-698)isOperator(664-667)getMethod(139-142)getValues(155-158)src/Database/Adapter/SQL.php (3)
getOperatorSQL(1790-1960)bindOperatorParams(1970-2111)getPDOType(2129-2129)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)src/Database/Adapter/MariaDB.php (3)
getOperatorSQL(1901-2059)getPDOType(1699-1707)quote(1887-1890)
🪛 GitHub Actions: CodeQL
src/Database/Validator/Operator.php
[error] 150-150: PHPStan: Access to undefined constant Utopia\Database\Database::INT_MAX. Command: ./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G
[error] 155-155: PHPStan: Access to undefined constant Utopia\Database\Database::INT_MIN. Command: ./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G
[error] 172-172: PHPStan: Access to undefined constant Utopia\Database\Database::INT_MAX. Command: ./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G
[error] 173-173: PHPStan: Access to undefined constant Utopia\Database\Database::INT_MIN. Command: ./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G
[error] 206-206: PHPStan: Access to undefined constant Utopia\Database\Database::INT_MAX. Command: ./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G
[error] 207-207: PHPStan: Access to undefined constant Utopia\Database\Database::INT_MIN. Command: ./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G
🪛 PHPMD (2.15.0)
src/Database/Database.php
5246-5246: Avoid unused local variables such as '$attribute'. (undefined)
(UnusedLocalVariable)
src/Database/Adapter/Postgres.php
1333-1333: Avoid unused parameters such as '$operators'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
🔇 Additional comments (6)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
383-410: LGTM! Good validation test for empty operator values.The test correctly validates that query operators reject empty value arrays with appropriate error messages. This aligns well with the operator validation framework introduced in this PR.
src/Database/Adapter/SQL.php (5)
16-16: LGTM! Operator import and signature extension.The import of the
Operatorclass and the extension ofgetUpsertStatementwith an$operatorsparameter are correct and align with the operator support added in this PR.Also applies to: 1617-1628
482-515: LGTM! Operator separation and SQL generation in updateDocuments.The logic correctly separates operators from regular attributes, generates appropriate SQL for each, and handles the trailing comma. The early return for empty columns prevents malformed SQL.
540-562: LGTM! Correct spatial and operator binding.The binding logic correctly:
- Routes operators through
bindOperatorParams- Uses
in_array()for spatial attribute detection (notisset())- Converts spatial arrays to WKT via
convertArrayToWKT()- Preserves PostgreSQL boolean values
This properly addresses the spatial binding issue from previous reviews.
2533-2590: LGTM! Operator extraction in upsertDocuments.The operator extraction using
Operator::extractOperators()correctly separates operators from regular attributes, builds the regular attributes with internal keys, and passes both togetUpsertStatement. This aligns with the operator support pattern across adapters.
1831-1845: Verify divide-by-zero handling for DIVIDE and MODULO operators.Lines 1831-1840 (DIVIDE) and 1842-1845 (MODULO) don't guard against division by zero. Ensure that validation occurs at the
Operatorclass level or in the calling code to prevent SQL errors when the divisor/modulo value is zero.If validation isn't in place, consider adding a check or wrapping with NULLIF:
-- For DIVIDE {$quotedColumn} = {$quotedColumn} / NULLIF(:$bindKey, 0) -- For MODULO {$quotedColumn} = MOD({$quotedColumn}, NULLIF(:$bindKey, 0))
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
5383-5410: Remove unused parameter or clarify dependency purpose.The
$documentparameter is never used in the test method body, yet the@depends testCreateDocumentannotation was added. This suggests incomplete refactoring.Options:
- If the dependency is unnecessary: Remove both the
@dependsannotation and the$documentparameter, reverting to the original signaturepublic function testEmptyOperatorValues(): void- If the dependency is needed for test ordering: Document why the dependency exists (e.g., to ensure the 'documents' collection is set up), but the parameter should still be removed since it's unused
The current implementation creates confusion about the test's intent.
+ /** + * @depends testCreateDocument + */ - public function testEmptyOperatorValues(Document $document): void + public function testEmptyOperatorValues(): void {Based on learnings
src/Database/Adapter/MariaDB.php (1)
880-891: Fix spatial value binding in createDocument (convert arrays to WKT).Placeholders for spatial cols use ST_GeomFromText(:key_X) but you JSON-encode arrays; must pass WKT. Also drops a stray $attribute filter.
- $attributeIndex = 0; - foreach ($attributes as $value) { - if (\is_array($value)) { - $value = \json_encode($value); - } - $bindKey = 'key_' . $attributeIndex; - $attribute = $this->filter($attribute); - $value = (\is_bool($value)) ? (int)$value : $value; - $stmt->bindValue(':' . $bindKey, $value, $this->getPDOType($value)); - $attributeIndex++; - } + $attributeIndex = 0; + foreach ($attributes as $attrName => $value) { + if (\in_array($attrName, $spatialAttributes, true)) { + if (\is_array($value)) { + $value = $this->convertArrayToWKT($value); + } + } elseif (\is_array($value)) { + $value = \json_encode($value); + } + $bindKey = 'key_' . $attributeIndex; + $value = (\is_bool($value)) ? (int)$value : $value; + $stmt->bindValue(':' . $bindKey, $value, $this->getPDOType($value)); + $attributeIndex++; + }src/Database/Adapter/SQLite.php (1)
968-971: Enable upserts: support flag is false but upsert is implemented.getUpsertStatement is provided, yet getSupportForUpserts() returns false. Flip to true to activate native ON CONFLICT path.
- public function getSupportForUpserts(): bool - { - return false; - } + public function getSupportForUpserts(): bool + { + return true; + }
♻️ Duplicate comments (12)
src/Database/Adapter/MariaDB.php (2)
1847-1851: Map duplicate key errors: only _uid ⇒ DuplicateException; others ⇒ UniqueException.Differentiate user doc ID collisions from other unique/PRIMARY violations.
- // Duplicate row - if ($e->getCode() === '23000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1062) { - return new DuplicateException('Document already exists', $e->getCode(), $e); - } + // Duplicate row: only _uid collisions are DuplicateException; others are UniqueException + if ($e->getCode() === '23000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1062) { + $msg = $e->errorInfo[2] ?? ''; + if (\is_string($msg) && \str_contains($msg, "_uid")) { + return new DuplicateException('Document already exists', $e->getCode(), $e); + } + return new UniqueException('Unique constraint violated', $e->getCode(), $e); + }Based on learnings
10-12: Import UniqueException for granular duplicate mapping.Add the import to support differentiating _uid vs other unique violations.
use Utopia\Database\Exception\Duplicate as DuplicateException; +use Utopia\Database\Exception\Unique as UniqueException; use Utopia\Database\Exception\Limit as LimitException;Based on learnings
src/Database/Adapter/SQL.php (3)
2111-2120: ARRAY_FILTER default condition mismatch disables filtering when not provided.Default is 'equals' but CASE checks 'equal'. Use 'equal' to match.
- $condition = $values[0] ?? 'equals'; + $condition = $values[0] ?? 'equal';
1905-1909: ARRAY_REMOVE sets column to NULL when value not found. Wrap with IFNULL.If JSON_SEARCH returns NULL, JSON_REMOVE(NULL) yields NULL; preserve original column.
- return "{$quotedColumn} = JSON_REMOVE({$quotedColumn}, JSON_UNQUOTE(JSON_SEARCH({$quotedColumn}, 'one', :$bindKey)))"; + return "{$quotedColumn} = IFNULL(JSON_REMOVE({$quotedColumn}, JSON_UNQUOTE(JSON_SEARCH({$quotedColumn}, 'one', :$bindKey))), {$quotedColumn})";
1910-1943: Preserve empty arrays for ARRAY_UNIQUE/INTERSECT/DIFF (avoid NULL).JSON_ARRAYAGG returns NULL on empty result; wrap with IFNULL(..., '[]').
case Operator::TYPE_ARRAY_UNIQUE: - return "{$quotedColumn} = (SELECT JSON_ARRAYAGG(DISTINCT value) FROM JSON_TABLE({$quotedColumn}, '$[*]' COLUMNS(value JSON PATH '$')) AS jt)"; + return "{$quotedColumn} = IFNULL((SELECT JSON_ARRAYAGG(DISTINCT value) FROM JSON_TABLE({$quotedColumn}, '$[*]' COLUMNS(value JSON PATH '$')) AS jt), '[]')"; case Operator::TYPE_ARRAY_INTERSECT: - return "{$quotedColumn} = ( + return "{$quotedColumn} = IFNULL(( SELECT JSON_ARRAYAGG(jt1.value) FROM JSON_TABLE({$quotedColumn}, '$[*]' COLUMNS(value JSON PATH '$')) AS jt1 WHERE jt1.value IN ( SELECT jt2.value FROM JSON_TABLE(:$bindKey, '$[*]' COLUMNS(value JSON PATH '$')) AS jt2 ) - )"; + ), '[]')"; case Operator::TYPE_ARRAY_DIFF: - return "{$quotedColumn} = ( + return "{$quotedColumn} = IFNULL(( SELECT JSON_ARRAYAGG(jt1.value) FROM JSON_TABLE({$quotedColumn}, '$[*]' COLUMNS(value JSON PATH '$')) AS jt1 WHERE jt1.value NOT IN ( SELECT jt2.value FROM JSON_TABLE(:$bindKey, '$[*]' COLUMNS(value JSON PATH '$')) AS jt2 ) - )"; + ), '[]')";src/Database/Validator/Operator.php (4)
247-263: Consider validating filter condition against a whitelist.While the code checks that the filter condition is a string, it doesn't verify it's a valid comparison operator. Invalid conditions would fail during SQL translation rather than at validation time.
Based on adapter implementations, valid conditions are:
'equals','notEquals','greaterThan','lessThan','null','notNull'. Consider adding this check:if (!\is_string($values[0])) { $this->message = "Cannot apply {$method} operator: condition must be a string"; return false; } + + $validConditions = ['equals', 'notEquals', 'greaterThan', 'lessThan', 'null', 'notNull']; + if (!\in_array($values[0], $validConditions, true)) { + $this->message = "Cannot apply {$method} operator: invalid condition '{$values[0]}'. Must be one of: " . implode(', ', $validConditions); + return false; + } break;
17-17: Initialize typed property to prevent potential errors.While the constructor initializes
$attributes, PHP typed properties should be initialized at declaration to prevent uninitialized property errors if accessed before the constructor completes.Apply this diff:
- protected array $attributes; + protected array $attributes = [];
125-125: Use strict equality to prevent type coercion bugs.The loose equality
== 0can incorrectly match non-zero values due to PHP's type coercion. For example,"0abc" == 0evaluates totrue.Apply this diff:
- if (($method === DatabaseOperator::TYPE_DIVIDE || $method === DatabaseOperator::TYPE_MODULO) && $values[0] == 0) { + if (($method === DatabaseOperator::TYPE_DIVIDE || $method === DatabaseOperator::TYPE_MODULO) && $values[0] === 0) {
238-246: Validate operator values for intersect/diff operations.The intersect and diff operators require array values to operate on, but currently only the target attribute's array type is checked. Missing validation that the operator value is a non-empty array could cause runtime errors.
Apply this diff:
case DatabaseOperator::TYPE_ARRAY_INTERSECT: case DatabaseOperator::TYPE_ARRAY_DIFF: if (!$isArray) { $this->message = "Cannot apply {$method} operator to non-array field '{$operator->getAttribute()}'"; return false; } + if (empty($values) || !\is_array($values[0])) { + $this->message = "{$method} operator requires a non-empty array value"; + return false; + } + break;src/Database/Adapter/Postgres.php (1)
2561-2562: Fix invalid PostgreSQL placeholder casting in arrayFilter numeric comparisons.The syntax
(:$valueKey::text)::numericis invalid PostgreSQL. Placeholders cannot be cast inline with::typesyntax. Additionally, JSON-encoded numeric values include quotes that must be stripped before numeric casting.Apply this diff to fix the casting:
- WHEN 'greaterThan' THEN (value::text)::numeric > (:$valueKey::text)::numeric - WHEN 'lessThan' THEN (value::text)::numeric < (:$valueKey::text)::numeric + WHEN 'greaterThan' THEN (value::text)::numeric > CAST(TRIM(BOTH '\"' FROM (:$valueKey)::text) AS numeric) + WHEN 'lessThan' THEN (value::text)::numeric < CAST(TRIM(BOTH '\"' FROM (:$valueKey)::text) AS numeric)src/Database/Adapter/SQLite.php (2)
623-625: Good: centralized exception handling (createDocument).Replaced local PDOException mapping with processException(); aligns with adapter-wide policy and prior review.
870-871: Good: centralized exception handling (updateDocument).Delegates to processException() for consistent error classification.
🧹 Nitpick comments (8)
src/Database/Adapter/SQL.php (1)
1949-1961: Support ARRAY_FILTER synonyms ('equals', 'notEquals', 'null', 'notNull') in CASE.Binder whitelists these; add CASE branches to avoid ELSE TRUE passthrough.
WHERE CASE :$conditionKey - WHEN 'equal' THEN value = :$valueKey - WHEN 'notEqual' THEN value != :$valueKey + WHEN 'equal' THEN value = :$valueKey + WHEN 'equals' THEN value = :$valueKey + WHEN 'notEqual' THEN value != :$valueKey + WHEN 'notEquals' THEN value != :$valueKey WHEN 'greaterThan' THEN CAST(value AS SIGNED) > CAST(:$valueKey AS SIGNED) WHEN 'lessThan' THEN CAST(value AS SIGNED) < CAST(:$valueKey AS SIGNED) - WHEN 'isNull' THEN value IS NULL - WHEN 'isNotNull' THEN value IS NOT NULL + WHEN 'isNull' THEN value IS NULL + WHEN 'null' THEN value IS NULL + WHEN 'isNotNull' THEN value IS NOT NULL + WHEN 'notNull' THEN value IS NOT NULL ELSE TRUE ENDtests/unit/OperatorTest.php (1)
151-161: Minor: remove “Deprecated” mentions for concat to avoid confusion.No deprecation is indicated in Operator; consider dropping the comments.
src/Database/Adapter/SQLite.php (3)
795-823: Bind index coordination looks correct; minor clarity tweak optional.Shared index space between key_{i} and op_{i} is consistent across build/bind loops. For readability, consider renaming $bindIndex/$bindIndexForBinding to a shared name (e.g., $idxBuild/$idxBind) and add a short comment noting the shared counter across operators and non-operators.
Also applies to: 842-859
1568-1572: Avoid DISTINCT inside json_group_array for portability.DISTINCT within json_group_array is not supported on some SQLite builds. Use a subquery DISTINCT instead.
- return "{$quotedColumn} = ( - SELECT json_group_array(DISTINCT value) - FROM json_each(IFNULL({$quotedColumn}, '[]')) - )"; + return "{$quotedColumn} = ( + SELECT json_group_array(value) + FROM ( + SELECT DISTINCT value + FROM json_each(IFNULL({$quotedColumn}, '[]')) + ) + )";
1596-1616: Window function dependency (ROW_NUMBER) may break on older SQLite.ARRAY_INSERT uses ROW_NUMBER() OVER (), which requires SQLite ≥ 3.25. Consider a fallback path (or feature check via PRAGMA compile_options or sqlite_version()) to avoid runtime errors on older environments.
bin/tasks/operators.php (3)
38-41: Include mysql in adapter docs and error message.Docs say “mariadb, postgres, sqlite” but mysql is supported in config. Update for consistency.
- ->param('adapter', '', new Text(0), 'Database adapter (mariadb, postgres, sqlite)') + ->param('adapter', '', new Text(0), 'Database adapter (mariadb, mysql, postgres, sqlite)') @@ - if (!isset($dbAdapters[$adapter])) { - Console::error("Adapter '{$adapter}' not supported. Available: mariadb, postgres, sqlite"); + if (!isset($dbAdapters[$adapter])) { + Console::error("Adapter '{$adapter}' not supported. Available: mariadb, mysql, postgres, sqlite"); return; }Also applies to: 95-98
568-574: Remove unused parameter $isBulk (PHPMD) and simplify call sites.$isBulk is unused; drop it to silence static analysis and reduce noise.
-function benchmarkOperation( +function benchmarkOperation( Database $database, int $iterations, string $operation, - bool $isBulk, bool $useOperators ): array { @@ - $safeBenchmark('UPDATE_SINGLE_NO_OPS', fn () => benchmarkOperation( + $safeBenchmark('UPDATE_SINGLE_NO_OPS', fn () => benchmarkOperation( $database, $iterations, 'updateDocument', - false, false )); @@ - $safeBenchmark('UPDATE_SINGLE_WITH_OPS', fn () => benchmarkOperation( + $safeBenchmark('UPDATE_SINGLE_WITH_OPS', fn () => benchmarkOperation( $database, $iterations, 'updateDocument', - false, true )); @@ - $safeBenchmark('UPDATE_BULK_NO_OPS', fn () => benchmarkOperation( + $safeBenchmark('UPDATE_BULK_NO_OPS', fn () => benchmarkOperation( $database, $iterations, 'updateDocuments', - false, false )); @@ - $safeBenchmark('UPDATE_BULK_WITH_OPS', fn () => benchmarkOperation( + $safeBenchmark('UPDATE_BULK_WITH_OPS', fn () => benchmarkOperation( $database, $iterations, 'updateDocuments', - false, true )); @@ - $safeBenchmark('UPSERT_SINGLE_NO_OPS', fn () => benchmarkOperation( + $safeBenchmark('UPSERT_SINGLE_NO_OPS', fn () => benchmarkOperation( $database, $iterations, 'upsertDocument', - false, false )); @@ - $safeBenchmark('UPSERT_SINGLE_WITH_OPS', fn () => benchmarkOperation( + $safeBenchmark('UPSERT_SINGLE_WITH_OPS', fn () => benchmarkOperation( $database, $iterations, 'upsertDocument', - false, true )); @@ - $safeBenchmark('UPSERT_BULK_NO_OPS', fn () => benchmarkOperation( + $safeBenchmark('UPSERT_BULK_NO_OPS', fn () => benchmarkOperation( $database, $iterations, 'upsertDocuments', - false, false )); @@ - $safeBenchmark('UPSERT_BULK_WITH_OPS', fn () => benchmarkOperation( + $safeBenchmark('UPSERT_BULK_WITH_OPS', fn () => benchmarkOperation( $database, $iterations, 'upsertDocuments', - false, true ));Also applies to: 206-236, 238-268, 254-268, 262-268, 230-237, 222-237
881-899: Compute fastest/slowest using operator-only results to avoid undefined keys.Some entries (e.g., UPDATE_* keys) lack 'operator'. Filter first to prevent notices and wrong picks.
- $fastest = array_reduce( - $results, + $operatorResults = array_values(array_filter($results, fn ($r) => isset($r['operator'], $r['speedup']))); + $fastest = array_reduce( + $operatorResults, fn ($carry, $item) => $carry === null || $item['speedup'] > $carry['speedup'] ? $item : $carry ); - $slowest = array_reduce( - $results, + $slowest = array_reduce( + $operatorResults, fn ($carry, $item) => $carry === null || $item['speedup'] < $carry['speedup'] ? $item : $carry );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bin/operators(1 hunks)bin/tasks/operators.php(1 hunks)src/Database/Adapter/MariaDB.php(9 hunks)src/Database/Adapter/Postgres.php(9 hunks)src/Database/Adapter/SQL.php(8 hunks)src/Database/Adapter/SQLite.php(9 hunks)src/Database/Database.php(20 hunks)src/Database/Validator/Operator.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(2 hunks)tests/unit/OperatorTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/operators
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
PR: utopia-php/database#733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
src/Database/Adapter/MariaDB.phpsrc/Database/Adapter/SQLite.php
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.php
🧬 Code graph analysis (9)
src/Database/Adapter/Postgres.php (2)
src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)increment(409-416)getMethod(139-142)getValues(155-158)src/Database/Adapter/SQL.php (4)
getOperatorSQL(1796-1965)bindOperatorParams(1975-2138)getPDOType(2226-2226)count(3222-3284)
src/Database/Adapter/MariaDB.php (6)
src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)increment(409-416)getMethod(139-142)getValues(155-158)src/Database/Adapter.php (3)
filter(1203-1212)count(825-825)quote(467-467)src/Database/Adapter/Postgres.php (4)
getOperatorSQL(2393-2596)bindOperatorParams(2607-2642)getPDOType(1885-1894)quote(2072-2075)src/Database/Adapter/SQL.php (5)
getOperatorSQL(1796-1965)getSpatialGeomFromText(1592-1604)bindOperatorParams(1975-2138)getPDOType(2226-2226)count(3222-3284)src/Database/Adapter/SQLite.php (2)
getOperatorSQL(1425-1709)bindOperatorParams(1371-1404)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
bin/tasks/operators.php (6)
src/Database/DateTime.php (3)
DateTime(7-86)now(19-23)format(29-32)src/Database/Helpers/Permission.php (3)
Permission(9-264)read(186-195)update(220-229)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)src/Database/PDO.php (1)
PDO(13-143)src/Database/Validator/Authorization.php (2)
Authorization(7-225)setRole(83-86)src/Database/Operator.php (20)
increment(409-416)decrement(425-432)multiply(510-517)divide(527-537)modulo(589-595)power(604-611)concat(486-489)replace(498-501)toggle(544-547)arrayAppend(441-444)arrayPrepend(452-455)arrayInsert(464-467)arrayRemove(475-478)arrayUnique(619-622)arrayIntersect(630-633)arrayDiff(641-644)arrayFilter(653-656)dateAddDays(556-559)dateSubDays(567-570)dateSetNow(577-580)
src/Database/Database.php (6)
src/Database/Validator/Operator.php (2)
Operator(10-363)isValid(60-85)src/Database/Operator.php (4)
Operator(14-698)extractOperators(675-696)setAttribute(188-193)getAttribute(147-150)src/Database/Document.php (7)
Document(12-470)getId(63-66)find(304-322)getArrayCopy(423-458)setAttribute(244-261)getAttributes(194-212)getAttribute(224-231)src/Database/Validator/Authorization.php (3)
Authorization(7-225)skip(160-170)isValid(55-77)src/Database/Adapter/SQL.php (2)
find(3005-3210)updateDocuments(460-738)src/Database/Adapter/Postgres.php (1)
updateDocument(1093-1314)
src/Database/Adapter/SQLite.php (7)
src/Database/Database.php (2)
Database(38-8478)getNamespace(837-840)src/Database/Adapter/Postgres.php (6)
processException(2023-2066)getOperatorSQL(2393-2596)bindOperatorParams(2607-2642)getPDOType(1885-1894)quote(2072-2075)getUpsertStatement(1326-1405)src/Database/Adapter/SQL.php (7)
processException(2444-2447)getOperatorSQL(1796-1965)bindOperatorParams(1975-2138)getPDOType(2226-2226)getPDO(2214-2217)getUpsertStatement(1626-1634)getSQLTable(1783-1786)src/Database/Adapter/MariaDB.php (5)
processException(1825-1891)getOperatorSQL(1907-2065)getPDOType(1705-1713)quote(1893-1896)getUpsertStatement(1206-1285)src/Database/Adapter/MySQL.php (2)
processException(148-165)getOperatorSQL(278-306)src/Database/Operator.php (2)
isOperator(664-667)increment(409-416)src/Database/Adapter.php (3)
filter(1203-1212)getNamespace(107-110)quote(467-467)
tests/unit/OperatorTest.php (2)
src/Database/Operator.php (36)
Operator(14-698)getMethod(139-142)getAttribute(147-150)getValues(155-158)getValue(164-167)increment(409-416)decrement(425-432)concat(486-489)replace(498-501)multiply(510-517)divide(527-537)toggle(544-547)dateSetNow(577-580)modulo(589-595)power(604-611)arrayAppend(441-444)arrayPrepend(452-455)arrayInsert(464-467)arrayRemove(475-478)setMethod(175-180)setAttribute(188-193)setValues(201-206)setValue(213-218)isNumericOperation(258-261)isArrayOperation(268-271)isStringOperation(278-281)isBooleanOperation(288-291)isDateOperation(299-302)isMethod(226-251)isOperator(664-667)extractOperators(675-696)toArray(380-387)toString(393-400)parseOperator(333-356)parse(311-324)parseOperators(366-375)src/Database/Exception/Operator.php (1)
Operator(7-9)
src/Database/Validator/Operator.php (3)
src/Database/Database.php (1)
Database(38-8478)src/Database/Operator.php (6)
Operator(14-698)getAttribute(147-150)getMethod(139-142)isMethod(226-251)getValues(155-158)getValue(164-167)src/Database/Document.php (1)
getId(63-66)
src/Database/Adapter/SQL.php (6)
src/Database/Validator/Operator.php (1)
Operator(10-363)src/Database/Operator.php (6)
Operator(14-698)isOperator(664-667)getMethod(139-142)getValues(155-158)replace(498-501)extractOperators(675-696)src/Database/Adapter/Postgres.php (4)
getOperatorSQL(2393-2596)bindOperatorParams(2607-2642)Postgres(21-2643)getUpsertStatement(1326-1405)src/Database/Adapter/SQLite.php (3)
getOperatorSQL(1425-1709)bindOperatorParams(1371-1404)getUpsertStatement(1723-1803)src/Database/Adapter/MariaDB.php (2)
getOperatorSQL(1907-2065)getUpsertStatement(1206-1285)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
src/Database/DateTime.php (1)
DateTime(7-86)src/Database/Document.php (1)
Document(12-470)
🪛 PHPMD (2.15.0)
bin/tasks/operators.php
572-572: Avoid unused parameters such as '$isBulk'. (undefined)
(UnusedFormalParameter)
src/Database/Database.php
5281-5281: Avoid unused local variables such as '$attribute'. (undefined)
(UnusedLocalVariable)
tests/e2e/Adapter/Scopes/DocumentTests.php
5386-5386: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (9)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
9-9: Remove the unusedUtopia\Database\DateTimeimport.The import on line 9 is not used anywhere in the file. All DateTime references use the built-in PHP
\DateTimeclass instead. Remove this import to keep the code clean.⛔ Skipped due to learnings
Learnt from: abnegate PR: utopia-php/database#721 File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439 Timestamp: 2025-10-03T02:04:17.803Z Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.src/Database/Adapter/Postgres.php (2)
12-18: LGTM: Necessary imports for operator support.The LimitException and Operator imports align with the new operator handling and overflow exception mapping added in this PR.
2050-2058: LGTM: Appropriate exception mapping for operator-induced overflow.Mapping PostgreSQL error codes
22003(numeric overflow) and22008(datetime overflow) toLimitExceptioncorrectly handles overflow/underflow from operator operations.src/Database/Validator/Operator.php (4)
136-159: LGTM: Overflow/underflow validation correctly uses defined constants.The predicted result overflow/underflow checks correctly use
Database::MAX_INTandDatabase::MIN_INT, which are properly defined in the Database class.
169-177: LGTM: Array item range validation for integer arrays.Correctly validates that integer array items are within valid bounds when using append/prepend operators.
264-274: LGTM: Concat validation correctly restricted to strings.The validation properly ensures concat operates only on non-array string fields with string values, addressing the past review concern.
275-288: LGTM: Proactive validation prevents concat overflow.Runtime validation that predicted concat result doesn't exceed the attribute's maximum size prevents truncation errors during update operations.
src/Database/Adapter/SQLite.php (2)
1506-1528: POWER guard is good; cache detection is solid.Runtime check for math functions with static cache is appropriate; error message clearly guides configuration.
Also applies to: 1335-1360
1266-1284: Cannot implement suggested fix—Utopia\Database\Exception\Unique does not exist.The review requests using
UniqueExceptionto distinguish user-defined unique constraint violations from_uidduplicates. However:
- No Unique exception class exists in the codebase
- Current adapters (MariaDB, Postgres, SQLite) use only
DuplicateExceptionfor all constraint violations- MariaDB.php does not currently distinguish constraint types by name; all violations (rows 1848–1850) map to DuplicateException, contradicting the claimed design pattern
The suggested refactor also requires creating the missing
UniqueExceptionclass and implementing the pattern across multiple adapters—a scope larger than SQLite.php alone.Before proceeding: Verify whether
UniqueExceptionshould be created separately and whether MariaDB's implementation should be corrected to match the intended design (distinguishing_uidviolations from other constraints).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Database/Validator/Operator.php (2)
238-246: Validate operator values for intersect and diff.The intersect and diff operators require array values to operate on, but currently only the target attribute's array type is checked. Missing validation that operator values are non-empty arrays could cause runtime errors in adapter implementations.
Apply this diff:
case DatabaseOperator::TYPE_ARRAY_INTERSECT: case DatabaseOperator::TYPE_ARRAY_DIFF: if (!$isArray) { $this->message = "Cannot apply {$method} operator to non-array field '{$operator->getAttribute()}'"; return false; } + if (empty($values) || !\is_array($values[0])) { + $this->message = "Cannot apply {$method} operator: requires a non-empty array value"; + return false; + } break;
125-128: Use strict equality to prevent type coercion bugs.The loose equality
== 0can incorrectly match non-zero values due to PHP's type coercion (e.g.,"0abc" == 0evaluates totrue).Apply this diff:
- if (($method === DatabaseOperator::TYPE_DIVIDE || $method === DatabaseOperator::TYPE_MODULO) && $values[0] == 0) { + if (($method === DatabaseOperator::TYPE_DIVIDE || $method === DatabaseOperator::TYPE_MODULO) && $values[0] === 0) {
🧹 Nitpick comments (1)
src/Database/Validator/Operator.php (1)
247-263: Consider validating filter condition operators.While the code checks that the filter condition is a string, it doesn't verify it's a valid comparison operator (e.g., '=', '!=', '>', '<', 'contains'). Invalid conditions would fail during SQL translation. If the set of valid conditions is well-defined, add validation to catch errors earlier.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Validator/Operator.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Validator/Operator.php (3)
src/Database/Database.php (3)
Database(38-8478)__construct(422-620)count(7252-7301)src/Database/Operator.php (7)
Operator(14-698)__construct(120-125)getAttribute(147-150)getMethod(139-142)isMethod(226-251)getValues(155-158)getValue(164-167)src/Database/Adapter/SQL.php (2)
__construct(58-61)count(3222-3284)
🔇 Additional comments (6)
src/Database/Validator/Operator.php (6)
1-37: LGTM: Class structure and initialization are solid.The property initialization and constructor logic correctly build the attribute lookup map. The initialized
$attributesproperty resolves the prior uninitialized property concern.
60-85: LGTM: Validation entry point is well-structured.The method correctly validates the operator type, method, and attribute existence before delegating to detailed per-attribute validation.
162-237: LGTM: Array operator validation is thorough.The append, prepend, unique, insert, and remove operators are well-validated with appropriate type checks, bounds validation, and runtime checks where needed.
264-290: LGTM: Concat validation is comprehensive.The validation correctly restricts concat to non-array string fields and includes excellent runtime validation for string length limits.
291-335: LGTM: Replace, toggle, and date operator validation is solid.All operators correctly validate their expected types and values. The default case properly handles unsupported methods.
347-362: LGTM: Helper methods are correct.The
isArray()andgetType()implementations are appropriate for validating single Operator instances.
# Conflicts: # src/Database/Database.php
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
5939-6003: Upserts skip inserting brand-new documentsWhen
$oldis empty and the payload carries no user attributes (only$id/ internal fields),$hasChangesnever flips true, so weunset($documents[$key])and never send the document to the adapter. That breaks the common “ensure this document exists” upsert flow—callingupsertDocument()with a placeholder now becomes a no-op. Please treat an empty$oldas a guaranteed change before we bail.if (!empty($operators)) { $hasChanges = true; } elseif (!empty($attribute)) { $hasChanges = true; } elseif (!$skipPermissionsUpdate) { $hasChanges = true; } else { // Check if any of the provided attributes differ from old document foreach ($regularUpdatesUserOnly as $attrKey => $value) { $oldValue = $oldAttributes[$attrKey] ?? null; if ($oldValue != $value) { $hasChanges = true; break; } } // Also check if old document has attributes that new document doesn't if (!$hasChanges) { ... } } + + if ($old->isEmpty()) { + $hasChanges = true; + } if (!$hasChanges) { // If not updating a single attribute and the // document is the same as the old one, skip it unset($documents[$key]); continue;
♻️ Duplicate comments (2)
src/Database/Adapter/SQL.php (1)
1966-1968: PreventarrayRemovefrom wiping the whole column.When
JSON_SEARCHcan’t find the requested value it returnsNULL, andJSON_REMOVEfed with aNULLpath collapses the entire JSON document toNULL. That means every attempt to remove a non-existent element replaces the array withNULL, effectively deleting user data. Please defend against theNULLpath (this was flagged earlier and still reproduces). Wrap the expression withIFNULL(..., {$quotedColumn})so we fall back to the original column when nothing is removed.Apply this diff:
- return "{$quotedColumn} = JSON_REMOVE({$quotedColumn}, JSON_UNQUOTE(JSON_SEARCH({$quotedColumn}, 'one', :$bindKey)))"; + return "{$quotedColumn} = IFNULL( + JSON_REMOVE({$quotedColumn}, JSON_UNQUOTE(JSON_SEARCH({$quotedColumn}, 'one', :$bindKey))), + {$quotedColumn} + )";src/Database/Adapter/MariaDB.php (1)
1852-1855: Differentiate_uidduplicates from other unique collisions.This block still maps every 1062 error to
DuplicateException, so_id/custom unique index violations masquerade as user-document collisions. Please inspecterrorInfo[2]for the_uidkey and returnUniqueExceptionotherwise; also add the corresponding import. Based on learningsuse Utopia\Database\Exception\Duplicate as DuplicateException; +use Utopia\Database\Exception\Unique as UniqueException; @@ - if ($e->getCode() === '23000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1062) { - return new DuplicateException('Document already exists', $e->getCode(), $e); - } + if ($e->getCode() === '23000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1062) { + $message = $e->errorInfo[2] ?? ''; + if (\str_contains($message, "for key '_uid'")) { + return new DuplicateException('Document already exists', $e->getCode(), $e); + } + + return new UniqueException('Unique constraint violated', $e->getCode(), $e); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Database/Adapter/MariaDB.php(9 hunks)src/Database/Adapter/Postgres.php(10 hunks)src/Database/Adapter/SQL.php(8 hunks)src/Database/Adapter/SQLite.php(9 hunks)src/Database/Database.php(21 hunks)src/Database/Validator/Query/Filter.php(1 hunks)tests/e2e/Adapter/Base.php(2 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(2 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Validator/Query/Filter.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
PR: utopia-php/database#733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
src/Database/Adapter/SQLite.phpsrc/Database/Adapter/MariaDB.php
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.php
🧬 Code graph analysis (7)
tests/e2e/Adapter/Scopes/SpatialTests.php (9)
src/Database/Database.php (4)
getDatabase(866-869)Database(38-8559)createDocument(4372-4469)updateDocument(4925-5163)tests/e2e/Adapter/Base.php (1)
getDatabase(42-42)src/Database/Adapter.php (4)
getDatabase(160-163)getSupportForSpatialAttributes(1071-1071)createDocument(701-701)updateDocument(725-725)src/Database/Adapter/MariaDB.php (3)
getSupportForSpatialAttributes(2082-2085)createDocument(824-935)updateDocument(950-1198)src/Database/Adapter/Postgres.php (3)
getSupportForSpatialAttributes(2092-2095)createDocument(979-1079)updateDocument(1093-1324)src/Database/Adapter/SQLite.php (3)
getSupportForSpatialAttributes(1003-1006)createDocument(524-629)updateDocument(643-884)src/Database/Adapter/SQL.php (1)
getSupportForSpatialAttributes(1540-1543)src/Database/Adapter/Pool.php (3)
getSupportForSpatialAttributes(438-441)createDocument(228-231)updateDocument(238-241)src/Database/Mirror.php (2)
createDocument(540-585)updateDocument(658-704)
src/Database/Adapter/Postgres.php (3)
src/Database/Database.php (2)
Database(38-8559)count(7321-7372)src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)increment(409-416)getMethod(139-142)getValues(155-158)src/Database/Adapter/SQL.php (6)
getOperatorSQL(1856-2025)getSpatialGeomFromText(1652-1664)bindOperatorParams(2035-2198)convertArrayToWKT(3049-3088)getPDOType(2312-2312)count(3324-3386)
src/Database/Adapter/SQLite.php (3)
src/Database/Adapter/SQL.php (7)
getOperatorSQL(1856-2025)getSpatialGeomFromText(1652-1664)bindOperatorParams(2035-2198)convertArrayToWKT(3049-3088)getPDOType(2312-2312)getPDO(2300-2303)getUpsertStatement(1686-1694)src/Database/Adapter.php (1)
quote(467-467)src/Database/Operator.php (2)
isOperator(664-667)increment(409-416)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
src/Database/Database.php (1)
Database(38-8559)src/Database/Validator/Queries/Document.php (1)
Document(10-44)src/Database/Document.php (1)
Document(12-470)
src/Database/Adapter/MariaDB.php (4)
src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)increment(409-416)getMethod(139-142)getValues(155-158)src/Database/Adapter/Postgres.php (3)
getOperatorSQL(2403-2608)bindOperatorParams(2619-2654)getPDOType(1895-1904)src/Database/Adapter/SQL.php (3)
getOperatorSQL(1856-2025)bindOperatorParams(2035-2198)getPDOType(2312-2312)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
src/Database/Adapter/SQL.php (8)
src/Database/Database.php (2)
Database(38-8559)getTenant(1141-1144)src/Database/Validator/Operator.php (1)
Operator(10-367)src/Database/Operator.php (7)
Operator(14-698)isOperator(664-667)getMethod(139-142)getValues(155-158)replace(498-501)getAttribute(147-150)extractOperators(675-696)src/Database/Adapter.php (5)
Adapter(16-1401)filter(1232-1241)quote(467-467)execute(1329-1329)getTenant(216-219)src/Database/Adapter/MariaDB.php (5)
getOperatorSQL(1912-2070)quote(1898-1901)getPDOType(1710-1718)processException(1830-1896)getUpsertStatement(1211-1290)src/Database/Adapter/Postgres.php (8)
getOperatorSQL(2403-2608)quote(2082-2085)bindOperatorParams(2619-2654)Postgres(21-2655)getPDOType(1895-1904)execute(86-106)processException(2033-2076)getUpsertStatement(1336-1415)src/Database/Adapter/SQLite.php (4)
getOperatorSQL(1435-1719)bindOperatorParams(1381-1414)processException(1276-1294)getUpsertStatement(1733-1813)src/Database/Adapter/MySQL.php (2)
getOperatorSQL(278-306)processException(148-165)
src/Database/Database.php (6)
src/Database/Validator/Operator.php (3)
Operator(10-367)isValid(60-85)getDescription(46-49)src/Database/Operator.php (4)
Operator(14-698)extractOperators(675-696)setAttribute(188-193)getAttribute(147-150)src/Database/Document.php (8)
Document(12-470)getId(63-66)find(304-322)getArrayCopy(423-458)setAttribute(244-261)getAttributes(194-212)isEmpty(396-399)getAttribute(224-231)src/Database/Validator/Authorization.php (3)
Authorization(7-225)skip(160-170)getDescription(41-44)src/Database/Mirror.php (2)
silent(172-175)updateDocument(658-704)src/Database/Adapter.php (3)
find(802-802)updateDocument(725-725)castingAfter(1369-1369)
🪛 GitHub Actions: Linter
src/Database/Adapter/SQL.php
[error] 1-1: PSR-12: statement_indentation violation detected by Pint. Fix indentation in this file.
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/DocumentTests.php
5431-5431: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
src/Database/Database.php
5322-5322: Avoid unused local variables such as '$attribute'. (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 (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
1-1: Verify removal of decode-related tests.The AI summary indicates three test methods were removed:
testDecodeWithDifferentSelectionTypes()testDecodeWithoutRelationships()testDecodeWithMultipleFilters()Removing tests reduces coverage for decode behavior. Please confirm this removal is intentional and that the decode functionality is adequately covered by remaining tests, or if this functionality has been deprecated.
| $bindIndex++; | ||
| return "{$quotedColumn} = ( | ||
| SELECT JSON_ARRAYAGG(value) | ||
| FROM JSON_TABLE({$quotedColumn}, '$[*]' COLUMNS(value JSON PATH '$')) AS jt | ||
| WHERE CASE :$conditionKey | ||
| WHEN 'equal' THEN value = :$valueKey | ||
| WHEN 'notEqual' THEN value != :$valueKey | ||
| WHEN 'greaterThan' THEN CAST(value AS SIGNED) > CAST(:$valueKey AS SIGNED) | ||
| WHEN 'lessThan' THEN CAST(value AS SIGNED) < CAST(:$valueKey AS SIGNED) | ||
| WHEN 'isNull' THEN value IS NULL | ||
| WHEN 'isNotNull' THEN value IS NOT NULL | ||
| ELSE TRUE |
There was a problem hiding this comment.
Make arrayFilter honour every allowed condition.
bindOperatorParams() now whitelists 'equals', 'notEquals', 'greaterThanOrEqual', 'lessThanOrEqual', 'null', and 'notNull', but this CASE block only recognises the earlier 'equal', 'notEqual', 'greaterThan', 'lessThan', 'isNull', and 'isNotNull'. When any of the new strings are used the CASE falls into ELSE TRUE, so the filter becomes a no-op. Add matching branches (or normalise the inputs) so every permitted condition gets the correct comparison.
Consider extending the CASE like so:
- WHEN 'equal' THEN value = :$valueKey
- WHEN 'notEqual' THEN value != :$valueKey
- WHEN 'greaterThan' THEN CAST(value AS SIGNED) > CAST(:$valueKey AS SIGNED)
- WHEN 'lessThan' THEN CAST(value AS SIGNED) < CAST(:$valueKey AS SIGNED)
- WHEN 'isNull' THEN value IS NULL
- WHEN 'isNotNull' THEN value IS NOT NULL
+ WHEN 'equal' THEN value = :$valueKey
+ WHEN 'equals' THEN value = :$valueKey
+ WHEN 'notEqual' THEN value != :$valueKey
+ WHEN 'notEquals' THEN value != :$valueKey
+ WHEN 'greaterThan' THEN CAST(value AS SIGNED) > CAST(:$valueKey AS SIGNED)
+ WHEN 'greaterThanOrEqual' THEN CAST(value AS SIGNED) >= CAST(:$valueKey AS SIGNED)
+ WHEN 'lessThan' THEN CAST(value AS SIGNED) < CAST(:$valueKey AS SIGNED)
+ WHEN 'lessThanOrEqual' THEN CAST(value AS SIGNED) <= CAST(:$valueKey AS SIGNED)
+ WHEN 'isNull' THEN value IS NULL
+ WHEN 'null' THEN value IS NULL
+ WHEN 'isNotNull' THEN value IS NOT NULL
+ WHEN 'notNull' THEN value IS NOT NULL…and keep the whitelist in bindOperatorParams() aligned with the cases you support.
| case 'notNull': | ||
| // Filter out null values - no bind parameter needed | ||
| return "{$quotedColumn} = ( | ||
| SELECT json_group_array(value) | ||
| FROM json_each(IFNULL({$quotedColumn}, '[]')) | ||
| WHERE value IS NOT NULL | ||
| )"; | ||
|
|
||
| case 'equals': | ||
| case 'notEquals': | ||
| case 'greaterThan': | ||
| case 'greaterThanOrEqual': | ||
| case 'lessThan': | ||
| case 'lessThanOrEqual': | ||
| if (\count($values) < 2) { | ||
| return "{$quotedColumn} = {$quotedColumn}"; | ||
| } | ||
|
|
||
| $bindKey = "op_{$bindIndex}"; | ||
| $bindIndex++; | ||
|
|
||
| $operator = match ($filterType) { | ||
| 'equals' => '=', | ||
| 'notEquals' => '!=', | ||
| 'greaterThan' => '>', | ||
| 'greaterThanOrEqual' => '>=', | ||
| 'lessThan' => '<', | ||
| 'lessThanOrEqual' => '<=', | ||
| default => throw new OperatorException('Unsupported filter type: ' . $filterType), | ||
| }; | ||
|
|
||
| return "{$quotedColumn} = ( | ||
| SELECT json_group_array(value) | ||
| FROM json_each(IFNULL({$quotedColumn}, '[]')) | ||
| WHERE value $operator :$bindKey | ||
| )"; | ||
|
|
||
| default: | ||
| return "{$quotedColumn} = {$quotedColumn}"; | ||
| } |
There was a problem hiding this comment.
Fix arrayFilter SQL: add null handling and numeric casts.
arrayFilter(...) currently ignores the 'null' condition (it just returns the original array), and the numeric branches compare raw JSON text, so "10" sorts before "2". Cast the JSON values/parameters to numeric for greater*/less* cases and add an explicit 'null' branch that keeps only JSON NULL entries. Example patch:
+ case 'null':
+ return "{$quotedColumn} = (
+ SELECT json_group_array(value)
+ FROM json_each(IFNULL({$quotedColumn}, '[]'))
+ WHERE value IS NULL
+ )";
case 'notNull':
// Filter out null values - no bind parameter needed
@@
- case 'equals':
- case 'notEquals':
- case 'greaterThan':
- case 'greaterThanOrEqual':
- case 'lessThan':
- case 'lessThanOrEqual':
+ case 'equals':
+ case 'notEquals':
+ case 'greaterThan':
+ case 'greaterThanOrEqual':
+ case 'lessThan':
+ case 'lessThanOrEqual':
@@
- $operator = match ($filterType) {
+ $operator = match ($filterType) {
'equals' => '=',
'notEquals' => '!=',
'greaterThan' => '>',
'greaterThanOrEqual' => '>=',
'lessThan' => '<',
'lessThanOrEqual' => '<=',
default => throw new OperatorException('Unsupported filter type: ' . $filterType),
};
+ $valueExpr = match ($filterType) {
+ 'greaterThan',
+ 'greaterThanOrEqual',
+ 'lessThan',
+ 'lessThanOrEqual' => 'CAST(value AS REAL)',
+ default => 'value',
+ };
+ $paramExpr = str_starts_with($valueExpr, 'CAST(')
+ ? "CAST(:$bindKey AS REAL)"
+ : ":$bindKey";
- return "{$quotedColumn} = (
+ return "{$quotedColumn} = (
SELECT json_group_array(value)
FROM json_each(IFNULL({$quotedColumn}, '[]'))
- WHERE value $operator :$bindKey
+ WHERE {$valueExpr} {$operator} {$paramExpr}
)";🤖 Prompt for AI Agents
In src/Database/Adapter/SQLite.php around lines 1658 to 1697, the arrayFilter
SQL currently lacks an explicit 'null' branch and compares JSON text for numeric
comparisons (so "10" < "2"); add a case for 'null' that returns only JSON NULL
entries (e.g. WHERE value IS NULL) and change the numeric comparison branches
(greaterThan, greaterThanOrEqual, lessThan, lessThanOrEqual and their negations
if any) to cast both the json_each value and the bound parameter to numeric
(e.g. CAST(value AS NUMERIC) and CAST(:op_X AS NUMERIC)) so comparisons are
numeric, keep the bind key logic intact, and preserve the default/equals
behavior when values count < 2.
| /** | ||
| * @depends testCreateDocument | ||
| */ | ||
| public function testEmptyOperatorValues(Document $document): void |
There was a problem hiding this comment.
Remove unused parameter or utilize it in the test.
The $document parameter added via the @depends annotation is never used in the test method body. The test validates Query behavior with empty arrays and doesn't require any document data.
Apply this diff to remove the unnecessary dependency:
- /**
- * @depends testCreateDocument
- */
- public function testEmptyOperatorValues(Document $document): void
+ public function testEmptyOperatorValues(): voidIf the dependency was intended for a specific purpose, please utilize the $document parameter in the test logic.
Based on static analysis hints.
📝 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.
| /** | |
| * @depends testCreateDocument | |
| */ | |
| public function testEmptyOperatorValues(Document $document): void | |
| public function testEmptyOperatorValues(): void |
🧰 Tools
🪛 PHPMD (2.15.0)
5431-5431: Avoid unused parameters such as '$document'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/DocumentTests.php around lines 5428 to 5431, the
testEmptyOperatorValues method declares a Document $document parameter via
@depends but never uses it; remove the unused dependency and parameter by
deleting the @depends annotation and the $document parameter from the method
signature, or if the document was intended, use the passed $document inside the
test assertions/setup instead of leaving it unused.
Applied PSR-12 statement_indentation fix
- Add negative multiplier/divisor support to MariaDB adapter - Fix spatial test to use required attributes for MariaDB compatibility - Fix array operator validation to allow non-array elements in intersect/diff
Apply same sign-aware overflow/underflow checks as Postgres and MariaDB
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Adapter/MariaDB.php (2)
880-891: createDocument: spatial arrays must be converted to WKT before binding; also remove stray $attribute filter.Currently arrays are json_encode’d, but column is ST_GeomFromText(:key_X). Bind WKT instead.
- $attributeIndex = 0; - foreach ($attributes as $value) { - if (\is_array($value)) { - $value = \json_encode($value); - } - - $bindKey = 'key_' . $attributeIndex; - $attribute = $this->filter($attribute); + $attributeIndex = 0; + foreach ($attributes as $attrName => $value) { + if (\in_array($attrName, $spatialAttributes, true)) { + if (\is_array($value)) { + $value = $this->convertArrayToWKT($value); + } + } elseif (\is_array($value)) { + $value = \json_encode($value); + } + + $bindKey = 'key_' . $attributeIndex; $value = (\is_bool($value)) ? (int)$value : $value; $stmt->bindValue(':' . $bindKey, $value, $this->getPDOType($value)); $attributeIndex++; }
1852-1855: Differentiate DuplicateException for _uid from other unique violations (map others to UniqueException).Per project design, only duplicate
_uidshould throwDuplicateException; other unique/PRIMARY constraint violations should throwUniqueException. The current code at lines 1852-1854 incorrectly throwsDuplicateExceptionfor all MySQL error 1062 (duplicate key) violations.Add the missing
UniqueExceptionimport and check the error message for the_uidkey to conditionally throw the appropriate exception:+use Utopia\Database\Exception\Unique as UniqueException; @@ // Duplicate row if ($e->getCode() === '23000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1062) { - return new DuplicateException('Document already exists', $e->getCode(), $e); + $msg = (string)($e->errorInfo[2] ?? $e->getMessage()); + if (\str_contains($msg, "for key '_uid'")) { + return new DuplicateException('Document already exists', $e->getCode(), $e); + } + return new UniqueException('Unique constraint violated', $e->getCode(), $e); }
♻️ Duplicate comments (4)
src/Database/Validator/Operator.php (3)
125-128: Use strict equality for zero checks (division/modulo).Loose equality can coerce strings like "0" or "0abc". Use === 0.
- if (($method === DatabaseOperator::TYPE_DIVIDE || $method === DatabaseOperator::TYPE_MODULO) && $values[0] == 0) { + if (($method === DatabaseOperator::TYPE_DIVIDE || $method === DatabaseOperator::TYPE_MODULO) && $values[0] === 0) {
141-148: Use strict inequality in predicted-result checks for divide/modulo arms.Avoid type coercion; use !== 0.
- DatabaseOperator::TYPE_DIVIDE => $operatorValue != 0 ? $currentValue / $operatorValue : $currentValue, - DatabaseOperator::TYPE_MODULO => $operatorValue != 0 ? $currentValue % $operatorValue : $currentValue, + DatabaseOperator::TYPE_DIVIDE => $operatorValue !== 0 ? $currentValue / $operatorValue : $currentValue, + DatabaseOperator::TYPE_MODULO => $operatorValue !== 0 ? $currentValue % $operatorValue : $currentValue,
251-267: Validate arrayFilter condition against an allowed set (align with adapters).Add whitelist to catch invalid conditions early.
case DatabaseOperator::TYPE_ARRAY_FILTER: if (!$isArray) { $this->message = "Cannot apply {$method} operator to non-array field '{$operator->getAttribute()}'"; return false; } if (\count($values) < 1 || \count($values) > 2) { $this->message = "Cannot apply {$method} operator: requires 1 or 2 values (condition and optional comparison value)"; return false; } if (!\is_string($values[0])) { $this->message = "Cannot apply {$method} operator: condition must be a string"; return false; } + + $valid = [ + 'equal','equals','notEqual','notEquals', + 'greaterThan','greaterThanOrEqual','lessThan','lessThanOrEqual', + 'isNull','isNotNull','null','notNull', + ]; + if (!\in_array($values[0], $valid, true)) { + $this->message = "Invalid array filter condition '{$values[0]}'"; + return false; + } break;src/Database/Adapter/SQL.php (1)
1965-1969: ARRAY_REMOVE can nullify the whole column when no match is found.JSON_SEARCH returns NULL if not found; JSON_REMOVE(column, NULL) yields NULL. Wrap with IFNULL to preserve original value.
- return "{$quotedColumn} = JSON_REMOVE({$quotedColumn}, JSON_UNQUOTE(JSON_SEARCH({$quotedColumn}, 'one', :$bindKey)))"; + return "{$quotedColumn} = IFNULL( + JSON_REMOVE({$quotedColumn}, JSON_UNQUOTE(JSON_SEARCH({$quotedColumn}, 'one', :$bindKey))), + {$quotedColumn} + )";
🧹 Nitpick comments (1)
src/Database/Adapter/SQL.php (1)
2171-2185: Normalize filter condition synonyms at bind time for portability.Optional: map 'equals'→'equal', 'notEquals'→'notEqual', 'null'→'isNull', 'notNull'→'isNotNull' before binding to reduce dialect divergence.
- $condition = $values[0] ?? 'equal'; + $condition = $values[0] ?? 'equal'; + $normalize = [ + 'equals' => 'equal', + 'notEquals' => 'notEqual', + 'null' => 'isNull', + 'notNull' => 'isNotNull', + ]; + $condition = $normalize[$condition] ?? $condition;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Adapter/MariaDB.php(9 hunks)src/Database/Adapter/SQL.php(8 hunks)src/Database/Validator/Operator.php(1 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/Scopes/SpatialTests.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
PR: utopia-php/database#733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
src/Database/Adapter/MariaDB.php
🧬 Code graph analysis (3)
src/Database/Validator/Operator.php (2)
src/Database/Database.php (2)
Database(38-8559)__construct(423-621)src/Database/Operator.php (7)
Operator(14-698)__construct(120-125)getAttribute(147-150)getMethod(139-142)isMethod(226-251)getValues(155-158)getValue(164-167)
src/Database/Adapter/MariaDB.php (5)
src/Database/Validator/Operator.php (1)
Operator(10-367)src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)increment(409-416)getMethod(139-142)getValues(155-158)src/Database/Adapter/SQL.php (4)
getOperatorSQL(1856-2025)bindOperatorParams(2035-2198)convertArrayToWKT(3049-3088)getPDOType(2312-2312)src/Database/Adapter/SQLite.php (2)
getOperatorSQL(1435-1719)bindOperatorParams(1381-1414)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
src/Database/Adapter/SQL.php (7)
src/Database/Operator.php (7)
Operator(14-698)isOperator(664-667)getMethod(139-142)getValues(155-158)replace(498-501)getAttribute(147-150)extractOperators(675-696)src/Database/Adapter.php (3)
Adapter(16-1401)filter(1232-1241)getTenant(216-219)src/Database/Adapter/MariaDB.php (4)
getOperatorSQL(1912-2072)getPDOType(1710-1718)processException(1830-1896)getUpsertStatement(1211-1290)src/Database/Adapter/SQLite.php (4)
getOperatorSQL(1435-1719)bindOperatorParams(1381-1414)processException(1276-1294)getUpsertStatement(1733-1813)src/Database/Adapter/Postgres.php (6)
getOperatorSQL(2403-2608)bindOperatorParams(2619-2654)Postgres(21-2655)getPDOType(1895-1904)processException(2033-2076)getUpsertStatement(1336-1415)src/Database/Adapter/MySQL.php (2)
getOperatorSQL(278-306)processException(148-165)src/Database/Document.php (5)
getAttribute(224-231)getId(63-66)getAttributes(194-212)getSequence(71-80)getTenant(178-187)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
| $bindIndex = 0; | ||
| $operators = []; | ||
|
|
||
| // Separate regular attributes from operators | ||
| foreach ($attributes as $attribute => $value) { | ||
| if (Operator::isOperator($value)) { | ||
| $operators[$attribute] = $value; | ||
| } | ||
| } | ||
|
|
||
| foreach ($attributes as $attribute => $value) { | ||
| $column = $this->filter($attribute); | ||
| $bindKey = 'key_' . $bindIndex; | ||
|
|
||
| if (in_array($attribute, $spatialAttributes)) { | ||
| $columns .= "`{$column}`" . '=' . $this->getSpatialGeomFromText(':' . $bindKey) . ','; | ||
| // Check if this is an operator or regular attribute | ||
| if (isset($operators[$attribute])) { | ||
| $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $bindIndex); | ||
| $columns .= $operatorSQL . ','; | ||
| } else { | ||
| $columns .= "`{$column}`" . '=:' . $bindKey . ','; | ||
| $bindKey = 'key_' . $bindIndex; | ||
|
|
||
| if (in_array($attribute, $spatialAttributes)) { | ||
| $columns .= "`{$column}`" . '=' . $this->getSpatialGeomFromText(':' . $bindKey) . ','; | ||
| } else { | ||
| $columns .= "`{$column}`" . '=:' . $bindKey . ','; | ||
| } | ||
| $bindIndex++; | ||
| } |
There was a problem hiding this comment.
updateDocument: operator param indexes desynchronize; mirror SQL fix (separate key_/op_ counters).
SQL placeholders for operators are created with the same counter as key_ then binding restarts from 0, breaking :op_n alignment.
- $bindIndex = 0;
+ // Use independent counters for regular values and operator params
+ $keyIndex = 0;
+ $opIndex = 0;
@@
- if (isset($operators[$attribute])) {
- $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $bindIndex);
+ if (isset($operators[$attribute])) {
+ $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $opIndex);
$columns .= $operatorSQL . ',';
} else {
- $bindKey = 'key_' . $bindIndex;
+ $bindKey = 'key_' . $keyIndex;
if (in_array($attribute, $spatialAttributes)) {
$columns .= "`{$column}`" . '=' . $this->getSpatialGeomFromText(':' . $bindKey) . ',';
} else {
$columns .= "`{$column}`" . '=:' . $bindKey . ',';
}
- $bindIndex++;
+ $keyIndex++;
}
@@
- $attributeIndex = 0;
+ $keyBindIndex = 0;
+ $opBindIndex = 0;
foreach ($attributes as $attribute => $value) {
// Handle operators separately
if (isset($operators[$attribute])) {
- $this->bindOperatorParams($stmt, $operators[$attribute], $attributeIndex);
+ $this->bindOperatorParams($stmt, $operators[$attribute], $opBindIndex);
} else {
// Convert spatial arrays to WKT, json_encode non-spatial arrays
if (\in_array($attribute, $spatialAttributes, true)) {
if (\is_array($value)) {
$value = $this->convertArrayToWKT($value);
}
} elseif (is_array($value)) {
$value = json_encode($value);
}
- $bindKey = 'key_' . $attributeIndex;
+ $bindKey = 'key_' . $keyBindIndex;
$value = (is_bool($value)) ? (int)$value : $value;
$stmt->bindValue(':' . $bindKey, $value, $this->getPDOType($value));
- $attributeIndex++;
+ $keyBindIndex++;
}
}Also applies to: 1162-1182
🤖 Prompt for AI Agents
In src/Database/Adapter/MariaDB.php around lines 1115-1141 (and similarly
1162-1182), the placeholder counters for regular keys and operator placeholders
are shared, causing :key_n and :op_n to get out of sync; introduce and use a
separate operator counter (e.g., opIndex) for generating operator placeholders
and pass/advance that counter when calling getOperatorSQL, while keeping
bindIndex for regular key placeholders, and update the binding logic in the
later block (1162-1182) to bind operator values using the opIndex sequence so
:op_n and :key_n align correctly.
| $conditionKey = "op_{$bindIndex}"; | ||
| $bindIndex++; | ||
| $valueKey = "op_{$bindIndex}"; | ||
| $bindIndex++; | ||
| // Note: parent binds value as JSON-encoded, so we need to unquote it for TEXT comparison | ||
| return "{$quotedColumn} = IFNULL(( | ||
| SELECT JSON_ARRAYAGG(value) | ||
| FROM JSON_TABLE({$quotedColumn}, '\$[*]' COLUMNS(value TEXT PATH '\$')) AS jt | ||
| WHERE CASE :$conditionKey | ||
| WHEN 'equals' THEN value = JSON_UNQUOTE(:$valueKey) | ||
| WHEN 'notEquals' THEN value != JSON_UNQUOTE(:$valueKey) | ||
| WHEN 'greaterThan' THEN CAST(value AS SIGNED) > JSON_UNQUOTE(:$valueKey) | ||
| WHEN 'lessThan' THEN CAST(value AS SIGNED) < JSON_UNQUOTE(:$valueKey) | ||
| WHEN 'null' THEN value IS NULL | ||
| WHEN 'notNull' THEN value IS NOT NULL | ||
| ELSE TRUE | ||
| END | ||
| ), JSON_ARRAY())"; |
There was a problem hiding this comment.
ARRAY_FILTER (MariaDB): add >= and <= condition support.
Whitelist allows greaterThanOrEqual/lessThanOrEqual; CASE doesn’t handle them.
- WHEN 'greaterThan' THEN CAST(value AS SIGNED) > JSON_UNQUOTE(:$valueKey)
- WHEN 'lessThan' THEN CAST(value AS SIGNED) < JSON_UNQUOTE(:$valueKey)
+ WHEN 'greaterThan' THEN CAST(value AS SIGNED) > JSON_UNQUOTE(:$valueKey)
+ WHEN 'greaterThanOrEqual' THEN CAST(value AS SIGNED) >= JSON_UNQUOTE(:$valueKey)
+ WHEN 'lessThan' THEN CAST(value AS SIGNED) < JSON_UNQUOTE(:$valueKey)
+ WHEN 'lessThanOrEqual' THEN CAST(value AS SIGNED) <= JSON_UNQUOTE(:$valueKey)🤖 Prompt for AI Agents
In src/Database/Adapter/MariaDB.php around lines 2049 to 2066, the CASE inside
the JSON_TABLE filter handles greaterThan and lessThan but is missing branches
for greaterThanOrEqual and lessThanOrEqual; add WHEN 'greaterThanOrEqual' THEN
CAST(value AS SIGNED) >= JSON_UNQUOTE(:$valueKey) and WHEN 'lessThanOrEqual'
THEN CAST(value AS SIGNED) <= JSON_UNQUOTE(:$valueKey) alongside the existing
comparisons so the whitelisted operators are supported and numeric comparisons
use the same CAST/JSON_UNQUOTE pattern.
| $bindIndex = 0; | ||
| $columns = ''; | ||
| $operators = []; | ||
|
|
||
| // Separate regular attributes from operators | ||
| foreach ($attributes as $attribute => $value) { | ||
| if (Operator::isOperator($value)) { | ||
| $operators[$attribute] = $value; | ||
| } | ||
| } | ||
|
|
||
| foreach ($attributes as $attribute => $value) { | ||
| $column = $this->filter($attribute); | ||
|
|
||
| if (in_array($attribute, $spatialAttributes)) { | ||
| // Check if this is an operator, spatial attribute, or regular attribute | ||
| if (isset($operators[$attribute])) { | ||
| $columns .= $this->getOperatorSQL($column, $operators[$attribute], $bindIndex); | ||
| } elseif (\in_array($attribute, $spatialAttributes)) { | ||
| $columns .= "{$this->quote($column)} = " . $this->getSpatialGeomFromText(":key_{$bindIndex}"); | ||
| $bindIndex++; | ||
| } else { | ||
| $columns .= "{$this->quote($column)} = :key_{$bindIndex}"; | ||
| $bindIndex++; | ||
| } | ||
|
|
||
| if ($attribute !== \array_key_last($attributes)) { | ||
| $columns .= ','; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix operator bind-index mismatch in updateDocuments (op_/key_ counters out of sync).
Placeholders for operators are generated with $bindIndex, but binding restarts from 0 ($attributeIndex). This desynchronizes :op_n names and causes PDO errors or wrong bindings. Use independent counters for key_ and op_ during SQL generation and binding.
- $bindIndex = 0;
- $columns = '';
- $operators = [];
+ $columns = '';
+ $operators = [];
+ // Use separate counters for regular values and operator params
+ $keyIndex = 0;
+ $opIndex = 0;
@@
- foreach ($attributes as $attribute => $value) {
+ foreach ($attributes as $attribute => $value) {
$column = $this->filter($attribute);
// Check if this is an operator, spatial attribute, or regular attribute
if (isset($operators[$attribute])) {
- $columns .= $this->getOperatorSQL($column, $operators[$attribute], $bindIndex);
+ $columns .= $this->getOperatorSQL($column, $operators[$attribute], $opIndex);
} elseif (\in_array($attribute, $spatialAttributes)) {
- $columns .= "{$this->quote($column)} = " . $this->getSpatialGeomFromText(":key_{$bindIndex}");
- $bindIndex++;
+ $columns .= "{$this->quote($column)} = " . $this->getSpatialGeomFromText(":key_{$keyIndex}");
+ $keyIndex++;
} else {
- $columns .= "{$this->quote($column)} = :key_{$bindIndex}";
- $bindIndex++;
+ $columns .= "{$this->quote($column)} = :key_{$keyIndex}";
+ $keyIndex++;
}
if ($attribute !== \array_key_last($attributes)) {
$columns .= ',';
}
}
@@
- $attributeIndex = 0;
+ $keyBindIndex = 0;
+ $opBindIndex = 0;
foreach ($attributes as $attributeName => $value) {
// Skip operators as they don't need value binding
if (isset($operators[$attributeName])) {
- $this->bindOperatorParams($stmt, $operators[$attributeName], $attributeIndex);
- continue;
+ $this->bindOperatorParams($stmt, $operators[$attributeName], $opBindIndex);
+ continue;
}
// Convert spatial arrays to WKT, json_encode non-spatial arrays
if (\in_array($attributeName, $spatialAttributes, true)) {
if (\is_array($value)) {
$value = $this->convertArrayToWKT($value);
}
} elseif (\is_array($value)) {
$value = \json_encode($value);
}
- $bindKey = 'key_' . $attributeIndex;
+ $bindKey = 'key_' . $keyBindIndex;
// For PostgreSQL, preserve boolean values directly
if (!($this instanceof \Utopia\Database\Adapter\Postgres && \is_bool($value))) {
$value = (\is_bool($value)) ? (int)$value : $value;
}
$stmt->bindValue(':' . $bindKey, $value, $this->getPDOType($value));
- $attributeIndex++;
+ $keyBindIndex++;
}Also applies to: 544-568
🤖 Prompt for AI Agents
In src/Database/Adapter/SQL.php around lines 486 to 515, the SQL generation uses
a single $bindIndex for both regular :key_n placeholders and operator-generated
:op_n placeholders which desynchronizes names vs bindings; fix by introducing
two separate counters (e.g. $keyIndex and $opIndex), use $keyIndex for
generating ":key_X" and $opIndex for generating operator placeholders passed
into getOperatorSQL (and increment each counter where used), then update the
subsequent binding logic (around lines 544-568) to bind values using the
matching $keyIndex and $opIndex names instead of assuming one shared counter so
:key_n and :op_n stay in sync with their values.
| case Operator::TYPE_ARRAY_INSERT: | ||
| $indexKey = "op_{$bindIndex}"; | ||
| $bindIndex++; | ||
| $valueKey = "op_{$bindIndex}"; | ||
| $bindIndex++; | ||
| return "{$quotedColumn} = JSON_INSERT({$quotedColumn}, CONCAT('$[', :$indexKey, ']'), :$valueKey)"; | ||
|
|
There was a problem hiding this comment.
ARRAY_INSERT should insert typed JSON, not a quoted string.
Bind value is JSON-encoded; JSON_INSERT with a plain placeholder inserts it as a string. Use JSON_EXTRACT to insert the JSON value.
- return "{$quotedColumn} = JSON_INSERT({$quotedColumn}, CONCAT('$[', :$indexKey, ']'), :$valueKey)";
+ return "{$quotedColumn} = JSON_INSERT({$quotedColumn}, CONCAT('$[', :$indexKey, ']'), JSON_EXTRACT(:$valueKey, '$'))";🤖 Prompt for AI Agents
In src/Database/Adapter/SQL.php around lines 1973 to 1979, the ARRAY_INSERT
branch currently inserts the bound JSON as a quoted string; update the
expression to insert a typed JSON value by wrapping the value placeholder with
JSON_EXTRACT so JSON_INSERT receives a JSON value instead of a string. Replace
the right-hand expression to use JSON_INSERT({$quotedColumn}, CONCAT('$[',
:opIndex, ']'), JSON_EXTRACT(:opValue, '$')) (keeping the same bind keys logic)
so the bound value is treated as JSON; ensure the bind still contains raw JSON
(not double-encoded) when passed.
| case Operator::TYPE_ARRAY_FILTER: | ||
| $conditionKey = "op_{$bindIndex}"; | ||
| $bindIndex++; | ||
| $valueKey = "op_{$bindIndex}"; | ||
| $bindIndex++; | ||
| return "{$quotedColumn} = ( | ||
| SELECT JSON_ARRAYAGG(value) | ||
| FROM JSON_TABLE({$quotedColumn}, '$[*]' COLUMNS(value JSON PATH '$')) AS jt | ||
| WHERE CASE :$conditionKey | ||
| WHEN 'equal' THEN value = :$valueKey | ||
| WHEN 'notEqual' THEN value != :$valueKey | ||
| WHEN 'greaterThan' THEN CAST(value AS SIGNED) > CAST(:$valueKey AS SIGNED) | ||
| WHEN 'lessThan' THEN CAST(value AS SIGNED) < CAST(:$valueKey AS SIGNED) | ||
| WHEN 'isNull' THEN value IS NULL | ||
| WHEN 'isNotNull' THEN value IS NOT NULL | ||
| ELSE TRUE | ||
| END | ||
| )"; |
There was a problem hiding this comment.
ARRAY_FILTER: expand CASE to cover all whitelisted conditions and compare JSON correctly.
Base CASE ignores 'equals', 'notEquals', 'greaterThanOrEqual', 'lessThanOrEqual', 'null', 'notNull'. Also comparisons should read JSON from the bound value.
- WHERE CASE :$conditionKey
- WHEN 'equal' THEN value = :$valueKey
- WHEN 'notEqual' THEN value != :$valueKey
- WHEN 'greaterThan' THEN CAST(value AS SIGNED) > CAST(:$valueKey AS SIGNED)
- WHEN 'lessThan' THEN CAST(value AS SIGNED) < CAST(:$valueKey AS SIGNED)
- WHEN 'isNull' THEN value IS NULL
- WHEN 'isNotNull' THEN value IS NOT NULL
+ WHERE CASE :$conditionKey
+ WHEN 'equal' THEN value = JSON_EXTRACT(:$valueKey, '$')
+ WHEN 'equals' THEN value = JSON_EXTRACT(:$valueKey, '$')
+ WHEN 'notEqual' THEN value != JSON_EXTRACT(:$valueKey, '$')
+ WHEN 'notEquals' THEN value != JSON_EXTRACT(:$valueKey, '$')
+ WHEN 'greaterThan' THEN CAST(value AS SIGNED) > CAST(JSON_EXTRACT(:$valueKey, '$') AS SIGNED)
+ WHEN 'greaterThanOrEqual' THEN CAST(value AS SIGNED) >= CAST(JSON_EXTRACT(:$valueKey, '$') AS SIGNED)
+ WHEN 'lessThan' THEN CAST(value AS SIGNED) < CAST(JSON_EXTRACT(:$valueKey, '$') AS SIGNED)
+ WHEN 'lessThanOrEqual' THEN CAST(value AS SIGNED) <= CAST(JSON_EXTRACT(:$valueKey, '$') AS SIGNED)
+ WHEN 'isNull' THEN value IS NULL
+ WHEN 'null' THEN value IS NULL
+ WHEN 'isNotNull' THEN value IS NOT NULL
+ WHEN 'notNull' THEN value IS NOT NULL
ELSE TRUE
END
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Adapter/SQLite.php(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
PR: utopia-php/database#733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
src/Database/Adapter/SQLite.php
🧬 Code graph analysis (1)
src/Database/Adapter/SQLite.php (5)
src/Database/Database.php (2)
Database(38-8559)getNamespace(838-841)src/Database/Exception.php (1)
Exception(7-21)src/Database/Adapter/MariaDB.php (5)
processException(1830-1896)getOperatorSQL(1912-2072)getPDOType(1710-1718)quote(1898-1901)getUpsertStatement(1211-1290)src/Database/Adapter/SQL.php (9)
processException(2538-2541)getSpatialAttributes(432-445)getOperatorSQL(1856-2025)getSpatialGeomFromText(1652-1664)bindOperatorParams(2035-2198)getPDOType(2312-2312)getPDO(2300-2303)getUpsertStatement(1686-1694)getSQLTable(1843-1846)src/Database/Operator.php (2)
isOperator(664-667)increment(409-416)
⏰ 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). (4)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (8)
src/Database/Adapter/SQLite.php (8)
12-18: LGTM: Imports are appropriate for operator support.The new imports (LimitException, OperatorException, Operator) are used throughout the file for the operator functionality.
624-624: Exception handling correctly delegates to centralized processor.Good fix! This now uses
processException()instead of local MySQL-specific error code matching, addressing the previous review feedback.
880-880: Exception handling correctly delegates to centralized processor.Consistent with the fix in createDocument, this properly delegates to
processException().
1288-1291: LGTM: Correct LimitException mapping for SQLite TOOBIG error.Error code 18 (SQLITE_TOOBIG) correctly maps to LimitException for oversized strings/BLOBs.
1344-1370: LGTM: Math function detection is well-implemented.The feature detection with static caching is efficient, and the POWER function test is appropriate for determining availability.
1372-1414: LGTM: Operator parameter binding correctly handles SQLite specifics.The selective binding for parameterless operators and the special ARRAY_FILTER logic align with the SQL generation in
getOperatorSQL().
1735-1815: LGTM: SQLite upsert implementation is correct but complex.The
ON CONFLICT DO UPDATEsyntax is appropriate for SQLite. The bind index management is subtle but correct:
- Non-operator updates use
excluded.{attribute}from the INSERT clause (no new parameters)- Only operator updates need new parameters in the UPDATE clause
- SQL generation and parameter binding iterate in the same order over operators
645-645: LGTM: Spatial attribute handling is correctly integrated.The spatial attribute flow correctly identifies spatial types, generates SQL with
getSpatialGeomFromText, and converts array values to WKT format before binding.Also applies to: 813-816, 857-863
| case Operator::TYPE_ARRAY_FILTER: | ||
| $values = $operator->getValues(); | ||
| if (empty($values)) { | ||
| // No filter criteria, return array unchanged | ||
| return "{$quotedColumn} = {$quotedColumn}"; | ||
| } | ||
|
|
||
| $filterType = $values[0]; // 'equals', 'notEquals', 'notNull', 'greaterThan', etc. | ||
|
|
||
| switch ($filterType) { | ||
| case 'notNull': | ||
| // Filter out null values - no bind parameter needed | ||
| return "{$quotedColumn} = ( | ||
| SELECT json_group_array(value) | ||
| FROM json_each(IFNULL({$quotedColumn}, '[]')) | ||
| WHERE value IS NOT NULL | ||
| )"; | ||
|
|
||
| case 'equals': | ||
| case 'notEquals': | ||
| case 'greaterThan': | ||
| case 'greaterThanOrEqual': | ||
| case 'lessThan': | ||
| case 'lessThanOrEqual': | ||
| if (\count($values) < 2) { | ||
| return "{$quotedColumn} = {$quotedColumn}"; | ||
| } | ||
|
|
||
| $bindKey = "op_{$bindIndex}"; | ||
| $bindIndex++; | ||
|
|
||
| $operator = match ($filterType) { | ||
| 'equals' => '=', | ||
| 'notEquals' => '!=', | ||
| 'greaterThan' => '>', | ||
| 'greaterThanOrEqual' => '>=', | ||
| 'lessThan' => '<', | ||
| 'lessThanOrEqual' => '<=', | ||
| default => throw new OperatorException('Unsupported filter type: ' . $filterType), | ||
| }; | ||
|
|
||
| return "{$quotedColumn} = ( | ||
| SELECT json_group_array(value) | ||
| FROM json_each(IFNULL({$quotedColumn}, '[]')) | ||
| WHERE value $operator :$bindKey | ||
| )"; | ||
|
|
||
| default: | ||
| return "{$quotedColumn} = {$quotedColumn}"; | ||
| } |
There was a problem hiding this comment.
Fix ARRAY_FILTER: add 'null' case and numeric casting for comparisons.
The past review identified two issues that remain unaddressed:
-
Missing 'null' filter: There's no case to filter for
nullvalues (onlynotNullat line 1660). Add a'null'case that keeps only JSON NULL entries:WHERE value IS NULL
-
Numeric comparisons use text comparison: Lines 1681-1695 compare JSON values as text, so
"10" < "2"will be true. For numeric operators (greaterThan,greaterThanOrEqual,lessThan,lessThanOrEqual), cast both the column value and parameter to numeric:WHERE CAST(value AS REAL) > CAST(:$bindKey AS REAL)
Apply this diff to fix both issues:
switch ($filterType) {
+ case 'null':
+ // Filter for null values - no bind parameter needed
+ return "{$quotedColumn} = (
+ SELECT json_group_array(value)
+ FROM json_each(IFNULL({$quotedColumn}, '[]'))
+ WHERE value IS NULL
+ )";
+
case 'notNull':
// Filter out null values - no bind parameter needed
return "{$quotedColumn} = (
SELECT json_group_array(value)
FROM json_each(IFNULL({$quotedColumn}, '[]'))
WHERE value IS NOT NULL
)";
case 'equals':
case 'notEquals':
case 'greaterThan':
case 'greaterThanOrEqual':
case 'lessThan':
case 'lessThanOrEqual':
if (\count($values) < 2) {
return "{$quotedColumn} = {$quotedColumn}";
}
$bindKey = "op_{$bindIndex}";
$bindIndex++;
$operator = match ($filterType) {
'equals' => '=',
'notEquals' => '!=',
'greaterThan' => '>',
'greaterThanOrEqual' => '>=',
'lessThan' => '<',
'lessThanOrEqual' => '<=',
default => throw new OperatorException('Unsupported filter type: ' . $filterType),
};
+
+ // For numeric comparisons, cast to REAL to ensure proper comparison
+ $needsCast = in_array($filterType, ['greaterThan', 'greaterThanOrEqual', 'lessThan', 'lessThanOrEqual']);
+ $valueExpr = $needsCast ? 'CAST(value AS REAL)' : 'value';
+ $paramExpr = $needsCast ? "CAST(:$bindKey AS REAL)" : ":$bindKey";
- return "{$quotedColumn} = (
+ return "{$quotedColumn} = (
SELECT json_group_array(value)
FROM json_each(IFNULL({$quotedColumn}, '[]'))
- WHERE value $operator :$bindKey
+ WHERE {$valueExpr} {$operator} {$paramExpr}
)";Based on past review comments.
🤖 Prompt for AI Agents
In src/Database/Adapter/SQLite.php around lines 1650 to 1699, the ARRAY_FILTER
switch needs two fixes: add a 'null' case alongside 'notNull' that returns only
JSON NULL entries using WHERE value IS NULL, and for numeric comparison filter
types (greaterThan, greaterThanOrEqual, lessThan, lessThanOrEqual) change the
WHERE clause to cast both sides to numeric (e.g. WHERE CAST(value AS REAL) >
CAST(:$bindKey AS REAL)) so comparisons use numeric ordering; keep equals and
notEquals as text comparisons, ensure the $bindKey is still created/incremented
as shown, and throw the same OperatorException for unsupported types.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Adapter/Postgres.php (1)
1243-1308: Desynchronized placeholders: separate counters for key_ vs op_.A single $bindIndex is used for both key_ and op_ placeholders when building SQL, but binding uses a fresh $attributeIndex, causing op_n/key_n misalignment and parameter binding failures. Use independent counters for operator and regular placeholders during both SQL construction and binding.
- $bindIndex = 0; - $operators = []; + // Separate counters for value placeholders and operator placeholders + $keyIndex = 0; + $opIndex = 0; + $operators = []; @@ - foreach ($attributes as $attribute => $value) { + foreach ($attributes as $attribute => $value) { $column = $this->filter($attribute); - - // Check if this is an operator, spatial attribute, or regular attribute - if (isset($operators[$attribute])) { - $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $bindIndex); - $columns .= $operatorSQL . ','; - } elseif (\in_array($attribute, $spatialAttributes, true)) { - $bindKey = 'key_' . $bindIndex; - $columns .= "\"{$column}\" = " . $this->getSpatialGeomFromText(':' . $bindKey) . ','; - $bindIndex++; - } else { - $bindKey = 'key_' . $bindIndex; - $columns .= "\"{$column}\"" . '=:' . $bindKey . ','; - $bindIndex++; - } + if (isset($operators[$attribute])) { + $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $opIndex); + $columns .= $operatorSQL . ','; + } elseif (\in_array($attribute, $spatialAttributes, true)) { + $bindKey = 'key_' . $keyIndex; + $columns .= "\"{$column}\" = " . $this->getSpatialGeomFromText(':' . $bindKey) . ','; + $keyIndex++; + } else { + $bindKey = 'key_' . $keyIndex; + $columns .= "\"{$column}\" = :" . $bindKey . ','; + $keyIndex++; + } } @@ - $attributeIndex = 0; + $keyBindIndex = 0; + $opBindIndex = 0; foreach ($attributes as $attribute => $value) { // Handle operators separately if (isset($operators[$attribute])) { - $this->bindOperatorParams($stmt, $operators[$attribute], $attributeIndex); + $this->bindOperatorParams($stmt, $operators[$attribute], $opBindIndex); } else { // Convert spatial arrays to WKT, json_encode non-spatial arrays if (\in_array($attribute, $spatialAttributes, true)) { if (\is_array($value)) { $value = $this->convertArrayToWKT($value); } } elseif (is_array($value)) { $value = json_encode($value); } - $bindKey = 'key_' . $attributeIndex; - $value = (is_bool($value)) ? ($value == true ? "true" : "false") : $value; + $bindKey = 'key_' . $keyBindIndex; + $value = (\is_bool($value)) ? ($value ? "true" : "false") : $value; $stmt->bindValue(':' . $bindKey, $value, $this->getPDOType($value)); - $attributeIndex++; + $keyBindIndex++; } }src/Database/Adapter/MariaDB.php (1)
880-891: Bug: undefined $attribute and missing spatial WKT conversion in createDocument binding.
- $attribute is not defined in this loop — causes a notice/error.
- Spatial attributes bound to ST_GeomFromText need WKT, not JSON. Convert arrays to WKT when binding. Also keep the attribute key to detect spatial fields.
- $attributeIndex = 0; - foreach ($attributes as $value) { - if (\is_array($value)) { - $value = \json_encode($value); - } - - $bindKey = 'key_' . $attributeIndex; - $attribute = $this->filter($attribute); - $value = (\is_bool($value)) ? (int)$value : $value; - $stmt->bindValue(':' . $bindKey, $value, $this->getPDOType($value)); - $attributeIndex++; - } + $attributeIndex = 0; + foreach ($attributes as $attribute => $value) { + if (\in_array($attribute, $spatialAttributes, true)) { + if (\is_array($value)) { + $value = $this->convertArrayToWKT($value); + } + } elseif (\is_array($value)) { + $value = \json_encode($value); + } + $bindKey = 'key_' . $attributeIndex; + $value = (\is_bool($value)) ? (int)$value : $value; + $stmt->bindValue(':' . $bindKey, $value, $this->getPDOType($value)); + $attributeIndex++; + }
♻️ Duplicate comments (6)
src/Database/Adapter/MariaDB.php (3)
1115-1182: Desynchronized placeholders in updateDocument: split key_ and op_ counters (same as Postgres).Shared counter for key_ and op_ produces mismatched bindings. Mirror the separate-counter approach used in the suggested Postgres fix.
- $bindIndex = 0; + $keyIndex = 0; + $opIndex = 0; $operators = []; @@ - if (isset($operators[$attribute])) { - $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $bindIndex); + if (isset($operators[$attribute])) { + $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $opIndex); $columns .= $operatorSQL . ','; } else { - $bindKey = 'key_' . $bindIndex; + $bindKey = 'key_' . $keyIndex; if (in_array($attribute, $spatialAttributes)) { $columns .= "`{$column}`" . '=' . $this->getSpatialGeomFromText(':' . $bindKey) . ','; } else { $columns .= "`{$column}`" . '=:' . $bindKey . ','; } - $bindIndex++; + $keyIndex++; } @@ - $attributeIndex = 0; + $keyBindIndex = 0; + $opBindIndex = 0; foreach ($attributes as $attribute => $value) { // Handle operators separately if (isset($operators[$attribute])) { - $this->bindOperatorParams($stmt, $operators[$attribute], $attributeIndex); + $this->bindOperatorParams($stmt, $operators[$attribute], $opBindIndex); } else { // Convert spatial arrays to WKT, json_encode non-spatial arrays if (\in_array($attribute, $spatialAttributes, true)) { if (\is_array($value)) { $value = $this->convertArrayToWKT($value); } } elseif (is_array($value)) { $value = json_encode($value); } - $bindKey = 'key_' . $attributeIndex; + $bindKey = 'key_' . $keyBindIndex; $value = (is_bool($value)) ? (int)$value : $value; $stmt->bindValue(':' . $bindKey, $value, $this->getPDOType($value)); - $attributeIndex++; + $keyBindIndex++; } }
1852-1855: Map duplicate violations: only _uid ⇒ DuplicateException; others ⇒ UniqueException.Aligns with intended semantics (user doc id vs other unique constraints).
-use Utopia\Database\Exception\Duplicate as DuplicateException; +use Utopia\Database\Exception\Duplicate as DuplicateException; +use Utopia\Database\Exception\Unique as UniqueException; @@ - // Duplicate row - if ($e->getCode() === '23000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1062) { - return new DuplicateException('Document already exists', $e->getCode(), $e); - } + // Duplicate row: only _uid collisions are DuplicateException; others are UniqueException + if ($e->getCode() === '23000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1062) { + $msg = $e->errorInfo[2] ?? ''; + if (\str_contains($msg, "for key '_uid'")) { + return new DuplicateException('Document already exists', $e->getCode(), $e); + } + return new UniqueException('Unique constraint violated', $e->getCode(), $e); + }Based on learnings
2046-2064: ARRAY_FILTER: add >= and <= conditions.Whitelist supports greaterThanOrEqual/lessThanOrEqual; implement them in CASE.
- WHEN 'greaterThan' THEN CAST(value AS DECIMAL(65,30)) > CAST(JSON_UNQUOTE(:$valueKey) AS DECIMAL(65,30)) - WHEN 'lessThan' THEN CAST(value AS DECIMAL(65,30)) < CAST(JSON_UNQUOTE(:$valueKey) AS DECIMAL(65,30)) + WHEN 'greaterThan' THEN CAST(value AS DECIMAL(65,30)) > CAST(JSON_UNQUOTE(:$valueKey) AS DECIMAL(65,30)) + WHEN 'greaterThanOrEqual' THEN CAST(value AS DECIMAL(65,30)) >= CAST(JSON_UNQUOTE(:$valueKey) AS DECIMAL(65,30)) + WHEN 'lessThan' THEN CAST(value AS DECIMAL(65,30)) < CAST(JSON_UNQUOTE(:$valueKey) AS DECIMAL(65,30)) + WHEN 'lessThanOrEqual' THEN CAST(value AS DECIMAL(65,30)) <= CAST(JSON_UNQUOTE(:$valueKey) AS DECIMAL(65,30))src/Database/Validator/Operator.php (1)
238-256: Intersect/diff must require array value(s).Add validation that the operator value is a non-empty array for both intersect and diff. This prevents adapter-time failures.
case DatabaseOperator::TYPE_ARRAY_INTERSECT: if (!$isArray) { $this->message = "Cannot use {$method} operator on non-array attribute '{$operator->getAttribute()}'"; return false; } - if (empty($values)) { - $this->message = "{$method} operator requires a non-empty array value"; + if (empty($values) || !\is_array($values[0]) || \count($values[0]) === 0) { + $this->message = "{$method} operator requires a non-empty array value"; return false; } break; case DatabaseOperator::TYPE_ARRAY_DIFF: if (!$isArray) { $this->message = "Cannot use {$method} operator on non-array attribute '{$operator->getAttribute()}'"; return false; } + + if (empty($values) || !\is_array($values[0]) || \count($values[0]) === 0) { + $this->message = "{$method} operator requires a non-empty array value"; + return false; + } break;src/Database/Adapter/SQLite.php (2)
1672-1679: Add missing 'null' filter case for ARRAY_FILTER.The
ARRAY_FILTERimplementation handles'notNull'(line 1673) to filter out null values, but lacks a corresponding'null'case to filter for null values only. This is an incomplete implementation of the filter types.Based on past review comments.
Add the
'null'case before'notNull':switch ($filterType) { + case 'null': + // Filter for null values - no bind parameter needed + return "{$quotedColumn} = ( + SELECT json_group_array(value) + FROM json_each(IFNULL({$quotedColumn}, '[]')) + WHERE value IS NULL + )"; + case 'notNull': // Filter out null values - no bind parameter needed return "{$quotedColumn} = ( SELECT json_group_array(value) FROM json_each(IFNULL({$quotedColumn}, '[]')) WHERE value IS NOT NULL )";
809-821: Add null check for getOperatorSQL return value to prevent malformed SQL.The method
getOperatorSQL()has return type?stringand may return null. At line 811-812, the result is used without null checking, then a comma is unconditionally appended at line 823. This can create invalid SQL if the operator SQL is null.The code at line 1802 in
getUpsertStatementproperly checksif ($operatorSQL !== null)before using the value. Apply the same pattern here:Based on past review comments.
Apply this diff:
// Check if this is an operator, spatial attribute, or regular attribute if (isset($operators[$attribute])) { $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $bindIndex); - $columns .= $operatorSQL; + if ($operatorSQL !== null) { + $columns .= $operatorSQL; + $columns .= ','; + } } elseif (\in_array($attribute, $spatialAttributes, true)) { $bindKey = 'key_' . $bindIndex; $columns .= "`{$column}` = " . $this->getSpatialGeomFromText(':' . $bindKey); $bindIndex++; + $columns .= ','; } else { $bindKey = 'key_' . $bindIndex; $columns .= "`{$column}`" . '=:' . $bindKey; $bindIndex++; + $columns .= ','; } - - $columns .= ',';
🧹 Nitpick comments (4)
src/Database/Validator/Operator.php (1)
257-273: Whitelist arrayFilter conditions here (align with adapters).Validate the condition string against supported values to fail fast and avoid adapter mismatches. Suggested set (supported across adapters in this PR): equals, notEquals, greaterThan, lessThan, null, notNull.
if (!\is_string($values[0])) { $this->message = "Cannot apply {$method} operator: condition must be a string"; return false; } + $validConditions = [ + 'equals', 'notEquals', + 'greaterThan', 'lessThan', + 'null', 'notNull', + ]; + if (!\in_array($values[0], $validConditions, true)) { + $this->message = "Invalid array filter condition '{$values[0]}'. Allowed: " . \implode(', ', $validConditions); + return false; + } + break;src/Database/Adapter/Postgres.php (1)
2571-2572: Postgres arrayFilter numeric cast: ensure portable placeholder casting.Safer to wrap the placeholder before ::text and cast via CAST(TRIM(...)) to avoid parser issues.
- WHEN 'greaterThan' THEN (value::text)::numeric > trim(both '\"' from :$valueKey::text)::numeric - WHEN 'lessThan' THEN (value::text)::numeric < trim(both '\"' from :$valueKey::text)::numeric + WHEN 'greaterThan' THEN (value::text)::numeric > CAST(TRIM(BOTH '\"' FROM (:$valueKey)::text) AS numeric) + WHEN 'lessThan' THEN (value::text)::numeric < CAST(TRIM(BOTH '\"' FROM (:$valueKey)::text) AS numeric)Please confirm with an integration test against Postgres 13+.
src/Database/Adapter/MariaDB.php (2)
2037-2044: ARRAY_REMOVE: compare against unquoted JSON scalar.Ensure TEXT vs JSON scalar comparison matches by unquoting the bound JSON value.
- WHERE value != :$bindKey + WHERE value != JSON_UNQUOTE(:$bindKey)
1696-1700: Minor: fix error text formatting in getSQLType default.There’s an extra ", " segment. Clean up message.
- default: - throw new DatabaseException('Unknown type: ' . $type . '. Must be one of ' . Database::VAR_STRING . ', ' . Database::VAR_INTEGER . ', ' . Database::VAR_FLOAT . ', ' . Database::VAR_BOOLEAN . ', ' . Database::VAR_DATETIME . ', ' . Database::VAR_RELATIONSHIP . ', ' . ', ' . Database::VAR_POINT . ', ' . Database::VAR_LINESTRING . ', ' . Database::VAR_POLYGON); + default: + throw new DatabaseException( + 'Unknown type: ' . $type . '. Must be one of ' . + Database::VAR_STRING . ', ' . Database::VAR_INTEGER . ', ' . Database::VAR_FLOAT . ', ' . + Database::VAR_BOOLEAN . ', ' . Database::VAR_DATETIME . ', ' . Database::VAR_RELATIONSHIP . ', ' . + Database::VAR_POINT . ', ' . Database::VAR_LINESTRING . ', ' . Database::VAR_POLYGON + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Adapter/MariaDB.php(9 hunks)src/Database/Adapter/Postgres.php(10 hunks)src/Database/Adapter/SQLite.php(9 hunks)src/Database/Validator/Operator.php(1 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/Scopes/SpatialTests.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
PR: utopia-php/database#733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
src/Database/Adapter/MariaDB.phpsrc/Database/Adapter/SQLite.php
🧬 Code graph analysis (4)
src/Database/Adapter/MariaDB.php (6)
src/Database/Validator/Operator.php (1)
Operator(10-373)src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)increment(409-416)getMethod(139-142)getValues(155-158)src/Database/Adapter/Postgres.php (4)
getOperatorSQL(2403-2606)bindOperatorParams(2617-2652)getPDOType(1895-1904)quote(2082-2085)src/Database/Adapter/SQLite.php (2)
getOperatorSQL(1450-1744)bindOperatorParams(1396-1429)src/Database/Adapter/SQL.php (5)
getOperatorSQL(1866-2035)bindOperatorParams(2045-2208)convertArrayToWKT(3059-3098)getPDOType(2322-2322)count(3334-3396)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
src/Database/Adapter/SQLite.php (6)
src/Database/Database.php (3)
Database(38-8559)updateDocument(4925-5163)count(7321-7372)src/Database/Adapter/MariaDB.php (5)
processException(1830-1896)updateDocument(950-1198)getOperatorSQL(1912-2070)getPDOType(1710-1718)getUpsertStatement(1211-1290)src/Database/Adapter/Postgres.php (6)
processException(2033-2076)updateDocument(1093-1324)getOperatorSQL(2403-2606)bindOperatorParams(2617-2652)getPDOType(1895-1904)getUpsertStatement(1336-1415)src/Database/Adapter/SQL.php (9)
processException(2548-2551)getSpatialAttributes(432-445)getOperatorSQL(1866-2035)getSpatialGeomFromText(1662-1674)bindOperatorParams(2045-2208)getPDOType(2322-2322)getPDO(2310-2313)count(3334-3396)getUpsertStatement(1696-1704)src/Database/Adapter/MySQL.php (2)
processException(148-165)getOperatorSQL(278-306)src/Database/Operator.php (2)
isOperator(664-667)increment(409-416)
src/Database/Adapter/Postgres.php (5)
src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)increment(409-416)getMethod(139-142)getValues(155-158)src/Database/Adapter/SQL.php (7)
getSpatialAttributes(432-445)getOperatorSQL(1866-2035)getSpatialGeomFromText(1662-1674)bindOperatorParams(2045-2208)convertArrayToWKT(3059-3098)getPDOType(2322-2322)count(3334-3396)src/Database/Adapter/MariaDB.php (2)
getOperatorSQL(1912-2070)getPDOType(1710-1718)src/Database/Adapter/SQLite.php (2)
getOperatorSQL(1450-1744)bindOperatorParams(1396-1429)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
src/Database/Validator/Operator.php (2)
src/Database/Database.php (2)
Database(38-8559)__construct(423-621)src/Database/Operator.php (7)
Operator(14-698)__construct(120-125)getAttribute(147-150)getMethod(139-142)isMethod(226-251)getValues(155-158)getValue(164-167)
🔇 Additional comments (7)
src/Database/Adapter/SQLite.php (7)
797-804: LGTM!The operator separation logic correctly identifies operator instances and builds a separate map for special handling during SQL generation and binding.
1360-1385: LGTM!The math function detection correctly tests for POWER availability and caches the result. The helpful error message at line 1532-1535 guides users when math functions aren't compiled into SQLite.
1396-1429: LGTM!The operator binding logic correctly handles SQLite-specific operators and delegates to the parent implementation for standard operators. The comment at line 1401-1404 appropriately notes that bindIndex increment happens in
getOperatorSQL(), not in binding, which maintains consistency.
1456-1552: LGTM!The numeric operators correctly implement:
- NULL handling via COALESCE
- Optional min/max clamping with CASE statements
- Division-by-zero protection
- Math function availability check for POWER
The overflow prevention logic for MULTIPLY (lines 1503-1504) correctly checks both positive and negative multiplier cases.
1565-1661: LGTM!The array operators correctly use SQLite's JSON functions (
json_each,json_group_array,DISTINCT) to manipulate arrays. The warning comment at lines 1434-1443 appropriately documents SQLite's type conversion behavior for JSON values, which is an important limitation to be aware of.The
ARRAY_INSERTimplementation (lines 1609-1641) cleverly uses window functions and UNION ALL to insert at the correct index.
1758-1838: LGTM!The
getUpsertStatementimplementation correctly:
- Uses SQLite's
ON CONFLICT ... DO UPDATEsyntax withexcludedreference- Properly checks
if ($operatorSQL !== null)before using operator SQL (line 1802)- Maintains binding order consistency between SQL generation and parameter binding
- Handles shared tables tenant isolation correctly
This implementation follows SQLite's upsert semantics and properly integrates operator support.
1554-1743: LGTM!The remaining operators are correctly implemented:
TOGGLEproperly treats NULL as 0 and flips between 0 and 1REPLACEuses SQLite's built-in REPLACE function- Date operators correctly use SQLite's
datetime()function with interval stringsDATE_SUB_DAYSappropriately usesabs()to ensure negative interval (line 1735)- Parent fallback at line 1742 handles any unsupported operators
| // Duplicate - SQLite uses various error codes for constraint violations: | ||
| // - Error code 19 is SQLITE_CONSTRAINT (includes UNIQUE violations) | ||
| // - Error code 1 is also used for some duplicate cases | ||
| // - SQL state '23000' is integrity constraint violation | ||
| if ( | ||
| ($e->getCode() === 'HY000' && isset($e->errorInfo[1]) && ($e->errorInfo[1] === 1 || $e->errorInfo[1] === 19)) || | ||
| $e->getCode() === '23000' | ||
| ) { | ||
| // Check if it's actually a duplicate/unique constraint violation | ||
| $message = $e->getMessage(); | ||
| if ( | ||
| (isset($e->errorInfo[1]) && $e->errorInfo[1] === 19) || | ||
| $e->getCode() === '23000' || | ||
| stripos($message, 'unique') !== false || | ||
| stripos($message, 'duplicate') !== false | ||
| ) { | ||
| return new DuplicateException('Document already exists', $e->getCode(), $e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider distinguishing _uid unique violations from other unique constraints.
The current code maps all UNIQUE constraint violations (error codes 1, 19, SQLSTATE 23000) to DuplicateException. However, based on learnings from the MariaDB adapter, only duplicate _uid violations should throw DuplicateException, while other unique constraint violations (including PRIMARY key and user-defined unique constraints) should throw UniqueException.
This distinction helps differentiate between user-facing document duplicates (_uid collisions) and internal/user-defined unique constraint violations. Consider parsing the error message to check if it references the _uid constraint before throwing DuplicateException.
Based on learnings.
| // Special validation for divide/modulo by zero | ||
| if (($method === DatabaseOperator::TYPE_DIVIDE || $method === DatabaseOperator::TYPE_MODULO) && $values[0] == 0) { | ||
| $this->message = "Cannot apply {$method} operator: " . ($method === DatabaseOperator::TYPE_DIVIDE ? "division" : "modulo") . " by zero"; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Use strict zero checks to avoid type coercion.
Replace loose comparisons with strict ones in divide/modulo validations and prediction logic.
- if (($method === DatabaseOperator::TYPE_DIVIDE || $method === DatabaseOperator::TYPE_MODULO) && $values[0] == 0) {
+ if (
+ ($method === DatabaseOperator::TYPE_DIVIDE || $method === DatabaseOperator::TYPE_MODULO)
+ && $values[0] === 0
+ ) {
$this->message = "Cannot apply {$method} operator: " . ($method === DatabaseOperator::TYPE_DIVIDE ? "division" : "modulo") . " by zero";
return false;
}
@@
- DatabaseOperator::TYPE_DIVIDE => $operatorValue != 0 ? $currentValue / $operatorValue : $currentValue,
- DatabaseOperator::TYPE_MODULO => $operatorValue != 0 ? $currentValue % $operatorValue : $currentValue,
+ DatabaseOperator::TYPE_DIVIDE => $operatorValue !== 0 ? $currentValue / $operatorValue : $currentValue,
+ DatabaseOperator::TYPE_MODULO => $operatorValue !== 0 ? $currentValue % $operatorValue : $currentValue,Also applies to: 140-148
🤖 Prompt for AI Agents
In src/Database/Validator/Operator.php around lines 124-128 (and similarly
140-148), the code uses loose equality (==) to check for zero which allows type
coercion; change these to strict comparisons (=== 0) so the checks only catch
actual numeric zero values, and ensure any null/empty-string cases are handled
explicitly if intended (e.g., use === 0 or === 0.0 as appropriate or cast to
int/float before comparing), update the divide/modulo validation and the
prediction logic to use strict equality to zero and add any necessary type
casting or guard clauses to preserve behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/Database/Adapter/SQLite.php (4)
624-625: Good: delegate to processException() (createDocument).Centralizing PDO exception mapping improves consistency and future-proofing.
809-827: Guard against null operator SQL and avoid stray commas in SET clause.getOperatorSQL() has a nullable return. Appending a null and then a comma can yield malformed SQL (e.g., embedded ", ,"). Mirror the upsert path by only appending when non-null; move the comma inside each branch.
Apply:
- if (isset($operators[$attribute])) { - $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $bindIndex); - $columns .= $operatorSQL; - } elseif (\in_array($attribute, $spatialAttributes, true)) { - $bindKey = 'key_' . $bindIndex; - $columns .= "`{$column}` = " . $this->getSpatialGeomFromText(':' . $bindKey); - $bindIndex++; - } else { - $bindKey = 'key_' . $bindIndex; - $columns .= "`{$column}`" . '=:' . $bindKey; - $bindIndex++; - } - - $columns .= ','; + if (isset($operators[$attribute])) { + $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $bindIndex); + if ($operatorSQL !== null) { + $columns .= $operatorSQL . ','; + } + } elseif (\in_array($attribute, $spatialAttributes, true)) { + $bindKey = 'key_' . $bindIndex; + $columns .= "`{$column}` = " . $this->getSpatialGeomFromText(':' . $bindKey) . ','; + $bindIndex++; + } else { + $bindKey = 'key_' . $bindIndex; + $columns .= "`{$column}` = :" . $bindKey . ','; + $bindIndex++; + }
1663-1723: ARRAY_FILTER: add explicit 'null' branch.‘null’ is not handled; only ‘notNull’ exists. Add a branch to keep only JSON NULL entries.
Apply:
switch ($filterType) { + case 'null': + // Keep only JSON NULL entries + return "{$quotedColumn} = ( + SELECT json_group_array(value) + FROM json_each(IFNULL({$quotedColumn}, '[]')) + WHERE value IS NULL + )"; case 'notNull': // Filter out null values - no bind parameter needed return "{$quotedColumn} = ( SELECT json_group_array(value) FROM json_each(IFNULL({$quotedColumn}, '[]')) WHERE value IS NOT NULL )";
1283-1306: Differentiate _uid duplicates from other unique violations.Map only _uid collisions to DuplicateException; throw UniqueException for other UNIQUE/PRIMARY/index violations. This matches the intended cross-adapter behavior.
Apply:
- // Duplicate - SQLite uses various error codes for constraint violations: + // Duplicate/Unique - SQLite uses various error codes for constraint violations: @@ - ) { - // Check if it's actually a duplicate/unique constraint violation - $message = $e->getMessage(); - if ( - (isset($e->errorInfo[1]) && $e->errorInfo[1] === 19) || - $e->getCode() === '23000' || - stripos($message, 'unique') !== false || - stripos($message, 'duplicate') !== false - ) { - return new DuplicateException('Document already exists', $e->getCode(), $e); - } - } + ) { + $message = $e->getMessage(); + if ( + (isset($e->errorInfo[1]) && $e->errorInfo[1] === 19) || + $e->getCode() === '23000' || + stripos($message, 'unique') !== false || + stripos($message, 'duplicate') !== false + ) { + // Treat only _uid collisions as Duplicate; others as Unique + if (stripos($message, '._uid') !== false) { + return new DuplicateException('Document already exists', $e->getCode(), $e); + } + return new \Utopia\Database\Exception\Unique('Unique constraint violation', $e->getCode(), $e); + } + }And add import:
use Utopia\Database\Exception\Duplicate as DuplicateException; +use Utopia\Database\Exception\Unique as UniqueException;Based on learnings.
🧹 Nitpick comments (1)
src/Database/Adapter/SQLite.php (1)
1396-1429: Clarify comment vs behavior in bindOperatorParams.The note says “bindIndex increment happens in getOperatorSQL(), NOT here”, but this method does increment for ARRAY_FILTER and delegates to parent (which increments). Reword to “Only non-binding operators are skipped here; others increment as needed.”
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/SQLite.php(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
PR: utopia-php/database#733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
src/Database/Adapter/SQLite.php
🧬 Code graph analysis (2)
src/Database/Adapter/Pool.php (4)
src/Database/Adapter.php (1)
getSupportForOperators(1085-1085)src/Database/Adapter/Mongo.php (1)
getSupportForOperators(2897-2900)src/Database/Adapter/SQL.php (1)
getSupportForOperators(1560-1563)src/Database/Mirror.php (1)
delegate(88-103)
src/Database/Adapter/SQLite.php (4)
src/Database/Adapter/MariaDB.php (5)
processException(1830-1896)getOperatorSQL(1912-2070)getPDOType(1710-1718)quote(1898-1901)getUpsertStatement(1211-1290)src/Database/Adapter/Postgres.php (6)
processException(2033-2076)getOperatorSQL(2403-2606)bindOperatorParams(2617-2652)getPDOType(1895-1904)quote(2082-2085)getUpsertStatement(1336-1415)src/Database/Adapter/SQL.php (8)
processException(2548-2551)getSpatialAttributes(432-445)getOperatorSQL(1866-2035)bindOperatorParams(2045-2208)convertArrayToWKT(3059-3098)getPDOType(2322-2322)getPDO(2310-2313)getUpsertStatement(1696-1704)src/Database/Operator.php (2)
isOperator(664-667)increment(409-416)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Unit Test
🔇 Additional comments (5)
src/Database/Adapter/Pool.php (1)
403-406: LGTM!The implementation correctly delegates operator support capability queries to the underlying adapter, matching the established pattern used throughout the Pool adapter.
src/Database/Adapter/SQLite.php (4)
12-19: Imports look good.Adding LimitException, OperatorException, and Operator is appropriate for the new operator-aware paths.
641-642: Docblock update is correct.Declaring DuplicateException on updateDocument aligns with centralized mapping.
1360-1385: Lightweight probe for math functions is fine.POWER check with caching is a pragmatic gate for TYPE_POWER usage.
1759-1839: Upsert statement generation looks solid.
- Uses ON CONFLICT with proper conflict keys.
- Operator SQL guarded with null-check before inclusion.
- Binding order matches SQL generation.
Consider adding a small unit covering: upsert with one operator (e.g., INCREMENT with max) + one regular attribute to assert both SQL and bound params line up.
| } elseif (\in_array($attribute, $spatialAttributes, true)) { | ||
| $bindKey = 'key_' . $bindIndex; | ||
| $columns .= "`{$column}` = " . $this->getSpatialGeomFromText(':' . $bindKey); | ||
| $bindIndex++; | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
SQLite spatial branch: confirm getSpatialGeomFromText() is safe under SQLite.
SQLite has no native spatial; ensure getSpatialGeomFromText(':param') resolves to a no-op passthrough (i.e., just ':param') here, otherwise this will error at runtime.
Also applies to: 856-861
🏁 Script executed:
#!/bin/bash
# Locate getSpatialGeomFromText() implementations and confirm SQLite behavior.
rg -n 'function\s+getSpatialGeomFromText' -C2 src | sed -n '1,200p'
rg -n 'getSpatialGeomFromText\(' -C2 src/Database/Adapter/SQLite.phpLength of output: 637
🏁 Script executed:
#!/bin/bash
# Inspect full implementation of getSpatialGeomFromText() in SQL.php
sed -n '1662,1680p' src/Database/Adapter/SQL.php
# Check if SQLite.php overrides getSpatialGeomFromText()
rg -n 'function\s+getSpatialGeomFromText' src/Database/Adapter/SQLite.php
# Check the second location (856-861)
sed -n '856,861p' src/Database/Adapter/SQLite.php
rg -n 'getSpatialGeomFromText' src/Database/Adapter/SQLite.php -C3 | tail -40Length of output: 1264
🏁 Script executed:
#!/bin/bash
# Get exact lines 856-861 to see second location
sed -n '856,861p' src/Database/Adapter/SQLite.php
# Check class hierarchy to confirm SQLite extends SQL
rg -n '^class\s+SQLite' -A1 src/Database/Adapter/SQLite.php | head -5Length of output: 391
SQLite spatial update requires override for getSpatialGeomFromText().
The inherited getSpatialGeomFromText() method (SQL.php:1662) returns ST_GeomFromText({$wktPlaceholder}, {$srid}), which SQLite does not support natively. Calling this at lines 815 and 856-861 will generate invalid SQL and fail at runtime.
SQLite.php must override getSpatialGeomFromText() to return the placeholder as-is (e.g., ':key_0') without wrapping it in a spatial function, or skip spatial binding entirely for SQLite.
| // Bind values for non-operator attributes and operator parameters | ||
| $bindIndexForBinding = 0; | ||
| foreach ($attributes as $attribute => $value) { | ||
| if (is_array($value)) { // arrays & objects should be saved as strings | ||
| // Handle operators separately | ||
| if (isset($operators[$attribute])) { | ||
| $this->bindOperatorParams($stmt, $operators[$attribute], $bindIndexForBinding); | ||
| continue; | ||
| } | ||
|
|
||
| // Convert spatial arrays to WKT, json_encode non-spatial arrays | ||
| if (\in_array($attribute, $spatialAttributes, true)) { | ||
| if (\is_array($value)) { | ||
| $value = $this->convertArrayToWKT($value); | ||
| } | ||
| } elseif (is_array($value)) { // arrays & objects should be saved as strings | ||
| $value = json_encode($value); | ||
| } | ||
|
|
||
| $bindKey = 'key_' . $attributeIndex; | ||
| $attribute = $this->filter($attribute); | ||
| $bindKey = 'key_' . $bindIndexForBinding; | ||
| $value = (is_bool($value)) ? (int)$value : $value; | ||
| $stmt->bindValue(':' . $bindKey, $value, $this->getPDOType($value)); | ||
| $attributeIndex++; | ||
| $bindIndexForBinding++; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify bind-index alignment for operators vs regular attributes.
Looks consistent now (operators bind via bindOperatorParams with the same index stream as SQL generation; regular/spatial use key_{n}). Please run a quick sanity test mixing TOGGLE (no params), ARRAY_FILTER (param), and a regular column to ensure no placeholder/key drift.
🏁 Script executed:
#!/bin/bash
# Grep operator binding sites and ensure bindIndex increments for param-using ops.
rg -n -C2 'bindOperatorParams\(|getOperatorSQL\(|op_\{\$bindIndex\}|\bkey_\{\$bindIndex\}' src/Database/Adapter/SQLite.php
# Spot-check: ensure TOGGLE has no :op_ placeholder and no bindIndex++ in bindOperatorParams branch.
rg -n -C3 'TYPE_TOGGLE|TYPE_ARRAY_FILTER' src/Database/Adapter/SQLite.phpLength of output: 9288
I need to see the complete ARRAY_FILTER implementation in getOperatorSQL() and verify the bindIndex increment after binding parameters in bindOperatorParams(). Let me fetch these details.
🏁 Script executed:
#!/bin/bash
# Get full ARRAY_FILTER case in getOperatorSQL
rg -n -A20 'case Operator::TYPE_ARRAY_FILTER:' src/Database/Adapter/SQLite.php | head -50
# Check if bindIndex is incremented after binding ARRAY_FILTER in bindOperatorParams
rg -n -A10 'bindKey = "op_{\$bindIndex}";' src/Database/Adapter/SQLite.php | grep -A3 'TYPE_ARRAY_FILTER' | head -20Length of output: 1145
🏁 Script executed:
#!/bin/bash
# Get full ARRAY_FILTER case in getOperatorSQL (lines 1663 onwards)
sed -n '1663,1720p' src/Database/Adapter/SQLite.php
# Check bindOperatorParams ARRAY_FILTER section - does it increment after binding?
sed -n '1409,1430p' src/Database/Adapter/SQLite.phpLength of output: 3791
Fix bind-index double-increment in ARRAY_FILTER operator.
The code increments $bindIndex twice for ARRAY_FILTER with comparison types: once in getOperatorSQL() (line 1691) during SQL generation, and again in bindOperatorParams() (line 1421) during binding. Since both phases operate on the same reference variable, this causes a skip in the bind-index sequence.
Remove the increment at line 1421 in bindOperatorParams() and rely solely on the increment in getOperatorSQL() (line 1691), consistent with how other operators manage bind-index state across both phases.
🤖 Prompt for AI Agents
In src/Database/Adapter/SQLite.php around lines 847 to 869: the ARRAY_FILTER
operator causes the bind index to be incremented twice because getOperatorSQL()
already advances the shared bind index (at line ~1691) during SQL generation,
and bindOperatorParams() increments it again (line ~1421) during binding; remove
the extra increment in bindOperatorParams() for the ARRAY_FILTER case so binding
uses the same bind-index sequence produced during SQL generation and keep all
other bindIndex++ behavior unchanged.
1 participant
Summary by CodeRabbit
New Features
Database
Validation & Errors
CLI
Tests